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

Remove forward-slash escape #486

Merged
merged 2 commits into from
May 17, 2021
Merged

Remove forward-slash escape #486

merged 2 commits into from
May 17, 2021

Conversation

alexwennerberg
Copy link
Contributor

@alexwennerberg alexwennerberg commented May 16, 2021

Reopens and resolves #245 based on new recommendation from OWASP

This was based off of the OWASP XSS prevention cheat sheet --
https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#output-encoding-rules-summary

However, there isn't really any attack vector based on forward slash alone, and
it's being removed in the next version of that document.

There is no proof that escaping forward slash will improve
defense against XSS, if all other special characters are escaped
properly, but it forces developers to use non-standard implementation of
the HTML escaping, what increases the risk of the mistake and makes the
implementation harder.

OWASP/CheatSheetSeries#516

This was based off of the OWASP XSS prevention cheat sheet --
https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#output-encoding-rules-summary

However, there isn't really any attack vector based on forward slash alone, and
it's being removed in the next version of that document.

> There is no proof that escaping forward slash will improve
> defense against XSS, if all other special characters are escaped
> properly, but it forces developers to use non-standard implementation of
> the HTML escaping, what increases the risk of the mistake and makes the
> implementation harder.

OWASP/CheatSheetSeries#516
@vallentin vallentin requested a review from djc May 17, 2021 08:44
djc
djc previously approved these changes May 17, 2021
@vallentin
Copy link
Collaborator

@alexwennerberg
Copy link
Contributor Author

This test needs to have // changed to // :)

https://github.com/djc/askama/blob/9232cafb6678160c3fe5154a7619a5e65c037a2b/testing/tests/filters.rs#L18-L27

Fixed! There was another test that broke too

@djc djc merged commit c0e7555 into rinja-rs:main May 17, 2021
@djc
Copy link
Collaborator

djc commented May 17, 2021

Thanks, this is nice!

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.

askama_escape Why escape slashes?
3 participants