fix(providers): route Actual's fetch_models through the credential-redirect guard - #80247
Closed
pierrenode wants to merge 1 commit into
Closed
Conversation
…direct guard ActualProfile.fetch_models() overrides ProviderProfile's default implementation with its own Actual-specific base_url resolution (ACTUAL_BASE_URL env var, hosted-vs-local normalization), but called raw urllib.request.urlopen(req, timeout=timeout) directly instead of the base class's open_credentialed_url(). Every other provider either uses the base class default or forwards to it via super() and gets SafeCredentialRedirectHandler for free — Actual is the only provider that attaches a Bearer token to its own Request object and opens it with the stdlib's default redirect handling, which forwards every header, including Authorization, across a cross-origin redirect. Actual's own feature surface makes the trigger realistic: ACTUAL_BASE_URL is a first-class, documented way to point this provider at a self-hosted or local-offline endpoint (see the local-loopback no-auth path already handled elsewhere in this provider), so a misconfigured or compromised endpoint 302-ing to another host leaks ACTUAL_API_KEY to it. Fix: import and call the same open_credentialed_url() the base class uses, keeping Actual's own URL-resolution logic unchanged. Adds an end-to-end regression test using two real local HTTP servers (no mocking of the security module itself) — one redirects, the other records the Authorization header it receives — mirroring test_urllib_security.py's own redirect tests. Also repoints the existing fetch_models test's mock from urllib.request.urlopen to hermes_cli.urllib_security.open_credentialed_url, since fetch_models no longer calls the former. Mutation-verified: the new redirect test fails on pre-fix code with the Authorization header observed at the redirect target.
Contributor
|
looks mergeable ActualProfile.fetch_models now uses the shared credential-safe opener for its model catalog request, preventing credential headers from crossing origins while preserving same-origin redirects. Security evidence:
Not checked:
Signed: GPT-5.6-luna-max in Codex |
Contributor
|
The fetch_models redirect-guard commit from this PR landed on main via #86492 with your authorship preserved. Thanks! |
Contributor
|
Closing: this work is now fully on main via #86492 (your commit cherry-picked with authorship preserved). Thanks again! |
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
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.
Summary
ActualProfile.fetch_models()(plugins/model-providers/actual/__init__.py) overridesProviderProfile's default implementation with its own Actual-specificbase_urlresolution, but called rawurllib.request.urlopen(req, timeout=timeout)directly instead of the base class'sopen_credentialed_url(). Every other provider either uses the base class default or forwards to it viasuper()and getsSafeCredentialRedirectHandlerfor free — Actual is the only provider that attaches aBearertoken to its ownRequestobject and opens it with the stdlib's default redirect handling, which forwards every header, includingAuthorization, across a cross-origin redirect.ACTUAL_BASE_URLis a first-class, documented way to point this provider at a self-hosted or local-offline endpoint (see the local-loopback no-auth path already handled elsewhere in this provider), so a misconfigured or compromised endpoint 302-ing to another host leaksACTUAL_API_KEYto it.open_credentialed_url()the base class uses, keeping Actual's own URL-resolution logic unchanged.Test plan
Authorizationheader it receives — mirroringtest_urllib_security.py's own redirect tests.fetch_modelstest's mock fromurllib.request.urlopentohermes_cli.urllib_security.open_credentialed_url, sincefetch_modelsno longer calls the former.Authorizationheader observed at the redirect target (assert ['Bearer actual-secret-token'] == [None]).tests/hermes_cli/test_actual_provider.py,tests/hermes_cli/test_models_dev_preferred_merge.py,tests/hermes_cli/test_urllib_security.py— 28 passed.ruff checkclean on changed files.