Repository navigation
tool_cb_hdr: treat an empty -J filename as no filename - #23469
JayOfTheKeyboard wants to merge 1 commit into
Conversation
With -O -J, a response header like `Content-Disposition: attachment; filename=""` makes `parse_filename()` return an empty string, and curl then tries to save to that empty name. A debug build aborts on the `DEBUGASSERT(fname && *fname)` in `tool_create_output_file()`, and a release build (8.18.0 here) exits with error 23 instead of using the name from the URL. With -L, a redirect whose Location has no filename part, like `?x=1`, takes the same path, and a name of "." or ".." fails in the same way. Return NULL for an empty name, "." or "..", from one check at the end of `parse_filename()`. It replaces the two checks after the slash and backslash cuts, which ran before the CR/LF trim and so let an unquoted `filename=dir/` through to the abort. Checking last also catches an empty name left by the Windows sanitizing. -J then has no effect when the server gives no usable filename, as the documentation says. Test 1210 now also gets a `filename=""` reply and checks that the file is saved under the name from the URL. It fails without this change.
9efb765 to
15db746
Compare
|
Done. The |
|
Analysis of PR #23469 at 9efb7659: Test ../../tests/http/test_60_h3_proxy.py::TestH3Proxy::test_60_12_quic_session_resumption failed, but it has been permanently failing lately, so it's probably NOT a fault of the PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (4, to be specific) so it may be that this failure is rather a systemic issue with this job and not with this specific PR. Test ../../tests/http/test_02_download.py::TestDownload::test_02_07_download_reuse[h3] failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Generated by Testclutch |
|
(Those CI failures seem unrelated) |
There was a problem hiding this comment.
🟢 Approval recommended
The focused validation change preserves fallback behavior and includes regression coverage, with no unresolved findings.
No open issues 🎉
What changed in this PR
Fixes -J filename handling so unusable server-provided names fall back to the URL-derived filename.
Changes:
- Reject empty filenames,
.and..after trimming and sanitization. - Extend test 1210 to verify fallback for
filename="".
| File | Description |
|---|---|
| tests/data/test1210 | Adds empty-filename regression coverage. |
| src/tool_cb_hdr.c | Consolidates unusable-filename validation at the end of parsing. |
Review effort: Balanced
|
Thanks! |
With -O -J, a response header like
Content-Disposition: attachment; filename=""makesparse_filename()return an empty string, and curlthen tries to save to that empty name. A debug build aborts on the
DEBUGASSERT(fname && *fname)intool_create_output_file(). A releasebuild (8.18.0 here) exits with error 23 instead of using the name from
the URL. With -L, a redirect to a Location with no filename part, like
?x=1, hits the same path, and a name of "." or ".." fails the sameway.
Return NULL for an empty name, "." or ".." from one check at the end of
parse_filename(). This replaces the two checks after the slash andbackslash cuts. Those ran before the CR/LF trim, so an unquoted
filename=dir/still got through to the abort. Checking last alsocatches an empty name left over from the Windows sanitizing. When the
server gives no usable filename, -J now has no effect, as the
documentation says.
Test 1210 now also gets a
filename=""reply and checks that the fileis saved under the name from the URL. It fails without this change.
Found with an AI-driven test run over curl, and reproduced with the test in this PR.