fix(tools): refuse a non-http orchestrator URL, as the sibling filter does - #429
Conversation
… does semgrep flags `dynamic-urllib-use-detected` on this file as Blocking, and tracing it out is what makes the finding real rather than noise: the URL comes from `self.valves.ORCHESTRATOR_URL`, an Open WebUI admin setting, so it is not caller-controlled — but urllib honours file://, ftp:// and data://, and there is no guard here at all. The sibling function file already refuses exactly this, with a comment naming the same risk: "a misconfigured Valve could read arbitrary local files through urlopen (ruff S310)". So one of the two paths that consume the same Valve was protected and the other was not. That asymmetry is the defect, not the rule firing. The check goes in the constructor rather than at each urlopen: the URL enters once and fans out to three call sites (health probe, MCP preflight, and the chat-scope read), so guarding the entry cannot be partially applied later. Probed both directions: http and https accepted; file://, ftp://, data: and a bare path refused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCVG3p5zphDdsnCptx4S7J
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Reviewed and approved on the merits; blocked on the base, not this diff.
Review outcome: merge-worthy. Second reviewer verified the guard sits at the single entry point with all three Correction to my own reasoning: I argued "the sibling file guards it." That is precedent — it defeats the "false positive, admin-controlled" reading by showing the repo already judged this risk real for this Valve. But the load-bearing argument is the privilege boundary: a WebUI admin who can edit Valves is not thereby entitled to read files inside the container, and |
7f36c56 to
f0a2fe7
Compare
…tools-reject-nonhttp-orchestrator-url
…ions The refusal shipped without a test, so an implementation that accepted every scheme — or refused every one, including http — would have passed. Four cases: http and https construct, `file:///etc/passwd` raises (the scheme that turns a network read into a local-file read and reports it as if it had come from the orchestrator), ftp/data/gopher raise, and a bare `host:port` with no scheme raises here rather than deep inside urlopen with a message naming neither the Valve nor the URL. Mutation-checked: neutering the check to `if False` reds five of them, so the positive and negative halves are both bound to the guard rather than to whatever the constructor happens to do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… client The constructor check closed three urlopen sites and left a fourth open: `_resolve_chat_scope_sync` builds its own Request straight from ORCHESTRATOR_URL and never constructs an `_MCPClient`. So the PR's own claim — that the check sits where the URL enters — was not true of every entry. That path is the worse one. It catches every exception and degrades to the base scope, so a `file://` Valve would have read a local file and reported the result as a scope, and any failure would have looked like an ordinary resolve miss. The check is now a module-level helper both entry points call, and the async `_resolve_chat_scope` reaches it by delegating to the sync method. The first version of these tests was VACUOUS: without the guard, urlopen raises on the bad scheme, the method degrades, and the return value is the base scope either way — so asserting the return value proved nothing. They now assert whether urlopen was REACHED, which is what the guard changes, and carry the control that an http URL still is attempted — otherwise a guard refusing everything would pass. Mutation-checked both directions: removing the guard reds three cases, refusing every scheme reds the control. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment said urlparse gives `orchestrator:8000/mcp` an empty scheme. It reads `orchestrator` as the scheme, so the case is refused for the same reason ftp:// is. The assertion was right and its stated mechanism was not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Targets the branch behind #346, clearing one of its two Blocking semgrep findings.
The finding is real, and tracing it is what shows why
semgrep raises
dynamic-urllib-use-detected(Blocking) onopenwebui/tools/computer_use_tools.py. Traced to the source:self.valves.ORCHESTRATOR_URL→_MCPClient(url, ...)→base→ threeurlopencall sites (health probe, MCP preflight, chat-scope read).So it is not caller-controlled — it is an Open WebUI admin setting. That is the argument for calling it a false positive, and it is not enough, because:
The sibling file already refuses exactly this.
openwebui/functions/computer_link_filter.py:330checks the scheme and rejects anything but http/https, with a comment naming the same risk:Two paths consume the same Valve. One was guarded, one was not. That asymmetry is the defect — not the rule firing.
Where the check goes
The constructor, not the three call sites. The URL enters once and fans out; guarding the entry cannot later be applied to two of three places and missed on the third.
Probed both directions
http://localhost:8000https://orchestrator.internalfile:///etc/passwdftp://host/xdata:text/plain,hi/etc/passwd(no scheme)Scope
This clears one finding. #346 carries 14 total: the other Blocking one is on
computer_link_filter.py— which, given the guard quoted above, is worth re-reading before assuming it needs a fix — plus 12dangerous-subprocess-use-tainted-env-argsacross the journey harness, which are a separate argument about test-harness code driving docker from env.