fix(adapters): preserve Responses URL authority - #1718
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe Responses URL builder now uses ChangesResponses URL handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized URL-authority fix preserves the intended request destination while keeping existing routing behavior unchanged; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
@coderabbitai review Please review exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
requested='23d10de33ce8eb25bfd12c0e69e3f9447a2f0e26'
echo '== HEAD =='
git rev-parse HEAD
echo '== requested commit =='
git rev-parse "$requested"
echo '== commit metadata =='
git show -s --format='%H%nParents: %P%nSubject: %s' "$requested"
echo '== changed files =='
git diff-tree --no-commit-id --name-status -r "$requested"
echo '== exact diff =='
git diff "${requested}^" "$requested" --check
git diff --find-renames --find-copies "${requested}^" "$requested" -- .
echo '== outlines of changed source files =='
git diff-tree --no-commit-id --name-only -r "$requested" |
while IFS= read -r f; do
case "$f" in
*.ts|*.tsx|*.js|*.jsx)
echo "--- $f"
ast-grep outline "$f" --items all --view expanded || true
;;
esac
done
echo '== URL helper candidates and usages =='
rg -n -i -C 4 \
'(responsesPath|/v1/responses|new URL\s*\(|key-auth|keyAuth|baseURL|baseUrl)' \
-g '*.ts' -g '*.tsx' -g '*.js' -g '*.jsx' .Length of output: 201513 🏁 Script executed: #!/bin/bash
set -euo pipefail
requested='23d10de33ce8eb25bfd12c0e69e3f9447a2f0e26'
printf '%s\n' '== commit identity =='
printf 'HEAD: '; git rev-parse HEAD
printf 'requested: '; git rev-parse "$requested"
git show -s --format='parent: %P%nsubject: %s' "$requested"
printf '%s\n' '== changed paths =='
git diff-tree --no-commit-id --name-status -r "$requested"
printf '%s\n' '== patch =='
git diff --no-ext-diff --unified=80 "${requested}^" "$requested" -- \
$(git diff-tree --no-commit-id --name-only -r "$requested")
printf '%s\n' '== changed-file outlines =='
while IFS= read -r file; do
case "$file" in
*.ts|*.tsx|*.js|*.jsx)
printf '\n--- %s ---\n' "$file"
ast-grep outline "$file" --items all --view expanded
;;
esac
done < <(git diff-tree --no-commit-id --name-only -r "$requested")
printf '%s\n' '== focused helper symbols and test labels =='
git grep -n -E \
'(responsesPath|responses.*URL|response.*URL|/v1/responses|authMode.*key|authMode.*forward)' \
"$requested" -- \
'*.ts' '*.tsx' '*.js' '*.jsx' |
head -n 500Length of output: 10751 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== URL helper with line numbers =='
nl -ba src/adapters/openai-responses-url.ts
echo '== adapter references to the helper and route selection =='
rg -n -C 12 '\bopenaiResponsesUrl\s*\(|\bresponsesPath\b|\bauthMode\b' \
src/adapters tests/openai-responses-passthrough.test.ts
echo '== baseUrl validation declarations and rules =='
rg -n -C 12 \
'(baseUrl.*(URL|url|credentials|query|fragment)|new URL\(.*baseUrl|URL\(.*baseUrl|must not include embedded credentials)' \
src tests \
-g '*.ts' -g '*.tsx'
echo '== focused changed test section with line numbers =='
nl -ba tests/openai-responses-passthrough.test.ts | sed -n '100,210p'
echo '== all direct helper usages at requested commit =='
git grep -n -E '\bopenaiResponsesUrl\s*\(' HEAD -- '*.ts' '*.tsx'Length of output: 1184 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== URL helper with line numbers =='
awk '{ printf "%6d %s\n", NR, $0 }' src/adapters/openai-responses-url.ts
echo '== adapter references to the helper and route selection =='
rg -n -C 12 '\bopenaiResponsesUrl\s*\(|\bresponsesPath\b|\bauthMode\b' \
src/adapters tests/openai-responses-passthrough.test.ts
echo '== baseUrl validation declarations and rules =='
rg -n -C 12 \
'(baseUrl.*(URL|url|credentials|query|fragment)|new URL\(.*baseUrl|URL\(.*baseUrl|must not include embedded credentials)' \
src tests \
-g '*.ts' -g '*.tsx' | head -n 800
echo '== focused changed test section with line numbers =='
awk 'NR >= 100 && NR <= 210 { printf "%6d %s\n", NR, $0 }' \
tests/openai-responses-passthrough.test.ts
echo '== all direct helper usages at requested commit =='
git grep -n -E '\bopenaiResponsesUrl\s*\(' HEAD -- '*.ts' '*.tsx'Length of output: 133208 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== provider baseUrl validation locations =='
rg -n -C 8 \
'(baseUrl must not|baseUrl.*(query|fragment|credentials)|embedded credentials|search.*=.*""|hash.*=.*""|protocol.*https?:)' \
src \
-g '*.ts' | head -n 500
echo '== configuration module candidates =='
fd -t f -e ts . src | rg '(^|/)(config|schema|validation|provider).*[.]ts$' | head -n 200
echo '== exact config validation function context =='
rg -n -C 30 \
'function (providerConfigError|validateProvider|providerManagementConfigError)|export function (providerConfigError|validateProvider|providerManagementConfigError)' \
src \
-g '*.ts' | head -n 700
echo '== exact diff hygiene =='
git diff --check 81ada7cd092d4be3b25f3013c996cd3262a2f99b 23d10de33ce8eb25bfd12c0e69e3f9447a2f0e26
git status --shortLength of output: 40269
No findings. The change in The helper is used only for non-forward requests with no
The added tests cover the affected hostname cases, a non-default port, and an IPv6 literal. ✅ Action performedReview finished.
|
23d10de to
eca01d7
Compare
|
@coderabbitai review Please review exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
requested='eca01d7780c7cc70646d2e8d31caa5b207904149'
echo '== Exact commit identity =='
echo -n 'HEAD: '; git rev-parse HEAD
echo -n 'Requested: '; git rev-parse "$requested"
git show -s --format='Parents: %P%nSubject: %s%n%n%B' "$requested"
echo '== Changed paths =='
git diff-tree --no-commit-id --name-status -r "$requested"
echo '== Diff check =='
git diff --check "${requested}^" "$requested"
echo '== Incremental patch =='
git diff --no-ext-diff --find-renames --find-copies --unified=80 \
"${requested}^" "$requested" -- \
$(git diff-tree --no-commit-id --name-only -r "$requested")
echo '== Changed TypeScript file outlines =='
git diff-tree --no-commit-id --name-only -r "$requested" |
while IFS= read -r file; do
case "$file" in
*.ts|*.tsx|*.js|*.jsx)
echo "--- $file"
ast-grep outline "$file" --items all --view expanded || true
;;
esac
done
echo '== Focused references to changed URL helper and routing controls =='
rg -n -C 10 \
'\bopenaiResponsesUrl\s*\(|\bresponsesPath\b|\bauthMode\b|/v1/responses|new URL\s*\(' \
src tests -g '*.ts' -g '*.tsx' | head -n 1200
echo '== Working tree status =='
git status --shortLength of output: 99170 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Helper and all call sites =='
rg -n -C 20 '\bopenaiResponsesUrl\s*\(' \
src tests -g '*.ts' -g '*.tsx'
echo '== Responses adapter URL selection =='
ast-grep outline src/adapters/openai-responses.ts --items all --view expanded | head -n 500
rg -n -C 35 \
'(responsesPath|openaiResponsesUrl|buildRequest|baseUrl)' \
src/adapters/openai-responses.ts
echo '== URL helper and test file relevant sections =='
nl -ba src/adapters/openai-responses-url.ts
nl -ba tests/openai-responses-passthrough.test.ts | sed -n '1,210p'
echo '== Validation call graph and URL validation tests =='
rg -n -C 12 \
'\bproviderBaseUrlConfigError\s*\(|baseUrl must not include query strings or fragments|baseUrl must not include embedded credentials' \
src tests -g '*.ts' -g '*.tsx'
echo '== Direct helper tests and URL-shape expectations =='
rg -n -C 10 \
'(openaiResponsesUrl|responses URL|Responses key-auth URL construction|baseUrl.*[?#])' \
tests src/adapters -g '*.ts' -g '*.tsx'
echo '== HTTPS policy for provider POST requests =='
rg -n -C 15 \
'provider POST URL must use HTTPS|providerOutboundRequest|providerPost' \
src/lib/provider-outbound.ts src/adapters/openai-responses.tsLength of output: 21505 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_lines() {
local file="$1"
local start="$2"
local end="$3"
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
}
echo '== URL helper =='
show_lines src/adapters/openai-responses-url.ts 1 80
echo '== Key-auth URL tests =='
show_lines tests/openai-responses-passthrough.test.ts 120 190
show_lines tests/openai-responses-passthrough.test.ts 2015 2040
echo '== Provider URL validation and configuration entry point =='
show_lines src/config.ts 745 785
rg -n -C 20 \
'(providerBaseUrlConfigError\(|superRefine|safeParse\(|parse\()' \
src/config.ts | head -n 800
echo '== Helper import and URL test coverage across the repository =='
rg -n -C 6 \
'from ["'\''].*openai-responses-url|openaiResponsesUrl\s*\(' \
src tests -g '*.ts' -g '*.tsx'
echo '== Query or fragment provider URL tests =='
rg -n -C 8 \
'(query strings or fragments|embedded credentials|baseUrl.*[?]x=|baseUrl.*#)' \
tests -g '*.ts' -g '*.tsx' | head -n 700
echo '== Exact diff against the requested parent =='
git diff --check eca01d7780c7cc70646d2e8d31caa5b207904149^ eca01d7780c7cc70646d2e8d31caa5b207904149Length of output: 58591
No findings.
I also applied the existing repository context for provider URL validation.
🧠 Learnings used
|
|
Thanks for the fix — parsing with new URL() and editing only pathname is the right call, and the table tests cover the authority-mangling cases well. Keeping this as draft for process reasons only: as an external-contributor draft it has not had full CI (fork limitation), so it needs the readiness checklist completed and a maintainer-triggered full CI run before merge. Flagging for maintainer CI. |
lidge-jun
added
provider
b2f4e38 to
cd4f2fb
Compare
cd4f2fb to
22184b2
Compare
|
Rebased onto current Exact-head validation passed: typecheck, 68 focused Responses URL/passthrough tests (219 assertions), privacy scan, and diff check. Stable patch ID is unchanged. The local-all-CI, findings, and ready boxes remain unchecked until exact-head maintained CI and bot review complete. Maintainer action requested: please approve the fork-gated Cross-platform CI run 32027698956 and React Doctor run 32027698715. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Reviewed exact head 22184b219cd450a017a7d6867822d506a815f848.
Approve. I found no actionable P0–P2 correctness, compatibility, architecture, or test-coverage issue in this patch. Parsing the validated base URL and normalizing only pathname correctly prevents host/authority components such as responses, v1, non-default ports, and IPv6 literals from being consumed by path suffix handling, while preserving the existing custom responsesPath branch.
The focused regression coverage is appropriate and exercises the concrete authority-preservation failure modes.
Merge should still wait for the exact-head Cross-platform CI run to finish green and for the repository's separate explicit security-review requirement for credential-destination changes to be satisfied.
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Explicit security review for exact head 22184b219cd450a017a7d6867822d506a815f848: APPROVE.
I found no actionable P0–P2 security issue in this credential-destination change.
Security-specific checks:
openaiResponsesUrl()now parses the already-validated base URL and mutates onlypathname; scheme, hostname, port, and authority are preserved instead of being exposed to string-wide suffix replacement.- API-key auth is attached only after the request URL is constructed, so this patch removes the previous authority-confusion path rather than introducing a new credential destination.
- Provider config validation continues to require HTTP(S), reject embedded userinfo, query strings, and fragments, and apply the existing destination/private-network policy before this adapter path is used.
- Hostnames named
responsesorv1, non-default ports, and IPv6 literals are covered by focused regressions, including the concrete authority-preservation cases that matter for credential routing. - The change does not alter forward-mode header passthrough, custom
responsesPath, redirect handling, secret storage, logging, or authentication semantics outside this URL helper.
Security gate satisfied from my review.
|
Thanks @luvs01 — this is a small patch with an important payoff. The fix makes the Responses URL construction operate on the parsed pathname instead of the entire URL string, so host authorities such as The focused regression coverage around those authority edge cases is also very useful. Thanks for tightening this up. |
3 participants
Summary
responsesorv1, non-default ports, and IPv6 literalsresponsesPathrouting unchangedThe previous string-wide suffix replacement could treat a hostname as a path segment (for example
https://responses) and construct a different authority. Because the adapter attaches API credentials to the resulting request, this is both a routing-correctness and credential-destination fix.Scope and compatibility
This changes only the default key-auth
/v1/responsesURL helper. Provider configuration already requires an HTTP(S) URL and rejects embedded credentials, queries, and fragments. URL serialization may canonicalize equivalent syntax such as host case or a default port; it does not change the destination.No UI is changed.
Verification
Exact rebased range:
b5a6654786a2bafb0759dae92bbcfc47515de1c3→22184b219cd450a017a7d6867822d506a815f848.git diff --checkpassedff189d1726cd47d768ce4566aa53b86fcf7cfd6b87bad3b6-a829-4953-be42-c9b01e63de00: no P0–P2 findings; snapshotcodex-security-snapshot/v1:sha256:78b507ea3064e622652c4f6de2a2bea99c2ffe31d78e373e375015315696a669The full repository suite is not claimed green. A Bun 1.3.14 attempt reproducibly exited with an internal assertion in the unrelated
api-storage-policy-run.test.ts, including when isolated. A Bun 1.4 canary run completed with existing unrelated Windows effective-account/ACL/symlink/process-timeout and catalog failures. Maintained exact-head CI remains authoritative.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
/v1bases, custom ports, and IPv6 addresses./v1/responses.