jquery / jquery-ui

The official jQuery user interface library.
https://jqueryui.com
Other
11.26k stars 5.32k forks source link

Build: Fix an XSS in the test server HTML serving logic #2309

Closed mgol closed 1 month ago

mgol commented 1 month ago

The test server has a rule for /tests/unit/*/*.html paths that serves a proper local file. However, the parameters after /unit/ were so far not escaped, leading to possibly reading a file from outside of the Git repository. Fix that by replacing non-alphanumeric characters that are also not - or _.

This should resolve one CodeQL alert.

mgol commented 1 month ago

@timmywil this limits it to the tests/unit/ directory, how does it not fix it? The mitigation proposed by the check description doesn’t guarantee anything else.

timmywil commented 1 month ago

I meant that it doesn't fix the codeql check, which is all we really care about here as there isn't a real vulnerability since the test server is not hosted. Why not refactor this in a way that passes codeql so we don't have to dismiss the alert?

mgol commented 1 month ago

I meant that it doesn't fix the codeql check, which is all we really care about here as there isn't a real vulnerability since the test server is not hosted.

No, I care about it precisely because it might be a real vulnerability. I've dismissed a number of purely test/demo reports that we don't need to care about. But here we have a dev server running on a local network; any device on that network can access its resources this way.

TBH, I don't know how to exploit it considering that the matcher already excludes / - and this is perhaps where I should make the regex also exclude ., etc.

But OK, I can try to do it the way CodeQL is happy.

mgol commented 1 month ago

Updating the regex in the matcher seems to make CodeQL happy. It now complains about a lack of rate limiter for file system access, but caring about potential DoS on a local test server would probably be too much, so I'd leave it as-is.