Skip to content

fix(ego-browser): name downloads by guid and confine the reported path - #224

Open
Hotragn wants to merge 1 commit into
citrolabs:devfrom
Hotragn:fix/downloads-guid-naming
Open

fix(ego-browser): name downloads by guid and confine the reported path#224
Hotragn wants to merge 1 commit into
citrolabs:devfrom
Hotragn:fix/downloads-guid-naming

Conversation

@Hotragn

@Hotragn Hotragn commented Aug 6, 2026

Copy link
Copy Markdown

Summary

page.waitForEvent("download") reports join(downloadDir, suggestedFilename) as the download path, but under behavior: "allow" the browser chooses the on-disk name itself: it sanitizes suggestedFilename per platform (path separators everywhere; :?"<>|*, reserved device names, and trailing dots/spaces on Windows) and deduplicates collisions with " (1)" suffixes. Whenever any of that fires, download.path() points at a file that was never written and saveAs() fails with ENOENT. This PR switches the primary path to Chromium's allowAndName behavior — the same scheme Playwright uses — so the on-disk name is the download guid and the reported path can no longer drift from it.

Related issue

No open issue. When closing #130/#132, @WUXM5 noted that Playwright "uses GUID-based naming together with path containment checks" and that additional boundary hardening might be addressed separately — this implements exactly that design. The user-visible defect it fixes (path drift under sanitization/deduplication) is distinct from the traversal claim that #130 investigated and rejected.

Changes

  • src/driver/downloads.tsBrowser.setDownloadBehavior now requests behavior: "allowAndName", so the browser writes the file as its download guid inside the per-download temp dir; download.path() returns that guid path. suggestedFilename() is unchanged and still reports the server-suggested name; saveAs(target) still copies to the caller's chosen name.
  • The Page.setDownloadBehavior fallback (runtimes without the Browser-level command) keeps behavior: "allow" — the Page-level command does not support allowAndName — and continues deriving the path from suggestedFilename there, as does the primary path if an event ever arrives without a guid.
  • Every reported path now passes a containment check against the download directory before it is returned. Chromium sanitizes separators out of suggestedFilename before emitting the events, so a well-behaved runtime never trips it; it hardens the boundary against an event source that does not sanitize (the defense-in-depth gap acknowledged in Security: path traversal in download facade via server-controlled suggestedFilename (arbitrary local file read) #130).
  • src/driver/downloads.test.mjs — updated facade/fallback assertions plus two new tests: missing-guid derivation, and rejection of an escaping composed path.

Verification

Run from package/ego-browser (Windows 11, Node 24; suite is platform-neutral):

npm test                      -> 313 pass, 0 fail (3 download tests before, 5 after)
npm run typecheck             -> ok
npm run validate:site-skills  -> site skills ok
npm audit --audit-level=moderate -> 0 vulnerabilities

Revert-check: with src/driver/downloads.ts reverted and the new tests kept, "returns a Playwright-style download facade" (guid naming) and "rejects a path that escapes the download directory" both fail; with the fix they pass.

Real-browser check I could not run here (no macOS install): a download whose Content-Disposition filename contains a character the platform sanitizes (for example report:v2.pdf on macOS via a colon on Windows-style checks, or any name that collides with an existing file so the browser writes file (1).png) — before this change download.path() names the unsanitized/original variant, after it names the guid file the browser actually wrote. The existing scripts/real-browser-e2e/cases/downloads.mjs case covers the happy path end-to-end and is worth a run on macOS before merge.

Impact

  • Public helper API or behavior
  • Agent skill or instructions
  • Site learning
  • Installation or update flow
  • Build, CI, or release process
  • Documentation only
  • No externally visible impact

download.path() now returns a guid-named file instead of a suggestedFilename-named file. That matches Playwright's contract (agents are told to use suggestedFilename() for the display name and saveAs() for a stable destination, both unchanged), so scripts written against Playwright semantics keep working; only code that assumed the temp-path basename equals the suggested name would notice — and that assumption is exactly what breaks today under sanitization or dedup.

Checklist

  • The PR targets the correct base branch (dev for normal changes; only dev may target main).
  • The change is focused and does not include unrelated cleanup.
  • Tests were added or updated for behavior changes, or the reason they are unnecessary is explained above.
  • Relevant tests and validation commands pass locally.
  • Public helper JSDoc and agent-facing documentation are updated when the helper surface changes.
  • No credentials, tokens, cookies, personal data, or other secrets are included.
  • A release-note label is selected (fix).

@Hotragn

Hotragn commented Aug 11, 2026

Copy link
Copy Markdown
Author

Flagging this myself before it costs anyone review time: I think this PR is superseded on the sprint-1.3.0 line.

That branch adds playwright-core@1.52.0 as a runtime dependency and removes src/driver/ entirely, so the src/driver/downloads.ts this PR modifies no longer exists there. Playwright's own download implementation already does both things this PR argues for — GUID-based on-disk naming and a containment check on the reported path — which is the design @WUXM5 described when closing #132, so adopting Playwright wholesale gets there more thoroughly than my patch does.

The PR is still green and mergeable against dev (where src/driver/downloads.ts is present and the drift bug is real), so I have left it open rather than closing unilaterally — dev is the base CONTRIBUTING documents. Happy to close it immediately if sprint-1.3.0 is the line that ships; just say so, or I will close it once that work reaches dev/main.

Same situation applies to #251 and #252, which touch src/video-recorder.ts and src/driver/screencast.ts — both also absent on sprint-1.3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant