Prevent secret leaks from environment table wrapping - #3561
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Prevent secret leaks from environment table wrapping
The fix correctly addresses the root cause from #3548: NoWrap + Ellipsis() on the value column keeps each env value on one physical console row (so runner-side ::add-mask:: can still match), MakeSingleLine neutralizes embedded CR/LF as a belt-and-suspenders measure, and the switch from Markup.Escape(...) strings to Text cells is a nice side benefit — Text never interprets markup, so it's safer against control-sequence injection than the old escaped-string approach. Tests cover the wrap/mask/CRLF cases well, including a dedicated narrow-width render check.
One architectural point worth considering, not a blocker:
Sensitive-name detection duplicates an existing configurable pattern instead of reusing it. HttpRequestFormatter/HttpResponseFormatter already solve the "which names are secret-ish" problem via HttpLoggingOptions.SensitiveHeaderNames — a public, IOptions-configurable, user-overridable list with a documented default. This PR introduces a second, parallel mechanism: SensitiveEnvironmentVariableNameParts is a private static readonly string[] baked into PipelineInitializer, with no way for a consuming pipeline to extend or override it.
Why that matters here specifically: the substring match (name.Contains("PWD", ...), .Contains("KEY", ...)) is intentionally broad per the issue's spec, but it also means standard, non-secret shell variables like PWD/OLDPWD are now always masked, and any consumer whose own naming convention happens to contain one of these substrings (e.g. a TURNKEY_ENDPOINT or PRIMARY_KEY_COLUMN env var) loses visibility into that value in the startup table with no opt-out. The HttpLoggingOptions precedent exists precisely to let consumers tune this kind of heuristic per-project.
Suggestion: lift SensitiveEnvironmentVariableNameParts into a small options record (or extend SecretMaskingOptions) following the HttpLoggingOptions.SensitiveHeaderNames shape — public static Default list, IOptions<T>-injected, user-overridable via PipelineBuilder.Services. That keeps the two "mask by name heuristic" implementations in the codebase consistent, and gives consumers an escape hatch for false positives without needing another PR/fork. Not required for this fix to be correct or safe to ship, but worth a follow-up given PWD will now be masked in effectively every pipeline run.
Everything else looks solid — validation section in the PR description matches what I see in the diff, and I didn't find any correctness issues with the masking/truncation logic itself.
There was a problem hiding this comment.
Code review
No new changes since the previous review (the only commit added since 00ac5f7 is a merge of main, with no functional diff to the touched files) — so this is a verification pass rather than a fresh review.
Independently verified locally against the current HEAD (a7ccb9b):
dotnet build ModularPipelines.sln -c Release: 0 errors, 0 warningsPipelineInitializerTestsvia TUnit--treenode-filter: 10/10 passed
The masking/truncation logic in CreateEnvironmentVariablesTable holds up: sensitive-name detection short-circuits before obfuscation runs, NoWrap + .Ellipsis() keeps each value on one physical console row, and MakeSingleLine escapes embedded CR/LF as a second line of defense. Switching from Markup.Escape(...) strings to Text cells is a good safety improvement too. No correctness issues found.
Outstanding architectural point from the prior review — still unaddressed, still not a blocker:
SensitiveEnvironmentVariableNameParts (PipelineInitializer.cs#L24-L32) is a hardcoded private static readonly string[] with no way for a consuming pipeline to extend or override it. This duplicates a pattern the codebase already solves correctly elsewhere:
SecretMaskingOptions(src/ModularPipelines/Options/SecretMaskingOptions.cs) already exists as a DI-configurable options record for exactly this kind of "how do we detect/mask secrets" behavior — consumers configure it viabuilder.Services.Configure<SecretMaskingOptions>(...).HttpLoggingOptions.SensitiveHeaderNamesfollows the same shape (IReadOnlyList<string>+ staticDefault) for the equivalent "which names are secret-ish" problem on the HTTP logging side.
Concretely, the substring match here is intentionally broad (Contains("KEY"), Contains("PWD"), etc.), so it now always masks PWD/OLDPWD and any variable whose name happens to contain one of these substrings (e.g. TURNKEY_ENDPOINT, PRIMARY_KEY_COLUMN). Every consumer of this framework loses visibility into those values in the startup table, with no opt-out short of a fork or a follow-up PR to this repo.
Suggested follow-up: fold the sensitive name-part list into SecretMaskingOptions and inject it into PipelineInitializer the same way ISecretObfuscator is already injected. That keeps the two "mask by name heuristic" mechanisms in the codebase consistent and gives consumers a real escape hatch for false positives.
Not required for this fix to be correct or safe to ship — flagging again since it's still open, for the author/maintainer to decide whether to track as a follow-up issue.
There was a problem hiding this comment.
Code review
Reviewed the secret-masking / table-rendering changes in PipelineInitializer.CreateEnvironmentVariablesTable. The masking logic itself (name-heuristic short-circuit before obfuscation, CR/LF normalization applied after obfuscation, ordinal-ignore-case matching, Text instead of Markup.Escape to avoid re-interpretation) is sound and the new tests cover the intended behaviors well. One high-signal correctness bug found in the layout change — see inline comment.
No CLAUDE.md compliance issues found (only the root CLAUDE.md applies to these files, and the changes don't touch build/CI/module/codegen conventions it governs).
df0a3b6 to
2587cd8
Compare
|
Addressed the unresolved narrow-terminal review finding in
TDD: the focused test failed before the production change because the name rendered one character per line; it now passes. Validation:
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2587cd8 to
6ee031e
Compare
|
Rebased onto current @codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ee031ef63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "SECRET", | ||
| "PASSWORD", | ||
| "KEY", | ||
| "PWD", |
There was a problem hiding this comment.
When trace logging is enabled on Linux or macOS, the process environment normally contains PWD as the current working directory (and may contain OLDPWD), but this substring is treated as a password marker and replaces those paths with the secret mask. This regresses useful environment diagnostics on virtually every Unix shell; keep masking names such as SERVICE_PWD while special-casing the standard directory variables or using delimiter-aware matching.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9962bc32f1. Exact PWD and OLDPWD names now bypass the heuristic, while credential-like names such as SERVICE_PWD remain masked. Added regressions for both standard variables; the focused class passes 12/12.
Code reviewNo issues found. Checked for bugs, CLAUDE.md compliance, and correctness of the secret-masking logic. What I checked:
No CLAUDE.md violations, no compile/logic issues, no gaps in the masking heuristic that would defeat the PR's stated purpose. |
6ee031e to
9962bc3
Compare
|
Addressed the P2 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9962bc32f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "KEY", | ||
| "PWD", | ||
| "CREDENTIAL", | ||
| ]; |
There was a problem hiding this comment.
When trace logging is enabled and a credential was not registered with ISecretObfuscator, common secret-bearing names such as DOCKER_AUTH_CONFIG and NPM_CONFIG__AUTH match none of these fragments, so their raw credential content is rendered in the environment table (or exposed as an ellipsized prefix). Include authentication-related names in the sensitive-name predicate, with narrow exemptions if necessary for non-secret variables such as SSH_AUTH_SOCK.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5409ad9e42. Names containing AUTH are now masked, covering DOCKER_AUTH_CONFIG and NPM_CONFIG__AUTH; SSH_AUTH_SOCK is explicitly exempted as non-secret. Both masked and exemption cases have regression coverage.
Code reviewNo issues found. This is a follow-up review of the changes since the last Claude review — specifically commit What changed since the last review:
Validation performed:
Nothing further to flag — the PWD/OLDPWD fix is correct, narrowly scoped, and tested. |
Render environment values as single-line ellipsized cells and conservatively mask sensitive variable names even when their values were not registered with the secret provider.
Allow the ellipsized value column to shrink so narrow terminals do not render environment names vertically. Refs #3548.
Exclude standard PWD and OLDPWD names from heuristic masking while retaining credential-like *_PWD protection. Refs #3548
Treat authentication-bearing environment variable names as sensitive while preserving SSH_AUTH_SOCK as a narrow non-secret exemption. Refs #3548
9962bc3 to
5409ad9
Compare
|
Addressed authentication-bearing environment names in @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5409ad9e42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "KEY", | ||
| "PWD", | ||
| "CREDENTIAL", | ||
| "AUTH", | ||
| ]; |
There was a problem hiding this comment.
When trace logging runs without native CI masking, the documented Redis setup places a credential-bearing connection string in REDIS_URL (docs/docs/distributed/github-actions.md:49-63), but this name matches none of these fragments, and RedisDistributedOptions.ConnectionString is not marked with [SecretValue]; consequently CreateEnvironmentVariablesTable can render the password verbatim when it was not separately registered. The rebased code now covers AUTH, but REDIS_URL remains unmatched, so mask this documented name or register its value before the environment table is rendered.
AGENTS.md reference: AGENTS.md:L149-L153
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bf8a51b782. Exact REDIS_URL is now treated as sensitive, preventing the documented credential-bearing connection string from reaching trace output even when it was not registered separately. Added a regression; PipelineInitializerTests pass 16/16 and scoped format is clean.
Code reviewReviewed the latest commit, What changed: Correctness: the exemption check runs before the substring Architectural point (repeating from earlier review rounds, still unaddressed — not a blocker): this commit is a good illustration of why the earlier suggestion still stands. Each round has added one more hardcoded substring to The codebase already has the right shape for this in two places:
Since this PR keeps discovering new substrings/exemptions through review iteration, that's a reasonable signal the heuristic has stabilized enough to lift into an options record (sensitive name-parts + exemption list) injected into No bugs or CLAUDE.md violations found in this commit's diff. Tests are well-targeted for the new behavior. |
Prevent documented REDIS_URL credentials from reaching trace output.
|
Addressed the latest P1 review by masking documented |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf8a51b782
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "KEY", | ||
| "PWD", | ||
| "CREDENTIAL", | ||
| "AUTH", |
There was a problem hiding this comment.
When trace logging is enabled without native CI masking or prior secret registration, AZURE_APPCONFIG_CONNECTION_STRING still bypasses this predicate because its name contains none of the listed fragments. The repository explicitly documents this variable as containing an access key (src/ModularPipelines.Azure/Options/AzAppConfigRevisionListOptions.Generated.cs:36), so a sufficiently wide environment table can emit that credential verbatim; include connection-string names in the sensitive-name handling rather than limiting the exact-name fallback to Redis.
AGENTS.md reference: AGENTS.md:L149-L153
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8dd0fe0f63. Sensitive-name matching now covers CONNECTION_STRING, .NET-style CONNECTIONSTRING, and Azure CONNSTR variants, while retaining the exact REDIS_URL fallback. Regressions cover AZURE_APPCONFIG_CONNECTION_STRING, ConnectionStrings__Default, and SQLCONNSTR_Main; focused tests pass 19/19.
Code reviewReviewed the latest commit, What changed: adds a new Correctness: the new Architectural point (repeating from earlier review rounds — still unaddressed, still not a blocker, but worth escalating): this commit adds a third hardcoded, private list to this same mechanism —
Folding Not required for this fix to be correct or safe to ship. Given this is now the third round adding to the same hardcoded heuristic, I'd suggest tracking the options-record refactor as a concrete follow-up issue rather than continuing to patch the array in-place. |
Cover standard and Azure connection-string environment variable names in trace output.
|
Expanded masking to standard, .NET, and Azure connection-string environment variable conventions with regression coverage. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dd0fe0f63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "CONNECTION_STRING", | ||
| "CONNECTIONSTRING", | ||
| "CONNSTR", |
There was a problem hiding this comment.
When trace logging is enabled in an Azure Functions environment using connection-string authentication, AzureWebJobsStorage can contain a storage connection string with an AccountKey; none of the added CONNECTION_STRING, CONNECTIONSTRING, or CONNSTR fragments occur in that standard setting name, so the predicate returns false and the unregistered credential is rendered in the environment table. The final follow-up list still does not match AzureWebJobsStorage, so add it to the exact sensitive names.
AGENTS.md reference: AGENTS.md:L149-L153
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Review: #3561 — Prevent secret leaks from environment table wrapping
Verified against the linked root cause (#3548): GitHub's ::add-mask:: masking only matches a secret on a single physical line, so any value that wrapped across table rows defeated it. This PR fixes that correctly:
new Text(displayValue).Ellipsis()replaces the old markup string cell, guaranteeing a single physical row per value (and, as a side benefit, switches fromMarkup.EscapetoText, which is inherently safe against env values/names that happen to contain Spectre markup syntax like[red]).MakeSingleLineneutralizes embedded\r/\nso an obfuscated-but-multi-line value can't still spill across rows.- The name-heuristic list (
TOKEN,SECRET,PASSWORD,KEY,PWD,CREDENTIAL,AUTH,CONNECTION_STRING/CONNECTIONSTRING/CONNSTR, plusREDIS_URL) closes the gap where a sensitive-looking value was never registered with the secret provider and so the existingISecretObfuscatorcouldn't touch it — this is exactly what let the realGITHUB_TOKENleak in the incident. - Good attention to false-negative-vs-false-positive tradeoff:
PWD/OLDPWD/SSH_AUTH_SOCKare explicitly exempted so the broadPWD/AUTHsubstring matches don't nuke everyday non-secret vars, and there's a test for each exemption plus each masked pattern. - Pulled the checked logic out into a small pure static method (
CreateEnvironmentVariablesTable) that's unit-testable without going throughIConsoleWriter/DI — good testability call, and it's exercised at a real console width (40 cols) rather than just asserting on the string list, so the truncation behavior is actually verified, not just the substring detection. - I ran
PipelineInitializerTestslocally via the guarded script — 19/19 passing, matching the PR description's claim.
I don't have any correctness objections. One structural observation worth raising, not as a blocker for this PR (the linked issue scoped the fix to this one table), but as a follow-up:
The sensitive-name heuristic is now duplicated risk, not duplicated code — and there's already a second, un-covered leak surface for it. IsSensitiveEnvironmentVariableName/SensitiveEnvironmentVariableNameParts live as private static members of PipelineInitializer, scoped to this one render path. But Command.cs's CreateFailureResult (lines ~312-319) builds CommandResult.EnvironmentVariables for CommandException.Result using only _secretObfuscator.Obfuscate(pair.Value, execOpts) — i.e. the same value-only obfuscation that this PR just demonstrated is insufficient for unregistered secrets. CommandException's own XML doc explicitly invites module authors to inspect/print ex.Result for diagnostics, so an env var named e.g. API_KEY or ..._CONNECTION_STRING set for a failing subprocess and never registered as a [SecretValue] would flow through to user code (and potentially logs) completely unmasked — the identical bug class as #3548, just via a different code path.
Given CliCommandFactory.IsInternalEnvironmentVariable already establishes the pattern of a shared, name-based predicate used from both Command.cs and CommandResult.cs, I'd suggest promoting this new heuristic the same way — e.g. as a method on ISecretObfuscator/SecretObfuscator (which is the natural home for "should this be masked" policy, and already sits behind DI so it's swappable/testable), or at minimum an internal static helper shared via CliCommandFactory/a small dedicated type. That would let Command.CreateFailureResult apply the same name-based masking to CommandResult.EnvironmentVariables with a one-line change, instead of requiring a third bug report before the pattern gets generalized (the issue itself notes this is "distinct from #3475 (PipelineLevelLogger path)" — i.e. this has already had to be special-cased more than once).
Not blocking — happy to see this merge as-is and file the follow-up separately if you'd rather keep this PR narrowly scoped to the reported issue.
1 participant
Summary
Textcells so long values never become multiple physical log linesTOKEN,SECRET,PASSWORD,KEY,PWD, orCREDENTIAL, regardless of secret registrationValidation
PipelineInitializerTests: 10 passed, including narrow-console truncation and every sensitive-name heuristicModularPipelines.slnRelease build: 0 errors; 227 existing warningsRotation/expiry of any credentials exposed by historical logs remains an external owner/security action.
Closes #3548