fix: incorrect Content-Disposition encoding #25961
Conversation
update my repository
There was a problem hiding this comment.
Hey! thanks for this contribution 🎉 a few things here:
- Could you please fix lint?
- Could you please add a short sentence in the Notes section of the PR body explaining this fix?
- Could you please add a test for this behavior to ensure that it correctly addresses the issue?
Ah i see you added to the PR body:
I'll take a look again tomorrow :) |
| net::HttpContentDisposition header(value, std::string()); | ||
| std::string decodedFilename = | ||
| header.is_attachment() ? " attachement" : " inline"; | ||
| decodedFilename += "; filename=" + header.filename(); |
There was a problem hiding this comment.
Should the filename be wrapped with "?
There was a problem hiding this comment.
Not related to ".
We should at least have a test to verify that, this change would not break normal Content-Disposition header. |
Normal test has passed. |
|
@bigben0123 what is being asked of you is that you add a new test to directly test that this change does not break the normal, current functionality of the |
There was a problem hiding this comment.
The spec needs to test the code path of HttpResponseHeadersToV8, i.e. the code that converts the Content-Disposition response header to JS n webRequest module.
So we should also add a webRequest.onResponseStarted listener in the test to verify content-disposition is correctly passed. Something like this:
ses.webRequest.onResponseStarted((details) => {
expect(details.responseHeaders['content-disposition']).to.equal([' attachement; filename=aa%CC%ECaa.txt'])
});|
Congrats on merging your first pull request! 🎉🎉🎉 |
|
Release Notes Persisted
|
Description of Change
Closes #25628
Fixed by detecting the right charset using HttpContentDisposition.
Resolve if the filename's encoding is same as your local system charset, it can be showed correctly.
Checklist
npm testpassesTest is hard to write because the filename in http head cannot be encoded in GBK or other charset.
Release Notes
Notes: Fix webRequest module not recognizing the encoding of the filename in
Content-Dispositionheader.