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

Remove encodeurl as a dep #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

blakeembrey
Copy link
Member

@blakeembrey blakeembrey commented Sep 10, 2024

Closes #60 (comment). 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.

@blakeembrey
Copy link
Member Author

blakeembrey commented Sep 10, 2024

It looks like I'm basically reverting 3fe2b5e so I'll try and figure out why this was added to begin with.

Edit: I can't find a good reason.

@wesleytodd
Copy link
Member

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.

@dougwilson
Copy link
Contributor

dougwilson commented Sep 10, 2024

It was a fix I made for a security report.

@wesleytodd
Copy link
Member

wesleytodd commented Sep 10, 2024

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?

@dougwilson
Copy link
Contributor

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.

@blakeembrey
Copy link
Member Author

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?

@blakeembrey
Copy link
Member Author

Thanks @dougwilson for dealing with the reports all these years! It's definitely not fun, all good if you can't find it specifically.

@dougwilson
Copy link
Contributor

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.

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

Successfully merging this pull request may close these issues.

Update encodeurl to v2
3 participants