Skip to content

Fall back to a safe name when upload filename sanitization yields a traversal token - #6971

Merged
masenf merged 3 commits into
mainfrom
claude/rel-fix-upload-sanitizer
Aug 28, 2026
Merged

Fall back to a safe name when upload filename sanitization yields a traversal token#6971
masenf merged 3 commits into
mainfrom
claude/rel-fix-upload-sanitizer

Conversation

@masenf

@masenf masenf commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Defect

FINDING-007 (SECURITY, MEDIUM) from 0.9.9a1 pre-release testing: _sanitize_upload_filename in packages/reflex-components-core/src/reflex_components_core/core/_upload.py (introduced by #6753) returns the bare traversal token .. for filenames composed entirely of dot/traversal segments ("..", "./../.", "..\\", "/.."). With no safe segment left it falls through to PureWindowsPath(filename).name, which is .. for those inputs. Handlers using the canonical get_upload_dir() / file.name then point at the upload directory's parent, and the write raises an unhandled IsADirectoryError inside the ndjson streaming generator, producing an HTTP 500 — despite the docstring promising a safe relative path. Confirmed live against the buffered upload endpoint; the streamed path shares the same sanitizer.

Fix

Guard the sanitizer's final result: anything that reduces to "", ".", or ".." now falls back to the bare name upload, which stays inside the upload directory. The guard covers both the absolute/drive-path branch ("/..", "C:\\..") and the relative branch where no safe segment survives. Both buffered and streamed uploads go through this one function, so both paths are fixed. Behavior for everything else is unchanged: normal hostile names ("../../evil.txt" -> evil.txt, "a b<>|.txt" kept), relative directories, and legitimate dot-like names ("...", "....", unicode dot lookalikes) all sanitize exactly as before.

Test plan

  • New regression tests in tests/units/components/core/test_upload.py:
    • test_upload_filename_sanitization_traversal_only_falls_back — parametrized over "..", "./../.", "..\\", "/..", "/foo/..", "C:\\..", "..//..", "", "."; all fail against unfixed main (sanitizer returned .. or "") and pass with the fix.
    • test_buffered_upload_traversal_only_filename_falls_back — buffered path via _upload_file_from_starlette, asserts path == Path("upload") and name == "upload".
    • test_chunk_parser_traversal_only_filename_falls_back — streamed path via the chunk multipart parser, asserts the emitted chunk filename is upload and data intact.
    • test_upload_filename_sanitization_keeps_dot_like_names"...", "....", and unicode-dot variants unchanged; extended the existing drops-path-segments parametrization with "../../evil.txt" and "a b<>|.txt" to lock in existing behavior.
  • uv run pytest tests/units/components/core/test_upload.py — 56 passed.
  • uv run ruff check . / uv run ruff format . — clean.
  • uv run pyright reflex tests — 0 errors.
  • News fragment: packages/reflex-components-core/news/+upload-sanitizer-dots.bugfix.md.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EMjBXPozsNeQNSBZecNH8x


Generated by Claude Code

Review in cubic

…raversal token

_sanitize_upload_filename returned the bare traversal token ".." for
filenames composed entirely of dot/traversal segments (e.g. "..",
"./../.", "..\\", "/.."): with no safe segment left it fell through to
PureWindowsPath(filename).name, which is ".." for those inputs. Handlers
building get_upload_dir() / file.name then pointed at the upload dir's
parent and crashed with an unhandled IsADirectoryError -> HTTP 500,
despite the docstring promising a safe relative path.

Guard the final result: anything that sanitizes to "", "." or ".."
now falls back to the bare name "upload", which stays inside the upload
directory. Both the buffered and streamed upload paths share the
sanitizer, so both are covered. Legitimate dot-like names ("...",
unicode dot lookalikes) and normal hostile names ("../../evil.txt")
keep their existing behavior.

Found as FINDING-007 during 0.9.9a1 pre-release testing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EMjBXPozsNeQNSBZecNH8x
@masenf
masenf requested a review from a team as a code owner August 28, 2026 17:50
@codspeed-hq

codspeed-hq Bot commented Aug 28, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 27 untouched benchmarks
⏩ 8 skipped benchmarks1


Comparing claude/rel-fix-upload-sanitizer (b0bdd21) with main (57716e1)

Open in CodSpeed

Footnotes

  1. 8 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@greptile-apps

greptile-apps Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR hardens upload filename sanitization by falling back to upload when no usable path segment remains.

  • Filters segments composed only of dots and spaces to prevent traversal-like filenames.
  • Adds buffered and streamed upload regression coverage.
  • Documents the fix in a bugfix news fragment.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/reflex-components-core/src/reflex_components_core/core/_upload.py Adds a safe fallback for filenames that reduce to unusable dot-and-space-only segments.
tests/units/components/core/test_upload.py Adds sanitizer and end-to-end regression coverage for buffered and streamed uploads.
packages/reflex-components-core/news/+upload-sanitizer-dots.bugfix.md Records the upload filename sanitization fix.

Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile

Comment thread tests/units/components/core/test_upload.py

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 3 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/reflex-components-core/src/reflex_components_core/core/_upload.py Outdated
@masenf masenf added this to the v0.9.9 milestone Aug 28, 2026
claude added 2 commits August 28, 2026 18:37
Win32 strips trailing dots and spaces when it opens a path, so a segment
like ".. " navigates to the parent directory there even though it is not
literally "..". Filter segments on `part.strip(". ")` instead of an exact
token match so the fallback covers those variants in both the absolute/drive
and relative branches.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TWNuEGFrVZ5g9zMCzb3eU1
@masenf
masenf merged commit b0c850a into main Aug 28, 2026
111 checks passed
@masenf
masenf deleted the claude/rel-fix-upload-sanitizer branch August 28, 2026 18:56
masenf pushed a commit that referenced this pull request Aug 28, 2026
…-context-refactor-jv3pig

Picks up #6971, #6959 and #6966 — upload filename sanitization, Vite plugin
imports, and packaging excludes. Clean auto-merge; main's ``templates.py`` edit
is in the Vite config template, well away from ``render_iterable_tag``.
masenf pushed a commit that referenced this pull request Aug 28, 2026
A ~29s 1920x1080 motion graphic covering the end-user-facing changes in
0.9.9, sourced from the 0.9.9a1 and 0.9.9a2 changelog entries across
reflex, reflex-base, the component packages and reflex-hosting-cli.

Nine beats: the dev-mode main-thread win (#6905), call-site
auto-memoization of @rx.memo (#6949), the move to React Router 8 (#6854),
DevTools component naming (#6945), stdlib logging with --json plus the
client_error report (#6863, #6865, #6827), the cloud CLI's whoami/token
and autoscaling flags (#6918, #6884, #6948), and a grid of the smaller
fixes a user would notice (#6786, #6593, #6944, #6971, #6790, #6920).

Styled to match reflex.dev by reusing its design system rather than
approximating it: the dark violet/slate scales, wave-line motif, radial
bloom and type scale come from reflex-site-shared, and the fonts are the
same @fontsource-variable Instrument Sans and JetBrains Mono files the
site serves.

Source only — the rendered mp4 is a build artifact, regenerated by
`npm run build` and ignored. Lives outside the packaged tree (hatch only
includes /reflex), so it does not affect the published wheel or sdist.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016arkVKmoogzUtZNfZZSvSo
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.

2 participants