fix(auth): redact sensitive auth logging#293
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses sensitive-data leakage risks in auth-related logging by removing profile debug logs from OAuth/Apple flows and redacting direct-login passwords from request URLs passed into rest.SendErrorJSON (while preserving GET compatibility). It also adds regression tests for both the v1 (root) and v2 modules and documents CI/dev workflows in a new AGENTS.md.
Changes:
- Redact
passwdin direct-login request URLs used for error logging (v1 and v2). - Remove debug logs that print raw OAuth/Apple user profile data (v1 and v2).
- Add regression tests ensuring user profiles and passwords are not logged; add
AGENTS.mdcontributor guidance.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
provider/direct.go |
Uses a scrubbed request copy for rest.SendErrorJSON to avoid logging passwd from query strings. |
v2/provider/direct.go |
Same redaction behavior as v1 for the v2 module. |
provider/direct_test.go |
Adds a regression test verifying query password redaction and no mutation of the original request. |
v2/provider/direct_test.go |
Same regression coverage for v2. |
provider/oauth1.go |
Removes debug logging of raw/mapped user info to prevent sensitive profile leakage. |
v2/provider/oauth1.go |
Same logging removal for v2. |
provider/oauth2.go |
Removes debug logging of raw/mapped user info to prevent sensitive profile leakage. |
v2/provider/oauth2.go |
Same logging removal for v2. |
provider/apple.go |
Removes user-info debug log and redacts parse error log to avoid logging user contents. |
v2/provider/apple.go |
Same logging adjustment for v2. |
provider/oauth1_test.go |
Adds regression test to ensure OAuth1 login doesn’t log sensitive profile fields. |
v2/provider/oauth1_test.go |
Same regression coverage for v2. |
provider/oauth2_test.go |
Adds regression test to ensure OAuth2 login doesn’t log sensitive profile fields. |
v2/provider/oauth2_test.go |
Same regression coverage for v2. |
AGENTS.md |
Adds project guidance for running v1/v2 tests and lint consistent with CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+322
to
+340
| logBuf := strings.Builder{} | ||
| captureLog := func(p *Params) { | ||
| p.L = logger.Func(func(format string, args ...any) { | ||
| fmt.Fprintf(&logBuf, format, args...) | ||
| }) | ||
| } | ||
| teardown := prepOauth2Test(t, 8991, 8992, nil, captureLog) | ||
| defer teardown() | ||
|
|
||
| jar, err := cookiejar.New(nil) | ||
| require.NoError(t, err) | ||
| client := &http.Client{Jar: jar, Timeout: 5 * time.Second} | ||
|
|
||
| resp, err := client.Get("http://localhost:8991/login?site=remark") | ||
| require.NoError(t, err) | ||
| defer resp.Body.Close() | ||
| assert.Equal(t, http.StatusOK, resp.StatusCode) | ||
|
|
||
| logged := logBuf.String() |
Comment on lines
+331
to
+349
| logBuf := strings.Builder{} | ||
| captureLog := func(p *Params) { | ||
| p.L = logger.Func(func(format string, args ...any) { | ||
| fmt.Fprintf(&logBuf, format, args...) | ||
| }) | ||
| } | ||
| teardown := prepOauth2Test(t, 8991, 8992, nil, captureLog) | ||
| defer teardown() | ||
|
|
||
| jar, err := cookiejar.New(nil) | ||
| require.NoError(t, err) | ||
| client := &http.Client{Jar: jar, Timeout: 5 * time.Second} | ||
|
|
||
| resp, err := client.Get("http://localhost:8991/login?site=remark") | ||
| require.NoError(t, err) | ||
| defer resp.Body.Close() | ||
| assert.Equal(t, http.StatusOK, resp.StatusCode) | ||
|
|
||
| logged := logBuf.String() |
Comment on lines
+221
to
+239
| logBuf := strings.Builder{} | ||
| captureLog := func(p *Params) { | ||
| p.L = logger.Func(func(format string, args ...any) { | ||
| fmt.Fprintf(&logBuf, format, args...) | ||
| }) | ||
| } | ||
| teardown := prepOauth1Test(t, 8993, 8994, captureLog) | ||
| defer teardown() | ||
|
|
||
| jar, err := cookiejar.New(nil) | ||
| require.NoError(t, err) | ||
| client := &http.Client{Jar: jar, Timeout: timeout * time.Second} | ||
|
|
||
| resp, err := client.Get("http://localhost:8993/login?site=remark") | ||
| require.NoError(t, err) | ||
| defer resp.Body.Close() | ||
| assert.Equal(t, http.StatusOK, resp.StatusCode) | ||
|
|
||
| logged := logBuf.String() |
Comment on lines
+217
to
+235
| logBuf := strings.Builder{} | ||
| captureLog := func(p *Params) { | ||
| p.L = logger.Func(func(format string, args ...any) { | ||
| fmt.Fprintf(&logBuf, format, args...) | ||
| }) | ||
| } | ||
| teardown := prepOauth1Test(t, 8993, 8994, captureLog) | ||
| defer teardown() | ||
|
|
||
| jar, err := cookiejar.New(nil) | ||
| require.NoError(t, err) | ||
| client := &http.Client{Jar: jar, Timeout: timeout * time.Second} | ||
|
|
||
| resp, err := client.Get("http://localhost:8993/login?site=remark") | ||
| require.NoError(t, err) | ||
| defer resp.Body.Close() | ||
| assert.Equal(t, http.StatusOK, resp.StatusCode) | ||
|
|
||
| logged := logBuf.String() |
Coverage Report for CI Build 26410456224Coverage increased (+0.05%) to 85.475%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Title: fix(auth): redact sensitive auth logging
Fixes two auth logging leaks found during security review.
Changes
passwdfrom direct-login request URLs beforerest.SendErrorJSONlogs them, while keeping GET compatibilityAGENTS.mdguidance for CI lint reproduction and v1/v2 workflowVerification
go test -p 1 ./...cd v2 && go test -p 1 ./...go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.6.2 run --max-issues-per-linter=0 --max-same-issues=0cd v2 && go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.6.2 run --config ../.golangci.yml --max-issues-per-linter=0 --max-same-issues=0