Accept every sensitive config key as a value or a -path file - #722
Draft
coopernetes wants to merge 1 commit into
Draft
coopernetes wants to merge 1 commit into
coopernetes wants to merge 1 commit into
Conversation
Sensitive keys were split between value-only and file-only forms, and the OAuth client secret and token key files were read by the code that used them, so a missing file surfaced as a runtime error instead of a startup one. Each sensitive key now takes <key> or <key>-path. The loader resolves them once after binding, logs which form supplied each one (WARN for a YAML literal), fails on both forms, an unreadable file, or an OAuth client with no secret, and with secrets.require-file-sourcing refuses any value form. Resolved secrets are excluded from the config classes' toString. The token encryption key accepts 32 raw bytes or base64, and a configured key that fails to load fails startup; only the auto-generated dev key still degrades. closes #671 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sensitive config keys were split between value-only and file-only forms, and the OAuth client secret and token encryption key were read from disk by the code that used them, so a missing or rotated file surfaced as a runtime error from a controller or a refresh rather than at startup.
<key>or<key>-path. The existingproviders.<name>.oauth.client-secret-pathandscm-oauth.token-encryption-key-pathkeep their names and meaning.SecretsResolverresolves all of them once, after binding, and writes the value back;ScmOAuthLinkController,ScmOAuthTokenServiceandTokenCipherProviderno longer read secret files.-pathfile, or a provider withoauth.enabledand aclient-idbut no client secret in either form, fails startup.WARNonly for a YAML literal and the existing auto-generated./.data/token key.scm-oauth.token-encryption-keyaccepts 32 raw bytes or the base64 of 32 bytes; the error names both. A configured key that fails to load fails startup; the auto-generated dev key still only disables linking.secrets.require-file-sourcing(defaultfalse) refuses to start if any secret came from a value form.@ToString.Exclude, so logging a config object never prints one.docs/configuration/secrets.md, plus the key tables inproviders.mdandscm-oauth.md.client-secretnext to its placeholderclient-id, andtest/capture/secrets.env.exampleclears it alongsideCLIENT_SECRET_PATH.Tests:
SecretsResolverTest(every pair, both forms, unreadable file, missing OAuth client secret, key formats, enforcement, toString),FogwallConfigLoaderTest(-pathbound from YAML and from env, YAML vs env attribution including a mixed-case provider name, an empty env value clearing a YAML literal, secrets resolved on a hot-reload config),AesGcmTokenCipherTest,TokenCipherProviderTest. The OAuth paths that now take the resolved secret are covered end to end byBrokeredPushE2ETest/DeferredForwardingE2ETest, which configureclient-secret-pathandtoken-encryption-key-path.Decisions for review:
${...}substitution in YAML therefore counts as environment, and an env override equal to the YAML literal counts as YAML. Provider names are lowercased in both the file tree and the bound map, so the lookup matches mixed-case names.composeReloadresolves secrets again, without a provenance log, because building the reloaded sections reachesbuildProviderRegistry(viavalidateProviderReferencesandresolveProviderName), which constructs providers withapi-token. Reload still applies only policy sections, so a changed secret takes effect on restart, contrary to the issue's "covered by hot reload" line.-pathfile is read at startup even if the feature using it is off (e.g. a token key with no provider offering linking). A bad file is a startup error regardless.scm-oauth.token-encryption-keyas base64 text, so raw binary key files survive the String config field.require-file-sourcingdoes not refuse the auto-generated dev token key; it still logsWARN.FOGWALL_RELOAD_GIT_AUTH_PASSWORDis read straight from the environment, outside the config loader, and is not covered.test/capture/secrets.envthat sets onlyCLIENT_SECRET_PATHnow fails startup against the Playwright profile's placeholderclient-secretuntil it also setsCLIENT_SECRET="".OAuthClientoverridestoStringto redact the secret now that it carries the value.🤖 Generated with Claude Code