Skip to content

fix: sanitize download filenames to prevent path traversal (CWE-22)#8718

Closed
DevZenPro wants to merge 3 commits into
openclaw:mainfrom
DevZenPro:fix/sanitize-download-filename-path-traversal
Closed

fix: sanitize download filenames to prevent path traversal (CWE-22)#8718
DevZenPro wants to merge 3 commits into
openclaw:mainfrom
DevZenPro:fix/sanitize-download-filename-path-traversal

Conversation

@DevZenPro
Copy link
Copy Markdown

@DevZenPro DevZenPro commented Feb 4, 2026

Summary

Fixes #8696

The buildTempDownloadPath function constructs an output path directly from the remote server's suggested filename without sanitization. A malicious site can set a Content-Disposition filename containing path traversal sequences (e.g., ../../../etc/passwd) to write the downloaded file outside /tmp/openclaw/downloads, enabling arbitrary file overwrite.

Changes

src/browser/pw-tools-core.downloads.ts

  • Added sanitizeDownloadFilename() — a dedicated sanitization function that:

    • Normalizes backslash separators to forward slashes (cross-platform safety)
    • Strips directory components via path.basename()
    • Removes remaining .. traversal patterns and null bytes
    • Falls back to download.bin if the result is empty or only dots
  • Added resolved path guard in buildTempDownloadPath() — belt-and-suspenders check using path.resolve() + startsWith() to ensure the final path stays within the downloads directory, even if future edge cases bypass the filename sanitizer

src/browser/pw-tools-core.sanitize-download-filename.test.ts (new)

  • 10 test cases covering:
    • Normal filenames (pass-through)
    • Unix-style traversal (../../../etc/passwd)
    • Windows-style traversal (..\\..\\..\\windows\\system32\\config)
    • Dot-only and slash-only filenames
    • Null byte injection
    • Empty and whitespace-only inputs
    • Filenames with legitimate dots
    • Embedded traversal patterns

Testing

All new tests pass. All existing download tests continue to pass (verified with pnpm vitest run).

Security Impact

Closes a CVSS 8.8 (High) path traversal vulnerability that could allow arbitrary file overwrites on the host when the browser automation API processes downloads from malicious sites.

Greptile Overview

Greptile Summary

This PR hardens Playwright download handling by introducing sanitizeDownloadFilename() and using it in buildTempDownloadPath() so that server-suggested filenames can’t inject path separators / traversal sequences into the temporary download path. It also adds a “resolved path must remain within downloads dir” guard, plus a dedicated Vitest suite covering common traversal, separator, and empty/null-byte cases.

The change fits into the browser tooling layer (pw-tools-core.*) where waitForDownloadViaPlaywright() derives an output path from download.suggestedFilename() and persists the file via download.saveAs(); sanitizing at path construction time prevents a malicious Content-Disposition filename from escaping the intended downloads directory.

Confidence Score: 4/5

  • This PR is largely safe to merge and meaningfully improves download path safety, with a couple of edge-case robustness concerns.
  • Core mitigation (basename-based sanitization + resolved path guard) addresses the reported traversal vector and is covered by targeted tests. The main remaining concern is the brittleness of the startsWith-based directory containment check across filesystem/path normalization behaviors, plus potentially over-aggressive removal of .. sequences inside otherwise-benign basenames.
  • src/browser/pw-tools-core.downloads.ts

(2/5) Greptile learns from your feedback when you react with thumbs up/down!

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Comment thread src/browser/pw-tools-core.downloads.ts Outdated
Comment thread src/browser/pw-tools-core.downloads.ts Outdated
Fixes openclaw#8696

The buildTempDownloadPath function used the remote server's suggested
filename directly without sanitization, allowing a malicious site to
write files outside /tmp/openclaw/downloads via path traversal sequences
in Content-Disposition headers.

Changes:
- Add sanitizeDownloadFilename() that normalizes separators, strips
  directory components via path.basename(), removes traversal patterns
  and null bytes, with a fallback to 'download.bin'
- Add belt-and-suspenders resolved path check in buildTempDownloadPath()
  to ensure the output stays within the downloads directory
- Add comprehensive test suite covering traversal sequences, mixed
  separators, null bytes, edge cases, and normal filenames

refactor: address review feedback on path traversal fix

- Replace brittle startsWith() prefix check with path.relative() via
  isWithinDir() helper — robust across case-insensitive filesystems
- Remove aggressive '..' stripping from sanitizeDownloadFilename() that
  mangled legitimate filenames like 'my..notes.txt' — path.basename()
  already handles directory traversal; only reject exact '.' and '..'
- Add test for single dot basename and legitimate double-dot filenames

fix: use replaceAll for null byte removal to satisfy no-control-regex lint
@DevZenPro DevZenPro force-pushed the fix/sanitize-download-filename-path-traversal branch from 7cf5ef6 to 1727dc7 Compare February 4, 2026 10:00
@DevZenPro
Copy link
Copy Markdown
Author

CI Status

All checks introduced or affected by this PR are passing ✅:

  • lint (was failing → now passing after switching from regex /\0/g to replaceAll for null byte removal)
  • format, build, protocol, install-check, install-smoke, android

The only failing check is tsgo with:

src/cron/isolated-agent/run.ts(480,9): error TS2322: Type 'string | undefined' is not assignable to type 'string'.

This is a pre-existing issue in an unrelated file (cron/isolated-agent/run.ts) that this PR does not modify. The same error reproduces on all current CI runs — main branch CI runs are also queued/not completing.

@openclaw-barnacle
Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added stale Marked as stale due to inactivity and removed stale Marked as stale due to inactivity labels Feb 21, 2026
@steipete
Copy link
Copy Markdown
Contributor

Closing as AI-assisted stale-fix triage.

Linked issue #8696 ("[Bug]: Playwright download suggested filename allows path traversal writes") is currently closed and was closed on 2026-02-24T04:23:12Z with state reason not_planned.
Given that issue state, this fix PR is no longer needed in the active queue and is being closed as stale.

If the underlying bug is still reproducible on current main, please reopen this PR (or open a new focused fix PR) and reference both #8696 and #8718 for fast re-triage.

@steipete steipete closed this Feb 24, 2026
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.

[Bug]: Playwright download suggested filename allows path traversal writes

2 participants