feat(secrets): non-secret workspace variables - #6388
Conversation
Workspace secrets are redacted everywhere: masked from non-admins, scrubbed
out of traces by the resolved-secret projection, logged as ciphertext, and
exposed to Sim by name only. That is right for API keys and wrong for the
config values teams also keep there - a support email, a region, a base URL.
A variable is a workspace secret with its disclosure policy flipped. Same
table, same encryption at rest, same `{{NAME}}` namespace and uniqueness
guarantee; a per-key `credential.env_visibility` flag turns off redaction.
Read-side only. Write authorization is deliberately identical for both kinds:
non-secret and non-sensitive are different axes, and a member who cannot read
a secret must still not be able to repoint an `API_BASE_URL` that secret is
sent to.
The provenance exemption is the security-critical part. Resolvers call
recordResolved for every substitution and cannot tell a variable from a
secret, so a known non-secret name returns early - before the catalog lookup,
so a decryption failure cannot reroute it - instead of tripping the permanent
markIncomplete that would collapse the run's traces and omit every later
copilot tool result. The exempt set is a required constructor parameter: a
missing set over-redacts (safe), a wrong one under-redacts (not), so every
construction site is forced to name its source or fail to compile. It is
derived locally on each side of every boundary and never carried in
provenance, so no peer can assert that one of your secrets is non-secret.
Sim reads values from environment/variables.json under `nonSecretValues`,
filtered by the same mount policy that already gates name discovery.
Also fixes, found while wiring this up: fetchWorkspaceEnvironment and two
React Query `select` callbacks rebuilt the response field-by-field and would
have dropped the new field (the selects also broke structural sharing); a
visibility-only PUT hit the empty-body early return before the flip handler.
Sim's half of the mothership change. Regenerates the contracts, populates the
new snapshot kind, and lets the agent create non-secret variables.
nonSecretEnvVars carries {name, value}, so WORKSPACE.md now renders
`- NAME = value` for non-secret env vars and a bare `- NAME` for secrets,
inside the one Environment Variables section rather than under a second
heading that would give the model a third thing called a "variable".
That needs values here, so buildWorkspaceMdData now resolves the environment
snapshot alongside the credential rows. It sits behind the 2s effective-env
LRU, so it usually shares the decrypt the VFS materialization already did for
the same turn. Values are filtered by the same mount policy that gates name
discovery: a workspace that restricts what the agent can see must not have
that widened just because a key is non-secret.
set_environment_variables maps `kind` onto the visibility map, defaulting to
secret on anything unrecognized. It applies to new names only - upsert never
flips an existing key - so the agent cannot disclose a secret in passing.
Regenerating also dropped VfsSnapshotV1Job: the kind was retired on the Go
side but sim's committed mirror still carried it. Nothing referenced it. There
is no CI drift check between the repos, which is how it survived.
Also replaces nine hardcoded `../copilot/copilot/contracts/...` paths with a
shared resolver that probes both sibling layouts and honors MOTHERSHIP_REPO.
The scripts assumed the Go repo is cloned as `copilot`; a clone named
`mothership` failed the whole mship:generate pipeline with a missing-file
error that named neither the assumption nor the fix.
Two regression guards for the redaction path. The first covers every workspace that has never marked a key non-secret: with an empty exempt set all three keys still activate, redact, and export exactly as before, so the feature is provably inert where it is unused. The second covers the workspaces that do use it: exempting one key removes exactly that key and leaves the secrets beside it redacting.
Live-Postgres verification caught a real defect in the clause added for non-secret env vars. The other three clauses in getAccessibleEnvCredentials are all scoped to the user - an active credential membership, your own personal secret, workspace admin. The new one was not, so it returned a workspace's non-secret keys to ANY caller, including a user with no membership in that workspace at all. Callers do check access upstream today, so this was not reachable in practice. But the function's contract is "credentials this user may access", and a future caller trusting that name would have leaked. It now resolves workspace access itself and gates the bypass on it, rather than depending on every caller to remember. A caller without access still reaches their own personal credentials through the envOwnerUserId clause; only the bypass is withheld. getPersonalAndWorkspaceEnv passes both facts through from the access check it already performs, so the common path costs no extra query. Adds scripts/verify-env-acl.ts, which found this. The unit suites all run against a mocked @sim/db, so nothing exercised the actual SQL; this seeds a real workspace, two users, and a non-member, and asserts what the query must REFUSE. It also pins that credential_env_visibility_scope_check rejects both an INSERT and an UPDATE marking a personal secret non-secret.
…ions Browser testing showed the first cut had the wrong shape. Creating a variable meant creating a secret and then converting it, which is backwards for the common case, and the per-row "Non-secret" tag sat in the trailing auto-sized grid track, widening the column and knocking every other row's ... trigger out of alignment. Now there are two workspace sections, each with its own draft row, so either kind can be created directly. The heading carries the meaning, so the tag is gone along with the alignment problem it caused. Personal becomes "Personal secrets" - personal values cannot be non-secret, the schema check constraint sees to that, so there is no personal counterpart. Almost none of this reached the write path: the upsert mutation already accepted a visibility map and the server already applied it to new keys. Save just builds that map from whichever rows were drafted in the variables section. Two things the sectioning made necessary. A name drafted in one section that already exists in the other now blocks the save: visibility only applies to NEW keys, so it would otherwise have saved and silently kept its old policy, leaving a row whose section lied about its kind. And both draft arrays reset after a save, not just the original one - a saved key that stays drafted renders twice, which is exactly what happened. Converting moves a row between sections on its own, since the sections read the refetched visibility map.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview API ( Provenance — Settings UI — Splits workspace env into Workspace secrets and Workspace variables with separate draft rows, row actions to flip visibility (with Reviewed by Cursor Bugbot for commit d991a08. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
CI's check:audits gate caught a documented rule violation: CLAUDE.md says never write `e instanceof Error ? e.message : 'fallback'`, use getErrorMessage(e, fallback?) from @sim/utils/errors. The visibility-change handler did exactly the banned form. Worth noting for next time: check:audits is not part of `bun run check`, so a local run of check + lint + type-check + tests passes while CI fails.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1373fe2. Configure here.
Greptile SummaryThe PR adds non-secret workspace variables while retaining encryption and existing write authorization, and it hardens visibility changes after prior review feedback.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported partial-write and permission-revocation races are resolved by transactional writes, transaction-scoped authorization reads, and locks covering every current authorization grant source.
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/workspaces/[id]/environment/route.ts | Consolidates workspace environment writes and disclosure changes into one transaction, resolving the previously reported partial-write paths. |
| apps/sim/lib/credentials/environment.ts | Adds visibility-aware access and serializes disclosure authorization against revocation of explicit, inherited, and per-credential grants. |
| apps/sim/app/workspace/[workspaceId]/settings/components/secrets/components/secrets-manager/secrets-manager.tsx | Splits secrets and variables into separate UI sections and supports confirmed visibility conversion while preserving rename visibility. |
| apps/sim/executor/utils/resolved-secret-trace-registry.ts | Exempts explicitly non-secret names from provenance redaction without weakening the default behavior for secrets. |
| packages/db/migrations/0285_brainy_mauler.sql | Adds the credential visibility field and database constraint needed to prevent personal credentials from becoming workspace-visible variables. |
Sequence Diagram
sequenceDiagram
participant U as Workspace user
participant API as Environment PUT route
participant DB as PostgreSQL transaction
participant ACL as Visibility authorization
participant AUD as Audit log
U->>API: Values and/or visibility changes
API->>DB: Begin transaction
DB->>DB: Acquire workspace advisory lock
DB->>DB: Upsert encrypted values
DB->>DB: Create missing credential rows
DB->>ACL: Lock permission, org-member, and credential-member grants
ACL->>ACL: Recheck current authorization
alt Authorized
ACL->>DB: Update env_visibility
DB-->>API: Commit all writes
API->>AUD: Record successful mutation
API-->>U: 200
else Denied or revoked
ACL-->>DB: Throw access error
DB-->>API: Roll back all writes
API-->>U: 403
end
Reviews (8): Last reviewed commit: "fix(db): add the visibility constraint N..." | Re-trigger Greptile
Two review findings, both real. Ordering: the PUT committed the value upsert and credential rows, then called the visibility change, which can 403. A request mixing an allowed new key with an unauthorized flip therefore persisted half of itself and still failed — state changed by a rejected call, and the audit record for the successful part never written. setWorkspaceEnvVisibility splits into authorize and apply so the route can deny before anything reaches the database. Covered by a test that asserts no credential creation and no apply happen on the denied path. Silent no-op: visibilityByKey only reaches credential CREATION, so asking for a policy on an existing key was dropped while the call still reported success. That includes a variable -> secret remediation, the case where a false success is most harmful — a caller told it worked would never retry. Flipping an existing key needs the stricter disclosure gate this path does not perform, so it now throws instead of pretending. Also consumes the consolidated envVars contract: one kind where a secret is name-only and a non-secret carries its value.
|
@cursor review |
The pre-flight authorization ran before the value upsert and the credential inserts, then its resolved credential IDs were applied afterwards. Access revoked in that window was never observed, so a former credential admin could still flip a secret to a workspace-visible variable. The flip now goes through setWorkspaceEnvVisibility inside its own transaction, which re-authorizes adjacent to the UPDATE. The pre-flight stays, with its result deliberately discarded, so a denied request still attempts no writes at all. applyWorkspaceEnvVisibilityChange is no longer exported — a decision carried across unrelated awaits is exactly what went stale, so there is no supported way to do that again.
|
@cursor review |
Two rounds of ordering fixes each produced the next finding, because the value upsert, the credential inserts, and the visibility flip were three separate commits with the disclosure check somewhere among them. Moving the check only changed which writes were stranded by a denial. The PUT now runs authorization and every write it guards in one transaction, so a denial rolls all of it back and there is no ordering left to get wrong. createWorkspaceEnvCredentials takes an executor to join it. upsertWorkspaceEnvVars had the same shape on the copilot tool path and now validates the requested visibility inside its own transaction, before the write, where the stored key set is exact. Authorization also share-locks the permissions and credential_member rows that grant the caller's access before reading them, so a concurrent revocation either is observed and denies, or waits for the transaction rather than racing the UPDATE. Two UI fixes from the same review: a renamed variable saves as delete-old plus create-new, so it now carries its visibility across the rename instead of silently reverting to secret; and variable drafts join allWorkspaceKeys, so personal-vs-workspace conflict detection no longer misses names drafted in the Variables section.
|
@cursor review |
The revocation locks covered the explicit workspace permission and the per-key credential membership, but workspace admin can also be INHERITED from an organization admin role. That grant lives in a `member` row no lock covered, so revoking it could still race the disclosure update. Locks now cover all three grants. Both authorization reads also go through the caller's transaction rather than the global client, so the decision runs on the same connection holding those locks instead of alongside them on a pooled one.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 40c5188. Configure here.
The rename carried visibility into the save payload but not into the render, which still resolved disclosure from server state only. The new name has no server visibility yet, so the row defaulted to secret: it jumped into Workspace secrets and bullet-masked a value the member can plainly read, until save landed.
|
@cursor review |
The carried value won over the server map, so a rename-back (or a rename onto an existing key) left an entry shadowing a key that still has a credential: a later confirmed convert-to-secret would apply server-side while the row stayed under Workspace variables and kept rendering unmasked until refresh. Server state now wins wherever it exists and the carried value only fills the gap where it doesn't, which is all it was ever for. A stale entry is inert under that ordering rather than needing to be cleared on every path that could invalidate it.
|
@cursor review |
The render treated a stale carried entry as inert, but handleSave still spread the raw map into the upsert payload — and since the PUT now applies visibility to existing keys, that entry could flip a real key. A rename-back let the next unrelated value save revert a confirmed convert-to-secret, and a rename onto an existing secret could turn it into a variable with no confirm dialog at all. Render and save now read one narrowed derivation, restricted to keys the server has no visibility for, so a carried value can only ever describe the new name it was created for. Sharing the derivation is the point: the last two defects were the two halves disagreeing.
check:migrations blocks a plain ADD CONSTRAINT CHECK on an existing table: it takes an ACCESS EXCLUSIVE lock for a full scan of credential and rejects concurrent writes for the duration. Split into the expand/contract form so the lock is brief and validation runs under SHARE UPDATE EXCLUSIVE, which readers and writers do not block on.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d991a08. Configure here.

