Skip to content

fix(ci): pin Node releases with incident key rejection - #56

Merged
joshavant merged 1 commit into
mainfrom
security/emudhra-node-release
Oct 22, 2026
Merged

joshavant merged 1 commit into
mainfrom
security/emudhra-node-release

Conversation

@joshavant

Copy link
Copy Markdown
Contributor

Select a fixed Node patch within each existing workflow release line so managed jobs reject the compromised eMudhra keys and cannot reuse an older cached patch.

Changed workflow YAML parsed successfully and the patch passes git diff --check; application source and dependency locks are unchanged. Independent review passed. The official Linux arm64 runtimes rejected all 12 incident chains in eight trust modes, with ordinary HTTPS controls passing. Native platform execution, the Actions runner and installed/deployed products remain separate proof and update obligations.

@clawsweeper

clawsweeper Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 21, 2026
@clawsweeper

clawsweeper Bot commented Oct 21, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge.

What this changes

This PR pins both CI Node runtimes to 26.12.1 to include rejection of compromised eMudhra keys.

Example: A CI runner has Node 26.12.0 cached.

  • Before: The Node 26 selector can reuse that older runtime.
  • After: The selector requires Node 26.12.1, whose TLS implementation includes the compromised-key blocklist.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The minimal patch is source-supported and CI passes, but the security behavior is asserted without inspectable final-effect proof.
Proof confidence 🧂 unranked krab (1/6) Real behavior proof is necessary before merge. See Before merge.
Patch quality 🦐 gold shrimp (3/6) Security review found an item that needs attention.

Product

Kind: Maintenance · Worth it: Yes · Fix scope: Complete
User problem: CI tooling can continue using an older Node 26 runtime without the compromised-key rejection update.
Reason: Selecting the security patch preserves the established Node release line and requires no additional production machinery; the supplied maintainer-authored classification settles direction.

Merge readiness

⛔ Blocked before merge - 3 items remain

Keep this PR open: main still permits older cached Node 26 releases, and the two-line pin is a focused security update with no identified code defect.

Priority: P2
Reviewed head: 1cac42624e014ec65def58cb0e18fc72c6d06f68

Before merge

  • Add real behavior proof - Authority-chain proof required: the body reports official Linux arm64 Node runtimes rejecting 12 incident chains in eight trust modes with ordinary HTTPS controls, but contains no terminal trace, logs, media, or linked artifact establishing those results. Attach the existing evidence identifying Node 26.12.1 and showing a compromised-key peer rejected before application data exchange and a valid peer accepted; redact IPs, API keys, and non-public endpoints. These jobs start fresh Node processes, so persisted pre-upgrade TLS sessions are outside the changed workflow path. After adding proof, update the PR body, then request a new review with @clawsweeper re-review. The PR author or someone with repository write access can request it.
  • Resolve security review attention item - No introduced security defect was identified, but the changed certificate trust boundary requires inspectable runtime evidence.
  • Resolve merge risk - Before merge, substantiate the changed TLS trust boundary by attaching the reported runtime trace showing a compromised-key peer rejected before application data exchange and an ordinary HTTPS peer accepted.

Findings

None.

Tests

  • Missing end-to-end proof: Attach the reported official-runtime TLS trace showing compromised-chain rejection before application data exchange and successful ordinary HTTPS traffic; existing CI exercises tooling compatibility rather than that security scenario.
Agent review details

How this fits together

GitHub Actions installs Node for Linux site tests and the Windows repair smoke script; the selected runtime executes repository tooling rather than becoming part of the distributed Go application.

flowchart TD
  A[Pull request or main push] --> B[CI workflow]
  B --> C[Setup Node 26.12.1]
  C --> D[Linux site tests]
  C --> E[Windows repair smoke]
  C --> F[Node TLS verification]
Loading

Technical review

Best possible solution:

Keep the two exact runtime pins and substantiate their security behavior without expanding this PR into runner or deployed-product upgrades.

Do we have a high-confidence way to reproduce the issue?

Source establishes that main's broad selector can accept an older cached Node 26 patch; no runtime attack reproduction was performed during this read-only review.

Is this the best way to solve the issue?

The existing setup-node owner already supports exact patch selection, making these two value changes the smallest complete fix for selecting the patched tooling runtime.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 2abfcd4bd66d.

Merge-risk options

Maintainer options:

  1. Attach the reported TLS verification evidence (recommended)
    Provide redacted terminal output or a linked artifact identifying the official Node 26.12.1 binary and showing compromised-chain rejection before application data exchange alongside a successful ordinary HTTPS control.

Provenance checked

Testing

Proof path: unit tests only.

Security

Needs attention: No introduced security defect was identified, but the changed certificate trust boundary requires inspectable runtime evidence.

Evidence

What I checked:

Likely related people:

  • Vincent Koc: Raw commit d5fddd6 adds .github/workflows/ci.yml:81 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: d5fddd68d9e2; files: .github/workflows/ci.yml)
  • Sebastien Tardif: Raw commit 6779177 adds .github/workflows/ci.yml:151 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: 6779177301d1; files: .github/workflows/ci.yml)

Labels

Label changes:

  • add P2: This is a focused CI runtime security update; the diff does not establish an urgent failure of the shipped application.
  • add merge-risk: 🚨 security-boundary: The runtime update changes certificate-chain acceptance, while the reported allowed and forbidden peer results lack inspectable final-effect evidence.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Label justifications:

  • P2: This is a focused CI runtime security update; the diff does not establish an urgent failure of the shipped application.
  • merge-risk: 🚨 security-boundary: The runtime update changes certificate-chain acceptance, while the reported allowed and forbidden peer results lack inspectable final-effect evidence.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Attach the reported Node 26.12.1 trace proving compromised-key rejection before application data exchange and successful ordinary HTTPS traffic.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 21, 2026, 5:55 PM ET / 21:55 UTC.

@joshavant
joshavant merged commit 4807551 into main Oct 22, 2026
19 checks passed
@joshavant
joshavant deleted the security/emudhra-node-release branch October 22, 2026 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant