Repository navigation
Conversation
Reject CR/LF-containing OTLP OIDC tokens before merging credentials and guard writes to GitHub Actions environment/output files. Cover malformed tokens, merged headers, and writer keys/values without exposing rejected data. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality 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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: 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! Ponytail ReviewerOutcome: 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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No actionable blocking issues found in the changed lines. The CR/LF rejection is applied before writing runner environment files, and the added regressions cover LF, CR, and CRLF cases without exposing rejected data. Static review only; tests were not rerun.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 1.48 AIC · ⌖ 0.539 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
🟢 Approval recommended
The localized guards address the injection path, with focused regression coverage and no blocking issues identified.
No open issues 🎉
What changed in this PR
Prevents OTLP token data from injecting environment variables during workflow setup.
Changes:
- Rejects tokens and environment-file keys or values containing CR/LF.
- Adds regression coverage for rejected inputs, secret-safe warnings, and valid single-line behavior.
| File | Description |
|---|---|
actions/setup/js/action_setup_otlp.test.cjs |
Adds token, merged-header, and writer regression tests. |
actions/setup/js/action_setup_otlp.cjs |
Adds newline guards before token merging and file writes. |
Review effort: Max
|
In response to the code quality notification: accepted as informational, with no code changes needed. The accompanying review found no actionable blocking issues in the changed lines. |
|
In response to the Test Quality Sentinel notification: accepted as informational, with no code changes requested. The reported_no_action status is not evidence of passing tests; validation is documented separately in the PR description. |
|
In response to the Matt Pocock Skills Reviewer notification: accepted as a review-start status only. It contains no actionable feedback, so no code changes are needed for this comment. |
|
In response to the Design Decision Gate notification: accepted as informational, with no design change or ADR requested. The reported_no_action status alone does not establish validation, and this comment provides no actionable change to implement. |
|
In response to the Ponytail Reviewer notification: accepted as informational, with no code changes requested. The reported_no_action status is not treated as validation evidence. |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd (per pr-triage) — the fix is solid and the regression is well covered. Approving with one minor optional test-coverage suggestion.
📋 Key Themes & Highlights
Key Themes
- Root cause addressed, not just symptom (
/diagnosing-bugs): the fix gates at the singleinputOTLPOIDCTokenentry point rather than patchingmergeAuthorizationHeaderin isolation, so a CRLF-tainted token can never reach the header/endpoint merge or the GITHUB_ENV write. This is a cleaner single-choke-point design than the inline-stripping approach originally suggested in the linked issue, and it avoids silently mutating a credential. - Non-leaking warnings: both new guards (
writeEnvLineand the token check) callcore.warningwith a generic message and never log the rejected key/value/token — tests explicitly assert the malicious payload never appears incore.info/core.warningcalls. Good defensive logging discipline. - Test coverage (
/tdd):it.each(["\n", "\r", "\r\n"])parametrization covers all three line-break variants for both the genericwriteEnvLineguard and the OIDC-token-specific guard, plus a "preserves valid single-line values" regression test guards against false positives. Left one minor inline suggestion for a dedicatedGH_AW_OTLP_ENDPOINTS-path test (currently only implicitly covered via the sharedwriteEnvLineguard).
Positive Highlights
- ✅ Rejecting the whole tainted value rather than sanitizing/stripping avoids producing a validly-formatted-but-wrong credential
- ✅
writeEnvLineguard is defense-in-depth that also protects other call sites (trace-id/span-id/output writes), not just the OIDC token path - ✅ Closes both linked vulnerability reports (env-file injection and HTTP header injection via OTLP headers) at the same choke point
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 140.3 AIC · ⌖ 15 AIC · ⊞ 10.1K
Comment /matt to run again
|
🎉 This pull request is included in a new release. Release: |
Summary
writeEnvLinerefuse keys or values containing CR/LF and issue a warning that never includes rejected data.Fixes #70460
Fixes #70120
Validation
npm run test:js -- action_setup_otlp.test.cjs action_conclusion_otlp.test.cjs action_otlp.test.cjs— 128 tests passed.make fmt-cjs && make lint-cjs— passed (existing repository warnings only).PATH="/opt/homebrew/bin:$PATH" make agent-report-progress— passed, including build, JavaScript typecheck/lint, and all 74 setup tests.