Filter query-string secrets out of logged URLs

Modify workhorse so that private-token and authenticity-token query-string parameters are not logged. Instead, they will be displayed as authenticity_token=[FILTERED]. The remainder of the query string will be displayed unaltered

Every URL logged should be passed through ScrubURLParams. I looked into having a wrapper around the logWriter in gitlab-workhorse/logging.go, but this would slow down logging significantly and wouldn't be guaranteed to work anyway (since the message may be split into two Write() calls across the query-string boundary).

Related to #71 (closed)

@jacobvosmaer-gitlab what do you think of this approach?

I'm a little unsure about testing - it's important that we not have any regressions, but unit testing is not a great way to ensure this.

Ideally, we'd be able to collect all the log output of a test run and check for a known private-token in the combined output. This would help catch new sites outputting the private token, as well as regressions, but it's not something the test suite is designed for.

I looked at having internal/helpers/logging_test.go, at least, but the implementation of loggingResponseWriter depends on a global logger instance.

/cc @stanhu

marked as a Work In Progress

changed the description

changed title from WIP: Don't log query strings - they may contain secrets to WIP: Don't log query strings

assigned to @nick.thomas

Taking @briann's comments on #71 (closed) into account

@nick.thomas I think firing requests with query strings we want to be filtered and scanning log output is a good idea.

If we do this in package main, maybe we can use startLogging from https://gitlab.com/gitlab-org/gitlab-workhorse/blob/master/logging.go to temporarily direct log output to a file and then examine the contents of that file.

unmarked as a Work In Progress

changed title from WIP: Don't log query strings to Filter query-string secrets out of logged URLs

changed the description

@jacobvosmaer-gitlab OK, I came up with a couple of tests, and I've also changed this so it just filters the known-sensitive parameters in the query string, rather than removing the query string entirely, as requested by @briann

assigned to @jacobvosmaer-gitlab

I can work similar regexp magic on the default omnibus nginx configuration

Hmm, what about rss_token ?

@nick.thomas Has that been released yet? It would be a good idea to filter it as well.

@briann it's hit master. I'll add it to this MR.

OK, RSS token added. Just waiting on review.

approved this merge request

Looks great.

/assign @nick.thomas

assigned to @nick.thomas

merged

mentioned in issue #71 (closed)