Skip to content

fix(download): require trust for injected sessions - #9524

Open
wunianze666-netizen wants to merge 2 commits into
invoke-ai:mainfrom
wunianze666-netizen:codex/fix-trusted-download-session
Open

fix(download): require trust for injected sessions#9524
wunianze666-netizen wants to merge 2 commits into
invoke-ai:mainfrom
wunianze666-netizen:codex/fix-trusted-download-session

Conversation

@wunianze666-netizen

@wunianze666-netizen wunianze666-netizen commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • prevent a caller-supplied requests.Session from silently bypassing the socket-level SSRF guard while private-address downloads are disabled
  • add a default-off requests_session_is_trusted opt-in and reject untrusted injected sessions in the guarded configuration
  • preserve the existing explicit allow_private_download_urls=true opt-out
  • document the trust boundary, mark test-only injected sessions explicitly, and add regression coverage for both rejection and trusted opt-in

The default production path is unchanged: when no session is injected, the download queue creates its SSRF-guarded session as before.

Related Issues / Discussions

Refs #9493, specifically follow-up item 6.

This PR intentionally does not close #9493 because that issue tracks several independent download-queue security follow-ups.

QA Instructions

Local validation:

  • py -m pytest tests/app/services/download/test_download_queue.py -q — 38 passed
  • the two new caller-supplied-session security regression tests passed
  • Ruff check and format-check passed for the changed Python files
  • git diff --check passed

Additional SSRF-suite validation produced 59 passing tests and one unchanged Windows environment failure: urllib.getproxies() reads the machine's system proxy registry. The failure is unrelated to this patch.

Reviewers may want to focus on the compatibility boundary:

  1. injected sessions are rejected only when the private-address policy is enabled and the caller has not explicitly trusted the session;
  2. sessions created internally continue to use the socket guard;
  3. operators who explicitly enable private download URLs retain the existing behavior.

Merge Plan

No special merge steps are required. This is a single backend security commit with focused tests and documentation; it does not change database schemas, Redux state, generated assets, or release metadata.

Checklist

  • The PR has a short but descriptive title, suitable for a changelog
  • Tests added / updated (if applicable)
  • ❗Changes to a redux slice have a corresponding migration — N/A, no Redux changes
  • Documentation added / updated (if applicable)
  • Updated What's New copy (if doing a release after this PR) — N/A, this is not a release PR

@github-actions github-actions Bot added python PRs that change python files services PRs that change app services python-tests PRs that change python tests docs PRs that change docs labels Aug 21, 2026
@lstein lstein self-assigned this Aug 24, 2026
@lstein lstein added the 6.14.1 label Aug 24, 2026
@lstein lstein moved this to 6.14.1: Bug fixes to 6.14.0 in Invoke - Community Roadmap Aug 24, 2026

@lstein lstein left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial review at 97c9645540.

All checks are green on this head: 38 download-queue tests, 1235 model-manager/model-install tests, 6 model-load tests, ruff check + format clean. The branch is 12 commits behind main, but nothing in the touched paths has moved, so there is no drift to resolve.

There is no correctness regression here. My concern is that the mechanism doesn't do what the PR describes, and I'd like items 1 and 2 addressed before merge.


1. The trust flag is not falsifiable — it rejects the real guard and accepts a bare Session

I ran the new constructor against the two sessions that matter (invokeai/app/util/ssrf is already imported in this file):

session passed in requests_session_is_trusted result
ssrf.build_guarded_session() — the actual socket-level guard omitted rejected with "bypasses the SSRF socket guard"
requests.Session() — no guard, trust_env=True True accepted; get_adapter(...) is not an SsrfGuardedAdapter

The gate reads a boolean the caller typed; it never inspects the session. So it refuses the one session that genuinely carries the protection, and admits the one that carries none. Anyone who can reach requests_session= can reach requests_session_is_trusted= in the same call — the PR itself demonstrates the pattern at ~25 sites, including the four SSRF regression tests that inject a session specifically to prove non-public URLs are refused. And there is no in-tree production injection site (dependencies.py:180 passes no session), so the net effect on the shipped app is zero.

The change that makes this measure something real:

if requests_session is not None:
    guarded = isinstance(requests_session.get_adapter("https://example.com"), SsrfGuardedAdapter)
    if not guarded and not self._app_config.allow_private_download_urls and not requests_session_is_trusted:
        raise ValueError(...)

