8000 process_filename_asterisk_before_filename by chiwenchen · Pull Request #1762 · rack/rack · GitHub
[go: up one dir, main page]
Skip to content

process_filename_asterisk_before_filename#1762

Closed
chiwenchen wants to merge 1 commit intorack:masterfrom
chiwenchen:parse_filename_asterisk_before_filename
Closed

process_filename_asterisk_before_filename#1762
chiwenchen wants to merge 1 commit intorack:masterfrom
chiwenchen:parse_filename_asterisk_before_filename

Conversation

@chiwenchen
Copy link
Contributor
@chiwenchen chiwenchen commented Jul 13, 2021 •
edited
Loading

when I post a form data with file that Content-Deposition contain both filename and filename* like below

"Content-Disposition: form-data; name=\"file\"; filename=\"\xE9\xA2\xA8\xE6\x99\xAF.jpeg\"; filename*=UTF-8''%E9%A2%A8%E6%99%AF.jpeg\r\nContent-Type: image/jpeg\r\n"

I am expecting that filename* having higher priority to be parsed, because it has encoding information that filename don't have.
and also in MDN Doc - Content Depositioon said

filename and filename* are present in a single header field value, filename* is preferred over filename when both are understood.

so I think it might be good to switch the condition order inside get_filename method.

PS: Since I don't know should we test the private method, so I just bypass the test case, please let me know if I still need to write test, thanks.

@chiwenchen chiwenchen mentioned this pull request Jul 13, 2021
Closed
@jeremyevans
Copy link
Contributor
jeremyevans commented Nov 3, 2021

I think this is a good change, but we should definitely have a test case for it. Should be simple to have a spec in spec_multipart.rb where filename and filename* are different in the input, and the resulting :filename is tested to be based off filename*.

@ioquatix
Copy link
Member
ioquatix commented Jan 20, 2022

Will fix up in a separate PR.

@ioquatix ioquatix closed this Jan 20, 2022
@ioquatix ioquatix mentioned this pull request Jan 20, 2022
Merged
@wtn wtn mentioned this pull request Oct 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chiwenchen @jeremyevans @ioquatix
0