From 8a7e9895c81de5073f1f84dc3ff8828893425855 Mon Sep 17 00:00:00 2001 From: Dennis Juhler Aagaard Date: Thu, 24 Sep 2026 17:09:47 +0200 Subject: [PATCH] test: strengthen oauth log redaction coverage --- stelloauth/patches/stelloauth-security.patch | 235 +++++++++++++++---- 1 file changed, 185 insertions(+), 50 deletions(-) diff --git a/stelloauth/patches/stelloauth-security.patch b/stelloauth/patches/stelloauth-security.patch index 7f97723..5cd8b60 100644 --- a/stelloauth/patches/stelloauth-security.patch +++ b/stelloauth/patches/stelloauth-security.patch @@ -1,5 +1,5 @@ diff --git a/internal/app/oauth.go b/internal/app/oauth.go -index f0b9548..82f356f 100644 +index f0b9548..0c59a11 100644 --- a/internal/app/oauth.go +++ b/internal/app/oauth.go @@ -162,12 +162,11 @@ func performChromedpOAuth( @@ -7,7 +7,7 @@ index f0b9548..82f356f 100644 // Capture OAuth redirect if strings.HasPrefix(reqURL, redirectPrefix) { - log.Printf("[%s] Redirect URL: %s", requestID, reqURL) -+ log.Printf("[%s] Captured OAuth redirect", requestID) ++ logCapturedOAuthRedirect(requestID, reqURL) parsed, err := url.Parse(reqURL) if err == nil { if code := parsed.Query().Get("code"); code != "" { @@ -21,7 +21,7 @@ index f0b9548..82f356f 100644 if parsed, perr := url.Parse(reqURL); perr == nil { flowError = friendlyOPError(parsed.Query().Get("code"), parsed.Query().Get("message")) - log.Printf("[%s] Stellantis error page: %s", requestID, reqURL) -+ log.Printf("[%s] Stellantis error page received", requestID) ++ logStellantisErrorPage(requestID, reqURL) } } else if debug != nil && isRelevantURL(reqURL) { // Only show relevant OAuth flow URLs in debug output @@ -30,12 +30,65 @@ index f0b9548..82f356f 100644 var currentURL string _ = chromedp.Run(browserCtx, chromedp.Location(¤tURL)) - log.Printf("Current URL: %s", currentURL) -+ log.Printf("Current browser location checked") ++ logCurrentBrowserLocation(currentURL) if !strings.HasPrefix(currentURL, redirectPrefix) { return "" } +@@ -462,3 +461,15 @@ func codeFromLocation(browserCtx context.Context, redirectPrefix string) string + } + return parsed.Query().Get("code") + } ++ ++func logCapturedOAuthRedirect(requestID, _ string) { ++ log.Printf("[%s] Captured OAuth redirect", requestID) ++} ++ ++func logStellantisErrorPage(requestID, _ string) { ++ log.Printf("[%s] Stellantis error page received", requestID) ++} ++ ++func logCurrentBrowserLocation(_ string) { ++ log.Printf("Current browser location checked") ++} +diff --git a/internal/app/oauth_test.go b/internal/app/oauth_test.go +index 3589572..b9ad90d 100644 +--- a/internal/app/oauth_test.go ++++ b/internal/app/oauth_test.go +@@ -6,6 +6,32 @@ import ( + "testing" + ) + ++func TestOAuthEventLogsDoNotContainURLsCodesOrTokens(t *testing.T) { ++ cookie := "event-cookie" ++ accessToken := "event-access-token" ++ refreshToken := "event-refresh-token" ++ redirectURL := "mypeugeot://oauth2redirect/dk?code=event-oauth-code&access_token=" + accessToken ++ errorURL := "https://idpcvs.peugeot.com/am/oauth2/OPErrorPage.php?cookie=" + cookie ++ currentURL := redirectURL + "&refresh_token=" + refreshToken ++ oauthCode := "event-oauth-code" ++ ++ logs := captureLogs(t, func() { ++ logCapturedOAuthRedirect("redirect-request-id", redirectURL) ++ logStellantisErrorPage("error-request-id", errorURL) ++ logCurrentBrowserLocation(currentURL) ++ }) ++ want := "[redirect-request-id] Captured OAuth redirect\n" + ++ "[error-request-id] Stellantis error page received\n" + ++ "Current browser location checked\n" ++ if logs != want { ++ t.Fatalf("logs = %q, want %q", logs, want) ++ } ++ assertLogRedacted(t, logs, ++ cookie, accessToken, refreshToken, ++ redirectURL, errorURL, currentURL, oauthCode, ++ ) ++} ++ + func TestFriendlyOPError(t *testing.T) { + cases := []struct { + name string diff --git a/internal/app/server.go b/internal/app/server.go -index 48a487e..98accf9 100644 +index 48a487e..946d8cf 100644 --- a/internal/app/server.go +++ b/internal/app/server.go @@ -121,6 +121,13 @@ func getClientIP(r *http.Request) string { @@ -52,7 +105,22 @@ index 48a487e..98accf9 100644 // refundIfExpired gives back the rate-limit charge when the OAuth attempt // failed with a transient "session expired" error (bounded by the limiter). func refundIfExpired(clientIP, requestID string, err error) { -@@ -136,7 +143,7 @@ func handleOAuth(w http.ResponseWriter, r *http.Request) { +@@ -129,6 +136,14 @@ func refundIfExpired(clientIP, requestID string, err error) { + } + } + ++func logOAuthRequest(requestID, clientIP string, req OAuthRequest) { ++ log.Printf("[%s] OAuth request from %s (%s/%s)", requestID, clientIP, req.Brand, req.Country) ++} ++ ++func logOAuthFailure(requestID string, _ error) { ++ log.Printf("[%s] OAuth failed", requestID) ++} ++ + func handleOAuth(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + sendError(w, "Method not allowed", http.StatusMethodNotAllowed) +@@ -136,7 +151,7 @@ func handleOAuth(w http.ResponseWriter, r *http.Request) { } // Get client IP early for rate limiting @@ -61,48 +129,49 @@ index 48a487e..98accf9 100644 // Check rate limit if !rateLimiter.isAllowed(clientIP) { -@@ -160,7 +167,7 @@ func handleOAuth(w http.ResponseWriter, r *http.Request) { +@@ -160,7 +175,7 @@ func handleOAuth(w http.ResponseWriter, r *http.Request) { // Generate request ID requestID := uuid.New().String() - log.Printf("[%s] OAuth request from %s for user %s (%s/%s)", requestID, clientIP, req.Email, req.Brand, req.Country) -+ log.Printf("[%s] OAuth request from %s (%s/%s)", requestID, clientIP, req.Brand, req.Country) ++ logOAuthRequest(requestID, clientIP, req) // Check if client accepts SSE if r.Header.Get("Accept") == "text/event-stream" { -@@ -171,7 +178,7 @@ func handleOAuth(w http.ResponseWriter, r *http.Request) { +@@ -171,7 +186,7 @@ func handleOAuth(w http.ResponseWriter, r *http.Request) { code, err := performOAuth(req, requestID, nil, nil) if err != nil { refundIfExpired(clientIP, requestID, err) - log.Printf("[%s] OAuth failed: %s", requestID, err.Error()) -+ log.Printf("[%s] OAuth failed", requestID) ++ logOAuthFailure(requestID, err) sendError(w, err.Error(), http.StatusBadRequest) return } -@@ -206,7 +213,7 @@ func handleOAuthSSE(w http.ResponseWriter, req OAuthRequest, requestID, clientIP +@@ -206,7 +221,7 @@ func handleOAuthSSE(w http.ResponseWriter, req OAuthRequest, requestID, clientIP code, err := performOAuth(req, requestID, progress, debug) if err != nil { refundIfExpired(clientIP, requestID, err) - log.Printf("[%s] OAuth failed: %s", requestID, err.Error()) -+ log.Printf("[%s] OAuth failed", requestID) ++ logOAuthFailure(requestID, err) _, _ = fmt.Fprintf(w, "data: {\"type\":\"error\",\"message\":\"%s\"}\n\n", err.Error()) flusher.Flush() return diff --git a/internal/app/server_test.go b/internal/app/server_test.go -index b56bb27..df05e37 100644 +index b56bb27..c9ab68e 100644 --- a/internal/app/server_test.go +++ b/internal/app/server_test.go -@@ -1,7 +1,9 @@ +@@ -1,7 +1,10 @@ package app import ( + "bytes" "encoding/json" ++ "errors" + "log" "net/http" "net/http/httptest" "os" -@@ -188,3 +190,56 @@ func TestParseClientIP(t *testing.T) { +@@ -188,3 +191,74 @@ func TestParseClientIP(t *testing.T) { } } } @@ -119,48 +188,66 @@ index b56bb27..df05e37 100644 + } +} + -+func TestRequestLogsDoNotContainCredentialsOrURLs(t *testing.T) { -+ sentinels := []string{ -+ "sentinel-email@example.invalid", -+ "sentinel-password", -+ "https://idpcvs.peugeot.com/am/oauth2/authorize?sentinel=authorize-url", -+ "mypeugeot://oauth2redirect/dk?sentinel=redirect-url", -+ "sentinel-oauth-code", -+ } -+ ++func captureLogs(t *testing.T, fn func()) string { ++ t.Helper() + var logs bytes.Buffer + originalWriter := log.Writer() + originalFlags := log.Flags() + log.SetOutput(&logs) + log.SetFlags(0) -+ t.Cleanup(func() { ++ defer func() { + log.SetOutput(originalWriter) + log.SetFlags(originalFlags) -+ }) ++ }() + -+ body, err := json.Marshal(OAuthRequest{ -+ Brand: "MyPeugeot", -+ Country: "NOT-A-COUNTRY", -+ Email: strings.Join(sentinels, "|"), -+ Password: "sentinel-password", -+ }) -+ if err != nil { -+ t.Fatalf("marshal request: %v", err) -+ } -+ req := httptest.NewRequest(http.MethodPost, "/oauth", bytes.NewReader(body)) -+ req.RemoteAddr = "192.0.2.10:1234" -+ w := httptest.NewRecorder() -+ -+ handleOAuth(w, req) ++ fn() ++ return logs.String() ++} + ++func assertLogRedacted(t *testing.T, logs string, sentinels ...string) { ++ t.Helper() + for _, sentinel := range sentinels { -+ if strings.Contains(logs.String(), sentinel) { -+ t.Errorf("logs contain sensitive value %q: %s", sentinel, logs.String()) ++ if strings.Contains(logs, sentinel) { ++ t.Errorf("logs contain sensitive value %q: %s", sentinel, logs) + } + } +} ++ ++func TestOAuthRequestAndFailureLogsDoNotContainSensitiveValues(t *testing.T) { ++ email := "sentinel-email@example.invalid" ++ password := "sentinel-password" ++ cookie := "sentinel-cookie" ++ accessToken := "sentinel-access-token" ++ refreshToken := "sentinel-refresh-token" ++ authorizeURL := "https://idpcvs.peugeot.com/am/oauth2/authorize?sentinel=authorize-url" ++ redirectURL := "mypeugeot://oauth2redirect/dk?sentinel=redirect-url" ++ oauthCode := "sentinel-oauth-code" ++ req := OAuthRequest{ ++ Brand: "MyPeugeot", ++ Country: "DK", ++ Email: email, ++ Password: password, ++ } ++ failure := errors.New(strings.Join([]string{ ++ cookie, accessToken, refreshToken, authorizeURL, redirectURL, oauthCode, ++ }, "|")) ++ ++ logs := captureLogs(t, func() { ++ logOAuthRequest("request-id", "192.0.2.10", req) ++ logOAuthFailure("request-id", failure) ++ }) ++ want := "[request-id] OAuth request from 192.0.2.10 (MyPeugeot/DK)\n" + ++ "[request-id] OAuth failed\n" ++ if logs != want { ++ t.Fatalf("logs = %q, want %q", logs, want) ++ } ++ assertLogRedacted(t, logs, ++ email, password, cookie, accessToken, refreshToken, ++ authorizeURL, redirectURL, oauthCode, ++ ) ++} diff --git a/internal/app/worker.go b/internal/app/worker.go -index 00b9cf8..0b6e6fd 100644 +index 00b9cf8..3dbe54a 100644 --- a/internal/app/worker.go +++ b/internal/app/worker.go @@ -7,9 +7,20 @@ import ( @@ -224,17 +311,17 @@ index 00b9cf8..0b6e6fd 100644 requestID := uuid.New().String() - log.Printf("[%s] worker OAuth request from %s for user %s", requestID, clientIP, req.Email) -+ log.Printf("[%s] worker OAuth request from %s", requestID, clientIP) ++ logWorkerOAuthRequest(requestID, clientIP, req) code, err := performChromedpOAuth(req.URL, req.Email, req.Password, scheme, requestID, nil, nil) if err != nil { refundIfExpired(clientIP, requestID, err) - log.Printf("[%s] worker OAuth failed: %s", requestID, err.Error()) -+ log.Printf("[%s] worker OAuth failed", requestID) ++ logWorkerOAuthFailure(requestID, err) sendWorkerError(w, err.Error(), http.StatusBadRequest) return } -@@ -80,6 +102,23 @@ func handleWorker(w http.ResponseWriter, r *http.Request) { +@@ -80,6 +102,31 @@ func handleWorker(w http.ResponseWriter, r *http.Request) { _ = json.MarshalWrite(w, workerCode{Code: code}) } @@ -254,18 +341,66 @@ index 00b9cf8..0b6e6fd 100644 + } + return nil +} ++ ++func logWorkerOAuthRequest(requestID, clientIP string, _ workerRequest) { ++ log.Printf("[%s] worker OAuth request from %s", requestID, clientIP) ++} ++ ++func logWorkerOAuthFailure(requestID string, _ error) { ++ log.Printf("[%s] worker OAuth failed", requestID) ++} + // redirectScheme extracts the custom redirect scheme (e.g. "mymap") from the // authorize URL's redirect_uri query parameter; the code-capture listener keys on // "://". diff --git a/internal/app/worker_test.go b/internal/app/worker_test.go -index e64964d..7870e47 100644 +index e64964d..2a05a3b 100644 --- a/internal/app/worker_test.go +++ b/internal/app/worker_test.go -@@ -8,6 +8,41 @@ import ( +@@ -2,12 +2,81 @@ package app + + import ( + "encoding/json" ++ "errors" + "net/http" + "net/http/httptest" + "strings" "testing" ) ++func TestWorkerRequestAndFailureLogsDoNotContainSensitiveValues(t *testing.T) { ++ email := "worker-email@example.invalid" ++ password := "worker-password" ++ cookie := "worker-cookie" ++ accessToken := "worker-access-token" ++ refreshToken := "worker-refresh-token" ++ authorizeURL := "https://idpcvs.peugeot.com/am/oauth2/authorize?sentinel=worker-authorize-url" ++ redirectURL := "mypeugeot://oauth2redirect/dk?sentinel=worker-redirect-url" ++ oauthCode := "worker-oauth-code" ++ req := workerRequest{ ++ URL: authorizeURL, ++ Email: email, ++ Password: password, ++ } ++ failure := errors.New(strings.Join([]string{ ++ cookie, accessToken, refreshToken, redirectURL, oauthCode, ++ }, "|")) ++ ++ logs := captureLogs(t, func() { ++ logWorkerOAuthRequest("worker-request-id", "192.0.2.20", req) ++ logWorkerOAuthFailure("worker-request-id", failure) ++ }) ++ want := "[worker-request-id] worker OAuth request from 192.0.2.20\n" + ++ "[worker-request-id] worker OAuth failed\n" ++ if logs != want { ++ t.Fatalf("logs = %q, want %q", logs, want) ++ } ++ assertLogRedacted(t, logs, ++ email, password, cookie, accessToken, refreshToken, ++ authorizeURL, redirectURL, oauthCode, ++ ) ++} ++ +func TestValidateWorkerURL(t *testing.T) { + allowed := []string{ + "https://idpcvs.citroen.com/am/oauth2/authorize?redirect_uri=mycitroen%3A%2F%2Foauth2redirect%2Fdk", @@ -304,7 +439,7 @@ index e64964d..7870e47 100644 func TestHandleWorker_MethodNotAllowed(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "/worker", nil) w := httptest.NewRecorder() -@@ -30,6 +65,25 @@ func TestHandleWorker_InvalidBody(t *testing.T) { +@@ -30,6 +99,25 @@ func TestHandleWorker_InvalidBody(t *testing.T) { } } @@ -330,7 +465,7 @@ index e64964d..7870e47 100644 func TestHandleWorker_MissingParams(t *testing.T) { body := `{"url":"","email":"","password":""}` req := httptest.NewRequest(http.MethodPost, "/worker", strings.NewReader(body)) -@@ -83,7 +137,7 @@ func TestRedirectScheme(t *testing.T) { +@@ -83,7 +171,7 @@ func TestRedirectScheme(t *testing.T) { }, { name: "opel custom scheme",