-
Notifications
You must be signed in to change notification settings - Fork 97
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request #67 from tilthouse/uri-parsing
URI encode before passing to URI object to deal with pathological URIs
- Loading branch information
Showing
1 changed file
with
4 additions
and
4 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1ef4515
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this commit was done and accepted without any tests around it....this has caused bugs in our application
1ef4515
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original committer tried to write a test but didn't succeed. Don't hesitate to open a pull-request with a failing test!
1ef4515
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not testable then it shouldn't be accepted. Tests should be required for new functionality, as is the case with widely used open source projects. Besides, the requirements in the readme state so.
What this commit does is encode url parameters, even if they are already encoded.
So if you have a url such as:
http://www.app.com/email?email=someemail%40somedomain.com
It gets encoded and redirected to:
https://www.app.com/email?email=someemail%254040somedomain.com
The test failed because the url it was testing is not valid from the get go. Rack ssl enforcer's job is not to fix invalid urls.
It's purpose is to redirect from http to https or visa versa with the original query string parameters.
Definitely not the desired behavior.
1ef4515
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, please open a new issue/pull-request with the comment above. Thank you!
1ef4515
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, will do!
1ef4515
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dan - i'm really sorry, please accept my apologies about that. I fully agree with you, will try to fix that this evening...
1ef4515
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tobmatth no worries at all - thanks for keeping this to be an awesome awesome library!
1ef4515
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also - should I write some tests that verify that already encoded parameters do not get encoded again?
1ef4515
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'd be greatly appreciated!