fix(action): don't mask non-secret config values in run logs - #69
fix(action): don't mask non-secret config values in run logs#69theoephraim wants to merge 1 commit into
Conversation
e1f34b5 to
bf6d9cf
Compare
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — carves known non-credential config keys out of value-based GitHub Actions log masking while preserving mask-by-default for everything else.
- Add
isNonSecretConfigNameallowlist — new explicitSetinutils/secrets.ts(PULLFROG_MODEL,PULLFROG_AGENT,AWS_REGION,BEDROCK_MODEL_ID,VERTEX_MODEL_ID,VERTEX_LOCATION,GOOGLE_CLOUD_PROJECT), matched case-insensitively. - Skip masking for allowlisted keys in
sanitizeSecret—utils/normalizeEnv.tsstill trims these values but returns beforecore.setSecret, so a short value likeVERTEX_LOCATION=globalno longer rewrites unrelated log text to***. - Document the dual-purpose channel in
main.ts— a comment on thedbSecretsinjection loop notes it carries both credentials and config. - Tests in
utils/normalizeEnv.test.ts— mockcore.setSecretand assert whether it is called: credential-shaped keys, unknown keys, andVERTEX_SERVICE_ACCOUNT_JSONstill mask; config keys are trimmed but not masked.
I verified the change is fail-closed and self-consistent: the allowlist names match none of SENSITIVE_PATTERNS, so the normalizeEnv masking loop (gated on isSensitiveEnvName) is unaffected and only the unconditional main.ts dbSecrets path changes behavior. The allowlisted names match the env vars actually consumed in utils/vertex.ts and models.ts, VERTEX_SERVICE_ACCOUNT_JSON is correctly excluded and pinned against the tempting "just gate on isSensitiveEnvName" refactor, and case-insensitive matching handles lowercase dbSecret keys.
Claude Opus (free via Pullfrog for OSS) ｜ 𝕏
Account-level secrets are injected via sanitizeSecret, which called core.setSecret unconditionally. That channel also carries non-credential config (model ids, regions, project/location identifiers), and GitHub Actions masks by value, so a dashboard-stored VERTEX_LOCATION=global rewrote every unrelated occurrence of "global" in the run log to ***. Masked model ids also made the "which model ran?" lines unreadable. Add an explicit non-secret allowlist and skip masking for those keys. Values are still trimmed, since a trailing newline breaks exact-match lookups. Anything not on the allowlist is still masked, so unrecognised keys fail closed — notably VERTEX_SERVICE_ACCOUNT_JSON, which matches none of SENSITIVE_PATTERNS and is protected only by mask-by-default.
bf6d9cf to
e52822b
Compare
|
Rebased onto main (clean, no conflicts) and extended the allowlist for the backends that landed since this was opened: the five Azure config values from #1226 (everything but the API key — the console flow stores all of them in the account-secret channel, and masking |
1 participant
Feel free to toss or reimplement. Some findings after wiring up pullfrog to azure foundry (we have some credits there to use)
On this one in particular, obviously some kind of marking specific vars as sensitive or not could be useful. Varlock probably overkill but maybe some way to integrate...
Problem
Account-level secrets are injected into
process.envinmain.tsviasanitizeSecret, which callscore.setSecretunconditionally:But that channel carries more than credentials. The Bedrock and Vertex setup errors tell users to configure
AWS_REGION,BEDROCK_MODEL_ID,GOOGLE_CLOUD_PROJECT, andVERTEX_LOCATION, and those can be stored as account secrets like anything else. Since #1226 the Azure console flow goes further and recommends storing all fiveAZURE_*values that way, four of which are plain config.GitHub Actions masks by value, not by variable. So a stored
VERTEX_LOCATION=globalcallscore.setSecret("global")and every unrelated occurrence of the word "global" in the run log becomes***for the rest of the job. A maskedPULLFROG_MODELmakes the "which model ran?" lines unreadable at exactly the moment someone is debugging why the wrong model ran. The new Azure values are the worst offenders: maskingAZURE_MAX_OUTPUT=128000orAZURE_USE_CHAT_COMPLETIONS=trueshreds token-count lines and everytruein the log.The two callers of
sanitizeSecretdisagree on this today, which is what made it easy to miss:normalizeEnvalready gates onisSensitiveEnvNamebefore calling it, themain.tsdbSecrets loop does not.Fix
An explicit non-secret allowlist in
utils/secrets.ts, checked insanitizeSecretjust before the mask call. Allowlisted keys are still trimmed (a trailing newline on a model id breaks exact-match lookups just as badly as it breaks masking) but not registered as masks.On staying fail-closed
The tempting one-line version of this fix is to gate the
main.tsloop onisSensitiveEnvName, mirroringnormalizeEnv. That would be a security regression:VERTEX_SERVICE_ACCOUNT_JSONmatches none ofSENSITIVE_PATTERNS(_KEY$,_SECRET$,_TOKEN$,_PASSWORD$,_CREDENTIAL$), so unconditional masking is currently the only thing protecting it.So this goes the other way: mask-by-default is preserved and only known-non-secret names are carved out. An unrecognised key is still treated as a credential. There's a test pinning the service-account case specifically, since it's the trap a future refactor would fall into.
For the same reason,
OPENAI_COMPATIBLE_BASE_URLdeliberately stays OFF the allowlist even though its siblings are config: gateway URLs can carry account ids or embedded credentials in the path. There's a test pinning that too.Tests
Eight masking-policy cases in
utils/normalizeEnv.test.tspin whensetSecretis and isn't called, including the fail-closed traps (VERTEX_SERVICE_ACCOUNT_JSON,OPENAI_COMPATIBLE_BASE_URL). Verified the behavioral ones fail when the fix is disabled, so they aren't vacuous.pnpm typecheckis clean and the file is 20/20. I didn't run the full suite (no GitHub App credentials locally).Scope
The original version deliberately excluded Azure names so it stood on its own for Bedrock/Vertex users. Rebased onto main after #1226 landed, the allowlist now also covers the new backends:
AZURE_RESOURCE_NAME,AZURE_DEPLOYMENT,AZURE_CONTEXT,AZURE_MAX_OUTPUT,AZURE_USE_CHAT_COMPLETIONS, and the non-secretOPENAI_COMPATIBLE_MODEL/OPENAI_COMPATIBLE_CONTEXT/OPENAI_COMPATIBLE_MAX_OUTPUT(base URL excluded, see above).