Don't follow HTTP redirects on LLM API requests - #10281
Open
dpage wants to merge 2 commits into
Open
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 6 remain after this review. WalkthroughThe change adds a shared opener that blocks HTTP redirects for LLM requests. Provider calls and model discovery use the opener. Tests cover redirect rejection, normal responses, and default redirect behavior. ChangesLLM redirect safety
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change stops LLM API requests from following redirects and preserves ordinary provider error handling; no actionable merge-blocking risk remains, subject to normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ALLOWED_LLM_API_URLS is checked against the URL pgAdmin is configured with, but the providers then issued the request through urllib.request.urlopen(), whose default handler would follow a Location header on to a destination that check was never applied to. No legitimate LLM API replies with a redirect, so refuse them outright rather than re-validating each hop: urlopen_no_redirect() installs a handler that raises HTTPError instead, and the outbound calls in all four providers and in the model-refresh endpoints now go through it. This is hardening rather than a fix for an exploitable flaw, since returning the redirect in the first place requires control of a host that is already on the allowlist. Reported by Ziya Abdullayev.
HTTPRedirectHandler only gained http_error_308() in Python 3.11, so on the 3.9 and 3.10 interpreters we still support (and which CI runs) a 308 was never recognised as a redirect: it bypassed _NoRedirectHandler's redirect_request() entirely and surfaced from http_error_default() as a bare 'HTTP Error 308: Permanent Redirect'. The redirect was still not followed, so this was never a security gap, but the caller got no explanation of why the request had failed, and the test asserting our message failed on every CI job as a result. Mapping http_error_308 onto http_error_301, exactly as 3.11 itself does, routes 308 back through redirect_request() so that every redirect status behaves identically on every version we support.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ALLOWED_LLM_API_URLSis checked against the URL pgAdmin has been configured with, but the providers then issued the request throughurllib.request.urlopen(), whose default handler will happily follow aLocationheader on to a destination that check was never applied to. Validating the first URL does not imply validating every subsequent hop.No legitimate LLM API replies with a redirect, so this refuses them outright rather than re-validating each hop.
urlopen_no_redirect()inweb/pgadmin/llm/utils.pymirrorsurlopen(), including its handling of an explicit SSL context, but installs anHTTPRedirectHandlersubclass that raisesHTTPErrorinstead of following. All eleven outbound call sites now go through it: two each in the Anthropic, OpenAI and Docker providers, three in Ollama (including theis_available()probe), and the four model-refresh helpers inweb/pgadmin/llm/__init__.py. Every one of those sites already caughtHTTPErrororURLError, so a redirect surfaces as an ordinary provider error and no call site needed restructuring.I should be clear that this is hardening rather than a fix for an exploitable flaw, because returning the redirect in the first place requires control of a host that is already on the allowlist: either one of the vendor entries, which would mean the vendor itself had been compromised, or a loopback entry, which needs code execution on the pgAdmin host and therefore grants direct access to the same services anyway. The shipped configuration already documents that the loopback entries let any logged-in user reach any port on the server's own loopback interface. The residual case worth closing is an administrator who allowlists an internal self-hosted endpoint that happens to carry an open redirect, and in any event there is no reason for us to be following redirects here at all.
Tests are in
web/pgadmin/llm/tests/test_no_redirect.py. They stand up a real loopback HTTP server rather than mocking, and cover 301, 302, 303, 307 and 308 across both GET and POST, asserting each time that the redirect target recorded no hit. There is also a control case asserting that a plainurllib.request.urlopendoes follow the same fixture, so the refusal tests cannot pass vacuously against a fixture that never redirected.Reported by Ziya Abdullayev, whose report prompted the change.
Summary by CodeRabbit
Security
Bug Fixes