fix(settings): removing a secret provider clears the fields that used it#31651

🟠 High src/serverSettings.ts:1175

Removing a secret provider while a patch echoes its credential as SECRET_REDACTED leaves a dangling credential in the persisted settings. withoutRemovedSecretProviderReferences runs before persistSettingsSecrets restores echoed references, so it sees only the marker and cannot clear the reference; resolve echoed references before this cleanup.

Fixed in 8788c6e13ac: echoed SECRET_REDACTED markers are restored to their references before the cleanup, so a removed provider's reference is cleared. The removal test now echoes the usage limit key as the marker in the same patch; it fails without the fix.

The commit addresses the issue. I couldn't verify it on the current branch, which appears to be at 033bc1f.

The PR head is now 8788c6e13ac, which contains the fix; the mirror had not caught up when you checked.

🟠 High src/settingsSecrets.ts:252

Removing the provider constructor leaves its credential and environment references intact, despite the confirmation that they will be unset. The in operator also finds inherited properties, so constructor is not added to removed; use an own-property check instead.

Fixed in 8788c6e13ac: the removed-provider check uses Object.hasOwn.

I couldn’t access that commit, and the current PR head still uses id in next.secretProviders. Could you check whether the fix was pushed?

The PR head is now 8788c6e13ac, which contains the fix; the mirror had not caught up when you checked.

External image Minor · Functional Correctness · Quick win

Check for an own provider entry.

If the provider ID is constructor or toString, removing it leaves an ordinary secretProviders object whose prototype still has that property. The in check then reports that the provider remains, so the helper leaves its references in place. These IDs satisfy SecretProviderId. Use Object.hasOwn(next.secretProviders, id) and add one such ID to the removal test. (raw.githubusercontent.com)

✅ Confirmed as addressed by @juliusmarminge

Fixed in 8788c6e13ac: the removed-provider check uses Object.hasOwn.

@juliusmarminge Thanks for fixing the removed-provider check.

Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Functional Correctness · Quick win

Keep the affected-fields list inside the dialog viewport.

If a provider has many references, this unbounded list can extend beyond the height-limited popup and obscure the confirmation footer. Give the list a bounded height and vertical scrolling. The dialog’s description already has a scroll limit, but that limit does not apply to this sibling list. (raw.githubusercontent.com)

✅ Confirmed as addressed by @juliusmarminge

Fixed in 8788c6e13ac: the list has a bounded height and scrolls.

@juliusmarminge Thanks for fixing the list in 8788c6e13ac.

Review thread resolved.

_{You are interacting with an AI system.}

External image Minor · Data Integrity & Integration · Quick win

Clear removed references after managed-provider vetting.

When maxRuntimeMode is set, vetEnvironment can restore a removed provider’s environment reference from stored settings. The reference can then remain in settings.json. Run cleanup again after vetting, and add a focused test for this policy case.

✅ Confirmed as addressed by @juliusmarminge

Fixed in a4746c0cae2: the cleanup now runs after both vetting steps, just before persistence. Adding the test also exposed that vetting compared environment values by identity, so a re-decoded secret reference read as an edit to a managed variable and the save was refused; it now compares by value. The runtime-ceiling test now removes a provider whose reference sits in a hidden variable; it fails without the fix.

@juliusmarminge Confirmed in a4746c0cae2: cleanup runs after both vetting steps, and the runtime-ceiling test covers a hidden reference. Environment values now compare by value.

I inspected the changes but did not run the test.

Review thread resolved.

_{You are interacting with an AI system.}

External image Major · Security & Privacy · Quick win

Require authorization for fields cleared by provider removal.

A provider-only patch requires only AuthProvidersManageScope. It can also clear source-control and EAS token references, which normally require AuthSettingsWriteScope. Re-adding the provider does not restore those references.

Require settings-write authority when removal clears those fields, or define an explicit authorization rule for the cascade.

✅ Confirmed as addressed by @juliusmarminge

Fixed in bcb7fccde04: a settings patch that removes a secret provider now also requires settings write, since the removal clears settings fields that read from it. Adding a provider still needs only provider management. RpcAuthorization's provider-only test now expects the removal to be denied without settings write; it fails without the fix.

@juliusmarminge Verified in bcb7fccde04: removing a secret provider requires both AuthProvidersManageScope and AuthSettingsWriteScope. Adding a provider still requires only provider management.

The authorization test checks that removal is denied without settings write. I did not run the test.

Review thread resolved.

_{You are interacting with an AI system.}