finalhandler
finalhandler copied to clipboard
pillarjs
Remove encodeurl as a dep
Closes https://github.com/pillarjs/finalhandler/issues/60#issuecomment-2341323316. It already escapes the HTML which should be good enough for visualizing, I'm not sure there's much value in also encoding it since that makes it not match the actual path used in the URL, which may be slightly confusing.
It looks like I'm basically reverting https://github.com/pillarjs/finalhandler/commit/3fe2b5e4480b3eb67f6eef1da3797e2f8039d7e2 so I'll try and figure out why this was added to begin with.
Edit: I can't find a good reason.
Yeah I can't either. @dougwilson might remember. Either way, I think we can land this as a nice small improvement to our transitive deps without much impact.
Since it is out, I think we can discuss here publicly. Is the report that there was a way to XSS via the url? Would the encode html not catch that?
It was so long ago I don't remember the details. I tried to search my email on phone but it doesn't go back all those years. I remember the premise being seemingly a non issue but it was easy enough to just use that to remove all the special chars from the output (because HTML encode only encodes those that affect HTML) that I just did it to make the researcher happy.
Haha 😅 OK @wesleytodd, take it or leave it is fine, also just as easy to bump to v2 of encodeurl instead. Feel free to close if you don't want to open that can of worms. I can't imagine how it's exploitable without a vulnerability existing in the HTML escape.
The only thing I can imagine is someone changing req.url to something unsafe and this package rendering it?
Thanks @dougwilson for dealing with the reports all these years! It's definitely not fun, all good if you can't find it specifically.
I can't imagine how it's exploitable without a vulnerability existing in the HTML escape.
Imo it is not realistically exploitable ;) . But some reporters can be aggressive and the premise of how some attack works makes no sense. I remember this one particularly bc I did an eye roll. The exploit was doing something weird with the response, not within a web browser.
Sorry for the delay replying here, but I think we decided to bump encodeurl across the board and deal with it later right?
I don't think there's any decision here, and it's your decision as the maintainer for the package. I don't mind either way, please close the PR if you prefer to keep encoding things.
Merged the bump to 2.x. We can address the needs when we start work on express@6 which may include removing some of these packages anyway.