Implement personal access tokens, storage profiles, and deployments management#35
Implement personal access tokens, storage profiles, and deployments management#35krishna-santosh wants to merge 16 commits into
Conversation
New migration creating tables for personal access tokens, deployments (triggers/jobs), and storage profiles across all three backends.
Replace simple read/write permission model with fine-grained TokenScope enum supporting site, content, files, schema, webhooks, deployments, and MCP scopes. Add PersonalAccessToken types, encode/decode helpers, and backward-compatible 'read'/'write' aliases. Update authorization model with new Action variants (WebhooksTrigger, Deployments*) and token_hard_denied helper. Extend repository traits with personal token storage interface and implement across all three backends.
…ment Introduce PersonalTokenActor with scope-based auth enforcement. Support vcms_pat_ token prefix in API auth middleware. Resolve personal token site context via X-VCMS-Site header. Add PAT CRUD (create/list/revoke) service, handler, and /account/tokens routes. Update GraphQL context to pass requested site and scopes. Provide in-memory PAT repository stubs for testing.
Register deployment and storage_profile modules in lib.rs and services. Add deployment and storage profile routes in router. Wire storage profile registration and deployment reconcilation at server startup. Add storage_profile_id to CLI backup create options.
Introduce StorageProfile model, service (CRUD + probe), and HTTP handler. Add storage_profile_id optional column to Site model and include it in all site queries across backends. Assign storage profile on site creation. Expose profile ID in site listing responses. Wire storage profile CRUD and probe routes under /instance/storage-profiles. Update file storage resolution to COALESCE storage profile over legacy storage_provider.
Add DestinationSpec::Profile variant for routing backups through configured storage profiles. Include storage_profile_id in backup and schedule creation payloads. Persist profile ID in backup metadata tables.
Add DeploymentTrigger and DeploymentJob models, service with reconcile_interrupted for crash recovery, HTTP handler, and routes under /sites/{site_id}/deployments. Extend WebhookService with protect/reveal_deployment_config helpers for encrypting deployment URL and headers.
Add list_sites tool returning sites accessible to the authenticated user. Enforce that all site-scoped MCP tools require an explicit site_id parameter (except list_sites). Clean up whitespace (CRLF→LF) and update test assertions to match.
Add API functions and TypeScript types for scoped site tokens, personal access tokens (PAT), storage profiles (CRUD + probe), deployments (triggers, jobs, history), and storage_profile_id on backups and sites.
Add PersonalTokensCard component for creating, listing, and revoking personal access tokens. Integrate into the account page.
…ge profiles Rebuild storage settings page with storage profile CRUD (create, list, probe, delete). Add storage profile picker to the create-site dialog. Require operator role instead of instance_owner only.
…authorization service Pass X-VCMS-Site header from GraphQL router to GqlContext for personal token site resolution. Update AuthorizationService to use scope_for_action and PersonalTokenActor for scope-based enforcement.
…backup selector, and update routing Add deployments management page under site settings. Add MCP configuration page. Add storage profile selector to backup creation form. Update sidebar with new navigation items. Regenerate route tree. Pass role prop to GeneralSection. Update site settings layout and routing.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis change adds scoped personal access tokens, configurable storage profiles, deployment trigger execution, and multi-site MCP support across database migrations, backend services and routes, authentication, backup flows, and dashboard interfaces. ChangesScoped token authentication
Storage profile integration
Deployment automation
Multi-site MCP access
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: 3
Note
Due to the large number of review comments, Critical 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 (1)
apps/backend/src/handlers/site_handler.rs (1)
69-84: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSite is left orphaned if
assign_sitefails after creation.
create_sitecreates the site, then separately assigns the storage profile. Ifassign_sitefails, the handler returns a bare 400 error — the site row already exists (with the defaultpayload.storage_provider, not the requested profile) but the client has no indication of that or a reference to clean it up.♻️ Suggested direction
Either delete the just-created site on assignment failure, or include the created site's id in the error response so the caller can reconcile/retry the assignment instead of re-creating the site.
🤖 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/site_handler.rs` around lines 69 - 84, Update the error branch in the site creation handler around create_site and assign_site so a failed storage profile assignment does not leave the client without reconciliation information: either delete the newly created site before returning the error, or include the created site’s id in the error response. Preserve the successful response and existing assignment behavior.
🟠 Major comments (18)
apps/dashboard/src/components/site-settings/general-section.tsx-41-45 (1)
41-45: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve owner/admin roles and separate management capabilities.
apps/dashboard/src/components/site-settings/use-site-role.ts:12-21maps every role other than"editor"to"viewer"and setscanManageonly for instance operators. The new branch therefore displays site owners/admins as viewers and removes their allowed site-management UI. Widen the role contract and use separate capability flags so operator-only actions, such as site deletion, remain restricted.As per coding guidelines, instance roles and site roles must follow the documented RBAC model, including owner/admin powers, implicit site authority for instance operators, and operator-only access to schema/webhooks/API keys/member management/site deletion.
Also applies to: 102-127
🤖 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/dashboard/src/components/site-settings/general-section.tsx` around lines 41 - 45, Update the site-role flow centered on use-site-role.ts and the GeneralSection role contract to preserve owner and admin roles instead of mapping them to viewer. Widen the role type to include the documented RBAC roles, derive site-management capability separately from operator-only capabilities, and ensure instance operators retain implicit site authority while schema, webhooks, API keys, member management, and site deletion remain restricted to operators.Source: Coding guidelines
apps/backend/src/mcp/resources/site_schema.rs-55-58 (1)
55-58: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce resource-specific scopes.
Line 55 checks only
SiteRead, so a token lackingschema.readorcontent.readcan enumerate these resources;read_resourcerepeats that same check and allows direct reads. Filter listed resources by their required action and authorize/schemaand collections withSchemaRead, and singletons withContentReadbefore loading them.🤖 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/mcp/resources/site_schema.rs` around lines 55 - 58, Update the resource listing and read paths around the existing SiteRead authorization and read_resource flow to enforce resource-specific actions: filter enumerated resources using SchemaRead for /schema and ContentRead for collections or singletons, and require SchemaRead or ContentRead respectively before loading direct resources. Retain SiteRead as the general site-level check, but do not allow it alone to list or read protected resources.apps/backend/src/mcp/server.rs-51-57 (1)
51-57: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFilter site discovery by
site.read.Line 54 returns every membership for a personal token after only the
mcp.usemiddleware check. A token withoutsite.readcan therefore enumerate site IDs and names. Filter each result throughself.authorizer.require_site_action(&actor, site_id, Action::SiteRead)before serializing it.Suggested fix
+use crate::models::authorization::Action; + async fn list_sites(&self, ctx: RequestContext<RoleServer>) -> Result<CallToolResult, McpError> { let actor = self.resolve_actor(&ctx)?; - match self.services.site.list_sites_for_actor(&actor).await { - Ok(v) => crate::mcp::auth::ok_result(&v), + match self.services.site.list_sites_for_actor(&actor).await { + Ok(sites) => { + let mut visible_sites = Vec::new(); + for site in sites { + let Some(site_id) = site.get("id").and_then(|value| value.as_str()) else { + continue; + }; + if self.authorizer.require_site_action(&actor, site_id, Action::SiteRead).await.is_ok() { + visible_sites.push(site); + } + } + crate::mcp::auth::ok_result(&visible_sites) + } Err(e) => Err(crate::mcp::auth::map_err(e)), } }🤖 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/mcp/server.rs` around lines 51 - 57, Update list_sites to enforce Action::SiteRead for each site returned by list_sites_for_actor before serialization. Use self.authorizer.require_site_action with the resolved actor and each site ID, retain only authorized sites, and pass the filtered results to ok_result while preserving existing error mapping.apps/backend/src/handlers/storage_profile_handler.rs-43-48 (1)
43-48: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake profile persistence and registry replacement atomic.
A failed
register_allreturns an error after the database mutation has already succeeded. Creation leaves an unregistered profile persisted; update first removes the working provider, so failure leaves the persisted update without a usable registry entry.
apps/backend/src/handlers/storage_profile_handler.rs#L43-L48: roll back the newly created profile, or atomically validate/register it before reporting success.apps/backend/src/handlers/storage_profile_handler.rs#L80-L86: retain or restore the previous provider and persisted profile when replacement registration fails.🤖 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/storage_profile_handler.rs` around lines 43 - 48, Make storage profile persistence and registry replacement atomic in the create flow around storage_profile.create and register_all: validate/register before committing, or roll back the newly persisted profile if registration fails. In the update flow around the replacement logic at lines 80-86, retain or restore the previous provider and persisted profile when register_all fails, so failed replacements leave the original working state intact.apps/backend/src/server.rs-73-80 (1)
73-80: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA single bad storage profile blocks the entire server from starting.
register_allpropagates the firstbuild_providererror via?for every enabled S3 profile (services/storage_profile.rslines 211-224), and here that error abortsrun()entirely via.map_err(...)?. One profile with stale/undecryptable credentials or an unreachable endpoint prevents the whole instance from booting — even for sites/backups unrelated to that profile. Contrast withinitialize_storage()just above, which warns and continues on provider init failure, and withreconcile_interrupted()right below, which logs and continues rather than failing hard.♻️ Suggested direction
Have
register_allcollect and log per-profile failures instead of short-circuiting the whole loop, so one broken profile doesn't prevent unrelated storage from being registered/the server from starting.🤖 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 73 - 80, Update StorageProfile::register_all so it handles each profile’s build_provider failure independently: log the failing profile and error, then continue registering the remaining profiles instead of propagating the first error. Adjust the server startup call in run to avoid aborting on register_all failure while preserving startup and the existing reconciliation flow.apps/backend/src/services/storage_profile.rs-22-27 (1)
22-27: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winStorage-profile credentials reuse the webhook encryption key.
StorageProfileService::newderives its AES key fromconfig.webhook_encryption_key(perservices/mod.rswiring:storage_profile::StorageProfileService::new(pool.clone(), &config.webhook_encryption_key)), the same secret that seedsWebhookService's encryption key. Reusing one secret to derive keys for two independent encryption domains removes cryptographic separation between them — compromise or rotation of one affects the other.🤖 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/storage_profile.rs` around lines 22 - 27, Update StorageProfileService::new and its services/mod.rs wiring to accept and derive from a dedicated storage-profile encryption secret instead of config.webhook_encryption_key. Add or use the corresponding configuration key, pass it only to StorageProfileService, and keep WebhookService on its existing webhook-specific secret.apps/backend/src/services/storage_profile.rs-112-138 (1)
112-138: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
reference_countignores completed backups, letting a profile with historical backups be deleted/disabled.The
backupssub-query only countsstatus IN ('pending','running'). A profile that has many completed backups but no active jobs, sites, or schedules will reportreference_count() == 0, sodelete()(line 139) and disabling inupdate()(line 65) both succeed. Once the profile is removed from the registry, restoring any of those completed backups later fails atresolve_destination(services/backup/mod.rs) becauseself.storage.get(&id)can't find the profile anymore — with no warning given at delete/disable time.♻️ Suggested direction
Count all backups referencing the profile regardless of status (or surface a distinct warning/blocking message when historical backups exist), e.g.:
-SELECT (SELECT COUNT(*) FROM sites WHERE storage_profile_id=?) + (SELECT COUNT(*) FROM backup_schedules WHERE storage_profile_id=?) + (SELECT COUNT(*) FROM backups WHERE storage_profile_id=? AND status IN ('pending','running')) +SELECT (SELECT COUNT(*) FROM sites WHERE storage_profile_id=?) + (SELECT COUNT(*) FROM backup_schedules WHERE storage_profile_id=?) + (SELECT COUNT(*) FROM backups WHERE storage_profile_id=?)🤖 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/storage_profile.rs` around lines 112 - 138, Update reference_count to count every backup whose storage_profile_id matches, removing the pending/running status filter in the SQLite, PostgreSQL, and MySQL subqueries. Preserve the existing counts for sites and backup_schedules and the delete/update callers’ handling of the returned total.apps/backend/src/services/deployment.rs-52-54 (1)
52-54: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRace on
is_primaryinvariant (clear-then-insert/update, not transactional).
clear_primaryand the subsequent insert/update are two separate statements with no transaction. Two concurrentcreate/updatecalls withis_primary=truecan both pass the clear step before either writes its row, leaving more than one primary trigger for the site.Also applies to: 91-93
🤖 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/deployment.rs` around lines 52 - 54, Make the primary-trigger mutation in the create/update flow atomic so concurrent calls cannot leave multiple primary rows for a site. Update the logic around clear_primary and the subsequent insert/update to execute as one transaction, preserving the is_primary behavior while ensuring the clear and write are serialized.apps/backend/src/services/deployment.rs-10-22 (1)
10-22: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
DeploymentServicebypasses the repository layer.
DeploymentServiceholdsDbPooldirectly and issues rawsqlxqueries throughout (reconcile_interrupted,list,create,update,delete,history,insert_job,load_secret,finish), unlikeWebhookServicein the same PR, which delegates all persistence to awebhook_repo: Arc<dyn WebhookRepository>. The repo's stated architecture puts data access inapps/backend/src/repository/.As per coding guidelines, "Data access code must live in
apps/backend/src/repository/."🤖 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/deployment.rs` around lines 10 - 22, Refactor DeploymentService so it no longer stores DbPool or performs direct sqlx queries in reconcile_interrupted and the other deployment persistence methods (list, create, update, delete, history, insert_job, load_secret, and finish). Introduce and inject an Arc-backed deployment repository from the repository layer, then delegate all data access through that repository while preserving the existing service API and behavior.Source: Coding guidelines
apps/backend/migrations/mysql/20260721000000_tokens_deployments_storage.sql-4-4 (1)
4-4: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMissing FK/CHECK constraints present in the Postgres/SQLite siblings.
storage_profiles.kindhas noCHECK(kind IN ('filesystem','s3'))here (Postgres/SQLite both enforce it), andsites.storage_profile_idis added without aFOREIGN KEY REFERENCES storage_profiles(id)(Postgres/SQLite add it inline). MySQL 8.0.16+ supportsCHECKconstraints, so both gaps are avoidable — leaving MySQL as the only backend where an invalidkindor an orphanedstorage_profile_idcan be persisted.🛡️ Proposed fix
-CREATE TABLE IF NOT EXISTS storage_profiles (id VARCHAR(36) PRIMARY KEY,name VARCHAR(255) NOT NULL UNIQUE,kind VARCHAR(20) NOT NULL,endpoint TEXT,region VARCHAR(255),bucket VARCHAR(255),public_url TEXT,credentials_encrypted TEXT,enabled BOOLEAN NOT NULL DEFAULT TRUE,immutable BOOLEAN NOT NULL DEFAULT FALSE,created_by VARCHAR(36),created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,FOREIGN KEY(created_by) REFERENCES users(id)); +CREATE TABLE IF NOT EXISTS storage_profiles (id VARCHAR(36) PRIMARY KEY,name VARCHAR(255) NOT NULL UNIQUE,kind VARCHAR(20) NOT NULL CHECK(kind IN ('filesystem','s3')),endpoint TEXT,region VARCHAR(255),bucket VARCHAR(255),public_url TEXT,credentials_encrypted TEXT,enabled BOOLEAN NOT NULL DEFAULT TRUE,immutable BOOLEAN NOT NULL DEFAULT FALSE,created_by VARCHAR(36),created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,FOREIGN KEY(created_by) REFERENCES users(id)); ... -ALTER TABLE sites ADD COLUMN storage_profile_id VARCHAR(36) NULL; +ALTER TABLE sites ADD COLUMN storage_profile_id VARCHAR(36) NULL; +ALTER TABLE sites ADD CONSTRAINT fk_sites_storage_profile FOREIGN KEY (storage_profile_id) REFERENCES storage_profiles(id);Also applies to: 6-6
🤖 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/migrations/mysql/20260721000000_tokens_deployments_storage.sql` at line 4, Update the MySQL migration’s storage_profiles definition to enforce kind as either filesystem or s3 with a CHECK constraint, and update the sites.storage_profile_id addition to include a foreign key referencing storage_profiles(id). Keep the constraints aligned with the corresponding Postgres and SQLite migrations.apps/backend/migrations/mysql/20260721000000_tokens_deployments_storage.sql-12-12 (1)
12-12: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNo DB-level guarantee of a single primary trigger per site in MySQL.
Postgres (
CREATE UNIQUE INDEX ... ON deployment_triggers(site_id) WHERE is_primary=TRUE) and SQLite enforce "only one primary trigger per site" at the schema level. MySQL has no equivalent partial/unique constraint here, so concurrentcreate/updatecalls settingis_primary=trueon this backend rely entirely on the service-layerclear_primarystep (a separate statement, not shown to be transactional with the following insert/update) — a TOCTOU window that could leave two primary triggers for the same site on MySQL only.MySQL doesn't support partial unique indexes, but a generated-column workaround achieves the same guarantee:
ALTER TABLE deployment_triggers ADD COLUMN primary_site_id VARCHAR(36) GENERATED ALWAYS AS (IF(is_primary, site_id, NULL)) STORED; CREATE UNIQUE INDEX idx_deployment_primary ON deployment_triggers(primary_site_id);🤖 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/migrations/mysql/20260721000000_tokens_deployments_storage.sql` at line 12, The MySQL deployment_triggers schema lacks a database-level guarantee of one primary trigger per site. Update the deployment_triggers definition to add a stored generated primary_site_id column that contains site_id only when is_primary is true, then add a unique index on that column so concurrent create/update operations cannot create duplicate primaries.apps/backend/src/middleware/auth.rs-253-279 (1)
253-279: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winPersonal-token auth pays a bcrypt hash on every request — no HMAC fast path. The site-API-key path checks
token_hmacfirst, falling back to bcrypt only for legacy rows; the personal-token path always callsbcrypt::verifybecause the lookup row never carriestoken_hmac, even though the schema andNewPersonalTokenstore it.
apps/backend/src/middleware/auth.rs#L253-L279: extend the personal-token branch to comparecompute_key_hmac(token, hmac_secret)against a returnedtoken_hmacfirst, matching theApiKeybranch's pattern, falling back to bcrypt only when hmac is absent.apps/backend/src/repository/mysql/access_token.rs#L94-L120: addtoken_hmacto thefind_personal_by_prefixSELECTlist (and update the sharedPersonalTokenLookupRowtuple type plus the Postgres/SQLite equivalents accordingly).🤖 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/auth.rs` around lines 253 - 279, Update the personal-token authentication branch around the personal-token lookup to compare compute_key_hmac(token, hmac_secret) with the returned token_hmac before verifying with bcrypt, falling back to bcrypt only when token_hmac is absent; preserve rejection for mismatches and existing validation behavior. Extend find_personal_by_prefix and the shared PersonalTokenLookupRow tuple, including the Postgres and SQLite implementations, to select and return token_hmac. Affected sites: apps/backend/src/middleware/auth.rs:253-279 requires the HMAC-first authentication change; apps/backend/src/repository/mysql/access_token.rs:94-120 requires adding token_hmac to the SELECT and lookup tuple, with equivalent updates in the Postgres and SQLite implementations.apps/backend/src/repository/postgres/access_token.rs-94-120 (1)
94-120: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
find_personal_by_prefixdoesn't projecttoken_hmac.Unlike the site-token
find_by_prefix(which selectstoken_hmac), this query only selectsid,user_id,token_hash,expires_at,revoked_at,scopes_json,last_used_at. Thepersonal_access_tokensrow does have atoken_hmac(written bycreate_personalabove), it's just never read back here. See the consolidated note anchored atapps/backend/src/repository/traits.rslines 292-312.🤖 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/repository/postgres/access_token.rs` around lines 94 - 120, Update find_personal_by_prefix to include token_hmac in its SELECT projection, matching the PersonalTokenLookupRow fields and the site-token find_by_prefix query. Preserve the existing filters and returned columns while adding the persisted HMAC value.apps/backend/src/repository/sqlite/access_token.rs-94-113 (1)
94-113: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
find_personal_by_prefixomitstoken_hmac, same aspostgres/access_token.rs.See consolidated note anchored at
apps/backend/src/repository/traits.rslines 292-312.🤖 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/repository/sqlite/access_token.rs` around lines 94 - 113, Update find_personal_by_prefix and its PersonalTokenLookupRow query mapping to select and return token_hmac alongside token_hash, matching the corresponding PostgreSQL implementation and the repository trait contract.apps/backend/src/repository/sqlite/access_token.rs-47-56 (1)
47-56: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSame legacy-
permissionderivation issue aspostgres/access_token.rs.Identical
.contains(".write")heuristic defaults an empty scope set to"read"for the legacy gRPC-consumedpermissioncolumn. See consolidated note.🤖 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/repository/sqlite/access_token.rs` around lines 47 - 56, Update the access-token insertion logic in the SQLite repository to derive the legacy permission value from the token’s scopes rather than using permission.contains(".write"). Ensure empty scopes preserve the expected legacy gRPC permission behavior, while retaining the correct write/read mapping for applicable scoped tokens.apps/backend/src/models/access_token.rs-147-148 (1)
147-148: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winNo validation that
scopesis non-empty.
CreateSiteToken.scopesaccepts anyTokenScopes, including an empty set. Combined with the repository-layer.writesubstring heuristic (seepostgres/sqlite access_token.rs), a token created with zero scopes is persisted with a legacypermission = "read", granting unintended read access via the legacy gRPC auth path. Consider rejecting empty scope sets at creation time.🤖 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/models/access_token.rs` around lines 147 - 148, Validate that CreateSiteToken.scopes is non-empty before creating the token, and reject empty TokenScopes values with the existing validation/error mechanism. Keep valid scope handling unchanged and prevent empty scopes from reaching persistence or legacy authorization.apps/backend/src/repository/postgres/access_token.rs-47-56 (1)
47-56: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFragile legacy-
permissionderivation defaults to "read" for empty/no-write scope sets.
permissionhere is the JSON-encoded scopes string (encode_scopesoutput), and.contains(".write")is used to derive the legacy read/write column consumed by the gRPC interceptor (AccessTokenPermission::from_str). This works for the currently-defined*.writescope variants, but a token created with an empty scope set (not currently rejected anywhere — seemodels/access_token.rs::CreateSiteToken) serializes to"[]", which doesn't contain.write, so it's stored as"read"— granting unintended read access via the legacy gRPC path to a token that was meant to have zero scopes. This same logic is duplicated verbatim insqlite/access_token.rs.🤖 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/repository/postgres/access_token.rs` around lines 47 - 56, Replace the `.contains(".write")` fallback used to derive the legacy `permission` value in the Postgres access-token insert with logic that distinguishes write, non-empty read, and empty scope sets, ensuring empty scopes do not become `"read"` and grant legacy access. Apply the same change to the duplicated derivation in the SQLite access-token insert, reusing the existing scope representation or `encode_scopes` result consistently.apps/backend/src/handlers/access_token_handler.rs-71-115 (1)
71-115: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftCentralize personal-token mint policy
create_personal_tokenis still enforcing operator/scope rules locally instead of through the shared auth policy, so this mint-time RBAC can drift frommodels/authorization.rs. The current allowlist also blockswebhooks.triggerbut still lets non-operators mintdeployments.trigger; if that split is intentional, it should be encoded in the central policy, not here.🤖 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/access_token_handler.rs` around lines 71 - 115, Remove the local operator lookup and scope allowlist from create_personal_token, and delegate mint-time RBAC to the shared authorization policy in models/authorization.rs. Ensure the central policy explicitly defines the intended permissions for WebhooksTrigger and DeploymentsTrigger so this handler cannot drift from the shared rules.Source: Coding guidelines
🟡 Minor comments (10)
apps/dashboard/src/routes/_admin/_shell/index.tsx-251-262 (1)
251-262: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExclude disabled profiles from site creation.
The backend only assigns enabled profiles, but this selector offers every profile. Selecting a disabled profile makes site creation fail with
storage_profile_not_found.Proposed fix
- {storageProfiles.map((profile) => ( + {storageProfiles + .filter((profile) => profile.enabled) + .map((profile) => ( <SelectItem key={profile.id} value={profile.id}> <div className="flex items-center gap-2"> {profile.kind === "filesystem" ? ( @@ </div> </SelectItem> - ))} + ))}🤖 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/dashboard/src/routes/_admin/_shell/index.tsx` around lines 251 - 262, Filter storageProfiles to exclude disabled profiles before rendering the selector in the profile map, so site creation can only choose enabled profiles. Preserve the existing SelectItem rendering and profile display for eligible profiles.apps/dashboard/src/routes/_admin/sites.$siteId/settings.tsx-35-43 (1)
35-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the active General tab reachable for non-managers.
For non-managers, the base route still resolves to
active === "general"(Lines 20-22), but this change removes the onlyTabsTriggerwith that value. The read-only page can render without a selected tab, and users have no tab navigation back to General. Keep the trigger visible for read-only users, or redirect/fallback to a valid tab.🤖 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/dashboard/src/routes/_admin/sites`.$siteId/settings.tsx around lines 35 - 43, Update the General TabsTrigger visibility condition in the settings route so it remains rendered for non-managers and stays reachable when the base route resolves to active === "general". Preserve the existing canManage behavior and navigation target while ensuring read-only users retain a valid selected tab.apps/dashboard/src/routes/_admin/sites.$siteId/deployments.tsx-147-154 (1)
147-154: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winApply the editor/operator gate to the deployment action.
Direct navigation lets viewers invoke
triggerDeployment, whileapp-sidebar.tsxonly exposes Deploy to operators or editors. Reuse that same condition here to avoid an unauthorized action that only fails after clicking.Proposed fix
- const { canManage } = useSiteRole(siteId); + const { canManage, role } = useSiteRole(siteId); + const canDeploy = canManage || role === "editor"; ... - <Button - className="w-fit" - disabled={!item.enabled || trigger.isPending} - onClick={() => trigger.mutate(item.id)} - > - <Rocket data-icon="inline-start" /> - Deploy now - </Button> + {canDeploy ? ( + <Button + className="w-fit" + disabled={!item.enabled || trigger.isPending} + onClick={() => trigger.mutate(item.id)} + > + <Rocket data-icon="inline-start" /> + Deploy now + </Button> + ) : null}🤖 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/dashboard/src/routes/_admin/sites`.$siteId/deployments.tsx around lines 147 - 154, Update the deployment action in the deployments route so the Button is disabled unless the current user satisfies the same operator-or-editor permission condition used by app-sidebar.tsx. Preserve the existing item.enabled and trigger.isPending checks, and ensure unauthorized viewers cannot invoke trigger.mutate through this action.apps/dashboard/src/components/account/personal-tokens-card.tsx-114-126 (1)
114-126: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheckbox and its label aren't programmatically associated.
Neither
Checkboxgets anidnor doesFieldLabelget a matchinghtmlFor, so clicking the label text won't toggle the checkbox and screen readers won't announce the label for it.♿ Proposed fix
<Field key={scope} orientation="horizontal"> <Checkbox + id={`scope-${scope}`} checked={scopes.includes(scope)} onCheckedChange={(checked) => ... } /> - <FieldLabel>{scope}</FieldLabel> + <FieldLabel htmlFor={`scope-${scope}`}>{scope}</FieldLabel> </Field>🤖 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/dashboard/src/components/account/personal-tokens-card.tsx` around lines 114 - 126, Associate each scope checkbox with its label in the scopes rendering: assign a unique, stable id to the Checkbox and pass that same value as FieldLabel’s htmlFor. Keep the existing checked state and setScopes behavior unchanged.apps/dashboard/src/components/account/personal-tokens-card.tsx-186-195 (1)
186-195: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClipboard write isn't awaited/handled; success toast fires unconditionally.
navigator.clipboard.writeText(...)returns a promise that is neither awaited nor caught, sotoast.success("Token copied")runs even if the copy fails (e.g. permission denied, insecure context, older browser).🐛 Proposed fix
<Button size="icon" variant="outline" - onClick={() => { - navigator.clipboard.writeText(secret ?? ""); - toast.success("Token copied"); - }} + onClick={async () => { + try { + await navigator.clipboard.writeText(secret ?? ""); + toast.success("Token copied"); + } catch { + toast.error("Couldn't copy token"); + } + }} >🤖 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/dashboard/src/components/account/personal-tokens-card.tsx` around lines 186 - 195, Update the Button onClick handler in personal-tokens-card to await navigator.clipboard.writeText and handle rejected promises before showing feedback. Trigger the success toast only after a successful write, and add failure handling that reports the copy error appropriately without leaving the rejection unhandled.apps/dashboard/src/components/account/personal-tokens-card.tsx-158-165 (1)
158-165: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNo confirmation before revoking a token.
Clicking the trash icon immediately calls
revoke.mutate(token.id)with no confirm step. Revoking is irreversible and could break an in-use integration (CI, MCP client) with a single misclick.🛡️ Suggested fix: gate the destructive action behind a confirm dialog/AlertDialog
- <Button - size="icon" - variant="ghost" - aria-label={`Revoke ${token.name}`} - onClick={() => revoke.mutate(token.id)} - > - <Trash2 /> - </Button> + <Button + size="icon" + variant="ghost" + aria-label={`Revoke ${token.name}`} + onClick={() => { + if (window.confirm(`Revoke "${token.name}"? This cannot be undone.`)) { + revoke.mutate(token.id); + } + }} + > + <Trash2 /> + </Button>(Prefer the project's
AlertDialogcomponent overwindow.confirmfor consistency with the rest of the UI.)🤖 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/dashboard/src/components/account/personal-tokens-card.tsx` around lines 158 - 165, Update the revoke action in the personal tokens card to require confirmation through the project’s AlertDialog before invoking revoke.mutate(token.id). Keep the current revoke call as the confirmed action and ensure cancelling or dismissing the dialog does not revoke the token.apps/dashboard/src/components/account/personal-tokens-card.tsx-140-167 (1)
140-167: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShow revoked tokens as revoked or hide them from the list
list_personal_tokensreturnsrevoked_at, but this card ignores it, so revoked tokens can stay in the list looking active with a live revoke button. Either filter them out or render a revoked state and disable the action.🤖 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/dashboard/src/components/account/personal-tokens-card.tsx` around lines 140 - 167, Update the token rendering in the data map to handle token.revoked_at: either exclude revoked tokens from the list or visibly mark them as revoked and disable the revoke Button for those entries. Ensure revoked tokens cannot appear active or have an enabled revoke action.apps/backend/src/services/storage_profile.rs-35-54 (1)
35-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEmpty credentials aren't rejected at creation time.
create()validatesname/bucketnon-empty and the endpoint URL, but never checks thataccess_key_id/secret_access_keyare non-empty. An empty-credential profile will be created successfully and only fail later, confusingly, atprobe/build_providertime.💡 Proposed fix
pub async fn create(&self, v: CreateStorageProfile, by: &str) -> Result<StorageProfile, String> { if v.name.trim().is_empty() || v.bucket.trim().is_empty() { return Err("name_and_bucket_required".into()); } + if v.access_key_id.trim().is_empty() || v.secret_access_key.trim().is_empty() { + return Err("credentials_required".into()); + } url::Url::parse(&v.endpoint).map_err(|_| "invalid_endpoint")?;🤖 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/storage_profile.rs` around lines 35 - 54, Add validation in StorageProfileService::create for trimmed access_key_id and secret_access_key, returning the existing required-fields error before URL parsing, encryption, or database insertion when either credential is empty. Preserve the current validation and creation flow for complete credentials.apps/backend/src/services/deployment.rs-157-163 (1)
157-163: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDaily quota can be undercounted for high-volume triggers.
history()is capped atLIMIT 100, buttrigger()'s daily-quota check counts matches within that capped set (history.iter().filter(...).count()). If a trigger accumulates more than 100 jobs in a rolling day (an admin can setdaily_quotaabove 100, since only>= 0is validated), older-than-100 jobs within the last 24h are silently excluded from the count, allowing more triggers than the configured quota. Also notehistory()has nosite_idfilter — it currently relies on callers (e.g. thehistoryhandler) to pre-check trigger/site membership.Also applies to: 202-210
🤖 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/deployment.rs` around lines 157 - 163, The daily-quota calculation in trigger() must not rely on the 100-row cap of history(). Add a dedicated count/query for jobs matching the trigger within the rolling 24-hour window, scoped to the appropriate site, and use that count for quota enforcement so quotas above 100 are honored. Keep history()’s pagination behavior unchanged, and ensure the history handler’s trigger/site membership validation remains explicit.apps/backend/src/graphql/context.rs-58-63 (1)
58-63: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMisleading error when a token has scopes but lacks
ContentRead.When
permissionisSome(scopes)butscopesdoesn't containContentRead, the guard on the first match arm fails and control falls through to the catch-all_arm, returning"Site token authentication required"— the same message used for missing authentication — instead of a permission-denied message.require_writecorrectly distinguishes these two cases via itsSome(_) => Err("... does not have write permission")arm;require_readshould do the same.🐛 Proposed fix
pub fn require_read(&self) -> async_graphql::Result<()> { match (&self.actor, &self.permission) { - (Some(Actor::ApiKey(_) | Actor::PersonalToken(_)), Some(scopes)) - if scopes.contains(&TokenScope::ContentRead) => - { - Ok(()) - } + (Some(Actor::ApiKey(_) | Actor::PersonalToken(_)), Some(scopes)) => { + if scopes.contains(&TokenScope::ContentRead) { + Ok(()) + } else { + Err(async_graphql::Error::new( + "Access token does not have required permission", + )) + } + } (Some(Actor::ApiKey(_) | Actor::PersonalToken(_)), None) => Err(async_graphql::Error::new( "Access token does not have required permission", )), _ => Err(async_graphql::Error::new("Site token authentication required")), } }🤖 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/graphql/context.rs` around lines 58 - 63, Update require_read in the permission match to add a Some(scopes) case for API key or personal token actors whose scopes lack TokenScope::ContentRead, returning a permission-denied error analogous to require_write. Keep the existing success path for scopes containing ContentRead and the missing-scope authentication error unchanged.
🧹 Nitpick comments (12)
apps/dashboard/src/routeTree.gen.ts (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegenerate this file using the repository quote style.
These new imports use single quotes, contrary to the dashboard’s required Biome double-quote convention. Configure the route-tree generator or its post-generation formatting so regeneration does not reintroduce the mismatch.
As per coding guidelines, “Frontend formatting should follow Biome conventions: 2-space indent, double quotes, and organized imports.”
Also applies to: 35-35
🤖 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/dashboard/src/routeTree.gen.ts` at line 25, Update the route-tree generation or post-generation formatting workflow so imports emitted in routeTree.gen.ts use Biome’s double-quote style, including the imports represented by AdminSitesSiteIdDeploymentsRouteImport and the additional affected import. Regenerate the file and ensure organized imports and existing frontend formatting conventions are preserved.Source: Coding guidelines
apps/dashboard/src/components/account/personal-tokens-card.tsx (2)
70-73: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNo loading/error state for the token list query.
Unlike the sessions query in
account.tsx(which shows aSkeletonand error handling), this query silently shows an empty list on failure/first load, with notoast.erroror loading indicator.🤖 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/dashboard/src/components/account/personal-tokens-card.tsx` around lines 70 - 73, Update the personal-tokens query in the component using useQuery to track loading and error states instead of defaulting failures to an empty list; render a Skeleton while loading and call toast.error with the query error when it fails, matching the sessions handling in account.tsx while preserving the successful token list rendering.
75-75: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNo UI control to set a token expiration; always creates non-expiring tokens.
expires_atis hardcoded tonulleven though the type/API supports an expiry. Personal access tokens with no way to opt into expiration is a weaker default for a credential meant for MCP/CLI/API use.🤖 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/dashboard/src/components/account/personal-tokens-card.tsx` at line 75, Update the personal-token creation flow around the mutationFn to expose a UI control for selecting an expiration and pass the chosen value as expires_at instead of always sending null. Preserve the existing token name and scopes handling, and ensure the selected option can still represent a non-expiring token when explicitly chosen.apps/dashboard/src/lib/api.ts (1)
1063-1072: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeployment/storage-profile write helpers use
value: unknownfor request bodies.
createDeployment,updateDeployment,createStorageProfile, andupdateStorageProfileall acceptunknownand justJSON.stringifyit, losing compile-time validation of the payload shape at all call sites.Also applies to: 1093-1102
🤖 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/dashboard/src/lib/api.ts` around lines 1063 - 1072, Replace the value: unknown parameters in createDeployment, updateDeployment, createStorageProfile, and updateStorageProfile with the established request payload types for deployment and storage-profile writes. Keep JSON serialization unchanged, and ensure all call sites pass the corresponding typed payload so compile-time shape validation is enforced.apps/backend/src/services/site.rs (1)
86-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFormat the API-key response construction.
Line 86 exceeds the repository’s 120-column rustfmt width; expand this mapping into a formatted closure or helper.
As per coding guidelines, Rust code should use the repository toolchain settings, including rustfmt width 120 and 4-space indentation.
🤖 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/site.rs` at line 86, Reformat the Actor::ApiKey response mapping in the get_site flow so the JSON construction is expanded across multiple lines and remains within the repository’s 120-column rustfmt width, using 4-space indentation. Preserve the existing fields and "site_key" role value.Source: Coding guidelines
apps/backend/src/handlers/storage_profile_handler.rs (1)
19-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
AppError-basedResulthandlers.These handlers manually construct error responses on every path instead of using the backend’s shared HTTP-error contract.
As per coding guidelines, Rust backend code should use idiomatic
Result/error handling, Axum extractors, and the customAppErrorenum for HTTP errors.🤖 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/storage_profile_handler.rs` around lines 19 - 108, Refactor the storage profile handlers list, create, delete, update, and probe to return the backend’s AppError-based Result response type instead of manually constructing error responses. Propagate authorization and service failures with the appropriate AppError variants, preserve each handler’s existing success status and JSON payloads, and keep Axum extractors unchanged.Source: Coding guidelines
apps/backend/src/handlers/backup_handler.rs (1)
263-301: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSchedules aren't validated against an existing/enabled storage profile at create/update time.
Unlike the instant-backup path (
create_backup, which surfaces an invalidstorage_profile_idimmediately viarun_backup),create_schedule/update_schedulepersistbody.storage_profile_idwithout checking it exists and is enabled. An invalid/deleted id silently sits on the schedule until the next scheduled run fails (logged only viatracing::error!inscheduler.rs).Also applies to: 313-347
🤖 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 263 - 301, Validate body.storage_profile_id in both create_schedule and update_schedule before persisting the schedule, ensuring the referenced storage profile exists and is enabled; return the same appropriate error used by the instant-backup path when validation fails. Preserve schedules without a storage profile and only call meta::create_schedule or the corresponding update operation after validation succeeds.apps/backend/src/services/deployment.rs (1)
60-66: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRe-fetch entire list/history to locate the just-written row.
create,update, andinsert_jobeach re-runlist()/history()(fetching all triggers for the site, or up to 100 jobs) and then.find()client-side just to return the row that was just written, instead of selecting byiddirectly. This adds an avoidable full round trip per write and scales with the number of triggers/jobs.Also applies to: 103-108, 220-262
🤖 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/deployment.rs` around lines 60 - 66, Update the write methods around create, update, and insert_job to return the newly written row by selecting it directly with its known id, rather than calling list() or history() and searching with find(). Preserve the existing not-found error behavior where applicable while avoiding the full collection/history fetch.apps/backend/migrations/postgres/20260721000000_tokens_deployments_storage.sql (1)
6-6: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider
NOT VALID+ separateVALIDATE CONSTRAINTfor the new FKs on potentially large tables.Static analysis flags that adding these foreign keys inline with
ADD COLUMNtakes aSHARE ROW EXCLUSIVElock blocking writes onsites/backups/backup_scheduleswhile a table scan runs. For production tables of meaningful size, splitting intoADD COLUMN→ADD CONSTRAINT ... NOT VALID→VALIDATE CONSTRAINT(in a separate transaction) avoids the write-blocking window.Also applies to: 8-8, 9-9
🤖 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/migrations/postgres/20260721000000_tokens_deployments_storage.sql` at line 6, Update the foreign-key additions for sites.storage_profile_id, backups.storage_profile_id, and backup_schedules.storage_profile_id to add the columns without inline REFERENCES, then create separately named foreign-key constraints with NOT VALID and validate each constraint in a separate transaction or migration step. Preserve the existing storage_profiles(id) relationship while avoiding the write-blocking inline table scan.Source: Linters/SAST tools
apps/backend/src/models/authorization.rs (1)
174-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove
allows_api_key. Nothing in this codebase calls it, so it can drift from the new scope rules without providing any benefit.🤖 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/models/authorization.rs` around lines 174 - 189, Remove the unused allows_api_key function from the authorization model, along with any imports or related code that become unnecessary after its deletion; leave the surrounding authorization scope rules unchanged.apps/backend/src/services/access_token.rs (1)
130-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo unit tests for the new personal-token service methods.
list_personal_tokens,create_personal_token, andrevoke_personal_tokenare new, security-relevant functionality with no accompanying tests in this module'stestssubmodule (only site-token tests were updated). This is compounded byInMemoryAccessTokenRepository's personal-token methods being no-op stubs (seetest_helpers.rs), which would need fixing first to make such tests meaningful.🤖 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/access_token.rs` around lines 130 - 200, The new personal-token service methods lack meaningful test coverage because InMemoryAccessTokenRepository personal-token operations are no-op stubs. Implement the repository test-helper methods first, then add tests for list_personal_tokens, create_personal_token, and revoke_personal_token covering successful behavior and relevant validation/error paths, following the existing site-token tests’ structure.apps/backend/src/test_helpers.rs (1)
1197-1220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPersonal-token stubs don't actually store anything.
list_personal,create_personal,revoke_personal,find_personal_by_prefix, andtouch_personalare all no-op (empty vec /Ok(0)/Ok(())), unlike the siblinglist/create/delete/find_by_prefixmethods just above, which back ontoself.tokens. Any test exercisingAccessTokenService's personal-token flows againstInMemoryAccessTokenRepositorywould silently pass with a fake empty result set rather than genuinely testing create → list → revoke round-trips.🤖 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/test_helpers.rs` around lines 1197 - 1220, Implement the personal-token methods in InMemoryAccessTokenRepository using self.tokens, matching the behavior of the sibling list/create/delete/find_by_prefix methods above. Update list_personal, create_personal, revoke_personal, find_personal_by_prefix, and touch_personal to persist, query, mutate, and return personal-token data so create → list → revoke round-trips are exercised rather than returning no-op results.
…ment jobs Add partial unique index on deployment_jobs to enforce at most one queued or running job per trigger. MySQL uses a generated column approach, Postgres and SQLite use filtered unique indexes.
…oyment service Extract build_protected_client from deliver_to_url as a reusable method for deployments. Refactor deployment execute_job to use it. Add map_job_insert_error returning 'deployment_in_progress' on unique violation and a unit test for active job uniqueness.
…ns in GraphQL When a personal token supplies X-VCMS-Site, verify the user has SiteRead access before trusting the header, rather than blindly accepting the requested site.
1 participant
Summary by CodeRabbit