Prevent 'p' parameter of OPTIONS requests being logged.

[jonseymour]

Previously, OPTIONS requests were logged without passwords being redacted.

This occurred because password redaction is performed by the authentication
handler and a) the serveOptions handler did not have the parameter
that forces the authentication handler to be put on the request path and b)
the CORS handler returns early for OPTIONS requests
before the authentication handler is invoked.

With this commit, OPTIONS requests are also subject to the authentication handler
and the responsibility for the early return on OPTIONS request is moved from
the CORS handler to the authentication handler.

Signed-off-by: Jon Seymour jon@wildducktheories.com

[jonseymour]

[X] CHANGELOG.md updated
[X] Rebased/mergable
[X] Tests pass *
[X] Sign CLA (if not already signed)

AFAICT the tests do pass. I did see what looked to be spurious failures of the circle-ci tests initiated by the PR, but these failures appeared unrelated to my changes and appear to have been fixed subsequently by mechanisms unknown.

[otoolep]

@gunnaraasen

[gunnaraasen]

The big downside of this approach is that OPTIONS requests will be authenticated. Anyone using Basic Auth will receive an "unauthorized" response unless the Access-Control-Allow-Credentials response header is set, which seems unsafe.

@jonseymour I think it would make sense to keep the OPTIONS check in the CORS handler and sanitize the p parameter in serveOptions. E.g.

func (h *Handler) serveOptions(w http.ResponseWriter, r *http.Request) {
    q := r.URL.Query()
    if p := q.Get("p"); p != "" {
        q.Set("p", "[REDACTED]")
        r.URL.RawQuery = q.Encode()
    }
    w.WriteHeader(http.StatusNoContent)
}

[jonseymour]

@gunnaraasen If I keep the OPTIONS check in the CORS handler, the serveOptions method won't get invoked (because of the early return) and hence redaction won't occur.

Would it make more sense to move the redaction logic into the logging handler so that redaction is independent of whether authentication is enabled or not and also which path the request took to get to the logging path?

[gunnaraasen]

Thanks @jonseymour. I agree, moving the redaction code to the logger is a better approach.

Can you also remove the old sanitizing code in parseCredentials now that it's redundant?

[jonseymour]

@gunnaraasen Sure. See 1d5ff55.

[gunnaraasen]

+1

@otoolep @toddboom this needs another plus one.

[toddboom]

👍