[WRONG BRANCH] cli: validate /healthz version before displaying (prevent terminal-injection) - #374
[WRONG BRANCH] cli: validate /healthz version before displaying (prevent terminal-injection)#374luvs01 wants to merge 1 commit into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Its title has been prefixed with |
github-actions
Bot
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c89ef0d9c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const version = typeof body?.version === "string" ? body.version : undefined; | ||
| // Whoever holds the port controls this response. Only carry bounded semver text into | ||
| // diagnostics; dropping anything else prevents terminal controls reaching human output. | ||
| const version = isHealthzVersion(body?.version) ? body.version : undefined; |
There was a problem hiding this comment.
This validation only protects versions returned through proxyIdentityAt; collectStatus can still fall back to checkProxyHealth, which interpolates body.version directly into healthLabel and prints it. For example, when a stale PID/runtime record points to an attacker-held fallback port, a mismatched reported PID makes findLiveProxy reject the listener, but selectListenTarget can select that same recorded port and the fallback probe accepts { service: "opencodex", version: <OSC/newline payload> } without PID validation. Apply isHealthzVersion in checkProxyHealth as well and cover this fallback path with a status regression test.
Useful? React with 👍 / 👎.
1 participant
Motivation
/healthzversionfield is attacker-controlled whenever another process can bind the probed port, and unvalidated strings could include newlines or terminal-control sequences (OSC 52) that reach human-facing commands./healthz.versionas untrusted and validated it with a bounded semver test, so the liveness/version-skew path needed the same protection to avoid diagnostic forgery.Description
isHealthzVersioninsrc/server/proxy-liveness.tsthat accepts semver-like strings up to 64 bytes and rejects anything else.isHealthzVersioninproxyIdentityAtso the discoveredversionis only carried when it matches the bounded semver pattern.isHealthzVersioninsrc/update/job.ts(replacing the module-localisVersionLike) so the update restart probe keeps the same shape validation.tests/proxy-liveness.test.tsverifying that an adversarialversioncontaining a newline and OSC 52 is rejected and that valid prerelease/build semver is accepted.Testing
bun test tests/proxy-liveness.test.ts tests/cli-version-skew.test.tsand they passed (87 tests total)../node_modules/bun/bin/bun.exe test tests/update-job.test.tsand they passed (54 tests).bun run typecheckandbun run privacy:scan(both succeeded) andgit diff --check(no issues).bun testrun encountered unrelated environment-sensitive failures in other test areas (lab harness and platform-specific fixtures) that are outside the scope of this focused change.Codex Task