From ff59d9b0865470fb5c914c1e43f28e8c68cc761f Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Tue, 20 Oct 2026 21:39:55 -0700 Subject: [PATCH] fix(inspector): say the folder is already opening on a busy Show in folder A 429 busy from the reveal route now toasts "The folder is already opening." The browser workflow checks that the reveal POST carries the page's own Origin and an application/json body, and TESTING.md D38 records the tightened route, the toast table and the accepted residual risk of a header-less local proxy. --- TESTING.md | 65 ++++++++++++++++------- tests/browser/full-inspector.spec.mjs | 7 +++ tests/js/workspace-file-inspector.test.js | 3 +- utils/file_reveal.py | 13 ++--- web/js/workspace/file-inspector.mjs | 2 + 5 files changed, 65 insertions(+), 25 deletions(-) diff --git a/TESTING.md b/TESTING.md index eca7b024..841c9921 100644 --- a/TESTING.md +++ b/TESTING.md @@ -607,25 +607,54 @@ Recorded deviations on this page (spec ยง8): viewer's File & provenance the Path value stays text, followed in its `dd` by a "Show in folder" button (`Show in folder`), only for a listed file (`root_index`, `root_scope`, `path`), so never in the side panel or a recorded run. - It POSTs that identity to `/prompt_manager/images/reveal` and toasts "Opened the - folder on this computer.", "Show in folder works only in a browser on the computer - running ComfyUI." (403 `not-local`), "No file manager is available on this - computer." (501 `unavailable`) or "The folder could not be opened. Try again." - The route is local-only: a loopback `request.remote`, none of `Forwarded`, - `X-Forwarded-For`, `X-Real-IP` or `X-Forwarded-Host`, and (against DNS rebinding) a - loopback `Host` and, when sent, `Origin` (`localhost`, `*.localhost` or a loopback - address). 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 + It POSTs that identity as JSON to `/prompt_manager/images/reveal` and toasts: + + | 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." | + | 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 + `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 `_gallery_scope_error` (409 `source-changed`), and the path resolves only inside - that root to a regular media file (403/404). `utils/file_reveal.py` starts the - opener as an argv list with an absolute executable, no shell, DEVNULL streams and - `close_fds`, never waited on: Windows `explorer.exe` from `%SystemRoot%` (never - PATH or the working directory) with `/select,` and the path, macOS `open -R`, - Linux the FileManager1 `ShowItems` dbus call (waited at most 2 s, only to detect a - failed start) and else `xdg-open` on the folder. `tests/test_image_reveal.py` mocks - `Popen`, `sys.platform` and `shutil.which`; the browser server's opener is a stub - (`POST`/`GET /__test__/reveal`). The inspector comparison reads closed disclosures - as hidden, so the button needs no mapping there. + that root to a regular media file (403/404; a resolution `OSError` such as + `ENAMETOOLONG` is the same JSON 404, and the client's path is never logged at error + level). One reveal runs at a time per process with a 1 s cooldown after it + (`RevealGate`, 429 `busy`), on its own single worker (`reveal_executor`), never + ComfyUI's default executor. Just before launching, `utils/file_reveal.py` re-checks + that the real path is still a regular file inside the root (a swapped parent is + 404). Windows starts no process: `ILCreateFromPathW` and + `SHOpenFolderAndSelectItems` select the file through the shell API (Explorer's own + command line splits on commas, so a path like `output\a,\Windows\...` must never + 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 + 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`, + the shell API and `os.startfile`; the browser server's opener is a stub + (`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. - **D39**: a recipe-linked image shows its recipe's own category and tags (#224), not its positive's. The concept shows the prompt's category and tags for every image; the concept fixture's recipes have neither, so their images show no category chip diff --git a/tests/browser/full-inspector.spec.mjs b/tests/browser/full-inspector.spec.mjs index f5b7cef2..cd58f244 100644 --- a/tests/browser/full-inspector.spec.mjs +++ b/tests/browser/full-inspector.spec.mjs @@ -283,6 +283,9 @@ test('Show in folder asks the server to open the listed file and announces each const toast=page.locator('#workspace-toast'); const [sent]=await Promise.all([page.waitForRequest(entry=>entry.url().endsWith('/prompt_manager/images/reveal')),show.click()]); expect(sent.method()).toBe('POST'); + // The route takes only a same-origin JSON request, as a real browser sends it. + const sentHeaders=await sent.allHeaders(); + expect(sentHeaders.origin).toBe(new URL(page.url()).origin);expect(sentHeaders['content-type']).toBe('application/json'); 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}$/); @@ -293,6 +296,10 @@ test('Show in folder asks the server to open the listed file and announces each await expect(await request.post('/__test__/reveal',{data:{outcome}})).toBeOK(); await show.click();await expect(toast).toHaveText(message); } + // A reveal already in progress answers 429 busy. + 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. 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'}})); diff --git a/tests/js/workspace-file-inspector.test.js b/tests/js/workspace-file-inspector.test.js index ae97498b..3a01d7a1 100644 --- a/tests/js/workspace-file-inspector.test.js +++ b/tests/js/workspace-file-inspector.test.js @@ -82,6 +82,7 @@ test('Show in folder outcomes use the D38 toasts',async()=>{ 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.'); - for(const error of [{status:403,code:null},{status:409,code:'source-changed'},{status:500},new TypeError('offline')]) + 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/utils/file_reveal.py b/utils/file_reveal.py index 916191a5..43725a76 100644 --- a/utils/file_reveal.py +++ b/utils/file_reveal.py @@ -1,11 +1,12 @@ """Show one gallery file in this computer's file manager (TESTING.md D38). -The route that calls this is local-only (``is_local_request``), single-flight -(``RevealGate``) and hands in a -path it has already resolved inside a gallery root. 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, no shell and no inherited streams, -so neither a file name nor ``PATH`` can change what runs or what it parses. +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 +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 diff --git a/web/js/workspace/file-inspector.mjs b/web/js/workspace/file-inspector.mjs index 323e9ec0..00497199 100644 --- a/web/js/workspace/file-inspector.mjs +++ b/web/js/workspace/file-inspector.mjs @@ -22,6 +22,7 @@ const REVEAL_URL = '/prompt_manager/images/reveal'; 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_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']]; @@ -83,6 +84,7 @@ 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 === 501) return REVEAL_UNAVAILABLE; + if (error?.status === 429 && error.code === 'busy') return REVEAL_BUSY; return REVEAL_FAILED; }