Skip to content

feat(secrets): non-secret workspace variables - #6388

Open
mzxchandra wants to merge 16 commits into
stagingfrom
feat/workspace-variables
Open

feat(secrets): non-secret workspace variables#6388
mzxchandra wants to merge 16 commits into
stagingfrom
feat/workspace-variables

Conversation

@mzxchandra

Copy link
Copy Markdown
Contributor

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-key credential.env_visibility flag 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_URL that the secret is sent to. Only reads relax.

That made the write path almost untouched — upsertWorkspaceEnvVars and 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 recordResolved for 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 permanent markIncomplete that 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.ts seeds 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 → variable says the disclosure cannot be undone by switching back, and variable → secret prompts to rotate, because reverting the flag is not remediation.

Verification

  • Full suite: 20,584 passing after merging origin/staging
  • bun run check, check:api-validation:strict, mship:check, type-check all clean
  • Live Postgres ACL test: 10/10, including that credential_env_visibility_scope_check rejects both an INSERT and an UPDATE marking a personal secret non-secret
  • Browser-verified end to end: create, convert both directions, section move, re-mask

Not 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:generate assumed the Go repo is cloned as a sibling named copilot; a clone named mothership failed the whole pipeline with an error naming neither the assumption nor the fix. Nine scripts now share a resolver that probes both and honors MOTHERSHIP_REPO.
  • Regenerating dropped VfsSnapshotV1Job — retired on the Go side but still in sim's committed mirror. Direct evidence of the missing CI drift check.
  • fetchWorkspaceEnvironment and two React Query select callbacks rebuilt responses field-by-field and would have dropped the new field; the selects also broke structural sharing.

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.
@vercel

vercel Bot commented Aug 7, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
docs Ready Ready Preview Aug 7, 2026 10:28pm

Request Review

@cursor

cursor Bot commented Aug 7, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Changes disclosure policy, env ACL/masking, and secret provenance exemption paths—incorrect exempt sets or partial commits could leak secrets or over-expose values in traces and logs.

Overview
Introduces workspace variables alongside secrets: the same encrypted env keys can be marked variable so values are readable to any workspace member and are not treated as secrets in traces/logs.

API (environment route) — GET now returns a per-key visibility map and reveals plaintext for variable keys even when secrets stay masked. PUT accepts optional visibility updates, runs value upsert, credential creation, and setWorkspaceEnvVisibility inside one transaction (denied visibility changes roll back everything and skip audit), and supports visibility-only mutations.

ProvenanceResolvedSecretTraceRegistry now requires a nonSecretNames set (use EMPTY_NON_SECRET_NAMES when none). Names in that set are excluded from the secret catalog and short-circuit recordResolved before catalog lookup, so {{VARIABLE}} substitutions do not poison run provenance or force trace redaction. Construction sites across knowledge, MCP, memory, logging, webhooks, child workflows, and tests pass the exempt set from workspaceVariableKeys where applicable.

Settings UI — Splits workspace env into Workspace secrets and Workspace variables with separate draft rows, row actions to flip visibility (with ChipConfirmModal disclosure warnings), rename/save logic that preserves visibility, and duplicate-key guards across sections. SecretValueField shows variable values in full without bullet masking for viewers.

Reviewed by Cursor Bugbot for commit d991a08. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions github-actions Bot added the requires-mothership-merge Has a companion PR on the mothership/copilot side — merge in lockstep label Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

⚠️ Cross-repo companion check

One or more companion PRs aren't merged into staging yet. Merging this without them will leave copilot and sim out of sync — merge them in lockstep.

Comment thread apps/sim/app/api/workspaces/[id]/environment/route.ts Outdated
Comment thread apps/sim/lib/environment/utils.ts
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.
@mzxchandra

Copy link
Copy Markdown
Contributor Author

@greptile-apps

@mzxchandra

Copy link
Copy Markdown
Contributor Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

2 issues from previous reviews remain unresolved.

Fix All in Cursor

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 1373fe2. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adds non-secret workspace variables while retaining encryption and existing write authorization, and it hardens visibility changes after prior review feedback.

  • Returns variable visibility and readable values to authorized workspace members while continuing to mask secrets.
  • Applies environment value writes, credential creation, visibility authorization, and visibility updates in one transaction.
  • Locks each authorization grant source before rechecking permissions and changing disclosure policy.
  • Propagates visibility through execution, provenance, Copilot, and workspace settings flows.
  • Adds schema, migration, ACL verification, UI, and regression-test coverage.

Confidence Score: 5/5

The 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.

Important Files Changed

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
Loading

Reviews (8): Last reviewed commit: "fix(db): add the visibility constraint N..." | Re-trigger Greptile

Comment thread apps/sim/app/api/workspaces/[id]/environment/route.ts Outdated
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.
@mzxchandra

Copy link
Copy Markdown
Contributor Author

@greptile-apps

@mzxchandra

Copy link
Copy Markdown
Contributor Author

@cursor review

Comment thread apps/sim/app/api/workspaces/[id]/environment/route.ts Outdated
Comment thread apps/sim/lib/environment/utils.ts Outdated
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.
@mzxchandra

Copy link
Copy Markdown
Contributor Author

@greptile

@mzxchandra

Copy link
Copy Markdown
Contributor Author

@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.
@mzxchandra

Copy link
Copy Markdown
Contributor Author

@greptile

@mzxchandra

Copy link
Copy Markdown
Contributor Author

@cursor review

Comment thread apps/sim/lib/credentials/environment.ts Outdated
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.
@mzxchandra

Copy link
Copy Markdown
Contributor Author

@greptile

@mzxchandra

Copy link
Copy Markdown
Contributor Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

1 issue from previous review remains unresolved.

Fix All in Cursor

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.
@mzxchandra

Copy link
Copy Markdown
Contributor Author

@greptile

@mzxchandra

Copy link
Copy Markdown
Contributor Author

@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.
@mzxchandra

Copy link
Copy Markdown
Contributor Author

@greptile

@mzxchandra

Copy link
Copy Markdown
Contributor Author

@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.
@mzxchandra

Copy link
Copy Markdown
Contributor Author

@greptile

@mzxchandra

Copy link
Copy Markdown
Contributor Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

@mzxchandra

Copy link
Copy Markdown
Contributor Author

@icecrasher321

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

requires-mothership-merge Has a companion PR on the mothership/copilot side — merge in lockstep

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant