Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(static_file_handler): Support for blank spaces and percent encodi… #457

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hdf1996
Copy link

@hdf1996 hdf1996 commented Jul 25, 2020

Summary

Adds handling for whitespaces in url for static files handler.

Issue

#405

Note

Im really new at Rust, and even more with ownership and low level memory handling, so probably i've made some mistakes in the way i changed this to a String. I'm more than open to hear suggestions on how to do this any better.
Fix inspired by lawliet89@f1c03e2

@jolhoeft
Copy link
Member

This looks reasonable to me. We are now allocating a new String with each call, but I'm willing to pay that price. Could you add a unit test to verify percent decoding is working correctly?

@hdf1996
Copy link
Author

hdf1996 commented Jul 26, 2020

Thanks for the feedback @jolhoeft. I extracted the code to a fn to be able to test it easier, and included a few sample paths to the test

req.path_without_query().map(|path| {
debug!("{:?} {:?}{:?}", req.origin.method, self.root_path.display(), path);
let percent_decoded_path = percent_decode_path(path).unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about losing track of this. I am a little concerned about the unwrap here. If that gets triggered, the current thread will be killed. That may be an opening for a DOS, where a series of ill formed URLs exhausts all the threads. We should probably change how we are handling threads to avoid that, but I want to defer that until after I get the async migration completed.

This should probably be changed to return None if the path cannot be decoded.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, will do. I might take some time to do the change since im busy with some personal stuff, but i'll make sure to do the change as soon as i have some free time. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants