[24.1] Apply statsd arg sanitization to all pages

[mvdbeek]

The previous sanitization would only run on the or portion.

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[dannon]

Curious what's the motivation here? It was intentional to only sanitize the flexible part of the key there as the controller action key is something we compose ourselves and is always safe keys like web.login.index or whatever. Unless over the years that key got opened up?

[mvdbeek]

https://sentry.galaxyproject.org/share/issue/e2c535d2d4904d438a0a1524c991bba4/:

UnicodeEncodeError: 'ascii' codec can't encode characters in position 14-19: ordinal not in range(128)
  File "galaxy/web/framework/middleware/error.py", line 167, in __call__
    app_iter = self.application(environ, sr_checker)
  File "galaxy/web/framework/middleware/statsd.py", line 39, in __call__
    self.galaxy_stasd_client.timing(page, dt)
  File "galaxy/web/statsd_client.py", line 35, in timing
    self.statsd_client.timing(infix + path, time)
  File "statsd/client/base.py", line 33, in timing
    self._send_stat(stat, '%0.6f|ms' % delta, rate)
  File "statsd/client/base.py", line 61, in _send_stat
    self._after(self._prepare(stat, value, rate))
  File "statsd/client/base.py", line 76, in _after
    self._send(data)
  File "statsd/client/udp.py", line 42, in _send
    self._sock.sendto(data.encode('ascii'), self._addr)

It seems like action contains the query fragment. Given statsd is the only consumer I don't think there's much harm here ?

[mvdbeek]

Or, if you like I can do just the ascii encode/decode dance on controller_action_key, but page.strip("/").replace("/", ".") shouldn't have any further effect ?

[dannon]

@mvdbeek What about this?

diff --git a/lib/galaxy/web/framework/base.py b/lib/galaxy/web/framework/base.py
index ec45368623..79a36c5772 100644
--- a/lib/galaxy/web/framework/base.py
+++ b/lib/galaxy/web/framework/base.py
@@ -251,8 +251,13 @@ class WebApplication:
                 raise
         trans.controller = controller_name
         trans.action = action
+
+        # Action can still refer to invalid and/or inaccurate paths here, so we use the actual
+        # controller and method names to set the timing key.
+
+        action_tag = getattr(method, "__name__", "default")
         environ["controller_action_key"] = (
-            f"{'api' if environ['is_api_request'] else 'web'}.{controller_name}.{action or 'default'}"
+            f"{'api' if environ['is_api_request'] else 'web'}.{controller_name}.{action_tag}"
         )
         # Combine mapper args and query string / form args and call
         kwargs = trans.request.params.mixed()

It was never intended that action in controller_action_key accept free-form inputs and that's the bug to resolve, I think.

[mvdbeek]

Looks, good, let's do that.

[mvdbeek]

Been running this on main and working fine, let's merge ?