From db1c7ef20dbd82cb9dbb3fd33f7667f9d71a452c Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Fri, 30 Oct 2026 16:52:38 -0700 Subject: [PATCH] fix(images): show every Show in folder outcome above the viewer and report opener failures (#537) Closes #533 --- TESTING.md | 46 +-- py/api/images.py | 26 +- tests/browser/full-inspector.spec.mjs | 59 +++- tests/js/prompt-blocks.test.js | 2 +- tests/js/workspace-file-inspector.test.js | 6 +- tests/js/workspace-images-selection.test.js | 1 + tests/js/workspace-toast.test.js | 62 +++- tests/test_image_reveal.py | 353 ++++++++++++++------ utils/file_reveal.py | 162 +++++---- web/js/workspace/file-inspector.mjs | 8 +- web/js/workspace/toast.mjs | 33 +- 11 files changed, 520 insertions(+), 238 deletions(-) diff --git a/TESTING.md b/TESTING.md index fd7ae998..7c015897 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,20 +1566,27 @@ 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." | - 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 + **D176 (#533, beyond the concept):** the viewer is a modal, so `showWorkspaceToast` + moves `#workspace-toast` into the topmost open modal (by hit test when several are + open), keeping its fixed position, and back when that modal closes. A modal opened + after a toast covers it until the next toast. Otherwise every toast raised from a modal is + covered and inert while `toHaveText` still passes: assert `toBeVisible` and the + hit test as `full-inspector.spec.mjs` does. + + 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 @@ -1596,7 +1603,9 @@ Recorded deviations on this page (spec §8): reach it), falling back to `os.startfile(folder, "explore")`. macOS runs `/usr/bin/open -R`; Linux asks FileManager1 `ShowItems` with `dbus-send --print-reply --reply-timeout=2000` (a real reply or a fallback) and else runs - `xdg-open` on the folder. Those openers are argv lists resolved only on a fixed + `xdg-open` on the folder, or answers 501 without `DISPLAY` and `WAYLAND_DISPLAY`. + `open` and `xdg-open` get `OPENER_WAIT` (1 s): a nonzero exit is the 500 above, and + one still running counts as started and goes to the reaper (#533). Those openers are argv lists resolved only on a fixed system PATH, with no shell, DEVNULL streams, `close_fds`, their own session, a minimal environment (no venv PATH, PYTHONPATH or API keys) and a daemon-thread reaper. `tests/test_image_reveal.py` mocks `Popen`, `sys.platform`, `shutil.which`, @@ -1604,12 +1613,11 @@ 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. - Residual risk, accepted: a reverse proxy or tunnel that adds no forwarding header - (a bare nginx `proxy_pass` to 127.0.0.1, `ssh -R`, `socat`) cannot be told apart - from a local client, and a non-browser client behind it can forge a matching - `Host` and `Origin`. 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. The check stops a plain cross-site POST, not every other site: 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 confines it, one check ahead of every opener. - **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 424efb0b..090b971a 100644 --- a/tests/browser/full-inspector.spec.mjs +++ b/tests/browser/full-inspector.spec.mjs @@ -362,10 +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 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); } @@ -373,11 +377,56 @@ 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.'); - expect(await opened()).toEqual(['output/inspector/complete.png']); + 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 plain cross-site POST is refused, and the toast 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()).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); +}); +// #533 D176: real modals in the real top layer, with the page's own toast module. +test('a toast stays on screen across nested, switched and closing modals',async({page,request})=>{ + await fullFixture(page,request); + await page.evaluate(async()=>{ + const url=performance.getEntriesByType('resource').map(entry=>entry.name).find(name=>name.includes('/toast.mjs')); + const {showWorkspaceToast}=await import(url); + window.say=showWorkspaceToast; + window.modal=(id,first=false)=>{const dialog=document.createElement('dialog');dialog.id=id;dialog.textContent=id;document.body[first?'prepend':'append'](dialog);dialog.showModal();return dialog;}; + window.onTop=()=>{const node=document.getElementById('workspace-toast');const box=node.getBoundingClientRect();return box.width>0&&document.elementFromPoint(box.x+box.width/2,box.y+box.height/2)===node;}; + }); + const parent=()=>page.evaluate(()=>{const host=document.getElementById('workspace-toast')?.parentElement;return host?host.id||host.localName:null;}); + const onTop=()=>page.evaluate(()=>window.onTop()); + await page.evaluate(()=>{window.modal('a');say('one');}); + expect(await parent()).toBe('a');expect(await onTop()).toBe(true); + // Nested: the top one holds it, even earlier in the page than the one under it. + await page.evaluate(()=>{window.modal('b',true);say('two');}); + expect(await parent()).toBe('b');expect(await onTop()).toBe(true); + // The top one closes while the toast shows: the message stays, in the modal now on top. + await page.evaluate(()=>document.getElementById('b').close()); + await expect.poll(parent).toBe('a');expect(await onTop()).toBe(true); + await expect(page.locator('#workspace-toast')).toHaveText('two'); + // Switched: the next modal takes the next toast. + await page.evaluate(()=>{document.getElementById('a').close();window.modal('c');say('three');}); + expect(await parent()).toBe('c');expect(await onTop()).toBe(true); + // Closed and removed at once, as the workspace's dialogs do: home, still showing. + await page.evaluate(()=>{const c=document.getElementById('c');c.close();c.remove();}); + await expect.poll(parent).toBe('body');expect(await onTop()).toBe(true); + await expect(page.locator('#workspace-toast')).toHaveText('three'); + // Removed while open, with no close event: the next message still shows. + await page.evaluate(()=>{window.modal('d');say('four');document.getElementById('d').remove();say('five');}); + expect(await parent()).toBe('body');expect(await onTop()).toBe(true); + await expect(page.locator('#workspace-toast')).toHaveText('five'); }); test('full inspector shows six facts and scoped provenance',async({page,request},testInfo)=>{ await page.setViewportSize({width:1600,height:1100});await fullFixture(page,request);const panel=inspector(page); diff --git a/tests/js/prompt-blocks.test.js b/tests/js/prompt-blocks.test.js index ca96fe87..1b291f1a 100644 --- a/tests/js/prompt-blocks.test.js +++ b/tests/js/prompt-blocks.test.js @@ -13,7 +13,7 @@ function dom() { closest(selector){if(selector==='section'&&this.tagName==='section')return this;if(selector==='[data-send-role]'&&Object.hasOwn(this.dataset,'sendRole'))return this;if(selector==='[data-copy-role]'&&Object.hasOwn(this.dataset,'copyRole'))return this;if(selector==='[data-retry-prompts]'&&Object.hasOwn(this.dataset,'retryPrompts'))return this;return this.parent?.closest(selector);} click(target){this.listeners.get('click')?.({target});} } - const previous=global.document,toast=new Element('div');global.document={createElement:tag=>new Element(tag),createElementNS:(_,tag)=>new Element(tag),getElementById:id=>id==='workspace-toast'?toast:null}; + const previous=global.document,toast=new Element('div');global.document={querySelectorAll:()=>[],createElement:tag=>new Element(tag),createElementNS:(_,tag)=>new Element(tag),getElementById:id=>id==='workspace-toast'?toast:null}; const walk=(root,predicate)=>[...(predicate(root)?[root]:[]),...root.children.flatMap(child=>walk(child,predicate))]; return {Element,walk,toast,restore:()=>{global.document=previous;}}; } 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/js/workspace-images-selection.test.js b/tests/js/workspace-images-selection.test.js index 4590bc4c..0aa18166 100644 --- a/tests/js/workspace-images-selection.test.js +++ b/tests/js/workspace-images-selection.test.js @@ -69,6 +69,7 @@ function fakeDOM() { createElement: tag => new Element(tag), createTextNode: text => Object.assign(new Element('#text'), { _text: String(text) }), getElementById: id => (id === 'workspace-toast' ? toast : null), + querySelectorAll: () => [], }; return { Element, body, toast, restore() { globalThis.document = previous; } }; } diff --git a/tests/js/workspace-toast.test.js b/tests/js/workspace-toast.test.js index 907498f7..79eb3394 100644 --- a/tests/js/workspace-toast.test.js +++ b/tests/js/workspace-toast.test.js @@ -13,10 +13,11 @@ class FakeToast extends EventTarget { matches(selector) { assert.equal(selector, ':hover, :focus-within'); return this.held; } } -function install() { +function install(dialogs = []) { const toast = new FakeToast(); const previous = global.document; global.document = { + querySelectorAll: selector => { assert.equal(selector, 'dialog'); return dialogs; }, getElementById: id => id === 'workspace-toast' ? toast : null, createTextNode: text => ({ textContent: text }), createElement: tag => ({ tag, href: '', textContent: '' }), @@ -24,6 +25,18 @@ function install() { return { toast, restore: () => { global.document = previous; } }; } +// First: the module remembers a toast it has shown. +test('a page without the toast shows nothing and does not fail', async () => { + const { showWorkspaceToast } = await source; + const previous = global.document; + global.document = { getElementById: () => null }; + try { + assert.doesNotThrow(() => showWorkspaceToast('Nothing.')); + } finally { + global.document = previous; + } +}); + test('a plain toast clears after 6.5 s and a linked one after 20 s', async t => { const { showWorkspaceToast, TOAST_MS, LINK_TOAST_MS } = await source; t.mock.timers.enable({ apis: ['setTimeout'] }); @@ -84,13 +97,50 @@ test('a toast under the pointer or focus waits until they leave', async t => { } }); -test('a page without the toast shows nothing and does not fail', async () => { + +class FakeHost extends EventTarget { + constructor(modal = false, localName = 'dialog') { super(); this.modal = modal; this.localName = localName; this.nodes = []; } + matches(selector) { assert.equal(selector, ':modal'); return this.modal; } + append(node) { node.parentNode?.nodes.splice(node.parentNode.nodes.indexOf(node), 1); node.parentNode = this; this.nodes.push(node); } +} + +test('a toast shows inside the topmost open modal and goes home when it closes (#533)', async () => { const { showWorkspaceToast } = await source; - const previous = global.document; - global.document = { getElementById: () => null }; + const closed = new FakeHost(false), lower = new FakeHost(true), top = new FakeHost(true); + const dialogs = [closed, lower, top]; + const { toast, restore } = install(dialogs); + const home = new FakeHost(false, 'body');home.append(toast); try { - assert.doesNotThrow(() => showWorkspaceToast('Nothing.')); + showWorkspaceToast('Opened the folder on this computer.'); + assert.equal(toast.parentNode, top); + // The upper modal closes: the toast stays visible in the one under it. + top.modal = false;top.dispatchEvent(new Event('close')); + assert.equal(toast.parentNode, lower); + lower.modal = false;lower.dispatchEvent(new Event('close')); + assert.equal(toast.parentNode, home); + showWorkspaceToast('Next.'); + assert.equal(toast.parentNode, home); } finally { - global.document = previous; + restore(); + } +}); + +test('a toast removed along with its modal still shows the next message (#533)', async () => { + const { showWorkspaceToast } = await source; + const modal = new FakeHost(true); + const { toast, restore } = install([modal]); + const home = new FakeHost(false, 'body');home.append(toast); + try { + showWorkspaceToast('Inside.'); + assert.equal(toast.parentNode, modal); + modal.modal = false; + const byId = document.getElementById; + document.getElementById = () => null; // the dialog left the page with the toast in it + showWorkspaceToast('After.'); + assert.equal(toast.parentNode, home); + assert.equal(toast.textContent, 'After.'); + document.getElementById = byId; + } finally { + restore(); } }); diff --git a/tests/test_image_reveal.py b/tests/test_image_reveal.py index 50415862..7e3d7781 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,84 +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_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_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_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): @@ -171,7 +196,10 @@ class TestRevealInFileManager(unittest.TestCase): self.path.parent.mkdir(parents=True) self.path.write_bytes(b"png") self.popen = patch.object(subprocess, "Popen").start() + self.popen.return_value.wait.return_value = 0 self.reaped = patch.object(file_reveal, "_reap").start() + # A desktop session unless a test says otherwise; CI has no display. + patch.dict(os.environ, {"DISPLAY": ":0"}).start() self.addCleanup(patch.stopall) def reveal(self, path=None): @@ -262,8 +290,72 @@ class TestRevealInFileManager(unittest.TestCase): self.reveal() self.assertEqual(self.argv(), ["/usr/bin/open", "-R", str(self.path)]) self.assert_detached() - self.popen.return_value.wait.assert_not_called() - self.reaped.assert_called_once_with(self.popen.return_value) + self.popen.return_value.wait.assert_called_once_with( + timeout=file_reveal.OPENER_WAIT + ) + self.reaped.assert_not_called() + + def test_a_failing_opener_is_reported_on_macos_and_linux(self): + cases = { + "darwin": patch.object( + file_reveal, "_macos_open", return_value="/usr/bin/open" + ), + "linux": self.which({"xdg-open": "/usr/bin/xdg-open"}), + } + for platform, opener in cases.items(): + with self.subTest(platform): + self.popen.return_value.wait.return_value = 4 + with ( + patch.object(file_reveal.sys, "platform", platform), + opener, + self.assertRaises(file_reveal.OpenerFailed), + ): + self.reveal() + self.reaped.assert_not_called() + + def test_an_opener_still_running_counts_as_started_and_is_reaped(self): + cases = { + "darwin": patch.object( + file_reveal, "_macos_open", return_value="/usr/bin/open" + ), + "linux": self.which({"xdg-open": "/usr/bin/xdg-open"}), + } + for platform, opener in cases.items(): + with self.subTest(platform): + self.reaped.reset_mock() + process = self.popen.return_value + process.wait.side_effect = subprocess.TimeoutExpired("opener", 1) + with patch.object(file_reveal.sys, "platform", platform), opener: + self.reveal() + self.reaped.assert_called_once_with(process) + + def test_linux_without_a_display_or_a_service_is_unavailable(self): + with ( + patch.object(file_reveal.sys, "platform", "linux"), + patch.dict(os.environ, {"DISPLAY": "", "WAYLAND_DISPLAY": ""}), + self.which({"xdg-open": "/usr/bin/xdg-open"}), + self.assertRaises(OpenerUnavailable), + ): + self.reveal() + self.popen.assert_not_called() + + def test_linux_on_wayland_alone_still_runs_xdg_open(self): + with ( + patch.object(file_reveal.sys, "platform", "linux"), + patch.dict(os.environ, {"DISPLAY": "", "WAYLAND_DISPLAY": "wayland-0"}), + self.which({"xdg-open": "/usr/bin/xdg-open"}), + ): + self.reveal() + self.assertEqual(self.argv(), ["/usr/bin/xdg-open", str(self.path.parent)]) + + def test_the_service_answers_without_needing_a_display(self): + with ( + patch.object(file_reveal.sys, "platform", "linux"), + patch.dict(os.environ, {"DISPLAY": "", "WAYLAND_DISPLAY": ""}), + self.which({"dbus-send": "/usr/bin/dbus-send"}), + ): + self.reveal() + self.assertEqual(self.popen.call_count, 1) def test_macos_open_is_the_fixed_system_path(self): self.assertEqual(file_reveal.MACOS_OPEN, "/usr/bin/open") @@ -323,7 +415,14 @@ class TestRevealInFileManager(unittest.TestCase): "DESKTOP_SESSION": "plasma", "XDG_SESSION_DESKTOP": "KDE", "XDG_CONFIG_HOME": "/home/u/.config", + "XDG_DATA_HOME": "/home/u/.local/share", "XDG_DATA_DIRS": "/usr/share", + # What xdg-open reads to choose an opener (kde-open6 on Plasma 6). + "KDE_FULL_SESSION": "true", + "KDE_SESSION_VERSION": "6", + "GNOME_DESKTOP_SESSION_ID": "this-is-deprecated", + "MATE_DESKTOP_SESSION_ID": "this-is-mate", + "LXQT_SESSION_CONFIG": "session", } with patch.dict(os.environ, inherited, clear=True): env = file_reveal._child_env() @@ -342,7 +441,13 @@ class TestRevealInFileManager(unittest.TestCase): "DESKTOP_SESSION": "plasma", "XDG_SESSION_DESKTOP": "KDE", "XDG_CONFIG_HOME": "/home/u/.config", + "XDG_DATA_HOME": "/home/u/.local/share", "XDG_DATA_DIRS": "/usr/share", + "KDE_FULL_SESSION": "true", + "KDE_SESSION_VERSION": "6", + "GNOME_DESKTOP_SESSION_ID": "this-is-deprecated", + "MATE_DESKTOP_SESSION_ID": "this-is-mate", + "LXQT_SESSION_CONFIG": "session", }, ) self.assertNotIn("/venv", file_reveal.SYSTEM_PATH) @@ -387,15 +492,19 @@ class TestRevealInFileManager(unittest.TestCase): for name, (available, outcome) in cases.items(): with self.subTest(name): self.popen.reset_mock(side_effect=True, return_value=True) - self.reaped.reset_mock() dbus = MagicMock() + opener = MagicMock() + opener.wait.return_value = 0 if isinstance(outcome, OSError): - self.popen.side_effect = [outcome, MagicMock()] + self.popen.side_effect = [outcome, opener] elif isinstance(outcome, subprocess.TimeoutExpired): dbus.wait.side_effect = [outcome, None] - self.popen.side_effect = [dbus, MagicMock()] + self.popen.side_effect = [dbus, opener] + elif outcome is None: + self.popen.side_effect = [opener] else: - self.popen.return_value.wait.return_value = outcome + dbus.wait.return_value = outcome + self.popen.side_effect = [dbus, opener] with ( patch.object(file_reveal.sys, "platform", "linux"), self.which(available), @@ -406,7 +515,7 @@ class TestRevealInFileManager(unittest.TestCase): ["/usr/bin/xdg-open", str(self.path.parent)], ) self.assert_detached(self.popen.call_count - 1) - self.reaped.assert_called_once() + opener.wait.assert_called_once_with(timeout=file_reveal.OPENER_WAIT) if name == "hung": dbus.kill.assert_called_once_with() @@ -451,6 +560,33 @@ class TestRevealInFileManager(unittest.TestCase): shell.assert_not_called() self.popen.assert_not_called() + def test_a_path_outside_the_root_reaches_no_opener_on_any_branch(self): + # What limits a same-origin request's reach (#533): one check ahead of + # every branch, needing no symlink, so it also runs on plain Windows. + outside = self.root.parent / "elsewhere.png" + outside.write_bytes(b"png") + both = {"dbus-send": "/usr/bin/dbus-send", "xdg-open": "/usr/bin/xdg-open"} + shell = FakeShell() + startfile = MagicMock() + for platform in ("win32", "darwin", "linux"): + with ( + self.subTest(platform=platform), + patch.object(file_reveal.sys, "platform", platform), + patch.object( + file_reveal, + "_windows_libraries", + return_value=(shell.ole32, shell.shell32), + ) as libraries, + patch.object(file_reveal.os, "startfile", startfile, create=True), + patch.object(file_reveal, "_macos_open", return_value="/usr/bin/open"), + self.which(both), + self.assertRaises(file_reveal.RevealTargetChanged), + ): + self.reveal(outside) + libraries.assert_not_called() + startfile.assert_not_called() + self.popen.assert_not_called() + def test_a_path_on_another_drive_is_not_contained(self): # Windows: commonpath raises ValueError across drives. with ( @@ -686,7 +822,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})) @@ -736,40 +872,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_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 61f56822..bea0015a 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 @@ -51,6 +35,9 @@ REVEAL_COOLDOWN = 1.0 DBUS_REPLY_TIMEOUT_MS = 2000 # The dbus-send process itself gets a little longer before it is killed. DBUS_WAIT = 3.0 +# An opener that fails says so at once; one still running after this long has +# started a file manager (it may stay in the foreground). +OPENER_WAIT = 1.0 FILE_MANAGER_DEST = "org.freedesktop.FileManager1" # Openers are looked up here only, never on ComfyUI's (venv) PATH, and the # children get this as their PATH. @@ -72,16 +59,27 @@ CHILD_ENV_NAMES = ( "XDG_DATA_DIRS", "XDG_CONFIG_DIRS", "XDG_CONFIG_HOME", + "XDG_DATA_HOME", # The X11 cookie: a non-default path under GDM or SDDM. "XAUTHORITY", "DBUS_SESSION_BUS_ADDRESS", "LANG", "LC_ALL", + # What xdg-open reads to choose an opener (kde-open6 on Plasma 6). + "KDE_FULL_SESSION", + "KDE_SESSION_VERSION", + "GNOME_DESKTOP_SESSION_ID", + "MATE_DESKTOP_SESSION_ID", + "LXQT_SESSION_CONFIG", ) 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): + """The file manager's opener ran and reported a failure.""" class RevealTargetChanged(Exception): @@ -104,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: @@ -137,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 @@ -169,33 +143,42 @@ 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. That stops a plain cross-site POST, + whose browser sets ``Origin`` and ``Sec-Fetch-Site`` itself. It does not stop + a page whose name was rebound to this server, as it trusts ``Host`` and + ``X-Forwarded-Host`` (TESTING.md D38): what limits that is the path check. """ - 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: @@ -393,6 +376,19 @@ def _launch(argv: list[str]) -> subprocess.Popen: return subprocess.Popen(argv, **options) # nosec B603 +def _launch_opener(argv: list[str]) -> None: + """Start an opener and report its failure (OpenerFailed), never waiting on + a file manager it stays attached to: that one is left to the reaper.""" + process = _launch(argv) + try: + status = process.wait(timeout=OPENER_WAIT) + except subprocess.TimeoutExpired: + _reap(process) + return + if status != 0: + raise OpenerFailed(f"exit status {status}") + + def _show_items(path: Path) -> bool: """Ask the freedesktop FileManager1 service to select *path*; True only when the service replied (``--print-reply`` makes the exit status real).""" @@ -427,8 +423,9 @@ def reveal_in_file_manager(path: Path, root: Path) -> None: """Open the file manager at *path*, a regular file inside *root*. Re-checks that containment first (RevealTargetChanged). Raises - OpenerUnavailable when no opener exists and OSError when the one found - cannot start. Never waits on the file manager itself. + OpenerUnavailable when no opener exists or Linux has no graphical session, + OSError when the one found cannot start and OpenerFailed when it reports a + failure. Never waits on the file manager itself. """ path = Path(path) if not path.is_absolute(): @@ -441,11 +438,12 @@ def reveal_in_file_manager(path: Path, root: Path) -> None: opener = _macos_open() if opener is None: raise OpenerUnavailable() - _reap(_launch([opener, "-R", str(path)])) + _launch_opener([opener, "-R", str(path)]) return if _show_items(path): return opener = _executable("xdg-open") - if opener is None: + has_display = os.environ.get("DISPLAY") or os.environ.get("WAYLAND_DISPLAY") + if opener is None or not has_display: raise OpenerUnavailable() - _reap(_launch([opener, str(path.parent)])) + _launch_opener([opener, str(path.parent)]) 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; diff --git a/web/js/workspace/toast.mjs b/web/js/workspace/toast.mjs index 249509c9..8a530aaa 100644 --- a/web/js/workspace/toast.mjs +++ b/web/js/workspace/toast.mjs @@ -5,6 +5,34 @@ export const LINK_TOAST_MS = 20000; let toastTimer; // Stops waiting for the pointer or focus to leave a toast that a newer one replaced. let releaseWait = null; +// The toast and the page element it lives in when no modal is open. +let toastElement = null; +let toastHome = null; + +/** + * The open modal the user is in. With several, the top layer decides, not page order (a viewer can sit earlier in the + * page than the dialog it opened from): only the top one can be hit, the rest being inert. + */ +function topModal() { + const modals = [...document.querySelectorAll('dialog')].filter(dialog => dialog.matches(':modal')); + if (modals.length < 2) return modals[0] ?? null; + const hit = document.elementFromPoint?.(window.innerWidth / 2, window.innerHeight / 2)?.closest('dialog'); + return modals.includes(hit) ? hit : modals.at(-1); +} + +/** + * #533: a modal covers and inerts the rest of the page, so the toast shows inside the topmost one (D176) and + * follows it out when it closes. Called by that dialog's `close` too; the same listener is only ever added once. + */ +function placeToast() { + const toast = toastElement; + const parent = toast.parentNode; + if (parent && parent.localName !== 'dialog') toastHome = parent; + const modal = topModal(); + const host = modal ?? toastHome; + if (host && parent !== host) host.append(toast); + modal?.addEventListener('close', placeToast); +} /** Whether the pointer or keyboard focus rests on the toast. */ const held = toast => typeof toast.matches === 'function' && toast.matches(':hover, :focus-within'); @@ -41,8 +69,11 @@ function scheduleClear(toast, delay) { * for LINK_TOAST_MS and while the pointer or focus is on it. */ export function showWorkspaceToast(message) { - const toast = document.getElementById('workspace-toast'); + // Null only when a dialog left the page with the toast still in it. + const toast = document.getElementById('workspace-toast') ?? toastElement; if (!toast) return; + toastElement = toast; + placeToast(); releaseWait?.(); if (Array.isArray(message)) { toast.replaceChildren(...message.map(part => {