Repository navigation
fix(downloads): stop a hostile file name throwing out of the download handler - #540
Conversation
… handler FileNameSanitizer.sanitize runs on the name the download source chose, so its input is whatever a server puts in Content-Disposition. Two inputs got the wrong answer. A long extension threw. Nothing bounds how long an extension is: it is whatever follows the last dot. A name of "a." plus 300 characters gives an extension of 301, and subtracting that from the 255 limit gave a negative budget that reached take(), which rejects a negative count and throws out of the handler. The extension is now abandoned when it cannot fit, rather than shortened, because a truncated extension is not the file's type and inventing one is worse than having none. A name of only dots came back ending in a dot. For ".", ".." or "...", the extension computed in step 4 is a lone ".", and the "ensure at least one character" fallback appended it raw to produce "download." That re-created exactly the trailing dot step 5 removes, and a trailing dot is what is invalid on Windows. Truncation could re-expose one too, so the rule is re-applied to whatever the cut produces. The file had no tests. Eleven are added, four of which fail against the previous implementation. They also pin the behaviour that was already right, so the Windows device names, the traversal reduction and the control-character handling cannot regress silently. Verified against the local ktlint and detekt gates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-up on @arjun28115's fix: the documented intent is that an extension which cannot fit in the 255-character limit is abandoned rather than shortened (a truncated extension is not the file's type), but the budget <= 0 branch kept a prefix of the extension. Preserve the base name only, cut to the limit, and pin the behavior with a regression test. 12 tests, 0 failures; ktlint and detekt clean.
|
@claude Please review PR #540 at 7d1ee57 against dev. Focus on the two FileNameSanitizer defects (an unbounded extension driving a negative take() budget out of the download handler, and dot-only names coming back with a trailing dot), the new truncate helper including the follow-up branch that abandons an oversized extension rather than truncating it, and the twelve regression tests. The original contribution is by @arjun28115; the oversized-extension branch alignment and its pinning test are review-agent follow-ups. Please report findings without making code changes. |
|
Claude finished @kshivang's task in 5m 58s —— View job Review of #540 at
|
| step | value |
|---|---|
| 4 | substringAfterLast('.') is "", so extension = "." |
| 5 | trimmed to "a" + 252×x + ".pdf", 257 chars |
| 7 | budget = 255 - 1 = 254; substringBeforeLast('.') → 253 chars; take(254) → all 253; + "." → trimEnd removes it |
Result: a 253-character name with no extension. The real extension is .pdf, it was four characters, and the contract in the KDoc at :126 is "keeping [extension] only if there is room" - there was room (251 + .pdf = 255). The download lands as an extensionless file.
Recomputing the extension inside truncate, from the string actually being cut, fixes it and makes the helper self-consistent with its own doc comment. I checked this does not disturb the cases the tests pin: for "a." + 300×x the recomputed extension is identical, so that still returns "a".
Pre-existing (the old code passed the same stale ext), but the helper is now the place where it belongs. Fix this →
Finding 2 - truncation can create a Windows device name
truncate re-applies step 5's rule to the cut (:153, and the comment at :151-152 explains why) but not step 4's. Step 4 ran on the untruncated name, and truncation cuts the end, so the base name can shrink onto a reserved word:
Input "CONSOLE." + "x".repeat(251) (259 chars). Step 4 sees nameWithoutExtension = "CONSOLE" - not reserved, nothing happens. Step 7: budget = 255 - 252 = 3, "CONSOLE".take(3) → "CON", plus the 252-character extension → CON.xxx…, 255 chars.
Win32 resolves CON followed by any extension to the console device, so the sanitizer hands back exactly the name step 4 exists to prevent. Narrow (it needs a crafted 259-character suggested name) and low impact (a failed or diverted write, not a traversal), but the same reasoning that put the trimEnd inside the helper applies to the device check. Also pre-existing in kind - the old step 7 had the same take(budget) + ext - so this is a follow-up, not a regression. Fix this →
Finding 3 - the device-name test claims more than holds
FileNameSanitizerTest.kt:107, a name is defused whatever their case or extension, only exercises a single extension (CON.txt). With two, the check does not fire: step 4 uses substringBeforeLast('.'), so sanitize("CON.tar.gz") computes nameWithoutExtension = "CON.tar", misses the set, and returns CON.tar.gz unchanged - still the console device on Windows. sanitize("nul.tar.gz") likewise.
The behaviour is pre-existing; the test title is new, and it is the kind of title someone reads later as proof the case is covered. Either narrow the name or make the check use the segment before the first dot. Two smaller notes on the same test:
- the loop assertion is
assertFalse(sanitize(name).uppercase() == name.uppercase()), which only proves the string changed -assertEquals("_CON", …)would pin the shape the way line 114 already does forCON.txt; - the PR body says "Trailing spaces do not evade the Windows device-name check … there is now a test". There isn't:
"CON "appears in neither input list. The reasoning is right (the top-leveltrim()at:61removes it before step 4), so this is one line to add rather than a behaviour question.
Things I checked and found correct
budget <= 0is safe. That branch is only reachable whenextension.length >= 255, which implies a dot exists, sosubstringBeforeLast('.')cannot silently return the whole string there.- The
ifBlank { "download" }at:153looks unreachable given step 6 always prefixes"download"to a name that is nothing but an extension - I could not construct an input that reaches it. Fine as a net; not worth removing. assertEquals(255, result.length)at:90is exact, not approximate:budget = 251, 251 x's +.pdf= 255.- Multi-dot names keep the meaningful part.
"report.tar." + 300×x→"report.tar", so the abandon branch does not throw away a real extension when there is a second one. - The ASCII argument in the PR body holds - step 3 restricts the string to a fixed ASCII set before any length arithmetic, so 255 characters is 255 bytes.
- Test placement.
composeApphas exactlycommonMain/desktopMain/desktopTest, sodesktopTestis the only home for this; and the PR invoked:composeApp:desktopTest, not a baretest, which is the distinction AGENTS.md calls out. - Scope is as claimed. The diff is two files, neither of which is
FilePicker.ktorFluckEngine.kt, and theTODOatFluckEngine.kt:3268is untouched.
One thing worth a sentence in the KDoc rather than a code change: the 255 guarantee is about the sanitizer's return value, not the name on disk - FluckEngine.kt:3244 passes it to generateUniqueFilePath, which may append " (1)", and the save dialog lets the user type anything at all (the comment at :3249 already says so).
Verdict
Net clear improvement over dev: a crash out of a JxBrowser callback and an invalid-on-Windows return value are both gone, and a file that had no tests now has twelve, four of which fail against the implementation they pin. The three findings above are all narrow, all pre-existing in behaviour, and all reasonable as follow-ups rather than as changes to this PR - except finding 3's test title, which I would fix here, because it is the artifact a future reader will trust.
No code changes made, as requested.
· branch fix/filename-sanitizer-edges
…names Review follow-up on @arjun28115's fix: - truncate() took the extension computed before the trailing-dot trim. For exactly the hostile shape this PR targets - a name ending in a dot - that copy is a stale lone '.', so a real '.pdf' that fit was dropped and the download landed extensionless. The extension is now recomputed from the string being cut, as the helper's KDoc always claimed. - The cut's trim can also shrink the base onto a reserved device name step 4 missed: 'CON ' (space before the dot) is not a reserved segment, so with an oversized extension the result was bare 'CON', which Windows refuses. The cut result is now defused like step 4. - The device-name test title overclaimed ('whatever their case or extension'); it now names what it covers, pins the defused shape, and includes the trailing-space input the description claimed was tested. 14 tests, 0 failures; ktlint and detekt clean.
|
Review follow-up at
Validation on this head: |
|
@claude Please review PR #540 at 9d04f35 against dev. This single new commit addresses your review of 7d1ee57 (5631805020): (1) truncate() now recomputes the extension from the string being cut, with a regression test for "a" + 252×x + ".pdf..."; (2) the cut result is defused against reserved device names, closing a path where "CON " + 255×x previously returned bare "CON"; (3) the device-name test was renamed to match its actual coverage, pins the defused shape, and adds the trailing-space input. Note our disposition of your finding 2 (a device name with an extension is legal on Windows) is in 5632119270 - please confirm or correct that analysis. Verify the fixes and report any remaining substantiated correctness blocker. Review only; no code changes. This is the first request for this new head; the previous request completed on its predecessor. |
|
Claude finished @kshivang's task in 4m 34s —— View job Review of #540 at
|
| step | value |
|---|---|
| 4 | last dot is the final char, so extension = "." (stale) |
| 5 | trimmed to "a" + 252×x + ".pdf", 257 chars |
| 7 | recomputed extension = ".pdf"; budget = 251; base "a" + 252×x → take(251) → "a" + 250×x; + .pdf = 255 |
That is exactly FileNameSanitizerTest.kt:126-127. Against 7d1ee57 the same input produced a 253-character extensionless name, so the test fails on the parent and passes here - it pins what it claims. The already-pinned "a." + 300×x → "a" case is unaffected: the recomputed extension there is identical (301), budget = -46, and the budget <= 0 branch still yields "a".
Fix 3 - confirmed
_$name for all seven bare names, _CON for "CON ", _CON.txt - all trace correctly (the top-level trim() at :61 removes the padding before step 4, as the description always claimed). The title now matches the coverage and the comment at :108-112 records the multi-dot gap rather than papering over it. This is the one item I said I would fix in this PR, and it is fixed.
Fix 2 - the code is a real fix; the reasoning in 5632119270 is wrong, and the original case is still live
The new test's input is genuinely broken on 7d1ee57 and genuinely fixed here. "CON ." + 255×x: step 4 sees the segment "CON " (space) and does not fire; budget = 255 - 256 = -1; the budget <= 0 branch keeps "CON ", trimEnd strips the space, and the old code returned bare CON. The new line at :170 returns _CON. Confirmed, and a better catch than the trace that prompted it.
But the premise used to close finding 2 does not hold. The disposition says "a device name with an extension is a legal Windows filename (the reservation applies to the bare name / trailing-dot form)". Microsoft's Naming Files, Paths, and Namespaces says the opposite, in as many words:
Do not use the following reserved names for the name of a file: CON, PRN, AUX, NUL, COM1 … LPT9. Also avoid these names followed immediately by an extension; for example, NUL.txt and NUL.tar.gz are both equivalent to NUL.
Win32 path parsing stops the device comparison at the first . in the last path component, so any extension after a device name still resolves to the device. This repo already agrees with that: step 4 rewrites CON.txt to _CON.txt, and FileNameSanitizerTest.kt:117 pins it. If a device name with an extension were legal, that line and that whole branch of step 4 would be unnecessary.
So the original trace stands. "CONSOLE." + "x".repeat(251) (259 chars) at this head:
- step 4: segment before the last dot is
CONSOLE, not reserved; - step 7: recomputed
extension= 252 chars,budget = 3,"CONSOLE".take(3)→"CON", plus the extension →CON.xxx…, 255 chars; :170compares the wholecutagainst the set."CON.XXX…"is not in it, so nothing fires.
Result: CON. + 251×x, which Win32 resolves to the console device. The download writes nowhere and no file appears.
Two consequences worth stating precisely, because they are easy to get wrong on a follow-up:
- The new defuse only reaches the
budget <= 0branch. In theelsebranch the cut always ends in the appended extension, and every reserved name is a bare 3-4 character token, socut.uppercase() in WINDOWS_RESERVED_NAMEScan never match there. The KDoc at:143-146says the cut is defused "the way step 4 defuses", and it is not - step 4 strips the extension before comparing,:170does not. Either the comparison should match step 4, or the KDoc should say which branch it covers. - The obvious fix overflows the limit. Comparing
cut.substringBeforeLast('.', cut)instead would fire onCON.xxx…, but theelsebranch already returns exactlyMAX_FILENAME_LENGTH, so prefixing_gives 256 and breaks theevery result stays within the length limitinvariant. The budget has to becomeMAX_FILENAME_LENGTH - extension.length - 1on the defusing path (or the base defused before the cut).
Severity: not a blocker. It needs a crafted 259-character Content-Disposition name, the outcome is a lost download rather than a traversal or an overwrite, and the behaviour is pre-existing - 7d1ee57 and dev both do the same thing. It belongs in the same follow-up as the CON.tar.gz gap the new comment already records, since both are the single underlying fact that the device check reads the segment before the last dot instead of the first.
Things I re-checked at this head and found correct
- No length regression from the new defuse. It can only fire in the
budget <= 0branch (above), wherecutis a reserved token of at most 4 characters, so"$replacement$cut"is at most 5. Theelsebranch is unreachable for it.assertEquals(255, …)at:90and:127both still hold. extensionis still used by steps 4, 5 and 6, so dropping it from thetruncatesignature leaves no dead local.- The no-dot path is unchanged. With no dot,
substringBeforeLast('.')returns the whole string,budget = 255, and the result is the first 255 characters - same as before the recompute. - The
trimEndat:169is not dead, though not for the reason the comment gives. In theelsebranch the extension is appended after the cut, so a trailing dot or space cannot survive there; it earns its place in thebudget <= 0and no-extension branches, where the cut is the last thing that happens (take(255)can land on a space). namereachingtruncatenever ends in a dot, because step 5 runs first - so the recomputedextensioncan no longer be the lone"."that caused fix 1.- Step 6 cannot produce an over-long name that escapes step 7.
"." + 300×xtakes thesanitized == extensionbranch, builds a 309-character"download" + ext, and step 7 cuts it back to"download". - 14 tests, matching the commit message, and still
:composeApp:desktopTestrather than a baretest, which is the distinction AGENTS.md calls out. - Scope unchanged. Still two files;
FilePicker.kt,FluckEngine.ktand theTODOatFluckEngine.kt:3268are untouched.
Verdict
No remaining correctness blocker. Fixes 1 and 3 are confirmed as described, and fix 2's code is a genuine improvement even though it was landed on a premise I think is mistaken. The one correction to the record: a device name followed by an extension is the device on Windows, so finding 2's CONSOLE. + 251×x case is still open at this head, alongside the CON.tar.gz gap the new test comment already documents. Both are narrow, both pre-existing, and both are one follow-up.
No code changes made, as requested.
· branch fix/filename-sanitizer-edges
The review of 9d04f35 is right and the disposition that closed finding 2 was not. A device name followed by an extension is still the device: Microsoft's naming rules say NUL.txt and NUL.tar.gz are both equivalent to NUL, and this file already agreed, since step 4 rewrites CON.txt to _CON.txt. The check compared the segment before the LAST dot, which leaves two holes that are the same underlying fact. Multi-dot names were missed entirely. "CON.tar.gz" was compared as "CON.tar", which is not reserved, so it passed through untouched. Truncation could manufacture a device out of a name that was not one. "CONSOLE." followed by 251 characters is not reserved at step 4. Cutting it to fit the limit left "CON." and the rest, which Windows resolves to the console, so the download writes nowhere and no file appears. The defuse added in 9d04f35 did not catch it, because it compared the whole cut rather than the base, and only the budget <= 0 branch could ever match. Both close by comparing the segment before the first dot, in one helper used by step 4 and by the cut. Trailing spaces are trimmed before the comparison for the same reason: Windows ignores them. Defusing prepends one character, which would push a full-length result to 256. The cut is therefore redone one character shorter when that happens, which is always enough because the prefix is exactly one character. Three tests, all of which fail against 9d04f35. The last asserts the invariant rather than a case: no input in the list comes back as something Windows would resolve to a device. The title of the existing device test said "with a single extension", which was accurate before this and is not now, so it has been corrected rather than left to mislead. 17 tests, ktlint and detekt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both open items are closed at The review is right about the premise and I want to be explicit that I was wrong too. My PR description said I had "checked and found correct" that trailing spaces do not evade the device check, because The fix is the first dot rather than the last. Win32 stops parsing a device name at the first One helper now does the comparison, used by step 4 and by the cut:
On the length concern the review raised: defusing prepends exactly one character, so rather than let a full-length result reach 256 the cut is redone one character shorter. One retry is always sufficient, and Three tests, all of which fail against I also corrected the title of the existing device test. It said "with a single extension", which was an accurate description of the coverage before this commit and is misleading after it. On attribution, thank you for the note. For the record I am happy either way, and the follow-ups made the change materially better than what I opened. The extension recompute in Local run: 17 tests, ktlint and detekt clean. Still One thing I have deliberately not touched: |
|
Corrections to the record on the device-name question, and a note on concurrent work:
Validation at 40700c1 (local): |
|
@claude Please review PR #540 at 40700c1 against dev. New author commit since your review of 9d04f35: a shared defuseDeviceName helper now compares the segment before the FIRST dot (with trailing-space trim) at step 4 and at the cut, and truncate redoes the cut one character shorter when defusing would overflow the limit. Verify: (1) the first-dot comparison is correct against the documented Win32 rule, including the multi-dot and truncated cases; (2) the redo-at-limit-1 argument that one redo is always enough; (3) no length invariant is broken (255 limit) on any branch; (4) the three new tests pin what they claim. The disposition correction is in 5632345726. Report any remaining substantiated correctness blocker. Review only; no code changes. This is the first request for this new head; the previous request completed on its predecessor. |
|
Claude finished @kshivang's task in 7m 47s —— View job Review of #540 at
|
| input | segment compared | result |
|---|---|---|
CON.tar.gz |
CON |
_CON.tar.gz |
COM1.a.b |
COM1 |
_COM1.a.b |
aux.x.y.z |
aux |
_aux.x.y.z |
CONSOLE.txt |
CONSOLE |
CONSOLE.txt (unchanged - no over-fire) |
CON .txt / CON |
CON → CON |
_CON.txt / _CON |
.con |
"" |
.con |
The CONSOLE. + 251×x manufacture is closed. 259 chars; step 4 sees CONSOLE, not reserved; cutTo(255): ext = 252, budget = 3, "CONSOLE".take(3) → CON + ext = 255, defuse fires → 256 > 255 → cutTo(254): budget = 2 → CO. + 251×x, 254 chars, CO not reserved. Returned. Not a device, within the limit.
Worth noting explicitly because it is easy to misread the test: on that input the first pass's defuse is discarded - the answer is a two-character-shorter base (CO.), not _CON.. That is safe, and safe for a reason that generalises (next section).
2. The redo-at-limit-1 argument holds - and for a stronger reason than the comment gives
truncate:150-158's comment says one redo is enough "because the prefix is exactly one character". True, but the argument as written reads as if it depends on the retry not defusing. It does not, and the load-bearing invariant is stronger:
cutTo(name, limit)returns at mostlimit + 1, on every branch.
budget > 0:base.take(budget) + extension≤budget + ext.length=limit.budget <= 0:base.take(limit)≤limit.trimEndonly shrinks;ifBlank { "download" }is 8 chars and both limits are ≥ 254.defuseDeviceNameadds exactly 1.
So cutTo(254) ≤ 255 unconditionally, and a third pass is impossible even when the retry also defuses. That case is reachable, and the bound is exactly tight there - see the missing test below. I'd add half a sentence to the KDoc stating the limit + 1 bound rather than the prefix length, since that is what makes a second redo unnecessary.
Two sub-facts I checked because they would have broken the argument:
- The retry can never fall into the
budget <= 0branch from a defuse. For the defuse to fire in theelsebranch, the cut segment must be a reserved token, which is 3-4 characters, sobudget ≥ 3; the retry'sbudget - 1 ≥ 2 > 0. The retry therefore never silently abandons an extension the first pass kept. - A defuse in the
budget <= 0branch never triggers a retry. There the tidied cut is the reserved token itself (≤ 4 chars), socut.length ≤ 5and the prefix survives - which is what:134pins.
3. The 255 invariant holds on every branch, with zero slack
From the bound above, the final return is cutTo(255) (≤ 256, and taken only when ≤ 255) or cutTo(254) (≤ 255). The non-truncated paths return unchanged only when length ≤ 255, and step 4's defuse or step 6's fallback can only push past the limit into step 7, which handles it. Spot checks: "x"×300 + ".pdf" → 255 exactly (:90); "a" + 252×x + ".pdf..." → 255 exactly (:127); "a." + 300×x → "a".
Verified that the equality case is real, i.e. the invariant is tight rather than comfortable - see the input in the next section, which returns exactly 255 after a retry that itself defuses.
4. The three new tests pin what they claim
Each fails against 9d04f35, so none is decorative:
a device name with any number of extensions is still defused(:138) - at the parent,CON.tar.gzis compared asCON.tar, not reserved, returned unchanged →assertEquals("_CON.tar.gz", …)fails.CONSOLE.txtin the same test is the no-over-fire counterweight, which is the right thing to pin alongside it.truncation cannot manufacture a device name out of one that was not(:149) - at the parent that input returnsCON.+ 251×x, whosesubstringBefore('.')isCON→ fails. At this head,CO.+ 251×x → passes.no input produces a name Windows would resolve to a device(:163) -CON.tar.gz,NUL.tar.gz,COM1.a.b,lpt9.tar.gz,aux.x.y.zall fail at the parent. All eleven inputs traced clean at this head. The invariant shape is the right call for a file where both bugs were inputs nobody enumerated.
The retitle of :107 is accurate now (the old title's "single extension" caveat is exactly what the new tests removed).
Coverage gap, and a concrete input for it. No test exercises a retry that itself defuses - the one path where a defused cut survives into the returned value, and where the 255 bound is exactly tight. The existing cut-level cases do not reach it: CON . + 255×x (:134) is now caught by step 4's own space-trim before truncation ever runs (it was the cut-level defuse that caught it at 9d04f35, so that test no longer covers the path it was written for), and CONSOLE. + 251×x has its prefix thrown away by the retry. The gap closes with one assertion:
// budget lands inside the run of spaces, so both the 255 cut and the 254 retry
// trim onto "CON" - the retry defuses and the result is exactly at the limit.
val result = FileNameSanitizer.sanitize("CON X." + "x".repeat(249))
assertEquals("_CON ." + "x".repeat(249), result)
assertEquals(255, result.length)Trace: 256 chars in; step 4 compares CON X, not reserved; cutTo(255): ext = 250, budget = 5, "CON X".take(5) = CON → defused → 256; cutTo(254): budget = 4 → CON → defused → _CON . + 249×x = 255. Add this test →
One assertion weaker than its neighbours. :155 only asserts the result is not CON and is within the limit, so it would also pass if the implementation returned "download" or dropped the extension entirely. The neighbouring tests pin shapes (assertEquals("_CON", …), assertEquals("a" + 250×x + ".pdf", …)); assertEquals("CO." + "x".repeat(251), result) would pin this one the same way and would document that the redo shortens the base rather than defusing.
Things I checked at this head and found correct
- The reserved set is complete for what can reach the check. The docs also name
CONIN$/CONOUT$and the superscript formsCOM¹/COM²/COM³; step 3 maps$and every non-ASCII character to_before step 4 runs, so none of those can survive to be compared. (If a doc revision ever addsCOM0/LPT0, the set would need them - unchanged by this PR either way.) - Step 5's trim cannot re-expose a device. For
trimEnd('.', ' ')to land on a reserved token, the input must be that token followed only by dots and spaces - anddefuseDeviceName's owntrimEnd(' ')at:205already fires on exactly that shape (CON .,CON..). This is the whole reason the space-trim belongs inside the helper rather than at the call site. - No double prefix. If step 4 defuses, the string starts with
_, so the cut's segment is_CON…and never re-fires. - Step 4's
extensionis still needed (step 6's fallback) and still correctly uses the last dot: it answers "what is this file's type", a different question from "which segment does Win32 compare". The comment at:92-93points a reader at the helper; naming that the local is for the step-6 fallback would save the next reader the trip. isExecutableFileis unaffected.FluckEngine.kt:3266passes the sanitized name; a leading_and a defused multi-dot name both leave the final extension intact, soCON.exe→_CON.exeis still detected.- Callers unchanged. Still two files in the diff;
FilePicker.kt:91,FluckEngine.kt:3217and theTODOatFluckEngine.kt:3268are untouched. The behaviour change visible to them is a leading_on multi-dot device names, which then flows throughgenerateUniqueFilePathas any other name does. isExecutableFilereading the last extension is right, and the author's note is the correct disposition: the device rule and the execute rule genuinely disagree about which dot matters, because Windows executes the last extension.- 17 tests, matching the commit message, and still run via
:composeApp:desktopTestrather than a baretest, which is the distinction AGENTS.md calls out.
Verdict
Approve on correctness. The first-dot comparison is the right rule and strictly more defensive than what it replaced, the redo terminates with a bound that holds on every branch, the 255 invariant survives every path I could construct (including the tight one), and the three new tests all fail against their parent. The two items above - a test for the retry-that-defuses path and a shape assertion at :155 - are test-only and non-blocking.
No code changes made, as requested.
· branch fix/filename-sanitizer-edges
…shape Closes the two test-only items from the third Claude review of 40700c1: - new test for the one path where a defused cut survives into the returned value: the 255 cut and the 254 redo both trim onto CON, both defuse, and the result lands exactly at the 255 limit; - the CONSOLE. + 251x test now pins the full redo shape (CO. + 251x) instead of a non-device check that would also accept a fallback name; - the padded-name test comment now says step 4, not the cut, defuses it; - truncate's comment states the cutTo limit + 1 bound that makes one redo always enough, and the step 4 extension local says it serves the step 6 fallback.
|
Follow-up to the third Claude review (5632352390) at
Verified locally: @arjun28115 the correctness work is yours (with @claude co-authoring at |
|
Claude finished @kshivang's task in 2m 7s —— View job Review of #540 at
|
| pass | arithmetic | cut | defuse |
|---|---|---|---|
cutTo(255) |
ext = 250, budget = 5, "CON X".take(5) |
"CON ." + 249×x = 255 |
segment CON → CON → fires → 256 |
cutTo(254) |
ext = 250, budget = 4, "CON X".take(4) |
"CON ." + 249×x = 254 |
segment CON → CON → fires → 255 |
"_CON ." + 249×x = 1 + 4 + 1 + 249 = 255, exactly FileNameSanitizerTest.kt:143-144. This is the case I asked for: the only one I have constructed where a redo defuses, and the limit + 1 bound is tight rather than comfortable. It is the right input, and "_CON " is not itself reserved, so the defuse does not re-fire.
2. The CONSOLE. shape pin - confirmed
259 chars in; step 4 sees CONSOLE. cutTo(255): ext = 252, budget = 3, "CONSOLE".take(3) → CON + ext = 255, defuse fires → 256. cutTo(254): budget = 2 → "CO." + 251×x = 254, CO not reserved. :165 pins exactly that.
This is a real strengthening, not cosmetic: the previous assertFalse(… != "CON") + length <= 255 pair would have passed on "download", on a dropped extension, and on the defused "_CON." + 251×x had the redo gone the other way. The comment at :161-163 now also records why the answer is a two-character-shorter base rather than a prefixed one, which was the part most likely to be misread.
3. Both comment clarifications are accurate
:131-133-"CON ." + 255×x: step 4'sdefuseDeviceNamecomparessubstringBefore('.')="CON "→trimEnd(' ')→CON, so it fires before truncation; the cut then takes thebudget <= 0branch (ext = 256, budget = -1),take(255)on"_CON ", trims to"_CON", and_CONis not reserved. The test still asserts"_CON", so it pins the end-to-end shape as the comment says, and it no longer claims to cover the cut-level defuse (which the new test does).:93-94-extensionis read at exactly two places,:108(sanitized == extension) and:113, both of which are step 6. "Feeds only the fallback in step 6" is literally true, and it is the sentence that stops the next reader wondering why step 4 uses the last dot whiledefuseDeviceNameuses the first.:152-156- thelimit + 1bound is the right invariant and is stated correctly:budget > 0gives≤ budget + ext.length = limit,budget <= 0gives≤ limit,trimEndonly shrinks,ifBlank { "download" }is 8 characters against a limit of 254, and the defuse adds exactly one. SocutTo(254) ≤ 255unconditionally, and the redo terminates even when it defuses in turn - which item 1 is now the test for.
4. No production-behaviour change
The only FileNameSanitizer.kt hunks are the two comment blocks; no expression changed. The new test and the retitled assertions therefore pass at 40700c1 as well - they are pins on behaviour that commit established, not new fixes, which is what the description says. Cumulative diff is still the same two files (git diff 3b16790 HEAD: +317/-12), so FilePicker.kt, FluckEngine.kt and the TODO at FluckEngine.kt:3268 remain untouched. 18 @Test methods, matching the 18/18 reported, and still run via :composeApp:desktopTest rather than a bare test - the distinction AGENTS.md calls out.
One comment claim is overstated (comment-only, non-blocking)
:139 says the new test is "the only path where a defused cut survives into the returned value". A defused cut from the first pass can survive too, when the budget <= 0 branch trims a long first segment onto a device:
"CON" + " ".repeat(252) + "X." + "x".repeat(300) (557 chars) - step 4 compares "CON" + 252 spaces + "X", whose trimEnd(' ') ends in X, so it does not fire. cutTo(255): ext = 301, budget = -46, substringBeforeLast('.') is 256 chars, take(255) = "CON" + 252 spaces, trimEnd → CON, defuse → _CON, 4 chars, returned on the first pass.
So the unique property is the one the test name states - a redo that defuses, at the tight bound - not "the only defused cut that survives". Worth narrowing the comment to match the name, since the file's own history is of comments being read later as coverage claims (which is what item 3's retitle was about). Nothing about the behaviour is wrong: that input returns _CON, which is not a device and is within the limit. Fix this →
Optional, and smaller: the new input would also sit naturally in the no input produces a name Windows would resolve to a device list at :174-186, which is the invariant test both of this PR's device bugs were found by. "_CON ." passes it (segment _CON), so it is a one-line addition rather than a question.
Verdict
Approve. The two suggestions from the 40700c1 review are implemented as asked and are stronger than what they replaced - the redo path now has the test it was missing, and the CONSOLE. case is pinned to a shape rather than to a negation. Both new comments say something a reader could not otherwise derive, and the limit + 1 bound is the correct thing to have written down. The one item above is a comment narrowing, not a defect.
No code changes made, as requested.
· branch fix/filename-sanitizer-edges
… handler (#540) * fix(downloads): stop a hostile file name throwing out of the download handler FileNameSanitizer.sanitize runs on the name the download source chose, so its input is whatever a server puts in Content-Disposition. Two inputs got the wrong answer. A long extension threw. Nothing bounds how long an extension is: it is whatever follows the last dot. A name of "a." plus 300 characters gives an extension of 301, and subtracting that from the 255 limit gave a negative budget that reached take(), which rejects a negative count and throws out of the handler. The extension is now abandoned when it cannot fit, rather than shortened, because a truncated extension is not the file's type and inventing one is worse than having none. A name of only dots came back ending in a dot. For ".", ".." or "...", the extension computed in step 4 is a lone ".", and the "ensure at least one character" fallback appended it raw to produce "download." That re-created exactly the trailing dot step 5 removes, and a trailing dot is what is invalid on Windows. Truncation could re-expose one too, so the rule is re-applied to whatever the cut produces. The file had no tests. Eleven are added, four of which fail against the previous implementation. They also pin the behaviour that was already right, so the Windows device names, the traversal reduction and the control-character handling cannot regress silently. Verified against the local ktlint and detekt gates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(downloads): abandon an oversized extension instead of truncating it Review follow-up on @arjun28115's fix: the documented intent is that an extension which cannot fit in the 255-character limit is abandoned rather than shortened (a truncated extension is not the file's type), but the budget <= 0 branch kept a prefix of the extension. Preserve the base name only, cut to the limit, and pin the behavior with a regression test. 12 tests, 0 failures; ktlint and detekt clean. * fix(downloads): recompute the cut's extension and re-defuse reserved names Review follow-up on @arjun28115's fix: - truncate() took the extension computed before the trailing-dot trim. For exactly the hostile shape this PR targets - a name ending in a dot - that copy is a stale lone '.', so a real '.pdf' that fit was dropped and the download landed extensionless. The extension is now recomputed from the string being cut, as the helper's KDoc always claimed. - The cut's trim can also shrink the base onto a reserved device name step 4 missed: 'CON ' (space before the dot) is not a reserved segment, so with an oversized extension the result was bare 'CON', which Windows refuses. The cut result is now defused like step 4. - The device-name test title overclaimed ('whatever their case or extension'); it now names what it covers, pins the defused shape, and includes the trailing-space input the description claimed was tested. 14 tests, 0 failures; ktlint and detekt clean. * fix(downloads): compare a device name at the first dot, as Win32 does The review of 9d04f35 is right and the disposition that closed finding 2 was not. A device name followed by an extension is still the device: Microsoft's naming rules say NUL.txt and NUL.tar.gz are both equivalent to NUL, and this file already agreed, since step 4 rewrites CON.txt to _CON.txt. The check compared the segment before the LAST dot, which leaves two holes that are the same underlying fact. Multi-dot names were missed entirely. "CON.tar.gz" was compared as "CON.tar", which is not reserved, so it passed through untouched. Truncation could manufacture a device out of a name that was not one. "CONSOLE." followed by 251 characters is not reserved at step 4. Cutting it to fit the limit left "CON." and the rest, which Windows resolves to the console, so the download writes nowhere and no file appears. The defuse added in 9d04f35 did not catch it, because it compared the whole cut rather than the base, and only the budget <= 0 branch could ever match. Both close by comparing the segment before the first dot, in one helper used by step 4 and by the cut. Trailing spaces are trimmed before the comparison for the same reason: Windows ignores them. Defusing prepends one character, which would push a full-length result to 256. The cut is therefore redone one character shorter when that happens, which is always enough because the prefix is exactly one character. Three tests, all of which fail against 9d04f35. The last asserts the invariant rather than a case: no input in the list comes back as something Windows would resolve to a device. The title of the existing device test said "with a single extension", which was accurate before this and is not now, so it has been corrected rather than left to mislead. 17 tests, ktlint and detekt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(downloads): pin the retry-that-defuses cut and the CONSOLE redo shape Closes the two test-only items from the third Claude review of 40700c1: - new test for the one path where a defused cut survives into the returned value: the 255 cut and the 254 redo both trim onto CON, both defuse, and the result lands exactly at the 255 limit; - the CONSOLE. + 251x test now pins the full redo shape (CO. + 251x) instead of a non-device check that would also accept a fallback name; - the padded-name test comment now says step 4, not the cut, defuses it; - truncate's comment states the cutTo limit + 1 bound that makes one redo always enough, and the step 4 extension local says it serves the step 6 fallback. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Shivang <shivang.iitk@gmail.com>
Hostile download filenames previously could make FileNameSanitizer throw while truncating an oversized extension, aborting the download handler. Truncation could also leave or manufacture a Windows reserved device name. The sanitizer now abandons an extension that cannot fit, defuses the first-dot Windows device segment, and retries one character shorter if defusing the cut would exceed the 255-character limit.
Original contribution: Arjun Singla (@arjun28115), including the sanitizer redesign and author follow-up 40700c1 (Claude co-authored). Review-agent repairs 7d1ee57 and 9d04f35 addressed initial edge gaps; final agent commit 8cf9a06 strengthens regression assertions and clarifies comments. Agent repair is not original contributor work.
Validation at exact head 8cf9a06: 18 focused local tests, root ktlintCheck/detekt, and all nine hosted Build & Test checks pass (34585318297). Latest Claude review (34585338925) approves this head with no correctness blocker. Its optional comment narrowing is acknowledged: the test uniquely covers a redo that defuses at the tight bound, not every possible surviving defused cut. No production change is warranted for that wording.
Current review reuses the completed unchanged-head validation and checks compatibility with freshly fetched dev under the serialized merge lock. This useful fix is distinct from executable consent #484 and store endpoint/hash changes #571. No live app visual tests were performed: hostile names, dots-only names and Windows multi-dot device names should download without a handler exception; executable classification should still recognize the final extension.