Skip to content

Commit 44f331a

Browse files
committed
fix(secrets): never send carried rename visibility for a known key
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.
1 parent 626d845 commit 44f331a

1 file changed

Lines changed: 36 additions & 18 deletions

File tree

  • apps/sim/app/workspace/[workspaceId]/settings/components/secrets/components/secrets-manager

apps/sim/app/workspace/[workspaceId]/settings/components/secrets/components/secrets-manager/secrets-manager.tsx

Lines changed: 36 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -898,30 +898,47 @@ export function SecretsManager() {
898898
* each section can take its own slice. Derived once rather than filtered twice
899899
* so the two sections cannot disagree about which kind a key is.
900900
*/
901+
/**
902+
* The carried rename visibility, narrowed to keys the server has no opinion
903+
* about — the single source both the render and the save payload read.
904+
*
905+
* The narrowing is the safety property, not an optimization. A rename-back,
906+
* or a rename onto a key that already exists, leaves an entry for a key that
907+
* still has a credential; sending that in the save payload would flip a real
908+
* key's disclosure. Since the PUT now applies visibility to existing keys,
909+
* that could revert a confirmed convert-to-secret on the next unrelated value
910+
* save, or turn an existing secret into a variable with no confirm dialog at
911+
* all. Restricted to keys the server does not know, the carried value can
912+
* only ever describe the new name it was created for.
913+
*/
914+
const carriedRenameVisibility = useMemo(() => {
915+
const serverVisibility = workspaceEnvData?.visibility ?? {}
916+
const carried: Record<string, EnvVisibility> = {}
917+
for (const [key, visibility] of Object.entries(renamedKeyVisibility)) {
918+
if (!(key in serverVisibility)) carried[key] = visibility
919+
}
920+
return carried
921+
}, [renamedKeyVisibility, workspaceEnvData?.visibility])
922+
901923
const workspaceEntriesForRender = useMemo(() => {
902924
const entries = searchTerm.trim() ? filteredWorkspaceEntries : Object.entries(workspaceVars)
903925
const visibilityMap = workspaceEnvData?.visibility ?? {}
904926
return entries.map(([key, value]) => ({
905927
key,
906928
value,
907-
// Server state wins wherever it exists; the carried rename only fills the
908-
// gap where it doesn't. That ordering is the whole contract: a renamed key
909-
// has no server visibility yet, so without the fallback it would default
910-
// to `secret` and bounce into Workspace secrets, bullet-masking a value
911-
// the member can plainly read. But letting the carried value WIN would
912-
// shadow the server after a rename-back or a rename onto an existing key,
913-
// so a later confirmed convert-to-secret would apply server-side and the
914-
// row would still render as non-secret. Under this ordering a stale entry
915-
// is inert, rather than needing to be cleared on every path that could
916-
// invalidate it.
917-
visibility: visibilityMap[key] ?? renamedKeyVisibility[key] ?? ('secret' as EnvVisibility),
929+
// A renamed key has no server visibility yet, so without the carried
930+
// fallback it would default to `secret` and bounce into Workspace
931+
// secrets, bullet-masking a value the member can plainly read. The
932+
// carried map is already narrowed to keys the server does not know, so
933+
// the two can never disagree about a key that exists.
934+
visibility: visibilityMap[key] ?? carriedRenameVisibility[key] ?? ('secret' as EnvVisibility),
918935
}))
919936
}, [
920937
searchTerm,
921938
filteredWorkspaceEntries,
922939
workspaceVars,
923940
workspaceEnvData?.visibility,
924-
renamedKeyVisibility,
941+
carriedRenameVisibility,
925942
])
926943

927944
/**
@@ -1004,12 +1021,13 @@ export function SecretsManager() {
10041021
mergedWorkspaceVars[row.key] = row.value
10051022
}
10061023
}
1007-
// Names drafted in the Variables section. Only NEW keys can take a
1008-
// visibility here — the server never flips an existing key through this
1009-
// path (that needs the confirmed disclosure flow), which is why
1010-
// `duplicateDraftKeys` blocks saving a name that already exists as a secret
1011-
// rather than letting it save and silently stay secret.
1012-
const draftVariableVisibility: Record<string, EnvVisibility> = { ...renamedKeyVisibility }
1024+
// Names drafted in the Variables section, plus any carried across a rename.
1025+
// Both only ever describe keys the server does not already know: the PUT
1026+
// does apply visibility to existing keys, so an entry for an existing key
1027+
// would flip its disclosure with no confirm dialog. `duplicateDraftKeys`
1028+
// blocks a draft name that already exists, and `carriedRenameVisibility` is
1029+
// narrowed to unknown keys for the same reason.
1030+
const draftVariableVisibility: Record<string, EnvVisibility> = { ...carriedRenameVisibility }
10131031
for (const row of newWorkspaceVariableRows) {
10141032
if (row.key && row.value) {
10151033
mergedWorkspaceVars[row.key] = row.value

0 commit comments

Comments
 (0)