fix(privacy): detect SSH endpoints per token, and stop scanning on import - #4975
Conversation
privacy:scan knew about tokens, emails and home paths, and nothing about infrastructure. A working Host block was therefore publishable: the scan passes on dev today, where devlog/_fin/260731_pr_merge_round/022_remote_test_offload.md still carries a real HostName, account and Cloudflare ProxyCommand. #4623 removes them by hand; nothing stops the next devlog reintroducing them. Two detectors, both anchored to the SSH config grammar: HostName value must be the whole rest of the line ProxyCommand command line, matched separately `User` is deliberately not matched. It is an ordinary English word and even line-anchored it fires on wrapped prose — "…the\nuser configuration." and "…the\nuser notice." both matched during development, as did `hostname === undefined ? ...` in a test file before the value was anchored. The username is also the least sensitive part of a Host block, and MAINTAINER_HOME_USERNAME already covers it in path form. A ProxyCommand containing %h is NOT allowlisted. Only a bare %h is. The substitution token does not make the binary path, the access method or the tunnel any less of a leak — allowing it would have passed the exact line this exists to catch. Also moves the repo scan behind import.meta.main. It ran at module scope, so `import { scanText }` executed a full scan as a side effect and a failing scan called process.exit(1), killing the importing test process. Invisible while the tree is clean; adding the detector above broke privacy-scan-meta-key.test.ts, which does nothing but import the seam this file exports for testing. Refs #4623
… doc Review follow-ups from @lidge-jun on #4734. The scanText JSDoc still claimed the module runs its scan on import — the thing this PR removed. Left as-is, the next contributor writes against a side effect that no longer exists. Now states the import is side-effect free and why the seam exists at all. ProxyCommand findings move to their own kind and join REDACTED_FINDING_KINDS. The value carries the binary path, the access method and the tunnel options, and it was being echoed verbatim into stderr and CI logs — which are far more widely readable than the diff it was caught in. A bare HostName stays visible: that one is the context a reviewer needs to find the line. ...:44 ssh-endpoint: HostName ssh-macmini.lidgeai.com ...:46 ssh-proxy-command: <redacted> Also indents the runScan body a level, which the extraction had left flat.
… real host Two problems with this PR as it stood, both the same shape as the leak it exists to catch. The report printed the `ssh-endpoint` value while redacting the `ProxyCommand`. This scan runs in CI on a public repository, so a finding would have republished the endpoint into a public log — the scanner leaking what it was written to detect. `file:line` already locates it for whoever removes it, which is what the ProxyCommand kind has relied on all along. The regression fixture and the rationale comment both spelled out the real hostname and login. #4623 removes those from the devlog; keeping them here would have undone that cleanup and made this file their permanent home. The fixture now uses a synthetic endpoint — the regex cannot tell the difference — and the comment describes the incident without restating the values. Co-authored-by: Abhishek Sharma <abhicse24@gmail.com>
Two review nits from the PR gate. `scanText`'s JSDoc repeated the whole import-side-effect explanation that `runScan` already carried, so an edit to either left the other stale. It now states only its own concern — the test seam — and points at `runScan` for the rest. `runScan`'s JSDoc sat above the `if (import.meta.main)` guard rather than the function it describes, so tooling and readers attached it to the wrong construct. Moved onto the declaration; the guard reads fine unannotated.
|
Warning Review limit reachedNext included review available in 4 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
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. |
리뷰 · 우선순위 77 / 80이 PR은 #4734(@abhisheksharma2411)를 현재 지금 탐지기 쪽은 #4623이 손으로 지운 공개 데브로그 Host 블록을 다시 못 내보내게 막는 게이트입니다. #4623은 2026-09-17에 이미 머지됐고, 원 PR이 기다리던 «dev가 아직 빨갛다» 순서는 이미 풀렸습니다. 새 kind는 캐리가 고친 핵심 결함은 예전 의도적으로 안 잡는 것도 본문·테스트에 박혀 있습니다. 우선순위 77입니다. 보안 경계·우회 핀·import 부작용·레이아웃·#4623 이후 단독 랜딩이 현재 라인 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: 79d4519460
ℹ️ 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".
| if (/^\$\{?[A-Za-z_][A-Za-z0-9_]*\}?$/.test(token)) return true; | ||
| if (/^\{+[^{}]*\}*$/.test(token)) return true; | ||
| if (/^[}>]+$/.test(token)) return true; | ||
| // Judge `deploy@host.example.com:22` on its host part: userinfo and port name no |
There was a problem hiding this comment.
When the CLI scans this newly added comment, the existing email detector recognizes deploy@host.example.com, and isAllowedEmail does not allow that value in scripts/privacy-scan.ts. I ran bun run privacy:scan on this commit and it exits with code 1 at this exact line, so the mandatory privacy gate cannot pass; construct the example from fragments or use a non-email-shaped placeholder.
AGENTS.md reference: scripts/AGENTS.md:L21-L25
Useful? React with 👍 / 👎.
The allowance asked whether a directive value *contained* something allowlisted. For HostName that is the same as asking about its single token, but a ProxyCommand value is a command line, so one reserved name or one leading substitution cleared the whole line and the real endpoint with it. Two bypasses followed, both now pinned as tests: a value beginning with "$" was allowed outright by the templated-prefix rule, and an unanchored reserved-name test cleared a command whose proxy hop was example.com while it named the real host three tokens later. The same unanchoring read example.com.internal-buildfarm.net as documentation. Every token must now be a placeholder, and every placeholder rule is anchored to the whole token. A ProxyCommand is a leak by default; only a wholly templated value is documentation. Also accept the two ssh_config spellings that hid a directive from the detector entirely: a trailing "# comment" after a HostName value, and the "ProxyCommand=value" form. The "HostName=value" form is deliberately not accepted, because that shape is ordinary TypeScript and three such lines are in src/server/ports.ts and src/server/port-reclaim.ts today. Co-authored-by: Abhishek Sharma <abhicse24@gmail.com>
tests/test-layout.test.ts requires every test file to resolve to a domain, and tests/test-layout-tooling.test.ts checks the resolver against an independent fixture oracle. "privacy-scan-ssh-endpoint" is not covered by the ci-workflows regex seeds, so the new file needs an explicit entry in both tables. Co-authored-by: Abhishek Sharma <abhicse24@gmail.com>
79d4519 to
d244287
Compare
|
Merging with macOS legs outstanding, and recording why rather than leaving it implicit. At this exact head the full Linux suite (test 1/4 through 4/4), This change is platform-neutral, so waiting on a queue that is both saturated and known-unreliable would delay the work without adding information. The evidence that governs the release is not per-PR macOS legs; it is the full-platform Stating the boundary plainly: this is merged on Linux, gates and cross-platform smoke evidence at its exact head, with macOS coverage deferred to the candidate run rather than claimed here. |
|
Agreeing with the carry, and thanks for making the difference reviewable rather than silent — and for the The defect is real and the diagnosis is exactly right. The sharpest part: the unanchored reserved-name rule read Verified the landed scanner on All four bypasses closed, and the placeholder case still passes. Worth noting the one that matters most: One consequence worth stating as a known trade rather than leaving to be discovered. Requiring every token to be a placeholder means only a wholly abstract value passes: A documentation example written the way a user would actually copy it — real command words plus placeholders — is a finding, because No disagreement with any of it. The layout-table entries and the stale doc comment were both mine to have caught: running the repo's own mechanical gates is exactly the step I skipped. |
2 participants
Carries #4734 by @abhisheksharma2411, rebased onto current
devwith the review finding fixed.Summary
privacy:scanknew about tokens, emails and home paths, and nothing about infrastructure, so a devlog could publish a working SSHHostblock and the scan passed. That is how a runner hostname and a CloudflareProxyCommandshipped in a published devlog; #4623 removed them by hand and has since merged. This adds the two detectors that make the next one fail the gate instead, and stops the module running a full repo scan as an import side effect.Security boundary. These detectors are the gate that keeps infrastructure endpoints out of a public repository and out of CI logs. Both new finding kinds are redacted in the failure output, because this scan runs in CI on a public repository and printing the value would republish the endpoint into a more widely readable place than the diff it was caught in.
file:lineis enough to find it.The defect this carry fixes. The original
isAllowedSshEndpointasked whether the directive value contained something allowlisted. ForHostNamethat is the same as asking about its single token. ForProxyCommandit is the wrong question: the value is a command line, so one allowed token anywhere in it cleared the whole line, real endpoint included. Two bypasses followed, and both are now pinned as tests:$was allowed outright by the templated-prefix rule, so a command whose first token was a variable was never examined.example.com.internal-buildfarm.net— a real host that merely begins with a reserved name — as documentation.The replacement parse splits the value on whitespace and requires every token to be a placeholder, with each placeholder rule anchored to the whole token. Userinfo and a trailing port are stripped before the host part is judged, so neither can be the reason a real host reads as a placeholder. A
ProxyCommandis a leak by default; only a wholly templated value is documentation.HostNamebehavior is unchanged apart from the anchoring.Two ssh_config spellings that hid a directive from the detector entirely are now matched: a trailing
# commentafter aHostNamevalue, and theProxyCommand=valueform.What this does not guarantee. The
HostName=valueform is deliberately still unmatched, and that is a real residual gap rather than an oversight:hostname = "127.0.0.1",is ordinary TypeScript and three such lines are insrc/server/ports.tsandsrc/server/port-reclaim.tstoday, so accepting that spelling would fail the scan on this tree. A test pins the decision so the tradeoff is visible to whoever revisits it. More broadly, this is a textual scanner over tracked files: it catches theHostblock shape that actually shipped, not an endpoint described in prose, encoded, or split across lines, and it does not inspect untracked files or git history.Second fix, carried unchanged from the original. The module ran its scan at module scope, so
import { scanText }executed a full repo scan as a side effect — and a failing scan calledprocess.exit(1), taking the importing process with it. That is invisible while the tree is clean and bites the moment any detector finds something; adding these rules brokeprivacy-scan-meta-key.test.ts, which does nothing but import the seam this file exports for testing. The scan is now behindimport.meta.main. The CLI entry point is unchanged.Also fixed on the carry: the new test file had no entry in
scripts/test-layout/layout.jsonortests/fixtures/test-layout-expected.json.privacy-scan-ssh-endpointis not covered by theci-workflowsregex seeds, so both layout guards would have failed. And a doc comment left over from an earlier revision still claimedssh-endpointwas not redacted while the code redacted it; the comment now matches the code.Sequencing is resolved. The original PR noted it would be red on
devuntil #4623 landed. #4623 merged on 2026-09-17, and no tracked file matches either detector today, so this lands standalone.Verification
Local verification was not run: this lane forbids running any local suite, typecheck, build, or install. Hosted CI on this PR head is the executable verification.
Static checks performed in place of local execution:
privacy:scanred on merge.HostName=valueform, which is what proved it cannot be accepted: three lines insrc/server/ports.tsandsrc/server/port-reclaim.tswould become findings.privacy:scanreads it too. One assertion was rewritten becauseuser@host.tldinside a test string is anemailfinding.Checklist
Co-authored-by: Abhishek Sharma abhicse24@gmail.com