Repository navigation
Track credential handoffs through step outputs in secret-leak analysis - #71850
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
![github-actions[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
One opportunity to keep fast-mode reconciliation conservative and smaller.
net: -17 lines possible.
Generated by ✂️ Ponytail Reviewer for #71850 · codex · gpt56 · ~13 AIC · ⌖ ~6 AIC · ⊞ 14.8K
Comment /ponytail to run again
| return true | ||
| } | ||
| if fastSiblingSite(compiled.Path, authored.Path, target, from) { | ||
| return true |
There was a problem hiding this comment.
L68-91: shrink: positional sibling-site matching plus its path parser. Keep the exact source-to-compiled mapping above; unmatched entries already take the documented conservative fallback, removing 17 lines.
There was a problem hiding this comment.
I left this separate size optimization unchanged; the requested code changes were limited to the eight correctness findings in the linked review.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (847 new lines in 📄 Draft ADR committed: Decision captured in the draft: For
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Request changes
The new output transport analysis has a false-negative path: dynamic shell expansions can write multiple output records, while the analyzer binds taint only to the first statically inferred name. A secret can therefore reach another named step output without being reported. The inline comment includes the required conservative fix and regression.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~8 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| commandWord, _ := sliceAt(call.Args, 0) | ||
| first, _ := sliceAt(call.Args, 1) | ||
| name := literalShellWord(commandWord) | ||
| if name == "echo" && len(call.Args) == 2 && singleOutputWord(first) { |
There was a problem hiding this comment.
A dynamic expansion in the value written by echo can contain additional newline-delimited output records, but this path still binds the taint only to the statically inferred name. That lets a secret written as token=...\nother=... be consumed through steps.producer.outputs.other without propagation, producing a false negative.
💡 Preserve conservative tracking for dynamic records
singleOutputWord only rejects newlines in static text; it does not account for parameter or command substitutions. If the payload contains any expansion, bind the write to the wildcard output key (or emit a coverage gap) unless the record structure is proven single-line. Add a regression covering a variable containing a newline and a downstream consumer of the second output name.
There was a problem hiding this comment.
Fixed in fd9f316: dynamic expansions add wildcard output bindings, and the regression verifies a newline-delimited second output reaches its consumer.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd (via fallback heuristic — pr-triage ran successfully as bug_fix, confirming shell.go, step_outputs.go, javascript_workflow_fast.go as high-impact).
This is a well-scoped fix: the transport-vs-disclosure distinction is isolated behind transportOutput, is gated strictly (secret-leak only, workflowContext required, named+resolved+modeled), and is backed by an extensive regression suite (step_outputs_test.go, command_output_contents_test.go) covering shell, JavaScript, cross-language, fast-mode, and conservative-fallback paths. The compiler-lint integration test confirms end-to-end behavior for the motivating scenario.
📋 Key Themes & Highlights
Key Themes
- Readability of the policy gate (
step_outputs.go): the core decision combining rule ID, workflow context, name resolution, and transport modeling is a single 5-term||— worth a named boolean for future maintainers. - Minor shadowing:
copyas a local variable injavascript_workflow.goshadows the Go builtin; a descriptive name would help. - Thin direct coverage of path-matching arithmetic:
fastStepLocation/fastSiblingSiteinjavascript_workflow_fast.godo index-offset string parsing that's only exercised indirectly through end-to-end fast-mode tests; a few focused unit tests would pin the contract down before the path format is touched again.
Positive Highlights
- ✅ Conservative-by-default design: unresolved names/IDs, compound writes, and unmatched fast-mode entries all retain findings/gaps rather than silently suppressing them.
- ✅ Clear separation: custom (non-
secret-leak) rules keep seeingOutputfindings for transported values, preserving existing rule semantics (TestStepOutputJobBoundaryAndGenericRule). - ✅ Docs updated in lockstep (
workflow-linters.md) explaining the new transport semantics and its limits.
No blocking issues — the three inline notes above are maintainability/test-coverage suggestions, not correctness concerns.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~106 AIC · ⌖ ~16 AIC · ⊞ 10.3K
Comment /matt to run again
| j.files["$GITHUB_OUTPUT"] = union(j.files["$GITHUB_OUTPUT"], sources) | ||
| transport := j.a.stepOutput | ||
| if transport != nil && j.options != nil && j.options.fastMatched != nil && j.options.fastEntries == nil && len(j.options.fastSites[j.input.Name]) == 0 { | ||
| copy := *transport |
There was a problem hiding this comment.
[/codebase-design] Shadowing the builtin copy identifier as a local variable name hurts readability and could trip up future readers expecting the builtin.
💡 Suggested rename
transport := j.a.stepOutput
if transport != nil && j.options != nil && j.options.fastMatched != nil && j.options.fastEntries == nil && len(j.options.fastSites[j.input.Name]) == 0 {
unmodeled := *transport
unmodeled.modeled = false
transport = &unmodeled
...
}A name like unmodeled or fallback also documents intent better than copy.
@copilot please address this.
There was a problem hiding this comment.
Addressed in fd9f316: the transport copy was removed in favor of updating the shared per-step transport state.
| } | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] fastSiblingSite/fastStepLocation do subtle offset arithmetic (ci-ti == ai-fi) to correlate compiled vs. authored step indices, but there is no direct unit test for these helpers — coverage is only indirect via TestFastStepOutputKeepsEveryCompiledConsumer.
💡 Suggested tests
Add table-driven tests directly against fastStepLocation and fastSiblingSite covering: no steps[ substring, malformed index, nested steps[ (e.g. matrix/reusable workflow paths), and mismatched prefixes. This pins down the parsing contract before someone refactors the string-based path format and silently breaks matching.
@copilot please address this.
There was a problem hiding this comment.
I did not add the separate path-parser table tests because the requested code scope was limited to the linked correctness review; the fast-mode regressions cover the affected per-occurrence matching behavior.
| if len(sources) > 0 && transport != nil && (!named || !resolved) { | ||
| a.gapAt(site.Path, site.Line, "step output name or step id is unresolved; downstream output tracking is conservative") | ||
| } | ||
| if a.rule.ID != "secret-leak" || !a.workflowContext || !named || !resolved || !transport.modeled { |
There was a problem hiding this comment.
[/codebase-design] The final gate combines five conditions in one || expression, mixing rule identity, context, and two independent name/transport-resolution checks. This is the crux of the bug fix (deciding transport vs. disclosure) so its readability matters more than most lines in this diff.
💡 Suggested refactor
privateTransport := a.rule.ID == "secret-leak" && a.workflowContext && named && resolved && transport.modeled
if !privateTransport {
a.emit(site, sources)
}Naming the condition (privateTransport) documents the policy this function encodes and makes future changes (e.g. adding another rule ID) safer to reason about.
@copilot please address this.
There was a problem hiding this comment.
I left this readability-only refactor unchanged to keep code changes within the linked review's requested scope.
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
🟡 Changes recommended
Output-record inference and fast-mode verification can suppress disclosure findings without preserving downstream secret taint.
8 open issues
What changed in this PR
This PR updates gh-aw’s secret-leak analysis to distinguish private credential handoffs from disclosure while preserving taint for downstream checks.
Changes:
- Model named shell and JavaScript step outputs as same-job transport.
- Replay authored JavaScript through compiled consumers in fast mode and track command-file readbacks.
- Add transport, disclosure, and shared-import regressions, plus documentation.
| File | Description |
|---|---|
pkg/workflow/taint/workflow.go |
Establishes workflow transport context and integrates fast-mode analysis. |
pkg/workflow/taint/taint.go |
Adds analyzer state for step-output transport. |
pkg/workflow/taint/taint_test.go |
Updates expected findings and conservative coverage gaps. |
pkg/workflow/taint/step_outputs.go |
Centralizes output bindings and transport exemptions. |
pkg/workflow/taint/step_outputs_test.go |
Covers handoffs, downstream disclosures, and unresolved boundaries. |
pkg/workflow/taint/shell.go |
Adds output-record inference and command-file tracking. |
pkg/workflow/taint/safe_output_policy_test.go |
Updates expectations for unresolved output identities. |
pkg/workflow/taint/javascript.go |
Adds fast-mode entry-matching state. |
pkg/workflow/taint/javascript_workflow.go |
Connects JavaScript outputs and returns to workflow bindings. |
pkg/workflow/taint/javascript_workflow_fast.go |
Replays authored entries and reconciles findings. |
pkg/workflow/taint/javascript_fast_test.go |
Updates the expected compiled diagnostic location. |
pkg/workflow/taint/javascript_eval.go |
Filters and records fast-mode entries. |
pkg/workflow/taint/javascript_calls.go |
Models output transport and command-file reads. |
pkg/workflow/taint/command_output_contents_test.go |
Covers readback disclosures and step-local file lifetimes. |
pkg/workflow/taint/authored.go |
Separates standalone and workflow-aware authored analysis. |
pkg/workflow/compiler_lint_test.go |
Adds shared-import credential-resolver regressions. |
docs/src/content/docs/reference/workflow-linters.md |
Documents transport semantics and conservative fallbacks. |
Review effort: Max
| // Authored exemptions require a successful compiled-entry match. Compiler | ||
| // normalization or unavailable generated wrappers must remain conservative. | ||
| options.fastEntries = nil | ||
| scripts, scriptErr := analyzeAuthoredJavaScript(ctx, document, trackedRule, true) |
There was a problem hiding this comment.
Fixed in fd9f316: unmatched authored JavaScript provenance now marks step-output transport unmodeled when tainted stdout/files flow into shell writes, retaining the Output finding.
| if _, exists := j.options.fastSites[j.input.Name]; exists { | ||
| return |
There was a problem hiding this comment.
Fixed in fd9f316: fast-mode verification is now tracked per source occurrence, and each javascript instance uses its own verification result when transporting outputs.
| commandWord, _ := sliceAt(call.Args, 0) | ||
| first, _ := sliceAt(call.Args, 1) | ||
| name := literalShellWord(commandWord) | ||
| if name == "echo" && len(call.Args) == 2 && singleOutputWord(first) { |
There was a problem hiding this comment.
Fixed in fd9f316: backslash-bearing echo words are no longer inferred as a single named output, so escaped record boundaries retain conservative findings and wildcard propagation.
| } | ||
|
|
||
| func singleOutputWord(word *syntax.Word) bool { | ||
| if strings.ContainsAny(staticWord(word), "\r\n") { |
There was a problem hiding this comment.
Fixed in fd9f316: dynamic shell expansions now add wildcard output bindings, with a regression for a variable containing a second newline-delimited output record.
| s.bindings[key] = union(s.bindings[key], sources) | ||
| } | ||
| s.files[target] = union(s.files[target], sources) | ||
| s.a.transportOutput(site, name, sources, s.a.stepOutput) |
There was a problem hiding this comment.
Fixed in fd9f316: shell and JavaScript writes share per-step record state; unresolved or unterminated records bind wildcard taint and preserve Output findings.
| return err | ||
| } | ||
| previous := a.stepOutput | ||
| a.stepOutput = &stepOutputTransport{id: id, bindings: bindings, modeled: a.workflowContext} |
There was a problem hiding this comment.
Fixed in fd9f316: job environment.url is evaluated after steps against output bindings and emitted as a Display sink.
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the PR finisher pass. The linked correctness findings are fixed in
Implemented the eight linked correctness fixes in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
secret-leakreports private step-output credential handoffs as disclosure before evaluating their consumers. This shared root cause blocked 62 pinnedgh-caoworkflows; those occurrences do not establish 62 credential leaks.core.setOutput, and GitHub Script returns.The intended authentication handoff requires no suppression:
Compatibility limitation: The requested pinned 74-workflow rerun remains blocked by organization SAML authorization (403); no updated corpus pass counts are available.