Update WebHookLog URL sanitisation to support masked segments
Summary
As described in #382357 (closed), testing of web hooks with masked components in the URL triggers an exception (sentry example).
This is due to the SafeUrl
module assuming that:
- The including module has a
url
attribute; - and that attribute contains a valid, parseable URL.
This assumption fails when the URL on the WebHookLog
record contains masked segments, e.g.
http://{secretsubdomain}.example.com/webhook
Steps to reproduce
- Open a project.
- Go to Settings -> Webhooks and create a hook.
- Enter
http://example.com/hook
in the URL field. - Enable masking, with
example.com
->domain
. - Enable the hook for issue events. I don't think the specific choice matters.
- Save the hook, click 'Test' and select 'Issues events'
What is the current bug behavior?
The application returns a 500 error, with the exception Invalid character in host: '{domain}'
What is the expected correct behavior?
The application should correctly test the request with the interpolated URL, but create a WebHookLog
record with the masked URL.
Relevant logs and/or screenshots
Possible fixes
After chatting to @bmarjanovic (thanks, Bojan!) about this issue, we're looking at:
- Refactoring
WebHookLog
to useUrlSanitizer
instead ofSafeUrl
. The class and module have overlapping behaviour, but the former seems better supported within the codebase. - Add behaviour to
UrlSanitizer
to support filtering of URLs with masked components ({}
).
Edited by James Nutt