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

Protect against XSS in diff-view by using application/json #884

Merged
merged 2 commits into from
Jul 11, 2023

Conversation

olsen232
Copy link
Collaborator

@olsen232 olsen232 commented Jul 5, 2023

Description

As discussed in #883
No extra tests yet, this is all the time I was willing to spend on it for right now.

Not clear if tests would be super valuable - we could test the following:

  • does html.escape escape the characters that it says it escapes
  • does json.dumps create valid json (ie, can we load it using json.loads)
    but honestly it seems like these would be pretty well tested already

We can't really test the following:

  • is html.escape guaranteed XSS safe for whatever browser the user has
  • is json.dumps guaranteed XSS safe for whatever browser the user has (ie, it won't get tripped up by {"xss": "</script><script>alert(1)</script>"} or some nonsense)
  • does the user's browser respect <script type="application/json"> and not allow XSS inside it? (my one does so that's good)

Checklist:

  • Have you reviewed your own change?
  • Have you included test(s)?
  • Have you updated the changelog?

@olsen232 olsen232 requested a review from rcoup July 5, 2023 23:12
@olsen232
Copy link
Collaborator Author

olsen232 commented Jul 5, 2023

@enricofer Find slightly improved diff-view.html template that is more secure against XSS attacks - you can copy the changes to your own HTML template(s).

@olsen232 olsen232 requested a review from craigds July 10, 2023 22:56
@olsen232 olsen232 merged commit d8823a0 into master Jul 11, 2023
@olsen232 olsen232 deleted the html-diff-xss-protection branch July 11, 2023 04:50
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.

2 participants