DOMjudge / domjudge

DOMjudge programming contest jury system
https://www.domjudge.org
GNU General Public License v2.0
709 stars 248 forks source link

VarargsLogMessageProcessor breaks if you have a string which looks like a sprintf string but isn't #2395

Closed nickygerritsen closed 3 months ago

nickygerritsen commented 4 months ago

For example, when you visit a non existing page with a referrer like https://www.domjudge.org/demoweb/favicon.ico%3Fv=8.3.0DEV/846393821 (which happened), then VarargsLogMessageProcessor thinks the %3F is a sprintf string, but it isn't, and we get an error (as reported in Sentry)

eldering commented 4 months ago

It already has protection against this: in https://github.com/DOMjudge/domjudge/blob/main/webapp/src/Logger/VarargsLogMessageProcessor.php#L20 if no context is present (aka no extra arguments given), then it just prints the message unformatted.

If you pass extra arguments to the logger, then it's your responsibility that the primary format string does not contain any unescaped % characters.

nickygerritsen commented 4 months ago

Well, the problem is that Symfony internally passes context to an error, which we do not control. We also do not control the message.

nickygerritsen commented 4 months ago

Specifically this error: https://github.com/symfony/symfony/blob/7.1/src/Symfony/Component/HttpKernel/EventListener/RouterListener.php#L120 which then sets the context here: https://github.com/symfony/symfony/blob/7.1/src/Symfony/Component/HttpKernel/EventListener/ErrorListener.php#L171

when the referrer contains a %3F

nickygerritsen commented 4 months ago

So maybe the fix is to check if the context is a numerically indexed array and only then do our logic? That would fix this.