Refactor configuration system and enhance instance settings#34
Merged
Conversation
Replace figment-based config with strict config.toml/secrets.toml pair. Introduces RuntimePaths with portable/installed modes, BootstrapConfig for TOML parsing, domain-separated key derivation from master_key, and removes env/CLI overrides. Removes dotenvy, figment, directories deps.
Remove global CLI flags (--config, --bind, --database-url, --log-level), dotenvy loading, and config init/path subcommands. Add secrets reset --yes, config show reads DB settings. Main uses tokio::runtime::Builder manually. Doctor uses RuntimeContext. Cross-platform service run entrypoint.
Replace single hmac_secret with token_index_key, session_auth_key, signed_upload_key, and webhook_encryption_key derived from master_key via secrets::derive_key_hex(). Update middleware, gRPC, MCP, and handler consumers to use the correct domain key.
Change providers HashMap to RwLock<HashMap> for concurrent provider swap at runtime. register() and get() now take &self instead of &mut self. Adds remove() method for hot-swapping providers.
Add instance_settings table (all backends) storing owner-only runtime configuration: general, security, storage, and backups sections. AES-256-GCM encrypted credential storage. REST handler with GET/PUT per section. S3 probe before persisting storage credentials. Owner-only authorization.
Load instance settings at startup, apply to config. Wire with_settings() on auth, webhook, and backup services. Backup scheduler always runs, gated by settings. Dynamic CORS and upload limits from SettingsService.
Add enabled boolean column to site_webhooks (all backends) to disable webhooks without deleting. Encrypt headers with v1: prefix version tag. WebhookService gains with_settings() for runtime SSRF override. Dump scrubs headers_encrypted and enabled during backup.
Remove access_tokens from backup table registry (security-sensitive). Add instance_settings to backup registry. BackupService gains with_settings() for runtime destination swap. Restore returns RecoveryReport with recovery instructions. Credential blobs reloaded post-restore. Dump scrubs webhook secrets.
vcms service run is now the cross-platform installed-service entrypoint, no longer Windows-only. Adds is_installed() detection per platform. Portable mode prints startup banner with mode, data path, and endpoints.
Service definitions use 'vcms service run' instead of 'serve'. Remove environment variable overrides from service files. Windows MSI adds WiX Util extension for fine-grained ACLs. xtask config samples updated for new TOML structure. Release workflow adds WiX extension install.
Add General, Security, Storage, and Backup settings panels for owner-only instance configuration. Switch toggle component. Settings routes with owner-only visibility. API types and functions for instance settings CRUD. Route tree updated with new settings routes.
Test harness updated with domain-separated key fields, StorageRegistry no longer needs mut, SettingsService wired into create_router. Add settings integration tests for owner-only access and field validation. Remove stale env var references from MCP proxy tests.
Update AGENTS.md, CLAUDE.md, and README.md to reflect strict TOML config, portable/installed runtime modes, domain-separated keys, instance settings, and cross-platform service entrypoint.
Remove Cli dependency, use RuntimeContext directly. StorageRegistry::new() no longer needs mut. Update error messages and storage path defaults.
📝 WalkthroughWalkthroughThis PR replaces layered configuration with deterministic runtime paths, persisted secrets, and database-backed instance settings. It adds owner-only settings APIs and dashboard forms, updates authentication, routing, uploads, backup/restore, service startup, packaging, migrations, tests, and documentation. ChangesRuntime settings and service-mode overhaul
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/backend/src/handlers/backup_handler.rs (1)
223-244: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate downstream tests and clients to match the new response contract.
The success response for
run_restorewas changed from204 No Contentto200 OKwith a JSON body (report). According to the codebase context, downstream tests (e.g.,apps/backend/tests/rest/backups_tests.rs) still assert204and will fail.Additionally, as per coding guidelines, please ensure that any frontend data fetching integration (like the
restoreBackupAPI client) is updated to handle the new JSON response if necessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/handlers/backup_handler.rs` around lines 223 - 244, Update downstream restore consumers to match run_restore’s 200 OK JSON report contract: revise backup REST tests such as backups_tests.rs to assert status 200 and validate the returned report body instead of expecting 204 No Content. Inspect the restoreBackup frontend/API client and update its response parsing and typing to consume the JSON report where applicable, while preserving existing error handling.Source: Coding guidelines
apps/backend/src/main.rs (1)
238-243: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winApply persisted instance settings before constructing backup services.
Both CLI paths use bootstrap configuration directly. Unlike
server::run, they never load and applySettingsService, so owner-selected storage destinations, backup destinations, and encrypted credentials are ignored by backup and restore commands.Also applies to: 319-324
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/main.rs` around lines 238 - 243, Update both CLI paths around BackupService construction to load persisted instance settings through the existing SettingsService flow and apply them to the bootstrap configuration before calling init_db_with_config, initialize_storage, build_backup_destination, or BackupService::new. Ensure backup and restore commands use the resulting settings for storage destinations, backup destinations, and encrypted credentials, matching the behavior of server::run.apps/backend/src/services/mod.rs (1)
130-135: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRemove the mode-blind relative search-index fallback.
vcms_data/searchresolves against the process working directory even in installed mode, potentially splitting the index from the instance data root. Populate this path fromRuntimePaths, or fail closed when it is absent.As per coding guidelines, installed mode uses the fixed native-service root and only portable mode uses
<cwd>/vcms_data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/services/mod.rs` around lines 130 - 135, Update search_index_path to use the mode-aware RuntimePaths value rather than falling back to the mode-blind “vcms_data/search” path. Preserve the configured search_index_path when provided; otherwise derive the path from RuntimePaths, or fail closed if no valid runtime path exists, ensuring installed mode uses the native-service root and portable mode uses the current-directory vcms_data location.Source: Coding guidelines
🟡 Minor comments (4)
apps/backend/tests/mcp/stdio_proxy_tests.rs-23-29 (1)
23-29: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the stdio client test isolated from server configuration.
Inheriting
DATABASE_URL,HMAC_SECRET, orVCMS_HOMEmeans this test no longer proves that stdio startup depends only on the MCP URL and token. Restore theenv_removecalls so developer and CI environments cannot mask a regression.As per coding guidelines, only the diskless MCP stdio client may read
VCMS_MCP_TOKENandVCMS_MCP_URL; the server has no environment override layer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/tests/mcp/stdio_proxy_tests.rs` around lines 23 - 29, Update the stdio client setup in start to remove inherited DATABASE_URL, HMAC_SECRET, and VCMS_HOME environment variables from the spawned vcms process, while retaining only the VCMS_MCP_URL and VCMS_MCP_TOKEN overrides so startup remains isolated from server configuration.Sources: Coding guidelines, Learnings
apps/backend/src/handlers/settings_handler.rs-294-300 (1)
294-300: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAlways clean up the S3 probe object.
If
getfails, the early return skipsdelete, leaving one object per failed probe. Attempt cleanup regardless of the read result, while preserving the original probe error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/handlers/settings_handler.rs` around lines 294 - 300, Update the S3 probe flow around the temporary key, storage.get, and storage.delete so cleanup is attempted regardless of whether the read succeeds. Preserve and return the original get error after attempting deletion, while retaining the existing successful read and cleanup behavior.apps/backend/src/cli.rs-10-10 (1)
10-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the mojibake in CLI help.
—renders literally instead of as an em dash.Proposed fix
- long_about = "Velopulent CMS — self-hosted headless content management system.\n\n`vcms serve` runs a portable instance from ./vcms_data. When the native service is installed, operational commands use its fixed system data root.", + long_about = "Velopulent CMS — self-hosted headless content management system.\n\n`vcms serve` runs a portable instance from ./vcms_data. When the native service is installed, operational commands use its fixed system data root.",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/cli.rs` at line 10, Update the CLI help text in the command metadata containing long_about to replace the mojibake sequence “—” with the intended em dash character, preserving the surrounding wording and formatting.apps/backend/src/server.rs-140-159 (1)
140-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not advertise a disabled MCP endpoint.
The portable banner always prints the MCP URL, even when
config.mcp_enabledis false. Make this line conditional so startup output reflects the active API surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/server.rs` around lines 140 - 159, Update the portable startup banner in the runtime mode block so the MCP URL line is printed only when config.mcp_enabled is true. Adjust the corresponding format arguments to remain valid while preserving all other banner entries and their output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/src/diagnostics.rs`:
- Around line 94-96: Update sanitize so truncation never slices message at an
invalid UTF-8 byte boundary: select the largest valid character boundary at or
before the 240-byte limit, then append a proper Unicode ellipsis. Preserve the
existing unmodified return behavior for messages within the limit.
In `@apps/backend/src/main.rs`:
- Around line 130-135: Reorder the startup flow around init_db_with_config and
invalidate_credentials so all credential and trust-root invalidations are
performed within a single database transaction and committed successfully before
cms::secrets::replace updates the secrets file. Ensure any connection, query, or
commit failure aborts the transaction and prevents secret replacement, while
preserving the existing returned tokens, webhooks, and s3_sites behavior.
- Around line 112-132: Update the SecretsAction::Reset flow around
cms::secrets::fresh and cms::secrets::replace to explicitly warn the user that
resetting generates a new backup_encryption_key and makes existing encrypted
backups unreadable, requiring confirmation before proceeding or a clear
equivalent notice. Preserve the existing old_database_url handling and reset
behavior.
In `@apps/backend/src/middleware/api_auth.rs`:
- Line 54: Update both verify_access_token call sites in
apps/backend/src/middleware/api_auth.rs:54-54 and
apps/backend/src/middleware/auth.rs:120-120 to support persisted HMACs created
with the legacy hmac_secret while transitioning to config.token_index_key,
either by migrating stored_hmac values or by verifying with the legacy key as a
fallback. Keep bcrypt verification available when stored_hmac is absent, and
apply the same compatibility behavior in both middleware paths.
In `@apps/backend/src/paths.rs`:
- Around line 94-128: Update ensure and the platform-specific create_private_dir
helpers so existing runtime directories are also validated and hardened to
private permissions on every call. Apply Unix 0700 and Windows ACL protection
idempotently to the root and child directories, or return an error when their
permissions cannot be made secure; preserve startup failure propagation through
the existing std::io::Result flow.
In `@apps/backend/src/router/mod.rs`:
- Around line 173-186: Update the upload request handling around the existing
Content-Length check to enforce the current SettingsService upload limit while
streaming the body, including chunked requests without a Content-Length header.
Count received bytes and terminate the stream with PAYLOAD_TOO_LARGE once the
live configured limit is exceeded, while preserving the existing early rejection
for declared oversized uploads.
- Around line 154-162: Move the MCP route merge involving mcp::mcp_routes and
router.merge(mcp_router) before the global runtime-policy layer is applied, or
reapply that layer after the final merge. Ensure the resulting /mcp routes are
wrapped by dynamic_runtime_policy and honor the mcp_enabled gate.
In `@apps/backend/src/runtime.rs`:
- Around line 14-27: Update RuntimeContext::initialize to construct both secrets
and bootstrap from the same loaded secrets snapshot. Pass the secrets returned
by crate::secrets::ensure into Config::load, adjusting its signature and
implementation as needed so bootstrap keys are derived from that provided
snapshot instead of reloading secrets.toml. Preserve the existing initialization
order and returned fields.
In `@apps/backend/src/secrets.rs`:
- Around line 62-74: Update the Windows branch of secrets::replace to replace
the target atomically instead of using std::fs::copy, preserving the temporary
file when replacement fails so recovery remains possible. Keep the existing
restricted-permission handling and successful temporary-file cleanup, and leave
the non-Windows rename path unchanged.
In `@apps/backend/src/server.rs`:
- Around line 53-56: After `settings.apply_to_config(&mut config).await` in the
startup flow, invoke `config.validate_security()` again and propagate its error
before continuing. Ensure the effective configuration, including persisted
instance settings, is validated before the server starts.
In `@apps/backend/src/service/status.rs`:
- Around line 42-45: Update the macOS branch of the service-detection function
to use fallible metadata inspection instead of Path::is_file(). Return false
only when the plist lookup reports NotFound, and propagate all other I/O errors
so detection fails closed; preserve the existing path and successful file check
behavior.
In `@apps/backend/src/services/backup/mod.rs`:
- Around line 541-546: Restrict the storage reset logic around remove("s3") and
set_destination in the restore flow to RestoreTarget::WholeInstance only.
Site-scoped restores must preserve the existing instance runtime storage while
retaining the current reset behavior for whole-instance restores.
- Around line 209-215: Update the backup artifact model and flows around
set_destination and destination_snapshot so every existing backup row,
staged-upload key, restore, deletion, and retention operation retains and uses
the destination provider associated with that artifact rather than the current
mutable global destination. Persist the destination identity or store
destination-specific handles, while keeping new artifacts bound to the
destination active when they are created.
- Around line 547-549: Update apply_restore so settings.reload() cannot turn a
successfully committed restore into an ordinary failure. Validate restored
settings before the database commit, or explicitly represent a post-commit
reload failure as recovery-required while preserving the committed result and
preventing stale process state.
In `@apps/backend/src/services/settings.rs`:
- Around line 179-190: The publish flow in SettingsService::publish must
serialize updates and reject stale snapshots. Add a service-level write mutex,
hold it across validation, persistence, credentials replacement, and snapshot
publication, and use an atomic mutation/CAS operation that reloads or verifies
the current version within that critical section before persisting; return an
error for stale settings rather than overwriting newer sections.
- Around line 133-149: In the persisted-settings startup branch identified by
the match on row, validate the deserialized settings immediately after
serde_json::from_str and before returning them as active configuration. Reuse
the existing validate(&settings) behavior used by publish and reload, propagate
validation errors instead of silently accepting or repairing malformed settings,
and preserve the existing version and credentials handling.
In `@apps/backend/src/services/webhook.rs`:
- Line 42: Update the Services::assemble wiring and WebhookService::new
construction to pass the available SettingsService into the settings field
instead of leaving it as None. Ensure allow_private_targets() reads the attached
service so runtime configuration changes disable previously allowed private
webhook targets.
- Around line 284-286: Update the webhook delivery decryption flow around
decrypt_headers_checked to support previously persisted ciphertext without the
v1: prefix using the former key contract. When legacy decryption succeeds,
re-encrypt and persist the headers in the current format so subsequent
deliveries use the new contract. Preserve the existing error for values that
cannot be decrypted and keep the webhook API behavior stable.
In `@apps/dashboard/src/components/instance/settings-forms.tsx`:
- Around line 443-447: Expand the target-change checks to compare every
effective destination field: provider, bucket, region, and endpoint. Update the
storage check around targetChanged and the backup destination check in
apps/dashboard/src/components/instance/settings-forms.tsx at lines 443-447 and
550-554 respectively, preserving the existing confirmation behavior while
detecting changes to any of these fields.
- Around line 448-454: Clear the credential fields after successful mutations so
secrets do not remain in component state: update the storage form’s submit flow
around save.mutate at apps/dashboard/src/components/instance/settings-forms.tsx
lines 448-454, and apply the same success cleanup to the backup form at lines
555-561. Reset both access and secret key state only after the save succeeds.
---
Outside diff comments:
In `@apps/backend/src/handlers/backup_handler.rs`:
- Around line 223-244: Update downstream restore consumers to match
run_restore’s 200 OK JSON report contract: revise backup REST tests such as
backups_tests.rs to assert status 200 and validate the returned report body
instead of expecting 204 No Content. Inspect the restoreBackup frontend/API
client and update its response parsing and typing to consume the JSON report
where applicable, while preserving existing error handling.
In `@apps/backend/src/main.rs`:
- Around line 238-243: Update both CLI paths around BackupService construction
to load persisted instance settings through the existing SettingsService flow
and apply them to the bootstrap configuration before calling
init_db_with_config, initialize_storage, build_backup_destination, or
BackupService::new. Ensure backup and restore commands use the resulting
settings for storage destinations, backup destinations, and encrypted
credentials, matching the behavior of server::run.
In `@apps/backend/src/services/mod.rs`:
- Around line 130-135: Update search_index_path to use the mode-aware
RuntimePaths value rather than falling back to the mode-blind “vcms_data/search”
path. Preserve the configured search_index_path when provided; otherwise derive
the path from RuntimePaths, or fail closed if no valid runtime path exists,
ensuring installed mode uses the native-service root and portable mode uses the
current-directory vcms_data location.
---
Minor comments:
In `@apps/backend/src/cli.rs`:
- Line 10: Update the CLI help text in the command metadata containing
long_about to replace the mojibake sequence “—” with the intended em dash
character, preserving the surrounding wording and formatting.
In `@apps/backend/src/handlers/settings_handler.rs`:
- Around line 294-300: Update the S3 probe flow around the temporary key,
storage.get, and storage.delete so cleanup is attempted regardless of whether
the read succeeds. Preserve and return the original get error after attempting
deletion, while retaining the existing successful read and cleanup behavior.
In `@apps/backend/src/server.rs`:
- Around line 140-159: Update the portable startup banner in the runtime mode
block so the MCP URL line is printed only when config.mcp_enabled is true.
Adjust the corresponding format arguments to remain valid while preserving all
other banner entries and their output.
In `@apps/backend/tests/mcp/stdio_proxy_tests.rs`:
- Around line 23-29: Update the stdio client setup in start to remove inherited
DATABASE_URL, HMAC_SECRET, and VCMS_HOME environment variables from the spawned
vcms process, while retaining only the VCMS_MCP_URL and VCMS_MCP_TOKEN overrides
so startup remains isolated from server configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f998e540-fa5a-4593-8f5a-035334ff1279
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (74)
.github/workflows/release.ymlAGENTS.mdCLAUDE.mdREADME.mdapps/backend/Cargo.tomlapps/backend/migrations/mysql/20260619000000_instance_settings.sqlapps/backend/migrations/mysql/20260620000000_webhook_enabled.sqlapps/backend/migrations/postgres/20260619000000_instance_settings.sqlapps/backend/migrations/postgres/20260620000000_webhook_enabled.sqlapps/backend/migrations/sqlite/20260619000000_instance_settings.sqlapps/backend/migrations/sqlite/20260620000000_webhook_enabled.sqlapps/backend/project.jsonapps/backend/src/cli.rsapps/backend/src/config.rsapps/backend/src/diagnostics.rsapps/backend/src/grpc/auth.rsapps/backend/src/handlers/backup_handler.rsapps/backend/src/handlers/file_handler.rsapps/backend/src/handlers/settings_handler.rsapps/backend/src/lib.rsapps/backend/src/main.rsapps/backend/src/mcp/auth.rsapps/backend/src/mcp/tools/file.rsapps/backend/src/middleware/api_auth.rsapps/backend/src/middleware/auth.rsapps/backend/src/middleware/dashboard_auth.rsapps/backend/src/models/authorization.rsapps/backend/src/models/webhook.rsapps/backend/src/paths.rsapps/backend/src/repository/mysql/webhook.rsapps/backend/src/repository/postgres/webhook.rsapps/backend/src/repository/sqlite/webhook.rsapps/backend/src/router/graphql.rsapps/backend/src/router/instance.rsapps/backend/src/router/mod.rsapps/backend/src/runtime.rsapps/backend/src/secrets.rsapps/backend/src/server.rsapps/backend/src/service/mod.rsapps/backend/src/service/status.rsapps/backend/src/service/windows.rsapps/backend/src/services/auth.rsapps/backend/src/services/backup/mod.rsapps/backend/src/services/backup/scheduler.rsapps/backend/src/services/backup/schema.rsapps/backend/src/services/mod.rsapps/backend/src/services/settings.rsapps/backend/src/services/webhook.rsapps/backend/src/storage/mod.rsapps/backend/src/test_helpers.rsapps/backend/src/tracing.rsapps/backend/tests/common/grpc.rsapps/backend/tests/common/server.rsapps/backend/tests/mcp/stdio_proxy_tests.rsapps/backend/tests/rest/main.rsapps/backend/tests/rest/settings_tests.rsapps/dashboard/src/components/instance/settings-forms.tsxapps/dashboard/src/components/ui/switch.tsxapps/dashboard/src/lib/api.tsapps/dashboard/src/routeTree.gen.tsapps/dashboard/src/routes/_admin/_shell/settings.tsxapps/dashboard/src/routes/_admin/_shell/settings/backups.tsxapps/dashboard/src/routes/_admin/_shell/settings/index.tsxapps/dashboard/src/routes/_admin/_shell/settings/security.tsxapps/dashboard/src/routes/_admin/_shell/settings/storage.tsxpackaging/linux/vcms.servicepackaging/macos/com.velopulent.vcms.plistpackaging/windows/vcms.wxs.templatextask/README.mdxtask/src/linux.rsxtask/src/macos.rsxtask/src/portable.rsxtask/src/shared.rsxtask/src/windows.rs
Comment on lines
+94
to
+96
| fn sanitize(message: &str) -> String { | ||
| if message.len() > 240 { | ||
| format!("{}…", &message[..240]) | ||
| format!("{}…", &message[..240]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Truncate diagnostic errors on UTF-8 boundaries.
&message[..240] panics when byte 240 falls inside a multibyte character. The suffix is also corrupted.
Proposed fix
fn sanitize(message: &str) -> String {
- if message.len() > 240 {
- format!("{}…", &message[..240])
+ let mut chars = message.chars();
+ let prefix: String = chars.by_ref().take(240).collect();
+ if chars.next().is_some() {
+ format!("{prefix}…")
} else {
message.to_owned()
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn sanitize(message: &str) -> String { | |
| if message.len() > 240 { | |
| format!("{}…", &message[..240]) | |
| format!("{}…", &message[..240]) | |
| fn sanitize(message: &str) -> String { | |
| let mut chars = message.chars(); | |
| let prefix: String = chars.by_ref().take(240).collect(); | |
| if chars.next().is_some() { | |
| format!("{prefix}…") | |
| } else { | |
| message.to_owned() | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/diagnostics.rs` around lines 94 - 96, Update sanitize so
truncation never slices message at an invalid UTF-8 byte boundary: select the
largest valid character boundary at or before the 240-byte limit, then append a
proper Unicode ellipsis. Preserve the existing unmodified return behavior for
messages within the limit.
| }; | ||
|
|
||
| let actor = match verify_access_token(&token, &repository, &config.hmac_secret).await { | ||
| let actor = match verify_access_token(&token, &repository, &config.token_index_key).await { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Migrate persisted access-token HMACs before switching verification keys.
Existing rows populated using hmac_secret will not match token_index_key, and verify_access_token only uses bcrypt when stored_hmac is absent.
apps/backend/src/middleware/api_auth.rs#L54-L54: support the legacy key during transition or migrate existingstored_hmacvalues.apps/backend/src/middleware/auth.rs#L120-L120: use the same compatibility strategy for the shared extractor path.
📍 Affects 2 files
apps/backend/src/middleware/api_auth.rs#L54-L54(this comment)apps/backend/src/middleware/auth.rs#L120-L120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/middleware/api_auth.rs` at line 54, Update both
verify_access_token call sites in apps/backend/src/middleware/api_auth.rs:54-54
and apps/backend/src/middleware/auth.rs:120-120 to support persisted HMACs
created with the legacy hmac_secret while transitioning to
config.token_index_key, either by migrating stored_hmac values or by verifying
with the legacy key as a fallback. Keep bcrypt verification available when
stored_hmac is absent, and apply the same compatibility behavior in both
middleware paths.
Comment on lines
+94
to
128
| pub fn ensure(&self) -> std::io::Result<()> { | ||
| let root_existed = self.root.exists(); | ||
| create_private_dir(&self.root)?; | ||
| #[cfg(windows)] | ||
| if !root_existed { | ||
| harden_windows_acl(&self.root, true)?; | ||
| } | ||
| for path in [ | ||
| self.storage_dir(), | ||
| self.backups_dir(), | ||
| self.search_dir(), | ||
| self.logs_dir(), | ||
| ] { | ||
| create_private_dir(&path)?; | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| /// `.env` — optional environment file loaded at startup (config dir). | ||
| pub fn env_file() -> PathBuf { | ||
| layout().config().join(".env") | ||
| #[cfg(unix)] | ||
| fn create_private_dir(path: &Path) -> std::io::Result<()> { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| let existed = path.exists(); | ||
| std::fs::create_dir_all(path)?; | ||
| if existed { | ||
| Ok(()) | ||
| } else { | ||
| std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o700)) | ||
| } | ||
| } | ||
|
|
||
| /// `vcms.db` — the default SQLite database file (data dir). | ||
| pub fn default_db_path() -> PathBuf { | ||
| layout().data().join("vcms.db") | ||
| #[cfg(windows)] | ||
| fn create_private_dir(path: &Path) -> std::io::Result<()> { | ||
| std::fs::create_dir_all(path) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Harden or reject existing runtime directories.
ensure() only applies Unix mode 0700 and Windows ACLs when the root is newly created. An existing permissive vcms_data or installed root therefore remains usable, potentially exposing the database, uploads, backups, and logs. Apply the private permissions idempotently or fail startup when they are insecure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/paths.rs` around lines 94 - 128, Update ensure and the
platform-specific create_private_dir helpers so existing runtime directories are
also validated and hardened to private permissions on every call. Apply Unix
0700 and Windows ACL protection idempotently to the root and child directories,
or return an error when their permissions cannot be made secure; preserve
startup failure propagation through the existing std::io::Result flow.
| webhook_repo: Arc<dyn WebhookRepository>, | ||
| encryption_key: Arc<[u8; 32]>, | ||
| allow_private_targets: bool, | ||
| settings: Option<crate::services::settings::SettingsService>, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Attach SettingsService when constructing WebhookService.
The supplied Services::assemble wiring only calls WebhookService::new, so settings remains None and allow_private_targets() permanently uses its startup value. Disabling private webhook targets at runtime can therefore leave previously allowed SSRF destinations enabled.
Also applies to: 95-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/services/webhook.rs` at line 42, Update the
Services::assemble wiring and WebhookService::new construction to pass the
available SettingsService into the settings field instead of leaving it as None.
Ensure allow_private_targets() reads the attached service so runtime
configuration changes disable previously allowed private webhook targets.
Comment on lines
+284
to
+286
| let headers = decrypt_headers_checked(&webhook.headers_encrypted, &self.encryption_key).ok_or_else(|| { | ||
| WebhookError::DeliveryFailed("Webhook credentials cannot be decrypted; reconfigure this webhook".into()) | ||
| })?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Provide a migration path for existing encrypted headers.
All previously persisted webhook ciphertext lacks the new v1: prefix and was encrypted using the former key contract. These rows will now fail every delivery until manually reconfigured. Migrate or explicitly support decrypting and re-encrypting the legacy format during upgrade.
As per coding guidelines, “When adding features, identify handler/model boundaries and preserve API stability.”
Also applies to: 558-564
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/services/webhook.rs` around lines 284 - 286, Update the
webhook delivery decryption flow around decrypt_headers_checked to support
previously persisted ciphertext without the v1: prefix using the former key
contract. When legacy decryption succeeds, re-encrypt and persist the headers in
the current format so subsequent deliveries use the new contract. Preserve the
existing error for values that cannot be decrypted and keep the webhook API
behavior stable.
Source: Coding guidelines
Config::load now takes &PersistedSecrets as a parameter instead of loading secrets internally. Secrets reset now loads config with fresh secrets before replacing, invalidates credentials first, then replaces secrets atomically.
Detect provider switching (e.g. filesystem to S3), not just S3-to-S3. Include region in change comparison. S3 probe continues cleanup even if read fails. Clear credential fields on successful save. Add RestoreReport type for restore endpoint responses.
Only reset storage providers and switch to filesystem destination on whole-instance restore, not site restore. Update integration tests to assert RecoveryReport (200 + recovery_required array) instead of 204.
Consolidate per-backend invalidate_credentials into a macro_rules! with transactions. CLI backup/restore now loads instance settings and applies to config. Better secrets-reset warning about backup encryption. Fix em-dash encoding in CLI help. Clean env vars in stdio proxy tests.
- diagnostics: use floor_char_boundary for safe string truncation - paths: always apply permissions/ACLs on directory creation - service/status: handle macOS metadata errors gracefully - router: apply dynamic runtime policy to MCP routes - server: validate security config after settings load, conditional MCP line in startup banner - services/settings: validate settings on load from database
Add search_index_path to test config. Mirror server startup by spawning the single-writer search index consumer so gRPC tests can exercise full-text search.
Use poll-based assertions for search tests since indexing is asynchronous. Add test_search_indexes_are_isolated_across_concurrent_contexts to verify Tantivy indexes don't leak between isolated test contexts.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/backend/tests/grpc/entries_tests.rs (1)
134-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated poll-until-searchable loop into a shared helper.
Both
test_list_entries_with_searchandtest_search_indexes_are_isolated_across_concurrent_contextsduplicate the same "retry up to 50 times, sleep 20ms, break on match" pattern with hardcoded magic numbers. Consider factoring this into a small shared helper (e.g. in this file ortests/common) that takes the search term/expected slug and returns the matched response, to avoid drift if the retry budget needs tuning later.Separately, the comment on lines 204-206 justifying
"run"as proof of Tantivy stemming ("SQL LIKE cannot match 'run' against the stored word 'Running'") isn't quite accurate — SQLite'sLIKEis case-insensitive for ASCII by default, so%run%would match"Running"via plain substring/prefix matching regardless of stemming. This doesn't invalidate the isolation assertion itself, just the stated rationale.♻️ Example helper extraction
async fn poll_until_slug( client: &mut EntryServiceClient<Channel>, collection_id: String, search: &str, expected_slug: &str, ) -> Option<ListEntriesResponse> { for _ in 0..50 { let resp = client .list_entries(tonic::Request::new(ListEntriesRequest { collection_id: Some(collection_id.clone()), status: None, search: Some(search.into()), page: 1, per_page: 10, })) .await .unwrap() .into_inner(); if resp.items.iter().any(|item| item.slug == expected_slug) { return Some(resp); } tokio::time::sleep(std::time::Duration::from_millis(20)).await; } None }Also applies to: 165-227
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/tests/grpc/entries_tests.rs` around lines 134 - 155, The duplicated polling logic in test_list_entries_with_search and test_search_indexes_are_isolated_across_concurrent_contexts should use a shared async helper that accepts the client, collection ID, search term, and expected slug, centralizing the 50-attempt/20ms retry behavior and returning the matched response. Replace both inline loops with this helper, and revise the “run” Tantivy-stemming comment to avoid claiming SQLite LIKE cannot match “Running” as a substring.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/tests/common/grpc.rs`:
- Around line 85-96: The gRPC test indexer spawned in the service setup is
detached and survives GrpcTestContext teardown. Add a JoinHandle field to
GrpcTestContext, assign it from the tokio::spawn call in the search/indexer
setup, and abort or otherwise shut it down from the context’s Drop
implementation alongside the existing server cleanup.
---
Nitpick comments:
In `@apps/backend/tests/grpc/entries_tests.rs`:
- Around line 134-155: The duplicated polling logic in
test_list_entries_with_search and
test_search_indexes_are_isolated_across_concurrent_contexts should use a shared
async helper that accepts the client, collection ID, search term, and expected
slug, centralizing the 50-attempt/20ms retry behavior and returning the matched
response. Replace both inline loops with this helper, and revise the “run”
Tantivy-stemming comment to avoid claiming SQLite LIKE cannot match “Running” as
a substring.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 037abd8a-0c2c-47a1-b8ac-840cb30cac40
📒 Files selected for processing (7)
apps/backend/src/service/windows.rsapps/backend/src/services/backup/schema.rsapps/backend/src/services/settings.rsapps/backend/src/services/webhook.rsapps/backend/tests/common/grpc.rsapps/backend/tests/grpc/entries_tests.rsapps/backend/tests/rest/files_tests.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/backend/src/service/windows.rs
- apps/backend/src/services/backup/schema.rs
- apps/backend/src/services/settings.rs
- apps/backend/src/services/webhook.rs
Replace the non-atomic copy+delete with ReplaceFileW via windows-sys to ensure crash-safe atomic replacement of secrets.toml on Windows.
…conflicts Add a write_lock mutex and compare-and-swap pattern in publish(): the caller must supply the expected current snapshot, and the method verifies it matches the persisted row before writing. This prevents two concurrent admin UI saves from silently overwriting each other.
…ll chain Update all publish() call sites to capture the current settings and credentials snapshot before mutating, then pass both expected and new values to the new optimistic-locking publish API.
…n wrapper Wrap the raw StorageProvider with a BackupDestination that bundles a serializable spec (filesystem path or S3 credentials) alongside the provider. Destination references stored in the database are now encrypted dest:v1: locators that embed the spec, so staged uploads and recorded backups remain restorable even after the runtime destination changes. Also adds validate_restored_settings() to reject instance restores with invalid settings before committing.
…g constant Inject SettingsService into upload handlers to read the live upload_limit_mb from instance settings. The RequestBodyLimitLayer middleware is removed since the limit is now enforced at the handler level after settings are loaded. The StreamingUploadRequest gains a max_bytes field so the signed upload path can use the same budget as the multipart path.
Remove the hardcoded vcms_data/search default so search is disabled when no explicit search_index_path is configured, rather than silently falling back to an implicit path.
Extract the async polling loop into wait_for_search() to reduce duplication between test_search_indexes_are_isolated_across_concurrent_contexts and test_list_entries_with_search. Also store the indexer JoinHandle in GrpcTestContext with a Drop impl so the indexer task is properly aborted when the context is dropped.
Verifies that reducing upload_limit_mb via the settings API immediately takes effect for signed chunked uploads, not just multipart.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/tests/common/grpc.rs (1)
123-125: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMirror production settings injection and configuration application.
The gRPC test context currently loads instance settings too late, fails to apply them to the
Config, and skips injecting them intoauth,webhook, andbackup_service. This deviates from the production startup (inserver.rs) and will cause test panics or divergence when gRPC tests attempt to exercise settings-dependent logic.Move
SettingsService::loadup (immediately after pool initialization), apply it toconfig, and then inject it into the downstream services just likeserver.rsdoes.🛠️ Proposed fix to align test context with production startup
First, update the
Configbinding to be mutable and move the settings loading up (around line 79):- let config = Config { + let mut config = Config { database_url: "sqlite::memory:".to_string(),let pool = init_db_with_config(&config) .await .expect("Failed to initialize test database"); + + let settings = cms::services::settings::SettingsService::load(pool.clone(), &"11".repeat(32)) + .await + .expect("Failed to init instance settings"); + settings.apply_to_config(&mut config).await; let repository = Repository::new(&pool);Then, ensure
servicesis mutable and settings are injected intoauthandwebhook(around line 92):let config = Arc::new(config); let repository_arc = Arc::new(repository.clone()); - let services = Services::new(repository_arc.clone(), &pool, &config); + let mut services = Services::new(repository_arc.clone(), &pool, &config); + services.auth = Arc::new((*services.auth).clone().with_settings(settings.clone())); + services.webhook = Arc::new((*services.webhook).clone().with_settings(settings.clone()));Finally, inject settings into
BackupServicecreation and remove the late settings load (around line 117):let backup_service = Arc::new(cms::services::backup::BackupService::new( pool.clone(), storage_registry.clone(), backup_destination, &config, - )); - let settings = cms::services::settings::SettingsService::load(pool.clone(), &"11".repeat(32)) - .await - .expect("Failed to init instance settings"); + ).with_settings(settings.clone()));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/tests/common/grpc.rs` around lines 123 - 125, Align the gRPC test context setup with production startup: make the Config and services bindings mutable, load SettingsService immediately after pool initialization, apply the loaded settings to config, and inject them into auth, webhook, and BackupService construction. Remove the existing late SettingsService::load call while preserving the production initialization order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/backend/tests/common/grpc.rs`:
- Around line 123-125: Align the gRPC test context setup with production
startup: make the Config and services bindings mutable, load SettingsService
immediately after pool initialization, apply the loaded settings to config, and
inject them into auth, webhook, and BackupService construction. Remove the
existing late SettingsService::load call while preserving the production
initialization order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 29bd9ddb-642b-49bd-b642-7a3f0014789b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
apps/backend/Cargo.tomlapps/backend/src/handlers/file_handler.rsapps/backend/src/handlers/settings_handler.rsapps/backend/src/router/files.rsapps/backend/src/secrets.rsapps/backend/src/services/backup/mod.rsapps/backend/src/services/file.rsapps/backend/src/services/mod.rsapps/backend/src/services/settings.rsapps/backend/tests/common/grpc.rsapps/backend/tests/grpc/entries_tests.rsapps/backend/tests/rest/files_tests.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/backend/tests/grpc/entries_tests.rs
- apps/backend/src/handlers/settings_handler.rs
- apps/backend/src/secrets.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
recovery_required), and REST restore responses now succeed with a 200 + payload.service run.enabledsetting.config.toml+secrets.tomlmodel and new portable/installed behavior.