search

getgrav / grav-plugin-admin

Grav Admin Plugin
http://getgrav.org
MIT License
355 stars 227 forks source link

Inappropriate NOTICE: Grav found potential XSS issues in content: 'invalid_protocols' #2220

Closed jaderoca closed 2 years ago

jaderoca commented 2 years ago

Grav v1.7.26.1 - Admin v1.10.26.1 localhost on Pop!OS 21.10 64-bit (Ubuntu derivative)

To reproduce: In page content editor for new page, type "javascript: test" (case insensitive, but the colon is required as is the trailing word), then save Result: NOTICE: Grav found potential XSS issues in content: 'invalid_protocols'

Test for impact: In page content editor for new page type "javascript: alert(5);" then save. Result: when opened in the browser, that page displays the text as typed, not an alert box containing the numeral 5. Presumably this is because this usage is incorrect should one actually want the alert box. Impact: Minimal. I know there is no XSS issue because I'm not using "JavaScript:" in a form that can be recognized and processed as code. On the other hand, spurious warnings make extra work and detract from the use of warnings.

Comment: I don't know enough about the various protocols to effectively just go through them all for similar spurious warnings, but I suspect they're worth checking. Assuming it's worth addressing at all.

mahagr commented 2 years ago

The security checks are there to avoid million vulnerability reports about Grav, but the checks aren't perfect. We just have a quick blacklist of word combinations that can be used in attacks, but as there's no HTML or JS parsing going on, it is safer just to catch every possible attack and have false positives than to have false negatives.

jaderoca commented 2 years ago

Fair enough. I'm coming back to the web after a 2 decade absence, so I was a little spooked. Thanks!

mahagr commented 2 years ago

You can always turn off the checks you don't want to have :)

jaderoca commented 2 years ago

:) Yup. Over the last few days, I did enough digging to realize that you were "just" doing sanity checks. I'll take sanity checks over no checks any day. If I had realized that earlier, I wouldn't have raised the issue at all. Thanks!