Skip to content

Conversation

@dneustadt
Copy link

@dneustadt dneustadt commented May 9, 2021

Hi!

I watch your YouTube channel and really enjoy your content. I think these projects for enabling older machines to use the modern web are great so I thought I should have a quick look to see if I can contribute some advice.

I don't know if you're familiar with the concept of XSS or cross-site scripting attacks. In short they allow strangers to use your website as a disguise to run scams, phish for password etc.

As it stands, user inputs aren't safely escaped, which means it is possible to use special characters, like double-quotes, to inject markup or even JavaScript code into the site.

Fore example searching for "><script>alert('hi i\'m friendly neighbourhood XSS attack :)');</script><img src=" will actually run that JavaScript code. Where it really becomes a critical issue is that you can take that search query link and send it to someone: http://frogfind.com/?q=%22%3E%3Cscript%3Ealert%28%27hi+i%5C%27m+your+friendly+neighbourhood+XSS+attack+%3A%29%27%29%3B%3C%2Fscript%3E%3Cimg+src%3D%22
Most users won't study a link and just open it and that's where I use your trustworthy site to run my code on the end users machine. That's when I could ask for a password etc...

The changes I committed will prevent these kind of XSS attacks. While you did use strip_tags for the text output you didn't escape the query value within the input value, which as you can see above is very easy to break out of. Aside from being more secure htmlspecialchars will also allow users to search for HTML tags for example <title> and it will now show what you searched as it doesn't strip tags but instead converts the < and > to safe html entities which won't be interpreted as html.

Hope this helps. Keep up the great work :)

Copy link

@GoombaProgrammer GoombaProgrammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be added

@jibsaram
Copy link
Contributor

After #21 got merged there's a few conflicts now, mostly because the $loc value has been removed as it was never really used in FrogFind (presumably just a left-over from 86k.news, which might need the same fixes). If you're willing to update the PR, just pulling in main and resolving those bits that (used to) refer to the loc value should do the trick!

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.

3 participants