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

updated escape_html not to escape forward slash #2097

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

JunichiIto
Copy link
Contributor

fix: #2096

NOTE: This PR will break backwards compatibility.

@JunichiIto JunichiIto force-pushed the unescape-slash branch 2 times, most recently from b610b96 to bf734d6 Compare July 17, 2023 08:46
@jeremyevans
Copy link
Contributor

Can you switch this to use CGI.escapeHTML if available for improved performance?

@ioquatix
Copy link
Member

Can you switch this to use CGI.escapeHTML if available for improved performance?

I think we should do that, but I'm fine if it's a separate PR.

Copy link
Member

@ioquatix ioquatix left a comment

Choose a reason for hiding this comment

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

LGTM.

@jeremyevans jeremyevans merged commit af9e278 into rack:main Jul 17, 2023
13 of 14 checks passed
@JunichiIto
Copy link
Contributor Author

JunichiIto commented Jul 17, 2023

@ioquatix
Thank you for your approve.

@jeremyevans
Thank you for your merge.

Can you switch this to use CGI.escapeHTML if available for improved performance?

I have not measured yet, but I guess Rack::Utils.escape_html would be faster because CGI.escapeHTML has more lines:

https://github.com/ruby/cgi/blob/v0.3.6/lib/cgi/util.rb#L74-L91

And some behaviors are different:

  • Rack escapes ' to ' but CGI escapes to '. (hexadecimal vs decimal)
  • Rack cannot escape \300< (raises "invalid byte sequence in UTF-8 (ArgumentError)"), but CGI can escape it because CGI converts encoding to ASCII-8BIT before calling gsub!.

I'm not sure if it can be replaced with CGI.escapeHTML.

Related commits

@jeremyevans
Copy link
Contributor

CGI.escapeHTML is implemented in C in recent versions of the cgi gem, so it should definitely be faster (https://github.com/ruby/cgi/blob/master/ext/cgi/escape/escape.c#L458C6-L458C6). Basically, you require cgi/escape, if it succeeds you use it, and if it fails, then you can fallback to the existing implementation.

I don't think the behavior differences you mention are blockers to merging, though they may require test updates. CGI behavior using decimal is better as it saves a character :), and supporting non-UTF8 is also a useful feature.

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.

HTML escape of slash is not recommended by OWASP
3 participants