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
Sorry, something went wrong.
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 isNonSecretConfigName allowlist — new explicit Set in utils/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.ts still trims these values but returns before core.setSecret, so a short value like VERTEX_LOCATION=global no longer rewrites unrelated log text to ***.
- Document the dual-purpose channel in main.ts — a comment on the dbSecrets injection loop notes it carries both credentials and config.
- Tests in utils/normalizeEnv.test.ts — mock core.setSecret and assert whether it is called: credential-shaped keys, unknown keys, and VERTEX_SERVICE_ACCOUNT_JSON still 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.
｜ View workflow run ｜ Using Claude Opus (free via Pullfrog for OSS) ｜ 𝕏
Sorry, something went wrong.
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.
|
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 128000 or true is especially rough on run logs) plus the non-secret OPENAI_COMPATIBLE_* twins. OPENAI_COMPATIBLE_BASE_URL deliberately stays masked since gateway URLs can embed credentials — there's a test pinning that. Typecheck clean, 20/20 in the touched test file. |
Sorry, something went wrong.
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.env in main.ts via sanitizeSecret, which calls core.setSecret unconditionally:
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, and VERTEX_LOCATION, and those can be stored as account secrets like anything else. Since #1226 the Azure console flow goes further and recommends storing all five AZURE_* values that way, four of which are plain config.
GitHub Actions masks by value, not by variable. So a stored VERTEX_LOCATION=global calls core.setSecret("global") and every unrelated occurrence of the word "global" in the run log becomes *** for the rest of the job. A masked PULLFROG_MODEL makes 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: masking AZURE_MAX_OUTPUT=128000 or AZURE_USE_CHAT_COMPLETIONS=true shreds token-count lines and every true in the log.
The two callers of sanitizeSecret disagree on this today, which is what made it easy to miss: normalizeEnv already gates on isSensitiveEnvName before calling it, the main.ts dbSecrets loop does not.
Fix
An explicit non-secret allowlist in utils/secrets.ts, checked in sanitizeSecret just 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.ts loop on isSensitiveEnvName, mirroring normalizeEnv. That would be a security regression: VERTEX_SERVICE_ACCOUNT_JSON matches none of SENSITIVE_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_URL deliberately 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.ts pin when setSecret is 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 typecheck is 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-secret OPENAI_COMPATIBLE_MODEL / OPENAI_COMPATIBLE_CONTEXT / OPENAI_COMPATIBLE_MAX_OUTPUT (base URL excluded, see above).