diff --git a/TESTING.md b/TESTING.md index 15dffad5..a4c2ff86 100644 --- a/TESTING.md +++ b/TESTING.md @@ -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 diff --git a/py/api/images.py b/py/api/images.py index 45daa952..b7786203 100644 --- a/py/api/images.py +++ b/py/api/images.py @@ -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, ) diff --git a/tests/browser/full-inspector.spec.mjs b/tests/browser/full-inspector.spec.mjs index 3e41b500..475a1a36 100644 --- a/tests/browser/full-inspector.spec.mjs +++ b/tests/browser/full-inspector.spec.mjs @@ -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); diff --git a/tests/js/workspace-file-inspector.test.js b/tests/js/workspace-file-inspector.test.js index 7d082a7b..f6481912 100644 --- a/tests/js/workspace-file-inspector.test.js +++ b/tests/js/workspace-file-inspector.test.js @@ -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.'); diff --git a/tests/test_image_reveal.py b/tests/test_image_reveal.py index eff278e0..cc366654 100644 --- a/tests/test_image_reveal.py +++ b/tests/test_image_reveal.py @@ -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). diff --git a/utils/file_reveal.py b/utils/file_reveal.py index d7c61916..52ab5bd3 100644 --- a/utils/file_reveal.py +++ b/utils/file_reveal.py @@ -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: diff --git a/web/js/workspace/file-inspector.mjs b/web/js/workspace/file-inspector.mjs index b294ec96..4135152c 100644 --- a/web/js/workspace/file-inspector.mjs +++ b/web/js/workspace/file-inspector.mjs @@ -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;