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

Escape HTML from messages in error page #4430

Merged
merged 3 commits into from
Nov 9, 2021
Merged

Conversation

mauri
Copy link
Contributor

@mauri mauri commented Nov 3, 2021

Fixes #4355

Escapes HTML contained in error messages before rendering the error page.

@mauri mauri requested a review from a team as a code owner November 3, 2021 02:01
@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #4430 (704525b) into main (605c3c6) will increase coverage by 0.37%.
The diff coverage is 100.00%.

❗ Current head 704525b differs from pull request most recent head 7a6dc3e. Consider uploading reports for the commit 7a6dc3e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4430      +/-   ##
==========================================
+ Coverage   66.04%   66.41%   +0.37%     
==========================================
  Files          30       30              
  Lines        1602     1602              
  Branches      315      315              
==========================================
+ Hits         1058     1064       +6     
+ Misses        466      461       -5     
+ Partials       78       77       -1     
Impacted Files Coverage Δ
src/node/routes/errors.ts 91.30% <100.00%> (+21.73%) ⬆️
src/node/util.ts 79.78% <0.00%> (+0.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 605c3c6...7a6dc3e. Read the comment docs.

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 3, 2021

WOW! Not only did you fix this, but you added tests! This has to be a first (at least in a while) 🎉

@jsjoeio jsjoeio self-assigned this Nov 3, 2021
Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Thanks so much for adding this and writing tests!

medal to luke

query: {
to: "test",
},
} as unknown as express.Request
Copy link
Contributor

Choose a reason for hiding this comment

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

Bummer we have to do this but I don't see any better alternatives

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 3, 2021

The Docs preview is broken on forks at the moment but as long as everything else passes (it should), then we can merge!

@mauri
Copy link
Contributor Author

mauri commented Nov 3, 2021

Looks like E2E tests are failing to run, showing Try re-installing playwright with "npm install playwright"

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 3, 2021

Looks like E2E tests are failing to run, showing

Classic. Probably a yarn issue. Sorry about that. @code-asher and I will see if we can fix this.

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 3, 2021

Do you mind rebasing onto main and pushing up? (I don't think maintainers can edit)

We pushed a fix for playwright so hopefully everything works 🤞

@mauri
Copy link
Contributor Author

mauri commented Nov 3, 2021

@jsjoeio rebased onto main branch 👍

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 3, 2021

There's a high chance this doesn't work (e2e tests are failing on other PRs for some odd reason). We think this might fix but we'll see. So sorry for all the CI trouble!

@mauri
Copy link
Contributor Author

mauri commented Nov 8, 2021

@jsjoeio is there anything else I should do?

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 8, 2021

@mauri thanks for the ping! Looks like @code-asher updated your branch. Once CI finishes, we should be good to merge!

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 9, 2021

Not sure why macOS build is failing. Cancelling and re-running

image

@jsjoeio jsjoeio merged commit 31d5823 into coder:main Nov 9, 2021
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.

Cross Site Scripting(XSS)vulnerability in code-server
3 participants