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

withinString may throw an exception on malformed urls #359

Open
xPaw opened this issue Oct 5, 2017 · 4 comments
Open

withinString may throw an exception on malformed urls #359

xPaw opened this issue Oct 5, 2017 · 4 comments

Comments

@xPaw
Copy link

xPaw commented Oct 5, 2017

Basically this function is not safe enough to be called on user input as it may crash with "URI malformed" which is thrown by nodejs' implementation of decodeURIComponent

/usr/local/lib/node_modules/thelounge/node_modules/urijs/src/URI.js:595
      parts.password = t[0] ? URI.decode(t.join(':')) : null;
                                  ^

URIError: URI malformed
    at Function.decodeURIComponent [as decode] (native)
    at Function.URI.parseUserinfo (/usr/local/lib/node_modules/thelounge/node_modules/urijs/src/URI.js:595:35)
    at Function.URI.parseAuthority (/usr/local/lib/node_modules/thelounge/node_modules/urijs/src/URI.js:581:18)
    at Function.URI.parse (/usr/local/lib/node_modules/thelounge/node_modules/urijs/src/URI.js:516:24)
    at URI.p.href (/usr/local/lib/node_modules/thelounge/node_modules/urijs/src/URI.js:1174:25)
    at new URI (/usr/local/lib/node_modules/thelounge/node_modules/urijs/src/URI.js:70:10)
    at URI (/usr/local/lib/node_modules/thelounge/node_modules/urijs/src/URI.js:46:16)
    at /usr/local/lib/node_modules/thelounge/client/js/libs/handlebars/ircmessageparser/findLinks.js:27:24
    at Function.URI.withinString (/usr/local/lib/node_modules/thelounge/node_modules/urijs/src/URI.js:1003:20)
    at findLinks (/usr/local/lib/node_modules/thelounge/client/js/libs/handlebars/ircmessageparser/findLinks.js:25:6)

Right now the only suitable solution I see is wrapping it in a try/catch, which somewhat defeats the purpose of this function.

A link that throws: http://a:%p@c

@xPaw
Copy link
Author

xPaw commented Oct 6, 2017

Okay my bad on this one, it throws the exception within URI constructor, not withinString, so it's the same issue as #352.

@rodneyrehm
Copy link
Member

rodneyrehm commented Oct 7, 2017

It's not the same as #352 as that was caused by an unfortunate change in URI.js v1.18.11 (and reverted in v1.19.0).

What is the exact input you're dealing with here?

You'll probably want to make sure that the new URI() inside the withingString callback doesn't break for all the URLs in the string:

URI.withinString(text, function(uri) {
  try {
    var u = new URI(uri);
    // …
  } catch (error) {
    // ignore the uri
    return undefined;
  }
});

@luisnaranjo733
Copy link

@rodneyrehm FYI the workaround described above works but the type for withinString does not permit an undefined return value

@rodneyrehm
Copy link
Member

but the type for withinString does not permit an undefined return value

@luisnaranjo733 what type? if you're referring to TypeScript you're looking for https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/urijs as this project is currently not providing type definitions itself.

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

No branches or pull requests

3 participants