fix(files): honour the escape flag in every filename validity message - #64374
Open
torvalstrom wants to merge 1 commit into
Open
torvalstrom wants to merge 1 commit into
torvalstrom wants to merge 1 commit into
Conversation
torvalstrom
requested review from
kristian-zendato and
sorbaugh
and removed request for
a team
September 15, 2026 17:14
skjnldsv
force-pushed
the
fix/filename-validity-honour-escape
branch
from
September 15, 2026 18:55
51fd27f to
d549b64
Compare
skjnldsv
approved these changes
Sep 15, 2026
Member
|
thanks @torvalstrom ! Quick note, you need to clarify whether you used AI or not in the OR bidy as well as the commit comment with an assisted-by line. See our github template e.g: |
getFilenameValidity() documents its second parameter as "Escape the matched string in the error (only set when used in HTML)", but only the forbidden-character branch passed it on. The reserved-name and forbidden-extension branches hardcoded `escape: false`, so a caller that asked for escaping because it renders the result as HTML got the matched segment back raw. The segments those two branches report come from server configuration - forbidden_filenames and forbidden_filename_extensions - so this is not reachable by an unprivileged user, and none of the three call sites in the tree ask for escaping today. It is the parameter quietly not doing what it says that is worth fixing, before a caller relies on it. Note the forbidden-character check runs first, which is why the existing escape tests could only ever cover that branch: any name containing <, > or " is rejected there. The new tests use & instead, which is escaped by the translation layer but is not a forbidden filename character, so it reaches the other two branches. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Tor Valstrom <51440386+torvalstrom@users.noreply.github.com>
torvalstrom
force-pushed
the
fix/filename-validity-honour-escape
branch
from
September 15, 2026 19:56
d549b64 to
0b3dde1
Compare
Author
|
Thanks for the pointer — done, and applied to all five of my open PRs rather than just this one:
Each commit now carries Apologies for making you ask — I had read the template's checklist but skipped straight past the AI section at the bottom. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
getFilenameValidity()documents its second parameter asbut only the forbidden-character branch passes it on. The reserved-name and forbidden-extension branches hardcode
escape: false, so a caller that asks for escaping — because it renders the result as HTML — gets the matched segment back raw.Not a vulnerability, to be clear
The segments those two branches report come from server configuration (
forbidden_filenames,forbidden_filename_extensions), not from anything an unprivileged user controls, and all three call sites in the tree passescape = falsetoday. So there is nothing to exploit. What is worth fixing is a documented parameter quietly not doing what it says, before a caller starts relying on it.Why the existing tests could not catch it
The forbidden-character check runs first, so any name containing
<,>or"is rejected there — in the one branch that already honoured the flag. That is why theescape optionblock only ever exercised that path.The two new tests use
&instead: the translation layer escapes it, but it is not a forbidden filename character, so validation falls through to the reserved-name and extension branches.Before and after
Same tests, against the unmodified source:
and with the change:
npx vitest run apps/files/src/is green overall — 27 files, 198 tests — andnpx eslintis clean on both changed files.AI (if applicable)
Written with Claude Code (
claude-opus-5); every change was reviewed, built and tested by me before opening the PR, and the measurements quoted above are real command output rather than model claims. Commits carryAssisted-by: ClaudeCode:claude-opus-5.