Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. Reviewed September 23, 2026, 4:14 AM ET / 08:14 UTC (Revision 2). ClawSweeper reviewWhat this changesUse Node-compatible signing for GitHub App authentication so downloaded PKCS#1 keys and existing PKCS#8 keys both work, with regression tests and documentation. Merge readiness✅ Ready for maintainer review The fix remains necessary on main. The added Worker output satisfies the previous proof request, and this review found no blocking correctness or security concerns. Priority: P2 Review scores
Verification
How this fits togetherHermit’s shared GitHub authentication helper turns configured App credentials into installation-token headers. GitHub summaries and form actions use those headers for GitHub API requests. flowchart LR
A[Configured App credentials] --> B[Sign App JWT]
B --> C[GitHub installation token exchange]
C --> D{Token available?}
D -->|Yes| E[Authenticated headers]
D -->|No| F[Anonymous headers]
E --> G[Summary and form requests]
F --> G
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep one shared signer that accepts both supported PEM formats while preserving existing installation access, JWT claims, and fallback behavior. Do we have a high-confidence way to reproduce the issue? Yes, source establishes the failure: main passes PKCS#1 bytes to a PKCS#8-only importer and catches the error as anonymous fallback. This review did not execute a reproduction. Is this the best way to solve the issue? Yes. Using the Worker’s existing Node crypto support removes the faulty parser, and the supplied runtime evidence covers both downloaded keys and the existing PKCS#8 path. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against e5bc0b0bb5d4. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
|
@clawsweeper re-review The PR body now includes the saved workerd result for both key formats and the executed Web Crypto verification condition. The mocked GitHub exchange is explicitly identified. The production source is unchanged, and exact-head build/test and CodeQL checks are green. |
|
🦞👀 Re-review progress:
|
What Problem This Solves
Fixes: GitHub summaries silently use anonymous requests when configured with the RSA private key downloaded from a GitHub App, losing installation access and authenticated rate limits.
User Impact
Both GitHub-downloaded PKCS#1 keys and existing PKCS#8 PEM keys authenticate successfully. No key conversion or configuration migration is needed.
Why This Change Was Made
The old parser removed either PEM header but always passed the bytes to a PKCS#8-only importer. Use the Worker’s existing Node crypto compatibility to sign either supported PEM format, removing the manual key parser and base64 encoder.
Evidence
workerd(Wrangler 4.127.1, the repository’s compatibility date and Node compatibility). Generated PKCS#1 and PKCS#8 keys both produce signatures independently verified by Web Crypto. The GitHub token exchange is mocked; no live GitHub access is claimed.Validation commands:
bun install --frozen-lockfilein both packages;bun test tests/githubAuth.test.ts tests/githubSummary.test.ts;bun run typecheckin both packages;bun run deploy:dry-run;bun run test; focused recovery withbun test tests/lobsterArtFinalization.test.ts. The additional synthetic Worker proof runs the unchanged authentication code underwrangler dev --local.Inspectable Worker runtime result
The following line is copied from the successful Linux
workerdrun, launched withbunx wrangler dev --config tmp/auth-proof/wrangler.json --local --port 8797and invoked over localhost. The tested production source was unchanged when committed asaf2b07dd98da2275cf75eb73dd5e5a323c633208; the run occurred before that commit. The temporary proof Worker importedgetGitHubHeadersfrom the production module.{"runtime":"workerd","passed":["pkcs1","pkcs8"],"liveGitHub":false}For each PEM format, the harness intercepted the installation-token exchange and independently verified the produced JWT with a public key imported by Web Crypto. It returned HTTP 500 unless both verification and the synthetic returned authorization header succeeded. These are the corresponding conditions from the executed harness:
All key material was generated for this test. The GitHub exchange was mocked; this establishes Worker signing and header behavior, not live GitHub installation access. No keys, JWTs, real tokens, host addresses, or private endpoints are included here.
Exact-head Build and test and CodeQL checks passed.