fix(validation): Reject URLs with unexpected characters. - #2370
Conversation
ghost
assigned
| module.exports.isValidUrl = function (url, hostnameRegex) { | ||
| let parsed | ||
| try { | ||
| parsed = new URL(url) |
There was a problem hiding this comment.
I switched to the new URL class because it does the right thing with unescaped special characers:
> const { URL } = require('url')
> new URL('https://test\u010Dme.com').href
'https://xn--testme-l2a.com/'
> new URL('https://test.me/special/\u010D/chars').href
'https://test.me/special/%C4%8D/chars'
| module.exports.isValidUrl = function (redirect, hostnameRegex) { | ||
| var parsed = url.parse(redirect) | ||
| module.exports.url = function url(options) { | ||
| const validator = isA.string().max(512).uri(options) |
There was a problem hiding this comment.
I notice that in the place where we use this validator, it does max(255) itself. Is 512 here a touch on the generous side? Could we make this 255 and then eliminate the other call to max?
There was a problem hiding this comment.
I think 512 here was just copied blindly from the function above; maybe we can remove the length-limit here and let individual schemas suggest one which works better? (e.g. I assume the .max(255) comes from an actual constraint on db column size.
| return validator | ||
| } | ||
|
|
||
| module.exports.isValidUrl = function (url, hostnameRegex) { |
There was a problem hiding this comment.
There's at least 2 places in this module where this is dereferenced internally as module.exports.isValidUrl. If we named the function expression we could make those read nicer as isValidUrl or whatever.
(I realise this function already existed but as it's changed I figured it's fair game for comments)
There was a problem hiding this comment.
Actually, when I grep locally, this isn't even used outside the module. Do we need to export it?
One of the things that was a little bit confusing when I first read this diff was the dichotomy between the similarily-named url and isValidUrl exports. But if isValidUrl is just an internal helper for this module, that makes more sense to me, and I think it would be clearer if it was only callable locally.
There was a problem hiding this comment.
Good call, let's make it an internal helper.
Previously we could accept URLs with unescaped special characters such as newlines or unicode, which means we were depending on other layers of the code to handle them correctly. This change makes the requestor responsible for properly escaping any special characters in their URLs before passing them in to us.
|
Updated. |
2 participants
Previously we could accept URLs with unescaped special characters such as newlines or unicode, which means we were depending on other layers of the code to handle them correctly. This change makes the
requestor responsible for properly escaping any special characters in their URLs before passing them in to us.
@philbooth r?