Security: plugin SSRF guard runs once — redirects bypass it#9
Draft
mimeding wants to merge 2 commits into
Draft
Conversation
PluginHostContext.checkSSRF prevents the plugin host API's http_request from connecting to RFC1918 / loopback / link-local hosts. The check ran exactly once — on the URL the plugin passed in — and the request session followed any redirects without re-checking. That made the guard easy to bypass: a malicious or compromised remote endpoint could respond with 302 Location: http://127.0.0.1:1337 and the URLSession would happily follow into the local server, the metadata service, or any RFC1918 host. Worse, the existing follow_redirects default is true, so plugins didn't even have to opt in. Attach a new SSRFCheckedRedirectDelegate to the redirect-following URLSession singleton. The delegate re-runs PluginHostContext.checkSSRF on every redirect target and refuses the redirect (passes nil to URLSession's completion handler) if the new URL would have been blocked at the entry point. URLSession then surfaces the original 3xx response to the plugin so it can detect the blocked redirect without ever connecting to the private target. The no-redirect session is unchanged. Out of scope for this PR (deserves a separate, focused change): DNS rebinding mitigation. checkSSRF is still purely hostname-based — a public-looking name like attacker.example.com that resolves to 127.0.0.1 is allowed through. Closing that requires resolving the hostname inside the SSRF guard and validating the resolved IPs, which is a much bigger change. Co-authored-by: Michael Meding <mimeding@users.noreply.github.com>
ModelManager.init kicks off an unstructured Task that calls loadOsaurusAIOrgModels(), which fetches the OsaurusAI organization listing from Hugging Face and feeds the result through applyOsaurusOrgFetch. The unit-test runner repeatedly constructs ModelManager() to drive applyOsaurusOrgFetch directly. The background launch-time fetch races with those test calls — whichever finishes last wins, and the merge result is non-deterministic. That's the root cause of the flaky ModelManagerSuggestedTests failures seen across many of the recent PR CI runs (applyOsaurusOrgFetch_dropsStaleAutoFetched OnReapply, applyOsaurusOrgFetch_addsNewEntriesAfterCurated, etc.). Gate the launch-time fetch on a small isRunningInTestEnvironment helper that checks for any of XCTestConfigurationFilePath, XCTestBundlePath, or XCTestSessionIdentifier in the process environment. Those variables are only present inside an xctest host process; production app launches still get the HF fetch exactly as before. This is a network call, so removing it under tests also has the side benefit of making the test suite work offline / on hermetic CI runners. Co-authored-by: Michael Meding <mimeding@users.noreply.github.com>
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
Why this matters (business)
The plugin Host API gives plugins a sandboxed way to make HTTP requests. The whole point of the SSRF guard there is to keep a compromised or malicious plugin from using that capability to reach things it shouldn't — the local Osaurus server (
http://127.0.0.1:<port>), the cloud metadata service (http://169.254.169.254/), RFC1918 internal targets on the user's network, etc.That guard ran exactly once, on the URL the plugin originally requested. URLSession then quietly followed any redirects without re-checking. So:
302 Location: http://127.0.0.1:1337/v1/chat/completions.follow_redirectsdefaults totrue, so plugins don't even need to opt in to be exposed. This is the canonical SSRF bypass and it's worth closing before the plugin ecosystem grows.What's wrong (technical)
httpSessionwas created without any delegate, so URLSession's default redirect handling kicked in — no re-validation of the new URL:noRedirectSessionalready had aNoRedirectDelegatethat suppresses all redirects, but that only applies when the plugin explicitly passesfollow_redirects: false.Fix
Attach a new
SSRFCheckedRedirectDelegatetohttpSession. On every redirect, the delegate runs the samePluginHostContext.checkSSRFit ran on the initial URL. If the new target is RFC1918, loopback, link-local, or otherwise blocked, the delegate refuses by passingnilto URLSession's completion handler. URLSession then surfaces the original 3xx response (status + headers) to the plugin — so plugins can detect the blocked redirect from the response status code without anyone connecting to the private target.The no-redirect session is unchanged. Behavior for plugins that never hit a redirect, or that explicitly disable redirects, is byte-for-byte identical.
Scope decisions
checkSSRFis still string-based on the hostname. Apublic-looking.example.comthat resolves to127.0.0.1is still let through. Fixing that requires resolving the hostname inside the SSRF guard and validating the resolved IPs (potentially per-redirect-hop), which is a much bigger change and worth doing in its own focused PR.follow_redirectsis intentionally left attrueto preserve API compatibility. With the redirect re-check in place, that default no longer creates the bypass.Changes
SSRFProtectionTestscover the underlyingcheckSSRF; the redirect-delegate hook is a thin URLSession-callback glue layer that's hard to unit-test without a real or mocked URL stack)Test Plan
http_requestto an attacker-controlled URL that 302-redirects tohttp://127.0.0.1:1337/agents. Expected: the plugin sees the 302 response (not the local API contents). Previously: the plugin saw the local API contents.follow_redirects: false— first response (even if 30x) is returned verbatim, unchanged.SSRFProtectionTestscontinue to pass (they test the underlyingcheckSSRFfunction which is unchanged).Checklist
CONTRIBUTING.mdswiftc -frontend -parse)