[codex] fix: guard DPoP fallback URL construction#3503
Conversation
Co-authored-by: Codex <codex@openai.com>
…-guard [codex] fix: guard DPoP fallback URL construction
|
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 |
github-actions
Bot
added
vouch:unvouched
ApprovabilityVerdict: Needs human review Changes to auth code require human review. Additionally, this modifies error handling behavior - requests that previously used fallback URL construction will now return authentication errors when URL parsing fails. You can customize Macroscope's approvability policy. Learn more. |
| export function requestAbsoluteUrl(request: HttpServerRequest.HttpServerRequest): string | null { | ||
| try { | ||
| return new URL(request.originalUrl).href; | ||
| } catch { | ||
| const host = firstHeaderValue(request.headers.host) ?? "127.0.0.1"; | ||
| const forwardedProto = firstHeaderValue(request.headers["x-forwarded-proto"]); | ||
| const proto = forwardedProto === "https" || forwardedProto === "http" ? forwardedProto : "http"; | ||
| return new URL(request.originalUrl, `${proto}://${host}`).href; | ||
| try { | ||
| return new URL(request.originalUrl, `${proto}://${host}`).href; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
not sure why we don't just use HttpServerRequest.toURL(request). Returns an Option.Option<URL>
There was a problem hiding this comment.
Updated to use HttpServerRequest.toURL(request) and kept the invalid-url guard.
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
Head branch was pushed to by a user without write access
2 participants
Summary
Root cause
requestAbsoluteUrlcaught invalid absoluteoriginalUrlparsing, then built a fallback URL fromoriginalUrlandHostwithout guarding that secondnew URL(...)call.Impact
Malformed Host values during DPoP auth now fail as
ServerAuthInvalidCredentialErrorwithInvalid DPoP request URL.instead of surfacing as a defect. Valid absolute URLs and existing relative URL fallback behavior are preserved.Validation
PATH="$HOME/.vite-plus/bin:$PATH" vp test apps/server/src/auth/dpop.test.tsPATH="$HOME/.vite-plus/bin:$PATH" vp checkPATH="$HOME/.vite-plus/bin:$PATH" vp run typecheckNote
Medium Risk
Touches authentication and DPoP URL binding; malformed or previously edge-case URLs may now be rejected rather than verified or throwing at runtime.
Overview
DPoP proof verification no longer builds request URLs locally.
verifyRequestDpopProofnow resolves the request URL withHttpServerRequest.toURLand, when that returnsNone, fails withServerAuthInvalidCredentialError(Invalid DPoP request URL.) before callingverifyDpopProof.This removes the previous
requestAbsoluteUrl/firstHeaderValuefallback path that could still throw on a secondnew URL(...)whenHostor related inputs were malformed. Valid absolute URLs and normal relative-URL resolution behavior are intended to stay the same; unresolvable URLs become explicit credential failures instead of defects or ambiguous verification input.Reviewed by Cursor Bugbot for commit f1e04ed. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Guard DPoP proof verification against unresolvable request URLs
Replaces the local
requestAbsoluteUrlhelper in dpop.ts withHttpServerRequest.toURL()to derive the request URL for DPoP proof verification. WhentoURLreturnsNone,verifyRequestDpopProofnow rejects the request early with aServerAuthInvalidCredentialError(diagnostic: "Invalid DPoP request URL.") instead of proceeding with a potentially malformed URL.Macroscope summarized f1e04ed.