Redact request url

[tgwizard]

Goal

Currently, when the Bugsnag client extracts the URL from the request, it includes the full query parameters, without any filtering/redaction. This will leak any sensitive keys sent as query parameters, like access tokens.

This PR changes that so that we go through each query parameter and filter out sensitive ones (similar to how we do for request headers).

This PR also adds access_token as a default sensitive parameter, since it's very unlikely users want to include those in Bugsnag reports.

Changeset

Changed

extractRequestInfoFromReq

Tests

I've added tests for the function.

Discussion

Alternative Approaches

Outstanding Questions

One drawback of this change is that the URL query parameters sent to Bugsnag will no longer be in the order that the client sent them, but sorted alphabetically by key. This is because url.ParseQuery returns a (type-aliased) map[string][]string, and the corresponding Encode sorts on key.

We could attempt a custom implementation of the Encode function to sort according to the original order, but I'm not sure it's worth it.

Linked issues

Review

For the submitter, initial self-review:

• Commented on code changes inline explain the reasoning behind the approach
• Reviewed the test cases added for completeness and possible points for discussion
• A changelog entry was added for the goal of this pull request
• Check the scope of the changeset - is everything in the diff required for the pull request?
• This pull request is ready for:
  • Initial review of the intended approach, not yet feature complete
  • Structural review of the classes, functions, and properties modified
  • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

• Consistency across platforms for structures or concepts added or modified
• Consistency between the changeset and the goal stated above
• Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
• Usage friction - is the proposed change in usage cumbersome or complicated?
• Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
• Concurrency concerns - if components are accessed asynchronously, what issues will arise
• Thoroughness of added tests and any missing edge cases
• Idiomatic use of the language

[tgwizard]

Please review :)

[kinbiko]

Hi @tgwizard,

Thanks for submitting the PR. I've discussed with the team and this seems like something we should support. The code looks mostly good, my comments are mainly stylistic. I think we can live with the URL looking different in the dashboard, but it would be good to limit this discrepancy as much as possible. See the inline comments for a suggested approach that hopefully won't require us to rewrite Encode.

[kinbiko]

Once the sanitizeURL func is extracted, we can return here, and avoid a level of indentation when doing the parameter filtering below (no need for the else).

[tgwizard]

Thanks for the review @kinbiko! I've pushed a commit addressing most of your comments, please have another look. I'd be happy to squash the commits at merge time.

[tgwizard]

@kinbiko please review again :)

[kinbiko]

Hi @tgwizard,

Thank you for your patience. I think this is pretty close to being done, the main remaining point is on achieving the consistency of [FILTERED].
Your efforts are much appreciated, and we'll try and get this in a new release shortly.

[tgwizard]

I've addressed all but one of your comments, which I commented on why. Please have another look @kinbiko.

[tgwizard]

The tests fail for some seemingly unrelated reason - a timeout in the Martini tests.

[tgwizard]

@kinbiko please have another look, it should now all be fixed.

[kinbiko]

Hi @tgwizard,

Sorry about the wait -- I was struggling to find the time to come back and re-review this. I've taken this branch for a spin now, and it looks really good; only one minor issue.

Unfortunately, not filtering empty URL params leads to a pretty confusing Request tab; The URL displays an empty value but the params extracted from the built-in Bugsnag HTTP middleware show up as "[FILTERED]".

I've had a chat with another of the maintainers here and we believe it's best to be consistent and use "[FILTERED]" even for empty fields.

That being said, we do see the benefit of being able to tell if such fields are absent. If this is something you need for your particular use case you may use an OnBeforeNotify callback to extract this information, so all is not lost 🙂.

Other than this, I believe this change is good to go. Thank you ever so much for your patience. I realise I've been making you jump through a lot of hoops.

You are free to address this if you so wish, otherwise I'll merge and address myself as soon as I find the time.

[tgwizard]

No worries @kinbiko, I'm happy to make this right. The code is much better now than in my initial PR, which also makes me happy 😸 .

I've added a commit to also add [FILTERED] for empty values for sensitive query string parameters.

[tgwizard]

@kinbiko bump 😺

[snmaynard]

@tgwizard I believe @kinbiko is fixing up our CI issues on older Go versions and then the plan is for this to be merged. Should be done in the next day or so I think (but correct me if I'm wrong @kinbiko!)

[kinbiko]

Just as @snmaynard mentioned, we were having some CI issues that I needed to fix before this could go in. This is why your latest build failed (nothing to do with your changes), but the PR looks good now so it's going in our next release (most likely shipping Monday). 🚢

Thanks again for your work on this @tgwizard!