Surface broker push failures and keep the token out of argv - #220
Conversation
A failed push returned only "git-failed". runChild attaches git's stderr to the error, but the handler logged nothing but the reason code and returned the same bare code to the client, so the single most useful description of the failure was discarded. Diagnosing a refused publication required reading the broker source and reproducing the push by hand outside the broker. The handler now logs the exit code and the redacted stderr and stdout. Secrets are stripped from that output before it is logged, since an operator is likely to paste it into an issue. The documented startup used `sudo env KAIZEN_PUBLICATION_BROKER_TOKEN=...`, which places the credential in the argument vector, where any local process can read it through ps. That defeats the containment the broker exists to provide: the socket is correctly restricted and captureGitHubAuthEnv refuses ambient tokens for builder-capable processes, and then the startup command published the same credential to every process on the machine. The broker now reads the token from a root-owned 0600 file (--token-file) or stdin (--token-stdin), and warns when the deprecated environment variable is used. Refs #217, #218
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: kaizen-agents-org/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe broker now reads GitHub tokens from a root-owned file or stdin. It retains the environment variable as a deprecated fallback, validates token content, and redacts credentials from Git and refusal diagnostics. Onboarding instructions use the protected file method. ChangesSecure token handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
onboarding/scripts/kaizen-publication-broker.mjs (1)
75-94: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject conflicting explicit token sources.
Lines 75 and 94 allow
--token-stdinand--token-filetogether.readBrokerToken()then silently selects the file. Reject this combination so the operator cannot believe that stdin supplies the active token.Proposed fix
if (options.socketGid === undefined) options.socketGid = options.runGid; + if (options.tokenFile && options.tokenStdin) { + throw new Error('--token-file and --token-stdin are mutually exclusive'); + } if (!options.testMode && options.socketGid !== options.runGid) {As per path instructions, prioritize correctness, async error handling, CLI behavior, test coverage, and backward-compatible public contracts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@onboarding/scripts/kaizen-publication-broker.mjs` around lines 75 - 94, Update the argument parsing logic to reject using --token-stdin together with --token-file, regardless of their order. Add the conflict validation around the existing options.tokenStdin and options.tokenFile handling, while preserving each option’s standalone behavior and the existing readBrokerToken contract.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@onboarding/ADOPTING.md`:
- Around line 97-98: Update the --token-stdin example in ADOPTING.md to invoke
the installed broker at /usr/local/libexec/kaizen-publication-broker, matching
the installation path documented earlier while preserving the existing
token-piping usage.
In `@onboarding/scripts/kaizen-publication-broker.mjs`:
- Around line 403-410: Update the token-file handling around options.tokenFile
to open the path once with O_NOFOLLOW, reject symbolic links, and validate
ownership, regular-file status, and permissions using fstatSync() on that same
descriptor before reading from it. Replace the separate statSync/readFileSync
path with descriptor-based validation and reading, while preserving the existing
token trimming and error messages.
---
Outside diff comments:
In `@onboarding/scripts/kaizen-publication-broker.mjs`:
- Around line 75-94: Update the argument parsing logic to reject using
--token-stdin together with --token-file, regardless of their order. Add the
conflict validation around the existing options.tokenStdin and options.tokenFile
handling, while preserving each option’s standalone behavior and the existing
readBrokerToken contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: kaizen-agents-org/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4cca1c19-9a26-4fd9-9d99-85e265693f63
📒 Files selected for processing (2)
onboarding/ADOPTING.mdonboarding/scripts/kaizen-publication-broker.mjs
statSync follows symlinks and readFileSync re-resolves the path, so the file that was validated need not be the file that is read. Open once with O_NOFOLLOW, validate that descriptor with fstat, and read from it. Also use the installed broker path in the --token-stdin example.
PR GuardianMergeable, with one caveat recorded below. Feedback addressed
Both replied to in-thread and resolved. The first was a genuine security defect in code I added in this PR: validating the path and then reading it again checks one file and reads another if the path is swapped in between, and Verified after the fix:
Final gateTwo snapshots 35s apart, identical: Base is current with Caveat: CodeRabbit did not re-review the fix commitIts check reports Flagging rather than treating a rate-limited pass as a clean review. If you would prefer a fresh bot review before merging, |
|
✏️ Learnings added
|
1 participant
Two problems in the publication broker, both found while trying to complete a
smoke run against the pinned toolchain.
Closes #218.
1. A failed push was undiagnosable
The broker answered
{"ok":false,"error":"git-failed"}and logged only{"event":"request-refused","reason":"git-failed"}.runChildalready attaches git's full result to the error:…and the handler then threw that away, logging the reason code alone. The one
thing that explains the failure — git's stderr — never reached the operator or
the client.
Establishing anything about a refused push therefore required reading the
broker source, reconstructing the push by hand outside the broker, and
comparing. That exercise showed the push itself works: with the broker's exact
arguments, minimal environment, generated askpass helper and a valid token, the
same push succeeds. So the failure is specific to the broker's own execution
and remains unidentified — precisely because the stderr was dropped.
The handler now logs the exit code plus redacted
stderr/stdout:{"event":"request-refused","reason":"git-failed","exitCode":128, "stderr":"remote: Permission to owner/repo.git denied to x-access-token."}Secrets are stripped first (
gh*_,github_pat_, and anyhttps://user@hostuserinfo), because this output is exactly what an operator will paste into an
issue. Verified against sample git failures containing a token in the remote
URL — the credential is replaced while the diagnostic text survives.
2. The documented startup leaked the token through argv
ADOPTING.mdinstructed:sudo env KAIZEN_PUBLICATION_BROKER_TOKEN="$(gh auth token)" \ /usr/local/libexec/kaizen-publication-broker …sudo env VAR=valueplaces the token in the argument vector, which isworld-readable:
This defeats the containment the broker is built to provide. The socket is
correctly restricted to
root:<run-gid>andcaptureGitHubAuthEnvrefusesambient
GH_TOKEN/GITHUB_TOKENfor builder-capable processes — and then thedocumented startup published the same credential to every process on the
machine, including the agents that are meant to be excluded.
The broker now accepts:
--token-file <path>— must be a regular file, owned by root, with no groupor other permission bits. Rejected otherwise.
--token-stdin— for piping without storing.KAIZEN_PUBLICATION_BROKER_TOKEN— still honoured for compatibility, but thebroker logs
token-source-insecurewhen it is used.ADOPTING.mdnow documents the file-based form, and recommends a fine-grainedtoken scoped to the target repositories rather than an account-wide one.
Verification
onboarding/scripts/test-publication-broker.mjs— all 10 fixtures pass,including
refused requests do not push or expose credentials.--token-filepermission checks exercised directly: a0644file and anon-root-owned file are both rejected.
and bare
gh*_/github_pat_values are replaced, and non-secret text such asfatal: could not read Username for https://github.comis preserved intact.node --checkon the patched file.Not fixed here
#217 (workspaces created
0755while the broker requires0700) isindependent and still open. A run can work around it today by invoking Kaizen
under
umask 077, which makes every directory Kaizen creates0700; theproper fix belongs in workspace/worktree creation in kaizen-loop.
Summary by CodeRabbit
Security Improvements
Documentation