fix(images): open the folder for any same-origin browser, including through a proxy (#533)
Maintainer ruling: drop the local-only gate on the reveal route, like LoRA Manager. The route stays POST, JSON and same-origin, comparing Origin with X-Forwarded-Host or Host and with X-Forwarded-Proto or the socket's scheme, and keeps its path check. Remove is_local_request, which had no other caller.
This commit is contained in:
Vito Sansevero
+16
-25
@@ -763,7 +763,7 @@ filter Images, D21), the six facts, and a footer with "Send to ComfyUI" and
|
||||
"Prompt history" only when prompt roles resolved or a link is saved; the sticky panel
|
||||
scrolls inside the window. The viewer's details (`#viewer-details`) repeat that
|
||||
content without the cover, then "File & provenance" (the honest file facts, the
|
||||
`Prompt evidence` line, Download original and, for a listed file, D38's local-only
|
||||
`Prompt evidence` line, Download original and, for a listed file, D38's
|
||||
"Show in folder" after the Path) and "Workflow & raw metadata" (raw records and their
|
||||
two downloads), then the pair send and Prompt history buttons. D22 words an empty
|
||||
positive explicitly and makes confirmed prompt text focusable.
|
||||
@@ -1566,9 +1566,9 @@ Recorded deviations on this page (spec §8):
|
||||
|
||||
| Result | Toast |
|
||||
|---|---|
|
||||
| Success | "Opened the folder on this computer." |
|
||||
| 403 `not-local` | "Show in folder works only in a browser on the computer running ComfyUI." |
|
||||
| 501 `unavailable` | "No file manager is available on this computer." |
|
||||
| Success | "Opened the folder on the computer running ComfyUI." |
|
||||
| 403 `not-same-origin` | "The folder was not opened: this page's address does not match the server's." |
|
||||
| 501 `unavailable` | "No file manager is available on the computer running ComfyUI." |
|
||||
| 429 `busy` | "The folder is already opening." |
|
||||
| Anything else (415, 404, 409, 500, offline) | "The folder could not be opened. Try again." |
|
||||
|
||||
@@ -1579,14 +1579,14 @@ Recorded deviations on this page (spec §8):
|
||||
covered and inert while `toHaveText` still passes: assert `toBeVisible` and the
|
||||
hit test as `full-inspector.spec.mjs` does.
|
||||
|
||||
The route is local-only. It answers 403 `not-local` unless all of these hold: no
|
||||
forwarding-style header (`Forwarded`, `Via`, `X-Real-IP`, `CF-Connecting-IP`,
|
||||
`True-Client-IP`, `X-Client-IP`, `X-Cluster-Client-IP`, `Fastly-Client-IP`, and any
|
||||
`X-Forwarded-*`, `X-Original-*` or `*-Client-IP`); a loopback `request.remote`; a
|
||||
loopback `Host` (`localhost`, `*.localhost` or a loopback address, against DNS
|
||||
rebinding); an `Origin` equal to the request's own scheme, host and port (absent,
|
||||
`null`, another port or another scheme is refused; browsers send `Origin` on every
|
||||
fetch POST); and, when sent, `Sec-Fetch-Site: same-origin`. A body that is not
|
||||
The route opens the folder on the computer running ComfyUI wherever the browser is,
|
||||
through a proxy too (maintainer decision, #533, amending the original local-only
|
||||
rule; accepted risk: anyone who can reach the server can open file windows on it).
|
||||
It answers 403 `not-same-origin` unless `Origin` is present and matches: its host
|
||||
and port against `X-Forwarded-Host` when sent, else `Host`, with the scheme's default
|
||||
port filled in; its scheme against `X-Forwarded-Proto` when sent, else the socket's
|
||||
(an https page on an http socket passes, TLS having ended at a proxy); and, when
|
||||
sent, `Sec-Fetch-Site: same-origin`. A body that is not
|
||||
`application/json` is 415 `unsupported-media-type`, so any cross-origin request
|
||||
needs a CORS preflight. The body is strict (an int root, a 64-hex scope, a bounded
|
||||
relative path; absolute, drive and `..` paths are 400), the scope goes through
|
||||
@@ -1613,19 +1613,10 @@ Recorded deviations on this page (spec §8):
|
||||
(`POST`/`GET /__test__/reveal`) with no cooldown. The inspector comparison reads
|
||||
closed disclosures as hidden, so the button needs no mapping there.
|
||||
|
||||
LoRA Manager's open-location route has no local-only gate, so it also opens the
|
||||
server's file manager for a proxied or remote browser; ours says it cannot.
|
||||
|
||||
A browser behind a proxy that keeps its `Host` or `Origin`, or adds a forwarding
|
||||
header, or connects from another address (a Docker bridge) is always `not-local`.
|
||||
|
||||
Residual risk, accepted: a tunnel or proxy that adds no forwarding header and
|
||||
rewrites `Host` and `Origin` to loopback (`ssh -L` or `-R`, `socat`, a bare
|
||||
`proxy_pass` to 127.0.0.1 with both set), and a non-browser client behind a bare
|
||||
proxy that forges them, cannot be told apart from a local client: the folder opens
|
||||
on the host's screen and the toast says it opened. Such a client can open a folder of a listed file on the host's
|
||||
screen, at most once a second, and learn whether that file exists (which the
|
||||
listing routes already tell it). Operators who expose ComfyUI that way accept this.
|
||||
Limits of that check: a proxy that rewrites `Host` and sends no `X-Forwarded-Host` is
|
||||
refused with that toast until it sends one. A non-browser client can forge every
|
||||
header. A DNS-rebinding page has a matching `Host` and `Origin`, so it can open a
|
||||
folder of a listed file, at most once a second; the path check still confines it.
|
||||
- **D39** (ruled 2026-10-21: fallback): a recipe-linked image shows its recipe's own
|
||||
category when set, otherwise the linked positive prompt's, and its recipe's own tags
|
||||
when it has any, otherwise the positive's; each field falls back on its own and the
|
||||
|
||||
+14
-12
@@ -80,7 +80,7 @@ try:
|
||||
OpenerUnavailable,
|
||||
RevealGate,
|
||||
RevealTargetChanged,
|
||||
is_local_request,
|
||||
is_same_origin_request,
|
||||
reveal_executor,
|
||||
reveal_in_file_manager,
|
||||
)
|
||||
@@ -106,7 +106,7 @@ except ImportError:
|
||||
OpenerUnavailable,
|
||||
RevealGate,
|
||||
RevealTargetChanged,
|
||||
is_local_request,
|
||||
is_same_origin_request,
|
||||
reveal_executor,
|
||||
reveal_in_file_manager,
|
||||
)
|
||||
@@ -795,12 +795,12 @@ def _reveal_not_found():
|
||||
)
|
||||
|
||||
|
||||
def _not_local():
|
||||
def _not_same_origin():
|
||||
return web.json_response(
|
||||
{
|
||||
"success": False,
|
||||
"code": "not-local",
|
||||
"error": "Show in folder works only on the computer running ComfyUI.",
|
||||
"code": "not-same-origin",
|
||||
"error": "This page's address does not match the server's.",
|
||||
},
|
||||
status=403,
|
||||
)
|
||||
@@ -1514,7 +1514,7 @@ class ImageRoutesMixin:
|
||||
async def clear_image_favorite_route(request):
|
||||
return await self.update_image_favorite(request, favorite=False)
|
||||
|
||||
# POST, local-only: opens this computer's file manager (D38).
|
||||
# POST, same-origin: opens the server computer's file manager (D38).
|
||||
@routes.post("/prompt_manager/images/reveal")
|
||||
async def reveal_image_route(request):
|
||||
return await self.reveal_image(request)
|
||||
@@ -3682,12 +3682,12 @@ class ImageRoutesMixin:
|
||||
async def reveal_image(self, request):
|
||||
"""POST ``{root_index, root_scope, path}``: show the file in its folder.
|
||||
|
||||
Local-only (a same-origin browser request from a loopback client, no
|
||||
forwarding header) and JSON-only: the browser must run on the
|
||||
computer whose file manager opens. One reveal at a time (429 busy).
|
||||
Same-origin and JSON-only, directly or through a proxy: the folder
|
||||
opens on the computer running ComfyUI, wherever the browser is
|
||||
(maintainer decision, #533). One reveal at a time (429 busy).
|
||||
"""
|
||||
if not is_local_request(request.remote, request.headers, request.scheme):
|
||||
return _not_local()
|
||||
if not is_same_origin_request(request.headers, request.scheme):
|
||||
return _not_same_origin()
|
||||
if request.content_type != "application/json":
|
||||
# A non-JSON POST is a CORS "simple" request with no preflight.
|
||||
return web.json_response(
|
||||
@@ -3765,7 +3765,9 @@ class ImageRoutesMixin:
|
||||
{
|
||||
"success": False,
|
||||
"code": "unavailable",
|
||||
"error": "No file manager is available on this computer.",
|
||||
"error": (
|
||||
"No file manager is available on the computer running ComfyUI."
|
||||
),
|
||||
},
|
||||
status=501,
|
||||
)
|
||||
|
||||
@@ -362,14 +362,14 @@ test('Show in folder asks the server to open the listed file and announces each
|
||||
const body=sent.postDataJSON();
|
||||
expect(Object.keys(body).sort()).toEqual(['path','root_index','root_scope']);
|
||||
expect(body).toMatchObject({root_index:0,path:'inspector/complete.png'});expect(body.root_scope).toMatch(/^[0-9a-f]{64}$/);
|
||||
await expect(toast).toHaveText('Opened the folder on this computer.');
|
||||
await expect(toast).toHaveText('Opened the folder on the computer running ComfyUI.');
|
||||
// #533: the answer is really on screen and reachable above the modal viewer, not covered by it.
|
||||
await expect(toast).toBeVisible();
|
||||
expect(await toast.evaluate(node=>{const box=node.getBoundingClientRect();const top=document.elementFromPoint(box.x+box.width/2,box.y+box.height/2);return top===node;})).toBe(true);
|
||||
await expect(page.locator('#viewer-dialog').getByRole('status').filter({hasText:'Opened the folder on this computer.'})).toHaveCount(1);
|
||||
await expect(page.locator('#viewer-dialog').getByRole('status').filter({hasText:'Opened the folder on the computer running ComfyUI.'})).toHaveCount(1);
|
||||
const opened=async()=>(await (await request.get('/__test__/reveal')).json()).opened;
|
||||
expect(await opened()).toEqual(['output/inspector/complete.png']);
|
||||
for (const [outcome,message] of [['unavailable','No file manager is available on this computer.'],['fail','The folder could not be opened. Try again.']]) {
|
||||
for (const [outcome,message] of [['unavailable','No file manager is available on the computer running ComfyUI.'],['fail','The folder could not be opened. Try again.']]) {
|
||||
await expect(await request.post('/__test__/reveal',{data:{outcome}})).toBeOK();
|
||||
await show.click();await expect(toast).toHaveText(message);
|
||||
}
|
||||
@@ -377,12 +377,20 @@ test('Show in folder asks the server to open the listed file and announces each
|
||||
await page.route('**/prompt_manager/images/reveal',route=>route.fulfill({status:429,contentType:'application/json',body:JSON.stringify({success:false,code:'busy',error:'The folder is already opening.'})}));
|
||||
await show.click();await expect(toast).toHaveText('The folder is already opening.');
|
||||
await page.unroute('**/prompt_manager/images/reveal');
|
||||
// A proxied request counts as remote, so the real route answers 403 not-local.
|
||||
// A proxied click opens the folder: the proxy's headers, not the browser's address, name the page (#533).
|
||||
await expect(await request.post('/__test__/reveal',{data:{outcome:'opened'}})).toBeOK();
|
||||
await page.route('**/prompt_manager/images/reveal',route=>route.continue({headers:{...route.request().headers(),'x-forwarded-for':'203.0.113.7'}}));
|
||||
await show.click();await expect(toast).toHaveText('Show in folder works only in a browser on the computer running ComfyUI.');
|
||||
await show.click();await expect(toast).toHaveText('Opened the folder on the computer running ComfyUI.');
|
||||
expect(await opened()).toHaveLength(2);
|
||||
await page.unroute('**/prompt_manager/images/reveal');
|
||||
// A third-party page is refused, and says so.
|
||||
// Chromium keeps its own Origin on a routed fetch, so the real route is asked directly and the toast gets its answer.
|
||||
const refused=await request.post('/prompt_manager/images/reveal',{data:body,headers:{origin:'https://evil.example'}});
|
||||
expect(refused.status()).toBe(403);expect((await refused.json()).code).toBe('not-same-origin');
|
||||
await page.route('**/prompt_manager/images/reveal',route=>route.fulfill({status:403,contentType:'application/json',body:JSON.stringify({success:false,code:'not-same-origin',error:"This page's address does not match the server's."})}));
|
||||
await show.click();await expect(toast).toHaveText("The folder was not opened: this page's address does not match the server's.");
|
||||
await expect(toast).toBeVisible();
|
||||
expect(await opened()).toEqual(['output/inspector/complete.png']);
|
||||
expect(await opened()).toHaveLength(2);
|
||||
// #533: closing the viewer takes the toast back to the page, still showing the message.
|
||||
await page.keyboard.press('Escape');await expect(page.locator('#viewer-dialog')).toHaveCount(0);
|
||||
await expect(toast).toBeVisible();await expect(page.locator('body > #workspace-toast')).toHaveCount(1);
|
||||
|
||||
@@ -79,9 +79,9 @@ test('Show in folder needs the full listing identity (D38)',async()=>{
|
||||
});
|
||||
test('Show in folder outcomes use the D38 toasts',async()=>{
|
||||
const {revealOutcome}=await load();
|
||||
assert.equal(revealOutcome(null),'Opened the folder on this computer.');
|
||||
assert.equal(revealOutcome({status:403,code:'not-local'}),'Show in folder works only in a browser on the computer running ComfyUI.');
|
||||
assert.equal(revealOutcome({status:501,code:'unavailable'}),'No file manager is available on this computer.');
|
||||
assert.equal(revealOutcome(null),'Opened the folder on the computer running ComfyUI.');
|
||||
assert.equal(revealOutcome({status:403,code:'not-same-origin'}),'The folder was not opened: this page\'s address does not match the server\'s.');
|
||||
assert.equal(revealOutcome({status:501,code:'unavailable'}),'No file manager is available on the computer running ComfyUI.');
|
||||
assert.equal(revealOutcome({status:429,code:'busy'}),'The folder is already opening.');
|
||||
for(const error of [{status:403,code:null},{status:409,code:'source-changed'},{status:415,code:'unsupported-media-type'},{status:429,code:null},{status:500},new TypeError('offline')])
|
||||
assert.equal(revealOutcome(error),'The folder could not be opened. Try again.');
|
||||
|
||||
+130
-160
@@ -1,4 +1,4 @@
|
||||
"""Show in folder (TESTING.md D38): the local-only reveal route and its openers.
|
||||
"""Show in folder (TESTING.md D38): the same-origin reveal route and its openers.
|
||||
|
||||
No test launches a file manager: ``subprocess.Popen``, ``sys.platform`` and
|
||||
``shutil.which`` are mocked, and route tests swap the API's opener.
|
||||
@@ -29,28 +29,11 @@ except ImportError:
|
||||
import utils.file_reveal as file_reveal
|
||||
from utils.file_reveal import (
|
||||
OpenerUnavailable,
|
||||
is_local_request,
|
||||
is_same_origin_request,
|
||||
reveal_in_file_manager,
|
||||
)
|
||||
|
||||
REVEAL = "/prompt_manager/images/reveal"
|
||||
PROXY_HEADERS = (
|
||||
"Forwarded",
|
||||
"Via",
|
||||
"X-Forwarded-For",
|
||||
"X-Forwarded-Host",
|
||||
"X-Forwarded-Proto",
|
||||
"X-Forwarded-Port",
|
||||
"X-Forwarded-Server",
|
||||
"X-Real-IP",
|
||||
"X-Original-Forwarded-For",
|
||||
"X-Original-Host",
|
||||
"CF-Connecting-IP",
|
||||
"True-Client-IP",
|
||||
"X-Client-IP",
|
||||
"X-Cluster-Client-IP",
|
||||
"Fastly-Client-IP",
|
||||
)
|
||||
ORIGIN = "http://127.0.0.1:8188"
|
||||
|
||||
|
||||
@@ -60,134 +43,126 @@ def headers(**values):
|
||||
return CIMultiDict({key: value for key, value in base.items() if value is not None})
|
||||
|
||||
|
||||
class TestIsLocalRequest(unittest.TestCase):
|
||||
def test_same_origin_loopback_clients_by_loopback_name_are_local(self):
|
||||
for remote, host, origin in (
|
||||
("127.0.0.1", "127.0.0.1:8188", ORIGIN),
|
||||
("127.8.9.10", "localhost:8188", "http://localhost:8188"),
|
||||
("::1", "[::1]:8188", "http://[::1]:8188"),
|
||||
("::ffff:127.0.0.1", "comfy.localhost", "http://comfy.localhost"),
|
||||
("127.0.0.1", "LOCALHOST:8188", "http://localhost:8188"),
|
||||
("127.0.0.1", "localhost", "http://localhost:80"),
|
||||
SITE = "comfy.sansfam.org"
|
||||
SITE_ORIGIN = f"https://{SITE}"
|
||||
# What Caddy's plain reverse_proxy sends: https at the edge, http to ComfyUI.
|
||||
CADDY = {
|
||||
"Host": SITE,
|
||||
"Origin": SITE_ORIGIN,
|
||||
"X_Forwarded_For": "192.168.1.30",
|
||||
"X_Forwarded_Host": SITE,
|
||||
"X_Forwarded_Proto": "https",
|
||||
"Sec_Fetch_Site": "same-origin",
|
||||
}
|
||||
|
||||
|
||||
class TestIsSameOriginRequest(unittest.TestCase):
|
||||
def test_direct_browsers_on_any_address_are_same_origin(self):
|
||||
for host, origin in (
|
||||
("127.0.0.1:8188", ORIGIN),
|
||||
("localhost:8188", "http://localhost:8188"),
|
||||
("[::1]:8188", "http://[::1]:8188"),
|
||||
("LOCALHOST:8188", "http://localhost:8188"),
|
||||
("localhost", "http://localhost:80"),
|
||||
("10.0.140.30:8188", "http://10.0.140.30:8188"),
|
||||
("vito-cachyos:8188", "http://vito-cachyos:8188"),
|
||||
):
|
||||
with self.subTest(remote=remote, host=host):
|
||||
with self.subTest(host=host):
|
||||
self.assertTrue(
|
||||
is_local_request(remote, headers(Host=host, Origin=origin))
|
||||
is_same_origin_request(headers(Host=host, Origin=origin))
|
||||
)
|
||||
self.assertTrue(
|
||||
is_local_request(
|
||||
"127.0.0.1",
|
||||
headers(Host="localhost", Origin="https://localhost:443"),
|
||||
"https",
|
||||
is_same_origin_request(
|
||||
headers(Host="localhost", Origin="https://localhost:443"), "https"
|
||||
)
|
||||
)
|
||||
|
||||
def test_non_loopback_or_unknown_remote_is_not_local(self):
|
||||
for remote in ("192.168.1.5", "10.0.0.1", "::2", "fe80::1%eth0", "", None):
|
||||
with self.subTest(remote=remote):
|
||||
self.assertFalse(is_local_request(remote, headers()))
|
||||
def test_the_proxied_request_shape_is_same_origin(self):
|
||||
self.assertTrue(is_same_origin_request(headers(**CADDY), "http"))
|
||||
shapes = {
|
||||
"no forwarded headers, https at the edge": {
|
||||
"Host": SITE,
|
||||
"Origin": SITE_ORIGIN,
|
||||
},
|
||||
"proxy rewrites Host, sends X-Forwarded-Host": {
|
||||
**CADDY,
|
||||
"Host": "10.0.140.30:8188",
|
||||
},
|
||||
"a chain of proxies lists the client's first": {
|
||||
**CADDY,
|
||||
"X_Forwarded_Host": f"{SITE}, inner.lan",
|
||||
"X_Forwarded_Proto": "HTTPS, http",
|
||||
},
|
||||
"explicit default port on Host": {**CADDY, "Host": f"{SITE}:443"},
|
||||
"a non-default edge port": {
|
||||
**CADDY,
|
||||
"Host": f"{SITE}:8443",
|
||||
"X_Forwarded_Host": f"{SITE}:8443",
|
||||
"Origin": f"{SITE_ORIGIN}:8443",
|
||||
},
|
||||
}
|
||||
for name, shape in shapes.items():
|
||||
with self.subTest(name):
|
||||
self.assertTrue(is_same_origin_request(headers(**shape), "http"))
|
||||
|
||||
def test_any_forwarding_header_makes_loopback_remote(self):
|
||||
for name in (*PROXY_HEADERS, "X-Forwarded-Anything", "X-Original-URI"):
|
||||
for spelling in (name, name.lower()):
|
||||
with self.subTest(header=spelling):
|
||||
request_headers = headers()
|
||||
request_headers[spelling] = "127.0.0.1"
|
||||
self.assertFalse(is_local_request("127.0.0.1", request_headers))
|
||||
def test_a_proxy_that_rewrites_host_and_says_nothing_is_refused(self):
|
||||
self.assertFalse(
|
||||
is_same_origin_request(
|
||||
headers(Host="10.0.140.30:8188", Origin=SITE_ORIGIN), "http"
|
||||
)
|
||||
)
|
||||
|
||||
def test_origin_is_required_and_must_be_the_request_origin(self):
|
||||
# A header-less proxy or curl leaves Origin out; a page on another
|
||||
# loopback port or scheme sends its own (security review M1, L2).
|
||||
self.assertFalse(is_local_request("127.0.0.1", headers(Origin=None)))
|
||||
def test_cross_site_and_spoofed_requests_are_refused(self):
|
||||
evil = "https://evil.example"
|
||||
cases = {
|
||||
"cross-site Origin through the proxy": {**CADDY, "Origin": evil},
|
||||
"cross-site Origin, direct": {"Origin": evil},
|
||||
"another port": {"Origin": "http://127.0.0.1:3000"},
|
||||
"another scheme than the proxy says": {
|
||||
**CADDY,
|
||||
"Origin": f"http://{SITE}",
|
||||
},
|
||||
"forwarded host naming the attacker": {
|
||||
**CADDY,
|
||||
"X_Forwarded_Host": "evil.example",
|
||||
},
|
||||
"Host naming the attacker": {"Host": "evil.example", "Origin": ORIGIN},
|
||||
"lookalike suffix": {**CADDY, "Origin": f"https://{SITE}.evil.example"},
|
||||
"cross-site fetch metadata with a forged Origin": {
|
||||
**CADDY,
|
||||
"Sec_Fetch_Site": "cross-site",
|
||||
},
|
||||
"same-site fetch metadata": {**CADDY, "Sec_Fetch_Site": "same-site"},
|
||||
}
|
||||
for name, shape in cases.items():
|
||||
with self.subTest(name):
|
||||
self.assertFalse(is_same_origin_request(headers(**shape), "http"))
|
||||
|
||||
def test_a_missing_or_malformed_origin_is_refused(self):
|
||||
for origin in (
|
||||
None,
|
||||
"",
|
||||
"null",
|
||||
"http://localhost:3000",
|
||||
"http://127.0.0.1:3000",
|
||||
"http://127.0.0.1",
|
||||
"https://127.0.0.1:8188",
|
||||
"http://localhost:8188",
|
||||
"http://foo.localhost:8188",
|
||||
"http://evil.example:8188",
|
||||
"http://[bad",
|
||||
"http://127.0.0.1:8188/path",
|
||||
"http://user@127.0.0.1:8188",
|
||||
"file://127.0.0.1:8188",
|
||||
):
|
||||
with self.subTest(origin=origin):
|
||||
self.assertFalse(is_local_request("127.0.0.1", headers(Origin=origin)))
|
||||
self.assertFalse(is_local_request("127.0.0.1", headers(Origin=ORIGIN), "https"))
|
||||
self.assertFalse(is_same_origin_request(headers(Origin=origin)))
|
||||
|
||||
def test_a_browser_behind_a_reverse_proxy_is_never_local(self):
|
||||
# The maintainer's setup (#533): a Mac reaching the Linux box through
|
||||
# a Docker proxy at comfy.sansfam.org. Every line must fail safe.
|
||||
site = "comfy.sansfam.org"
|
||||
origin = f"https://{site}"
|
||||
forwarded = {"X_Forwarded_For": "192.168.1.30", "X_Forwarded_Proto": "https"}
|
||||
cases = {
|
||||
"docker bridge peer, forwarded headers": (
|
||||
"172.18.0.2",
|
||||
headers(Host=site, Origin=origin, **forwarded),
|
||||
),
|
||||
"docker bridge peer, bare proxy": (
|
||||
"172.18.0.2",
|
||||
headers(Host=site, Origin=origin),
|
||||
),
|
||||
"loopback peer, forwarded headers": (
|
||||
"127.0.0.1",
|
||||
headers(Host=site, Origin=origin, **forwarded),
|
||||
),
|
||||
"loopback peer, Host kept, no forwarded headers": (
|
||||
"127.0.0.1",
|
||||
headers(Host=site, Origin=origin),
|
||||
),
|
||||
"loopback peer, Host rewritten, Origin kept": (
|
||||
"127.0.0.1",
|
||||
headers(Origin=origin),
|
||||
),
|
||||
"loopback peer, everything rewritten but a forwarded header": (
|
||||
"127.0.0.1",
|
||||
headers(X_Real_IP="192.168.1.30"),
|
||||
),
|
||||
"spoofed local Host from another computer": (
|
||||
"192.168.1.30",
|
||||
headers(),
|
||||
),
|
||||
"spoofed local forwarded value": (
|
||||
"127.0.0.1",
|
||||
headers(X_Forwarded_For="127.0.0.1"),
|
||||
),
|
||||
}
|
||||
for name, (remote, request_headers) in cases.items():
|
||||
with self.subTest(name):
|
||||
self.assertFalse(is_local_request(remote, request_headers, "https"))
|
||||
self.assertFalse(is_local_request(remote, request_headers, "http"))
|
||||
def test_a_downgraded_direct_origin_is_refused(self):
|
||||
# http at the socket allows an https page (TLS ended before us), but a
|
||||
# TLS socket never takes an http page.
|
||||
self.assertFalse(is_same_origin_request(headers(), "https"))
|
||||
|
||||
def test_what_cannot_be_told_apart_from_a_local_browser(self):
|
||||
# Known limit, in TESTING.md: an ssh -L tunnel, or a proxy that rewrites
|
||||
# Host and Origin and adds no header, looks exactly like this.
|
||||
self.assertTrue(is_local_request("127.0.0.1", headers()))
|
||||
|
||||
def test_sec_fetch_site_must_be_same_origin_when_sent(self):
|
||||
self.assertTrue(
|
||||
is_local_request("127.0.0.1", headers(Sec_Fetch_Site="same-origin"))
|
||||
)
|
||||
for site in ("same-site", "cross-site", "none", ""):
|
||||
with self.subTest(site=site):
|
||||
self.assertFalse(
|
||||
is_local_request("127.0.0.1", headers(Sec_Fetch_Site=site))
|
||||
)
|
||||
|
||||
def test_foreign_host_is_not_local(self):
|
||||
# A DNS-rebinding page reaches 127.0.0.1 under its own name.
|
||||
for host in ("evil.example:8188", "", "[bad", "127.0.0.1:bad"):
|
||||
def test_a_missing_or_malformed_host_is_refused(self):
|
||||
for host in ("", "[bad", "127.0.0.1:bad"):
|
||||
with self.subTest(host=host):
|
||||
self.assertFalse(
|
||||
is_local_request(
|
||||
"127.0.0.1", headers(Host=host, Origin=f"http://{host}")
|
||||
)
|
||||
is_same_origin_request(headers(Host=host, Origin=f"http://{host}"))
|
||||
)
|
||||
self.assertFalse(is_local_request("127.0.0.1", CIMultiDict({"Origin": ORIGIN})))
|
||||
self.assertFalse(is_same_origin_request(CIMultiDict({"Origin": ORIGIN})))
|
||||
|
||||
|
||||
class TestRevealGate(unittest.TestCase):
|
||||
@@ -820,7 +795,7 @@ class TestRevealRoute(ImageAPITestCase):
|
||||
def body(self, path="sub/a.png", **changes):
|
||||
return {"root_index": 0, "root_scope": self.scope(), "path": path, **changes}
|
||||
|
||||
async def test_loopback_request_opens_the_resolved_file(self):
|
||||
async def test_same_origin_request_opens_the_resolved_file(self):
|
||||
target = make_png(self.output_dir / "sub" / "a.png")
|
||||
status, data = await self.post(self.body())
|
||||
self.assertEqual((status, data), (200, {"success": True}))
|
||||
@@ -870,52 +845,47 @@ class TestRevealRoute(ImageAPITestCase):
|
||||
self.assertEqual(status, 404)
|
||||
self.assertEqual(data, {"success": False, "error": "Image file not found"})
|
||||
|
||||
async def test_non_loopback_remote_is_not_local(self):
|
||||
async def test_a_proxied_click_opens_the_folder_on_this_computer(self):
|
||||
make_png(self.output_dir / "sub" / "a.png")
|
||||
with patch.object(
|
||||
web.BaseRequest, "remote", new_callable=PropertyMock
|
||||
) as remote:
|
||||
remote.return_value = "192.168.1.20"
|
||||
status, data = await self.post(self.body())
|
||||
self.assertEqual(status, 403)
|
||||
self.assertEqual(data["code"], "not-local")
|
||||
self.assertIs(data["success"], False)
|
||||
self.assertEqual(self.opened, [])
|
||||
remote.return_value = "172.18.0.2"
|
||||
status, _ = await self.post(
|
||||
self.body(),
|
||||
Host=SITE,
|
||||
Origin=SITE_ORIGIN,
|
||||
**{
|
||||
"X-Forwarded-For": "192.168.1.30",
|
||||
"X-Forwarded-Host": SITE,
|
||||
"X-Forwarded-Proto": "https",
|
||||
"Sec-Fetch-Site": "same-origin",
|
||||
},
|
||||
)
|
||||
self.assertEqual(status, 200)
|
||||
self.assertEqual(len(self.opened), 1)
|
||||
|
||||
async def test_loopback_with_a_proxy_header_is_not_local(self):
|
||||
make_png(self.output_dir / "sub" / "a.png")
|
||||
for name in PROXY_HEADERS:
|
||||
with self.subTest(header=name):
|
||||
status, data = await self.post(self.body(), **{name: "127.0.0.1"})
|
||||
self.assertEqual((status, data["code"]), (403, "not-local"))
|
||||
status, data = await self.post(self.body(), Host="evil.example:8188")
|
||||
self.assertEqual((status, data["code"]), (403, "not-local"))
|
||||
self.assertEqual(self.opened, [])
|
||||
|
||||
async def test_a_proxied_browser_gets_the_message_and_opens_nothing(self):
|
||||
make_png(self.output_dir / "sub" / "a.png")
|
||||
status, data = await self.post(
|
||||
self.body(),
|
||||
Host="comfy.sansfam.org",
|
||||
Origin="https://comfy.sansfam.org",
|
||||
**{"X-Forwarded-For": "192.168.1.30"},
|
||||
)
|
||||
self.assertEqual((status, data["code"]), (403, "not-local"))
|
||||
self.assertIn("only on the computer running ComfyUI", data["error"])
|
||||
self.assertEqual(self.opened, [])
|
||||
|
||||
async def test_missing_or_foreign_origin_is_not_local(self):
|
||||
async def test_a_cross_site_request_opens_nothing(self):
|
||||
make_png(self.output_dir / "sub" / "a.png")
|
||||
port = self.client.make_url("/").port
|
||||
for origin in (None, f"http://127.0.0.1:{port + 1}", "null"):
|
||||
with self.subTest(origin=origin):
|
||||
status, data = await self.post(self.body(), Origin=origin)
|
||||
self.assertEqual((status, data["code"]), (403, "not-local"))
|
||||
self.assertEqual((status, data["code"]), (403, "not-same-origin"))
|
||||
self.assertIs(data["success"], False)
|
||||
status, data = await self.post(self.body(), **{"Sec-Fetch-Site": "same-site"})
|
||||
self.assertEqual((status, data["code"]), (403, "not-local"))
|
||||
status, _ = await self.post(self.body(), **{"Sec-Fetch-Site": "same-origin"})
|
||||
self.assertEqual(status, 200)
|
||||
self.assertEqual(len(self.opened), 1)
|
||||
self.assertEqual((status, data["code"]), (403, "not-same-origin"))
|
||||
self.assertEqual(self.opened, [])
|
||||
|
||||
async def test_the_path_check_still_applies_to_a_proxied_click(self):
|
||||
make_png(self.output_dir / "sub" / "a.png")
|
||||
shape = {"Host": SITE, "Origin": SITE_ORIGIN, "X-Forwarded-Proto": "https"}
|
||||
for path in ("../secret.png", "/etc/passwd.png", "C:/x.png"):
|
||||
with self.subTest(path=path):
|
||||
body = {**self.body(), "path": path}
|
||||
status, _ = await self.post(body, **shape)
|
||||
self.assertEqual(status, 400)
|
||||
self.assertEqual(self.opened, [])
|
||||
|
||||
async def test_only_json_bodies_are_accepted(self):
|
||||
# text/plain would be a CORS "simple" request with no preflight (L2).
|
||||
|
||||
+47
-77
@@ -1,15 +1,16 @@
|
||||
"""Show one gallery file in this computer's file manager (TESTING.md D38).
|
||||
|
||||
The route that calls this is local-only (``is_local_request``) and
|
||||
single-flight (``RevealGate``), and hands in a path it has already resolved
|
||||
inside a gallery root, which is re-checked just before launching. Windows
|
||||
The route that calls this takes a same-origin browser request from anywhere,
|
||||
directly or through a proxy (``is_same_origin_request``), and the folder opens on
|
||||
the computer running ComfyUI (maintainer decision, #533). It is single-flight
|
||||
(``RevealGate``) and hands in a path it has already resolved inside a gallery
|
||||
root, which is re-checked just before launching. Windows
|
||||
goes through the shell API (no command line exists to parse); macOS and Linux
|
||||
openers run as an argument list with an absolute executable from a fixed
|
||||
system PATH, no shell, no inherited streams and a minimal environment, so
|
||||
neither a file name nor ``PATH`` can change what runs or what it parses.
|
||||
"""
|
||||
|
||||
import ipaddress
|
||||
import os
|
||||
import posixpath
|
||||
import shutil
|
||||
@@ -26,23 +27,6 @@ from contextlib import contextmanager
|
||||
from pathlib import Path
|
||||
from urllib.parse import urlsplit
|
||||
|
||||
# A local reverse proxy makes every client look like loopback, so a request
|
||||
# that carries any forwarding-style header counts as remote: these names, and
|
||||
# every X-Forwarded-*, X-Original-* and *-Client-IP header.
|
||||
FORWARDING_HEADERS = frozenset(
|
||||
(
|
||||
"forwarded",
|
||||
"via",
|
||||
"x-real-ip",
|
||||
"cf-connecting-ip",
|
||||
"true-client-ip",
|
||||
"x-client-ip",
|
||||
"x-cluster-client-ip",
|
||||
"fastly-client-ip",
|
||||
)
|
||||
)
|
||||
FORWARDING_PREFIXES = ("x-forwarded-", "x-original-")
|
||||
FORWARDING_SUFFIXES = ("-client-ip",)
|
||||
DEFAULT_PORTS = {"http": 80, "https": 443}
|
||||
# One reveal at a time per process, then a pause before the next.
|
||||
REVEAL_COOLDOWN = 1.0
|
||||
@@ -91,7 +75,7 @@ CHILD_ENV_NAMES = (
|
||||
|
||||
|
||||
class OpenerUnavailable(Exception):
|
||||
"""No file manager can be started on this computer."""
|
||||
"""No file manager can be started on the computer running ComfyUI."""
|
||||
|
||||
|
||||
class OpenerFailed(Exception):
|
||||
@@ -118,29 +102,6 @@ def reveal_executor() -> ThreadPoolExecutor:
|
||||
return _executor
|
||||
|
||||
|
||||
def _loopback_address(value: str | None) -> bool:
|
||||
if not value:
|
||||
return False
|
||||
try:
|
||||
address = ipaddress.ip_address(value)
|
||||
except ValueError:
|
||||
return False
|
||||
mapped = getattr(address, "ipv4_mapped", None)
|
||||
return (mapped or address).is_loopback
|
||||
|
||||
|
||||
def _loopback_host(host: str) -> bool:
|
||||
"""True for a Host name naming this computer.
|
||||
|
||||
Rejects every other name, so a DNS-rebinding page whose name now resolves
|
||||
to 127.0.0.1 still reads as remote.
|
||||
"""
|
||||
host = host.rstrip(".").lower()
|
||||
if host == "localhost" or host.endswith(".localhost"):
|
||||
return True
|
||||
return _loopback_address(host.strip("[]"))
|
||||
|
||||
|
||||
def _authority(authority: str) -> tuple[str, int | None] | None:
|
||||
"""``(host, port)`` of a Host value, or None when it does not parse."""
|
||||
try:
|
||||
@@ -151,30 +112,29 @@ def _authority(authority: str) -> tuple[str, int | None] | None:
|
||||
return (host, port) if host else None
|
||||
|
||||
|
||||
def _forwarded(headers: Mapping) -> bool:
|
||||
for name in headers.keys():
|
||||
name = name.lower()
|
||||
if (
|
||||
name in FORWARDING_HEADERS
|
||||
or name.startswith(FORWARDING_PREFIXES)
|
||||
or name.endswith(FORWARDING_SUFFIXES)
|
||||
):
|
||||
return True
|
||||
return False
|
||||
def _first_value(headers: Mapping, name: str) -> str | None:
|
||||
"""The client-side entry of a comma-separated forwarding header, if any."""
|
||||
value = headers.get(name)
|
||||
return (value.split(",")[0].strip() or None) if value else None
|
||||
|
||||
|
||||
def _same_origin(origin: str, scheme: str, host: tuple[str, int | None]) -> bool:
|
||||
"""True when *origin* is exactly this request's scheme://host:port."""
|
||||
default = DEFAULT_PORTS.get(scheme)
|
||||
if default is None:
|
||||
return False
|
||||
def _same_origin(
|
||||
origin: str, proto: str | None, scheme: str, host: tuple[str, int | None]
|
||||
) -> bool:
|
||||
"""True when *origin* names the page at *host*.
|
||||
|
||||
The port is compared after filling in the Origin scheme's default. The
|
||||
scheme must be the one the proxy reports, else this socket's; an https page
|
||||
on an http socket with no report passes, as a proxy ended TLS before us.
|
||||
"""
|
||||
try:
|
||||
parts = urlsplit(origin)
|
||||
origin_host, origin_port = parts.hostname, parts.port
|
||||
except ValueError:
|
||||
return False
|
||||
default = DEFAULT_PORTS.get(parts.scheme)
|
||||
if (
|
||||
parts.scheme != scheme
|
||||
default is None
|
||||
or not origin_host
|
||||
or parts.username is not None
|
||||
or parts.password is not None
|
||||
@@ -183,33 +143,43 @@ def _same_origin(origin: str, scheme: str, host: tuple[str, int | None]) -> bool
|
||||
or parts.fragment
|
||||
):
|
||||
return False
|
||||
if proto is not None:
|
||||
scheme_matches = parts.scheme == proto
|
||||
else:
|
||||
scheme_matches = parts.scheme == scheme or (
|
||||
scheme == "http" and parts.scheme == "https"
|
||||
)
|
||||
host_name, host_port = host
|
||||
return (origin_host.rstrip("."), origin_port or default) == (
|
||||
return scheme_matches and (origin_host.rstrip("."), origin_port or default) == (
|
||||
host_name.rstrip("."),
|
||||
host_port or default,
|
||||
)
|
||||
|
||||
|
||||
def is_local_request(remote: str | None, headers: Mapping, scheme="http") -> bool:
|
||||
"""True only for a same-origin browser request from this computer.
|
||||
def is_same_origin_request(headers: Mapping, scheme="http") -> bool:
|
||||
"""True only for a request whose page is this server, as the browser sees it.
|
||||
|
||||
That is: no forwarding-style header, a loopback client, a loopback name
|
||||
in Host, an ``Origin`` that is exactly this request's scheme://host:port
|
||||
(browsers send it on every fetch POST, so a missing one is refused), and
|
||||
``Sec-Fetch-Site: same-origin`` whenever that header is sent.
|
||||
Compares ``Origin`` (browsers send it on every fetch POST, so a missing one
|
||||
is refused) with ``X-Forwarded-Host`` when present, else ``Host``, and with
|
||||
``X-Forwarded-Proto`` when present, else *scheme*. ``Sec-Fetch-Site`` must
|
||||
be ``same-origin`` whenever it is sent. A third-party page in a browser cannot
|
||||
pass under its own address: it cannot change its ``Origin`` or
|
||||
``Sec-Fetch-Site``, and a custom forwarding header needs a preflight that
|
||||
ComfyUI's CORS option does not allow. The exception is a page whose own name
|
||||
was rebound to this server (TESTING.md D38, accepted).
|
||||
"""
|
||||
if _forwarded(headers):
|
||||
return False
|
||||
if not _loopback_address(remote):
|
||||
return False
|
||||
host = _authority(headers.get("Host", ""))
|
||||
if host is None or not _loopback_host(host[0]):
|
||||
return False
|
||||
origin = headers.get("Origin")
|
||||
if not origin or not _same_origin(origin, scheme, host):
|
||||
if not origin:
|
||||
return False
|
||||
site = headers.get("Sec-Fetch-Site")
|
||||
return site is None or site == "same-origin"
|
||||
if site is not None and site != "same-origin":
|
||||
return False
|
||||
authority = _first_value(headers, "X-Forwarded-Host") or headers.get("Host", "")
|
||||
host = _authority(authority)
|
||||
if host is None:
|
||||
return False
|
||||
proto = _first_value(headers, "X-Forwarded-Proto")
|
||||
return _same_origin(origin, proto.lower() if proto else None, scheme, host)
|
||||
|
||||
|
||||
class RevealGate:
|
||||
|
||||
@@ -26,9 +26,9 @@ const CATEGORY_PREFIX = {image: 'Image category: ', positive: 'Positive prompt c
|
||||
const CATEGORY_ORIGIN = {image: 'this image', positive: 'prompt', pair: 'recipe'};
|
||||
const REVEAL_URL = '/prompt_manager/images/reveal';
|
||||
// D38 Show in folder: the route's answers, in the maintainer's words.
|
||||
const REVEAL_OPENED = 'Opened the folder on this computer.';
|
||||
const REVEAL_NOT_LOCAL = 'Show in folder works only in a browser on the computer running ComfyUI.';
|
||||
const REVEAL_UNAVAILABLE = 'No file manager is available on this computer.';
|
||||
const REVEAL_OPENED = 'Opened the folder on the computer running ComfyUI.';
|
||||
const REVEAL_REFUSED = "The folder was not opened: this page's address does not match the server's.";
|
||||
const REVEAL_UNAVAILABLE = 'No file manager is available on the computer running ComfyUI.';
|
||||
const REVEAL_BUSY = 'The folder is already opening.';
|
||||
const REVEAL_FAILED = 'The folder could not be opened. Try again.';
|
||||
const FACTS = [['seed', 'Seed'], ['steps', 'Steps'], ['cfg', 'CFG'], ['sampler', 'Sampler'], ['scheduler', 'Scheduler'], ['model', 'Model']];
|
||||
@@ -109,7 +109,7 @@ export function revealTarget(identity) {
|
||||
/** The toast for a reveal outcome: null is success, otherwise the request's error (D38). */
|
||||
export function revealOutcome(error) {
|
||||
if (error === null) return REVEAL_OPENED;
|
||||
if (error?.status === 403 && error.code === 'not-local') return REVEAL_NOT_LOCAL;
|
||||
if (error?.status === 403 && error.code === 'not-same-origin') return REVEAL_REFUSED;
|
||||
if (error?.status === 501) return REVEAL_UNAVAILABLE;
|
||||
if (error?.status === 429 && error.code === 'busy') return REVEAL_BUSY;
|
||||
return REVEAL_FAILED;
|
||||
|
||||
Reference in New Issue
Block a user