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

0.7 escapes safe characters (change in behavior from 0.6) #24

Closed
sethboyles opened this issue Jan 13, 2022 · 5 comments
Closed

0.7 escapes safe characters (change in behavior from 0.6) #24

sethboyles opened this issue Jan 13, 2022 · 5 comments
Labels

Comments

@sethboyles
Copy link
Contributor

0.6 used URI.escape, which uses URI::DEFAULT_PARSER.regexp[:UNSAFE] (/[^\-_.!~*'()a-zA-Z\d;\/?:@&=+$,\[\]]/) under the hood to determine which characters to escape. This did not escape characters like forward slashes.

The change in 0.7 uses a much simpler regex that will result in URIs like this:

%2Fmy%2Furi

When they used to be result like this:

/my/uri

Is there a reason to escape more characters? We would prefer a non-breaking change that escaped the same characters as 0.6.

@sethboyles
Copy link
Contributor Author

@geemus would you consider a PR that fixes this regression? This is blocking us from upgrading to Ruby 3, so we would be happy to contribute

sethboyles added a commit to sethboyles/fog-local that referenced this issue Jan 19, 2022
* [fog#24] fixes regression

Authored-by: Seth Boyles <sboyles@pivotal.io>
@sethboyles
Copy link
Contributor Author

Made a PR anyway: #25

I also added a test that shows the regression.

sethboyles added a commit to sethboyles/fog-local that referenced this issue Jan 20, 2022
* fog#24 fixes regression introduced in 0.7

Authored-by: Seth Boyles <sboyles@pivotal.io>
@geemus
Copy link
Member

geemus commented Jan 20, 2022

Hey, thanks. Sorry I think I missed the initial issue as I wasn't actively watching the repo any more.

I believe we made those changes in response to the URI method deprecation and didn't intend for it to change behavior, so a more effective fix (and one that doesn't rely on our own regex) sounds great.

Copy link

This issue has been marked inactive and will be closed if no further activity occurs.

@geemus
Copy link
Member

geemus commented Mar 15, 2024

Looks like this was closed some time ago by #25 and we must have forgotten to close this issue at the time.

@geemus geemus closed this as completed Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants