[codex] fix: reject credentials in advertised endpoint URLs#3516
[codex] fix: reject credentials in advertised endpoint URLs#3516StiensWout wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
ApprovabilityVerdict: Needs human review This PR adds security validation to reject URLs with embedded credentials. While the change is small and well-tested, security-related changes warrant human review to verify the validation is appropriately placed and handles all relevant code paths. You can customize Macroscope's approvability policy. Learn more. |
4ea013b to
2ffeb29
Compare
Co-authored-by: Codex <codex@openai.com>
2ffeb29 to
13861a1
Compare
Summary
Why
URLs with embedded credentials should not be accepted as advertised endpoints because they can leak secrets through display, logging, or transport boundaries.
Impact
Credential-bearing advertised endpoints are rejected before they can be stored or used.
Validation
PATH="$HOME/.vite-plus/bin:$PATH" vp test packages/client-runtime/src/environment/endpoint.test.tsPATH="$HOME/.vite-plus/bin:$PATH" vp checkPATH="$HOME/.vite-plus/bin:$PATH" vp run -r --concurrency-limit 1 typecheckNote
Low Risk
Small input-validation hardening on endpoint URLs; only affects credential-bearing URLs that should not be advertised.
Overview
Advertised endpoint URLs with embedded userinfo are now rejected during normalization instead of being accepted and propagated.
normalizeHttpBaseUrlinadvertisedEndpoint.tsthrows"Endpoint URL must not include credentials."whenurl.usernameorurl.passwordis non-empty, before path/query/hash normalization. That applies to HTTP/HTTPS and ws/wss inputs (after protocol coercion), socreateAdvertisedEndpoint,deriveWsBaseUrl, and other callers that normalize through this helper fail early. Connection onboarding that normalizeshttpBaseUrlsurfaces the same error as invalid configuration.Tests cover
https://user:password@…,wss://user@…, and credential-bearingcreateAdvertisedEndpointinputs.Reviewed by Cursor Bugbot for commit 13861a1. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Reject credentials embedded in advertised endpoint URLs
Adds a credentials check to
normalizeHttpBaseUrlin advertisedEndpoint.ts. If the URL contains a non-emptyusernameorpassword, the function throws"Endpoint URL must not include credentials."before any normalization occurs. Behavioral Change: callers passing URLs with embedded credentials will now receive an error instead of silently proceeding.Macroscope summarized 13861a1.