Potential XSS mitigations and encoding corrections #151
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Encoding current directory name in directory listings. HTML in directory names (unlikely but possible) could previously have been rendered in the page. Encoding query strings in subdirectory links in directory listings. It is unclear if the query string values could actually have been rendered in any cases, but the unencoded values could produce inconsequentially incorrect links (e.g. "/?>" would produce subdirectory links like "/dir/>") and cause HTML validation to fail. Replacing text/plain responses messages for 400 and 500 errors with text/html response pages. This should not be neccessary in theory, but older versions of some browsers including Internet Explorer (and possibly newer versions in certain contexts) sometimes render text/plain content as text/html for backwards compatibility with poorly-configured servers, even if `X-Content-Type-Options: nosniff` is specified. Since the errors messages in these handlers could potentially include content reflected from the requests, the most conservative and safe approach is to use an HTML response with the error messages encoded. Also updates LICENSE.txt to include the current year and some additional contributors.
| @@ -1,6 +1,7 @@ | |||
| The MIT License (MIT) | |||
|
|
|||
| Copyright (c) 2013 Joshua Holbrook | |||
| Copyright (c) 2013-2015 Joshua Holbrook, Jon Ege Ronnenberg, James Halliday, | |||
| and Google Inc. | |||
There was a problem hiding this comment.
I'm making this PR for my work as a Google employee. It was requested that I add this to the copyright notice. As I personally understand things (I am not a lawyer), Google owns the copyright to the few lines being added. I added the largest contributors listed on GitHub to reflect their ownership as well.
jfhbrook
added a commit
that referenced
this pull request
Sep 14, 2015
I accepted #151 which included the request that Google Inc be added to the list of copyright holders. I won't lie, I was a little worried about what the ramifications would be for this project. I did a little research, and what I came up with was the following: 1. Yes, unless otherwise dealt with contributors have copyright on the lines of code they contributed to the project 2. Much of this hasn't been tested in court * Specifying "and contributors" and listing them in a separate document is *probably* okay Given the choice between not accepting PRs due to fears that someone's employer would try to sue the pants off me, and just doing the best I can and hoping that Google doesn't try to start something... well. I'm doing the best I can. Please don't sue me, Google.
|
Thanks! |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The change I submitted in #142 was adequate to prevent XSS in Firefox, but potentially not in all browsers. Previous versions of Internet Explorer have aggressively sniffed
text/plaincontent astext/htmldespite the header, and it may be possible to provoke this behaviour in current versions if compatibility modes are enabled. Bugs have also resulted in this behaviour in other browsers in some contexts, as recently as iOS 6 Safari. I'm not certain whether it's possible to trigger this for these specific error messages (I don't have a proof of concept), but in the interest of taking a conservative approach to security, I suggest replacing thetext/plainresponses with unencoded bodies withtext/htmlresponses with encoded bodies.This PR also corrects two other locations where data was being outputted without being encoded. There are probably not dangerous, but it would be more correct to encode them and avoid creating non-validating documents.
Please see here for a Travis build with the test changes but without the other changes, demonstrating that regressions cause the tests to fail.
Encoding current directory name in directory listings. HTML in directory names (unlikely but possible) could previously have been rendered in the page.
Encoding query strings in subdirectory links in directory listings. It is unclear if the query string values could actually have been rendered in any cases, but the unencoded values could produce inconsequentially incorrect links (e.g.
/?>would produce subdirectory links like/dir/>) and cause HTML validation to fail.Replacing text/plain responses messages for 400 and 500 errors with text/html response pages. This should not be neccessary in theory, but older versions of some browsers including Internet Explorer (and possibly newer versions in certain contexts) sometimes render text/plain content as text/html for backwards compatibility with poorly-configured servers, even if
X-Content-Type-Options: nosniffis specified. Since the errors messages in these handlers could potentially include content reflected from the requests, the most conservative and safe approach is to use an HTML response with the error messages encoded.Also updates LICENSE.txt to include the current year and some additional contributors.