fix(security): scs-legacy refuses acs_token over remote plaintext http (H4) - #13
Conversation
…p (H4) PR-F of the security-hardening plan. - Refuse to send the acs_token over plaintext http to a non-loopback host (it leaks via access logs/proxies/caches). https and loopback http are allowed; RELAIS_SCS_ALLOW_INSECURE=1 (or =true) is an explicit override. Loopback is decided by real IpAddr::is_loopback() (127.0.0.0/8, ::1) so a lookalike like 127.0.0.1.evil.com is correctly NOT loopback; scheme is lowercased so HTTP:// can't bypass; userinfo can't smuggle a host. - Enforced at exec start (AdapterError::Unsupported); with_base_url warns at construction. - Redact the acs_token from any upstream error body before it reaches logs/audit/callers (defense in depth). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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: 7546da6dcd
ℹ️ 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".
| let authority = rest.split(['/', '?']).next().unwrap_or(""); | ||
| let hostport = authority.rsplit('@').next().unwrap_or(authority); |
There was a problem hiding this comment.
When SCS_LEGACY_BASE_URL contains a fragment with an @, for example http://scs.example.com#@127.0.0.1, URL parsing still targets scs.example.com because the authority ends before #, but this manual split keeps the fragment in authority and rsplit('@') treats 127.0.0.1 as the host. That lets the new guard allow a remote plaintext HTTP endpoint and send the injected acs_token; parse with a URL parser or stop the authority at # before stripping userinfo.
Useful? React with 👍 / 👎.
| "SCS legacy base_url '{base_url}' is plaintext http to a non-loopback host; \ | ||
| acs_token requests will be refused unless RELAIS_SCS_ALLOW_INSECURE=1 — prefer https" |
There was a problem hiding this comment.
If a remote plaintext base URL includes userinfo, such as http://user:pass@scs.example.com, this new warning logs the full base_url verbatim before the request is refused. Since the transport parser explicitly accepts userinfo-shaped URLs, this can leak embedded credentials into tracing logs; log only a sanitized scheme/host or strip userinfo first.
Useful? React with 👍 / 👎.
1 participant
PR-F of the security-hardening plan. Finding H4: the legacy SCS adapter sends an
acs_tokenin the GET query / POST body (an upstream-mandated contract), and the default base URL is plaintexthttp://.Change
httpto a non-loopback host — it would leak through access logs, proxies, and caches.httpsand loopbackhttpare allowed;RELAIS_SCS_ALLOW_INSECURE=1(ortrue) is an explicit per-area override for trusted private networks.IpAddr::is_loopback()(127.0.0.0/8,::1), so a lookalike like127.0.0.1.evil.comis correctly not loopback. The scheme is lowercased soHTTP://can't bypass; bracketed IPv6 anduser:pass@hostuserinfo are handled.exec(AdapterError::Unsupported→ 400);with_base_urlwarns at construction if the configured URL would be refused.acs_tokenfrom any upstream error body before it can reach logs/audit/callers.The token transport mechanism (query for GET) is unchanged — it is the upstream contract; this PR governs where it's allowed to go and scrubs it from error text.
Tests
check_transport: https/loopback-http allowed; remote-http blocked unless override; lookalike + userinfo + uppercase-scheme rejected; 127/8 +[::1]accepted.cargo test --workspacegreen; clippy clean.Codex-reviewed; the flagged case-sensitive-scheme BLOCKER, error-body token leak, and env-format consistency are all fixed.
🤖 Generated with Claude Code