That drops the false rejection above and keeps the attestation exactly where it is needed — a mock transport that cannot be checked.

2. The error message's suggested remedy is strictly wider than the thing it blocks

...or use allow_private_download_urls to opt out of the private-address policy.

allow_private_download_urls=True also disables validate_download_url() at download_default.py:627, which runs for every download and, via _reject_unsafe_redirect, for every redirect hop — including on the default guarded session. Offering the global kill switch as the alternative to a narrow per-session opt-in inverts the risk gradient: a developer who follows the message to keep their injected session working turns off strictly more than the gate they were denied. Please drop that clause.

3. The docs promote injection to a supported configuration, but that path silently drops download_proxy

The new doc text says injection "may be useful in other contexts... Only opt in for a Session whose destination policy you trust." Verified with download_proxy="http://proxy.internal:3128" configured:

  • injected + trusted: _request_proxies is None, session.proxies == {}, trust_env is True, and no warn_if_proxied warning is logged
  • default guarded path, same config: session.proxies carries the proxy and the warning fires

So an operator following the new documentation gets their configured egress proxy silently ignored, with ambient ALL_PROXY honoured in its place. This is pre-existing behaviour, but this PR is what blesses injection as a supported, documented opt-in — at which point the gap becomes a documented-path defect. Either set _request_proxies on the injected branch too, or leave the doc saying "unit tests only".

4. Hard break for out-of-tree callers

The ValueError fires at service construction, i.e. at app startup, with no config-only remedy. A downstream integrator injecting a session for a corporate CA bundle or auth headers gets a non-booting app on upgrade and must edit code to recover. The isinstance acceptance in item 1 avoids that for the sessions that deserve to pass; for the rest, consider a loud warning rather than a raise.


Minor

  • Doc wording: "rejected while the private-address policy is enabled" reads as though private addresses were permitted. Say "while allow_private_download_urls is false (the default)". (Not this PR's fault, but the table you're adding to still documents event_handlers and quiet, which the constructor hasn't accepted in a long time.)
  • The rewritten comment still asserts "Sessions we build ourselves refuse to connect to a non-public address" — untrue of the elif self._app_config.allow_private_download_urls branch immediately below it, which builds a plain Session. Also pre-existing, but the comment was touched here.
  • test_caller_supplied_session_accepts_explicit_trust never closes its Session (the neighbouring tests close in finally) and asserts only object identity. The sibling test constructs InvokeAIAppConfig inside the pytest.raises block, so a validation error from the config would satisfy the match only by accident.

What I checked and found fine

  • Every in-tree injection site is updated: tests/app/services/download/test_download_queue.py and tests/backend/model_manager/model_manager_fixtures.py are the only two files that construct with a session, and no test root config sets allow_private_download_urls, so the fixture change was genuinely required.
  • Both new tests are load-bearing, and test_download_allows_non_public_source_when_opted_in pins the "opt-out on -> untrusted session still accepted" branch, so a future over-tightening of the gate would be caught.
  • Raising mid-__init__ leaves nothing dangling: no threads are started and the class has no finalizer that touches _requests.

@wunianze666-netizen

Copy link
Copy Markdown
Contributor Author

Addressed the requested review changes in de36b1a5a:

  • caller-supplied sessions carrying SsrfGuardedAdapter are now recognized directly and no longer require a trust attestation;
  • unguarded test transports still require requests_session_is_trusted=True while the private-address policy is active;
  • the error no longer suggests disabling allow_private_download_urls globally;
  • the architecture text now limits session injection to the unit-test facility rather than promoting it as a supported runtime configuration;
  • the constructor tests close their sessions, and config construction is outside the pytest.raises block.

I added a regression that failed on the reviewed head because build_guarded_session() was rejected. After the fix, all 39 download-queue tests pass. Ruff check and format check pass on the changed Python files. I left the branch unre-based because the review confirmed that the touched paths have not drifted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.14.1 docs PRs that change docs python PRs that change python files python-tests PRs that change python tests services PRs that change app services

Projects

Status: 6.14.1: Bug fixes to 6.14.0

Development

Successfully merging this pull request may close these issues.

Download-queue SSRF follow-ups (from #9492 review)

2 participants