Repository navigation
querystring.unescape() accepting input that is not URL escaped #10727
Description
Activity
If you're interested in catching the error yourself, why not use your own custom decoder? If you're using it via
querystring.parse(), you can pass in your own decoder there.While I happen to agree that the current behavior is not ideal, changing is not likely to happen, as unfortunate as that is. The new WHATWG URL includes improved handing of querystring content but even it currently still falls back on the querystring module (but only after it performs better handling of the pct-encoded content)
Is some software relying on node accepting data that should be URL encoded, but is actually not URL encoded?
Yes, almost certainly.
@not-an-aardvark if you are so certain, could you provide an example?
For example, this code will cause a server to return a 500 error on certain inputs if
querystring.unescapethrows.I haven't looked at any popular packages yet (I just found this example from a GitHub search), but if we changed the behavior, any server that uses
querystring.unescapeon untrusted user input would be liable to return a 500 error and/or crash when it encounters a malformed querystring. Even if throwing an error for malformed querystrings would be slightly more intuitive in this case, I don't think it would be a good idea to start making it throw now.@not-an-aardvark fixing such a bug certainly changes behavior – and I think that software should return a
400 Bad Requestin that case, not a500 Internal Server Error– but I do not see how anything in that code depends on accepting invalid input.Could a fix for this issue be scheduled for the next major version of Node to prevent suprises?
but I do not see how anything in that code depends on accepting invalid input.
If a client passes a malformed querystring and
querystring.unescapestarts throwing an error, the server will start crashing. This is a DoS risk. (Admittedly, that wouldn't happen in this particular case since Sails has graceful error handling, but it's very likely that people are running servers using similar behavior without graceful error handling.)Could a fix for this issue be scheduled for the next major version of Node to prevent suprises?
It could be released in a major version, but personally I don't think it would be worth it due to the potential for new DoS vulnerabilities when a server upgrades. People typically don't write tests for malformed querystrings.
You are right: People usually do not write tests for invalid input. That does not mean that it is a good idea to silently correct invalid input, as that can create subtle vulnerabilities that depend on parser differentials (like the Android master key vulnerability).
Since the patch is trivial … do you know how to get a bugfix scheduled for a major version?
@nmoskopp The first step would be to submit a PR.
For what it's worth, the URL Standard's percent decoder explicitly accepts broken encoding. querystring is IMO doing the right thing here.
Should this remain open?
No, IMO. Both of our application/x-www-form-urlencoded decoders are acting according to the spec.
The documentation for
querystring.unescape()says:Accepting invalid input usually makes software less safe, not more. Example: In hapijs/hapi#3422 data that is not URL encoded is passed to an application that should accept only url-encoded data (
application/x-www-form-urlencoded), leading to a failing test. Quote myself:I suggest to not catch the exception thrown in
querystring.unescape()so that Hapi can return a400 Bad Request.