Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upCorrect handling of whitespace in base64 URLs #24865
Conversation
highfive
commented
Nov 25, 2019
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon. |
highfive
commented
Nov 25, 2019
|
Heads up! This PR modifies the following files:
|
|
Good find on the data url decoding problems! |
| bytes = bytes | ||
| .into_iter() | ||
| .filter(|&b| b != b' ') | ||
| .filter(|&b| b != b' ' && b != b'\x0C' && b != b'\n' && b != b'\r' && b != b'\t') |
This comment has been minimized.
This comment has been minimized.
jdm
Nov 25, 2019
Member
While it's logical to extend the existing code here, we should honour SimonSapin's request in #2909 (comment) to make use of the data-url crate and replace all of this decoding code if possible.
We should be able to use DataUrl::process(url.as_str()) and return the value of decode_to_vec() along with the url's mime_type() value. We'll need to add data-url as a new dependency in components/net/Cargo.toml as well.
This comment has been minimized.
This comment has been minimized.
pshaughn
Nov 25, 2019
Author
Member
Will do! I've never added a Cargo dependency before, but I'll have to sooner or later and I may as well start with this one!
|
Now using data-url's version of forgiving base64 decoding, but still doing the rest of the MIME parse within data_loader.rs, as suggested in #2909 (comment). This is a full fix for #2909 as far as WPT tests are concerned, but integrating with the full data-url class eventually would be nice. |
|
Could you squash your commits into one? Assuming https://github.com/servo/servo is your |
|
@SimonSapin : Is there a way to do that within this pull request, or should I make a new pull request with the merged commit? |
|
#24870 is the commit-squashed version of this, sorry for the mess |
Fix #2909 (squashed version of PR #24865) <!-- Please describe your changes on the following line: --> Now using data-url::forgiving_base64 instead of base64, this fixes all the fetch/data-urls/base64.* tests. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not quite fix yet #2909 <!-- Either: --> - [X] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Fix #2909 (squashed version of PR #24865) <!-- Please describe your changes on the following line: --> Now using data-url::forgiving_base64 instead of base64, this fixes all the fetch/data-urls/base64.* tests. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not quite fix yet #2909 <!-- Either: --> - [X] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
pshaughn commentedNov 25, 2019
•
edited by jdm
Whitespace handling in base64 url parsing now treats tabs, form-feeds, and linefeeds the same way it treats regular spaces
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors