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

[mgol]

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.

[fnagel]

+1 by reading

[timmywil]

Doesn't fix it. The issue is using a user-provided value in the argument to readfile without limiting the reads to a certain location.

[mgol]

@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]

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]

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]

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.

[timmywil]

Interesting. How could it know it's not rate limited when that's often handled by a front door? Seems strange it's an error and not a warning. Anyway, thanks for humoring me. My OCD was kicking in and I didn't want us to address an error and then have to dismiss it anyway.

Whether we call this a "real" vulnerability I think is still debatable since this isn't production code, but I was certainly not opposed to addressing it.