What
Workspace secrets are redacted everywhere: masked from non-admins, scrubbed out of traces by the resolved-secret projection, logged as ciphertext, and exposed to Sim by name only. That is right for API keys and wrong for the config values teams also keep there — a support email, a region, a base URL.
A variable is a workspace secret with its disclosure policy flipped. Same table, same encryption at rest, same
{{NAME}}namespace and uniqueness guarantee; a per-keycredential.env_visibilityflag turns off redaction.Companion: simstudioai/mothership#415
Read-side only, deliberately
Write authorization is identical for both kinds. Non-secret and non-sensitive are different axes: a member who cannot read a secret must still not be able to repoint an
API_BASE_URLthat the secret is sent to. Only reads relax.That made the write path almost untouched —
upsertWorkspaceEnvVarsand the PUT/DELETE gates keep their existing logic. The risk concentrates in the read path and the provenance exemption, which is where the tests concentrate too.The provenance exemption is the security-critical part
Resolvers call
recordResolvedfor every substitution and cannot tell a variable from a secret. A known non-secret name returns early — before the catalog lookup, so a decryption failure cannot reroute it — instead of tripping the permanentmarkIncompletethat would collapse the run's traces to structure-only and omit every later copilot tool result.The exempt set is a required constructor parameter. A missing set over-redacts (safe); a wrong one under-redacts (not). Making it required forces every construction site to name its source or fail to compile — the plan predicted 9 sites, the compiler found 28. It is derived locally on each side of every boundary and never carried in provenance, so no peer can assert that one of your secrets is non-secret.
Two tests pin that the feature is inert where unused: with an empty exempt set every key still activates, redacts, and exports exactly as before.
A live ACL test caught a real defect
getAccessibleEnvCredentials's other three clauses are all scoped to the user. The new one was not, so it returned a workspace's non-secret keys to any caller — including a user with no membership in that workspace. Not reachable in practice (callers check upstream), but the function's contract is "credentials this user may access", and the next caller to trust that name would have leaked.The unit suites all run against a mocked
@sim/db, so nothing exercised the actual SQL.apps/sim/scripts/verify-env-acl.tsseeds a real Postgres and asserts what the query must refuse. It found this. The bypass now resolves workspace access itself rather than trusting callers to remember.UI
Two workspace sections — Workspace secrets and Workspace variables — each with its own draft row, so either kind is created directly. Browser testing killed the first cut, where the only path was "create a secret, then convert it" and a per-row tag knocked every other row's
...out of alignment.Converting moves a row between sections on its own. The confirm dialog carries the part permissions cannot:
secret → variablesays the disclosure cannot be undone by switching back, andvariable → secretprompts to rotate, because reverting the flag is not remediation.Verification
origin/stagingbun run check,check:api-validation:strict,mship:check,type-checkall cleancredential_env_visibility_scope_checkrejects both an INSERT and an UPDATE marking a personal secret non-secretNot verified
Live agent behavior. Local sim points at deployed
copilot.sim.ai, so "agent quotes a variable, refuses a secret" needs the companion mothership branch deployed. Worth doing before this reaches users.Also fixed along the way
bun run mship:generateassumed the Go repo is cloned as a sibling namedcopilot; a clone namedmothershipfailed the whole pipeline with an error naming neither the assumption nor the fix. Nine scripts now share a resolver that probes both and honorsMOTHERSHIP_REPO.VfsSnapshotV1Job— retired on the Go side but still in sim's committed mirror. Direct evidence of the missing CI drift check.fetchWorkspaceEnvironmentand two React Queryselectcallbacks rebuilt responses field-by-field and would have dropped the new field; the selects also broke structural sharing.