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.

Sign up for GitHub

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

Jump to bottom

Implement CGI.escapeURIComponent and CGI.unescapeURIComponent #26

Merged
merged 1 commit into from
Aug 16, 2022
Merged

Implement CGI.escapeURIComponent and CGI.unescapeURIComponent #26

merged 1 commit into from
Aug 16, 2022

Conversation

casperisfine
Copy link

Ref: https://bugs.ruby-lang.org/issues/18822

Ruby is somewhat missing an RFC 3986 compliant escape method.

olleolleolle
olleolleolle reviewed Jun 9, 2022
View reviewed changes
Copy link
Contributor

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

(My comments were about... comments.)

ext/cgi/escape/escape.c Outdated Show resolved Hide resolved
lib/cgi/util.rb Outdated Show resolved Hide resolved
lib/cgi/util.rb Outdated Show resolved Hide resolved
@casperisfine
Copy link
Author

Thanks @olleolleolle

@casperisfine casperisfine changed the title Implement CGI.url_encode and CGI.url_decode Implement CGI. escapeURIComponent and CGI.unescapeURIComponent Aug 3, 2022
@casperisfine casperisfine changed the title Implement CGI. escapeURIComponent and CGI.unescapeURIComponent Implement CGI.escapeURIComponent and CGI.unescapeURIComponent Aug 3, 2022
@byroot
Copy link
Member

byroot commented Aug 3, 2022

I updated the PR to match what was accepted at the last meeting.

@byroot byroot requested review from hsbt and nobu August 3, 2022 19:43
@ioquatix
Copy link
Member

ioquatix commented Aug 4, 2022

I'm curious what was the justification for the non-standard camel case names? To match the standards?

@nurse
Copy link
Member

nurse commented Aug 11, 2022

I'm curious what was the justification for the non-standard camel case names? To match the standards?

  • It's because there are already escapeHTML
  • The expected use case of escapeURIComponent is similar to encodeURIComponent of JavaScript, but the escape is slightly different from it

@ioquatix
Copy link
Member

It's because there are already escapeHTML

Is it good justification? Why not add alias escape_html. escapeHTML and escapeURIComponent is not standard Ruby convention. Do we want to introduce it?

mame
mame reviewed Aug 16, 2022
View reviewed changes
ext/cgi/escape/escape.c Outdated Show resolved Hide resolved
nobu
nobu reviewed Aug 16, 2022
View reviewed changes
ext/cgi/escape/escape.c Outdated Show resolved Hide resolved
ext/cgi/escape/escape.c Outdated Show resolved Hide resolved
ext/cgi/escape/escape.c Outdated Show resolved Hide resolved
ext/cgi/escape/escape.c Show resolved Hide resolved
ext/cgi/escape/escape.c Outdated Show resolved Hide resolved
ext/cgi/escape/escape.c Show resolved Hide resolved
ext/cgi/escape/escape.c Outdated Show resolved Hide resolved
ext/cgi/escape/escape.c Show resolved Hide resolved
@byroot
Implement CGI.url_encode and CGI.url_decode
c2729c7 
[Feature #18822]

Ruby is somewhat missing an RFC 3986 compliant escape method.
@byroot byroot merged commit 4de4012 into ruby:master Aug 16, 2022
@byroot
Copy link
Member

byroot commented Aug 16, 2022

Thank you for the review @nobu and @mame

byroot added a commit to ruby/erb that referenced this pull request Oct 25, 2022
@byroot
url_encode: use CGI.escapeURIComponent
83ea8c8 
Ref: ruby/cgi#26

This native implementation is much faster
and available in `cgi 0.3.3`.
@byroot byroot mentioned this pull request Oct 25, 2022
Merged
byroot added a commit to ruby/erb that referenced this pull request Oct 25, 2022
@byroot
url_encode: use CGI.escapeURIComponent
5b0ee7a 
Ref: ruby/cgi#26

This native implementation is much faster
and available in `cgi 0.3.3`.
k0kubun pushed a commit to ruby/erb that referenced this pull request Oct 25, 2022
@byroot
url_encode: use CGI.escapeURIComponent (#23)
2d90e9b 
Ref: ruby/cgi#26

This native implementation is much faster
and available in `cgi 0.3.3`.
matzbot pushed a commit to ruby/ruby that referenced this pull request Oct 25, 2022
@byroot @matzbot
[ruby/erb] url_encode: use CGI.escapeURIComponent
217fdbf 
(ruby/erb#23)

Ref: ruby/cgi#26

This native implementation is much faster
and available in `cgi 0.3.3`.

ruby/erb@2d90e9b010
tenderlove pushed a commit to Shopify/ruby that referenced this pull request Oct 27, 2022
@byroot @tenderworks
[ruby/erb] url_encode: use CGI.escapeURIComponent
70c545c 
(ruby/erb#23)

Ref: ruby/cgi#26

This native implementation is much faster
and available in `cgi 0.3.3`.

ruby/erb@2d90e9b010
@jrochkind
Copy link

Thank you, this is very helpful!

The expected use case of escapeURIComponent is similar to encodeURIComponent of JavaScript, but the escape is slightly different from it

I am curious to learn more about how the escape differs from Javascript encodeURIComponent, and why.

@byroot
Copy link
Member

byroot commented Feb 5, 2023 •
edited
Loading

how the escape differs from Javascript encodeURIComponent

From MDN:

encodeURIComponent escapes all characters except:
A–Z a–z 0–9 - _ . ! ~ * ' ( )

Whereas CGI.escapeURIComponent, escape all characters, except:

A–Z a–z 0–9 - _ . ~

https://github.com/casperisfine/cgi/blob/c2729c7f333dd68a318b943ff06c7830b5db53a9/ext/cgi/escape/escape.c#L187-L194

So it's a bit more strict even.

@jrochkind
Copy link

Thank you! Any clues as to motivation? The ruby choice to escape seems right to me, I wouldn't want ! * ' ( ) characters in a URI path or query component, but perhaps they are technically legal. Escaping where not required shouldn't actually harm things either way, so no matter I guess.

I guess this also just shows how many people have had such trouble getting this right and consistent over time.

@byroot
Copy link
Member

byroot commented Feb 5, 2023

Thank you! Any clues as to motivation?

Ruby follow a fairly recent RFC https://www.rfc-editor.org/rfc/rfc3986#section-2.3, and allow you to escape for pretty much all parts of the URL (path, query, anchor, etc).

The behavior of JS EncodeURIComponent is much older, and I'm unsure which RFC or other spec it tries to follow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olleolleolle olleolleolle olleolleolle left review comments

@mame mame mame left review comments

@nobu nobu nobu approved these changes

@hsbt hsbt Awaiting requested review from hsbt

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@casperisfine @byroot @ioquatix @nurse @jrochkind @olleolleolle @nobu @mame