From 32a2463070781f6987f1cd7248b10fb0a21768d5 Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Fri, 5 Jun 2026 13:02:38 -0500 Subject: [PATCH 01/13] fix: tolerate /userinfo responses without an email claim InitiateBrowserBasedLogin previously treated a missing "email" field in the /userinfo response as fatal, aborting the entire OAuth flow even though the token exchange had already succeeded. In practice Notehub's /userinfo can return HTTP 200 containing only the OIDC bookkeeping claims (sub, iss, aud, iat, ...), which made every browser-based sign-in fail with "could not retrieve email" -- and because the handler does not check the HTTP status code, the actual response was hidden from the user. Fall back to the subject identifier when email is absent so the caller still receives a populated AccessToken. The Email field is used only as a display/persistence identifier, so the fallback is sufficient for downstream consumers. Co-Authored-By: Claude Opus 4.7 --- notehub/auth.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/notehub/auth.go b/notehub/auth.go index 18dea81..220c6fe 100644 --- a/notehub/auth.go +++ b/notehub/auth.go @@ -253,10 +253,15 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { return } - email, ok := userinfoData["email"].(string) - if !ok { - errHandler("could not retrieve email") - return + // /userinfo may omit "email" depending on IdP configuration; fall + // back to the subject ID so we still have a stable user identifier. + email, _ := userinfoData["email"].(string) + if email == "" { + if sub, _ := userinfoData["sub"].(string); sub != "" { + email = sub + } else { + email = "(oauth)" + } } /////////////////////////////////////////// From e532f8b6e38764da757e010f2453fd5bd93fc4dd Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Fri, 5 Jun 2026 13:14:09 -0500 Subject: [PATCH 02/13] fix: check /userinfo status and require sub when email is absent Addresses PR review feedback: - Check the /userinfo HTTP status code and fail with the body included if it isn't 200. Previously the handler went straight to JSON-decoding, so any non-200 response surfaced as a generic "could not retrieve email" with no detail. - Remove the "(oauth)" placeholder fallback. OIDC requires sub to be present in a userinfo response, so treating its absence as an error surfaces server misbehavior instead of masking it with a non-stable identifier. Co-Authored-By: Claude Opus 4.7 --- notehub/auth.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/notehub/auth.go b/notehub/auth.go index 220c6fe..a2002d0 100644 --- a/notehub/auth.go +++ b/notehub/auth.go @@ -247,6 +247,11 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { return } + if userinfoResp.StatusCode != http.StatusOK { + errHandler(fmt.Sprintf("/userinfo returned HTTP %d: %s", userinfoResp.StatusCode, string(userinfoBody))) + return + } + var userinfoData map[string]interface{} if err := json.Unmarshal(userinfoBody, &userinfoData); err != nil { errHandler("could not unmarshal body from /userinfo: " + err.Error()) @@ -254,14 +259,16 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { } // /userinfo may omit "email" depending on IdP configuration; fall - // back to the subject ID so we still have a stable user identifier. + // back to the subject identifier, which OIDC requires the userinfo + // response to include. email, _ := userinfoData["email"].(string) if email == "" { - if sub, _ := userinfoData["sub"].(string); sub != "" { - email = sub - } else { - email = "(oauth)" + sub, _ := userinfoData["sub"].(string) + if sub == "" { + errHandler("/userinfo response missing both email and sub") + return } + email = sub } /////////////////////////////////////////// From 80bf85736d7ca27ab843f8d59c0457fe93046d8d Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Fri, 5 Jun 2026 13:21:59 -0500 Subject: [PATCH 03/13] fix: keep /userinfo response body out of browser-facing error page Addresses PR review feedback: errHandler writes its message into the localhost callback HTTP response without setting Content-Type, so a browser renders it as HTML. The previous change embedded the raw /userinfo body into that message, which could in principle execute injected markup from a hostile or misconfigured response. Split the audiences at the status-check callsite: the browser-facing response contains only the status code, while the returned Go error and the local log still include the full body for debugging. Co-Authored-By: Claude Opus 4.7 --- notehub/auth.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/notehub/auth.go b/notehub/auth.go index a2002d0..b5447bf 100644 --- a/notehub/auth.go +++ b/notehub/auth.go @@ -248,7 +248,17 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { } if userinfoResp.StatusCode != http.StatusOK { - errHandler(fmt.Sprintf("/userinfo returned HTTP %d: %s", userinfoResp.StatusCode, string(userinfoBody))) + // Keep the raw response body out of the browser-facing message + // (the localhost callback page renders as HTML by default and + // could otherwise execute injected markup from a hostile or + // misconfigured /userinfo response). The body is still + // captured in the returned Go error and the local log so the + // caller has the detail needed to diagnose the failure. + detail := fmt.Sprintf("/userinfo returned HTTP %d: %s", userinfoResp.StatusCode, string(userinfoBody)) + accessTokenErr = errors.New(detail) + fmt.Printf("error: %s\n", detail) + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprintf(w, "error: /userinfo returned HTTP %d", userinfoResp.StatusCode) return } From 76b6b126f205c469eadce22f4b454cd217de4aad Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Fri, 5 Jun 2026 13:37:50 -0500 Subject: [PATCH 04/13] fix: quote /userinfo body in diagnostic to neutralize terminal escapes Addresses PR review feedback: the response body is untrusted input and was being interpolated into the returned error and local log with %s, so an IdP that returned bytes like \x1b[ could have its escape sequences interpreted by the user's terminal when the diagnostic string was printed. Use %q so control characters are rendered as their printable escape forms instead. Co-Authored-By: Claude Opus 4.7 --- notehub/auth.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notehub/auth.go b/notehub/auth.go index b5447bf..cce612a 100644 --- a/notehub/auth.go +++ b/notehub/auth.go @@ -254,7 +254,7 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { // misconfigured /userinfo response). The body is still // captured in the returned Go error and the local log so the // caller has the detail needed to diagnose the failure. - detail := fmt.Sprintf("/userinfo returned HTTP %d: %s", userinfoResp.StatusCode, string(userinfoBody)) + detail := fmt.Sprintf("/userinfo returned HTTP %d: %q", userinfoResp.StatusCode, userinfoBody) accessTokenErr = errors.New(detail) fmt.Printf("error: %s\n", detail) w.WriteHeader(http.StatusInternalServerError) From 150adaa894fb90612cb100dd4fc5703f86c16145 Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Fri, 5 Jun 2026 13:47:01 -0500 Subject: [PATCH 05/13] fix: harden errHandler against HTML rendering and surface /oauth2/token status Addresses PR review feedback: errHandler writes its message into the localhost callback response with no Content-Type set, so a browser will render it as HTML by default. The previous round fixed only the /userinfo status-error callsite, but other callers (OAuth error_description from /oauth2/token, JSON unmarshal errors, ...) still pass server- controlled strings through the same Fprintf. Set Content-Type to text/plain at the errHandler level so the response is treated as text regardless of which callsite produced the message, eliminating the XSS class without touching individual sites. Separately, the /oauth2/token exchange did not check StatusCode -- a non-200 response with a non-JSON body fell into the generic "could not unmarshal" branch, hiding the underlying failure exactly the way the original /userinfo regression did. Surface the HTTP status and quoted body when unmarshal fails on a non-200 response. Co-Authored-By: Claude Opus 4.7 --- notehub/auth.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/notehub/auth.go b/notehub/auth.go index cce612a..3bd555c 100644 --- a/notehub/auth.go +++ b/notehub/auth.go @@ -150,6 +150,10 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { callbackState := r.URL.Query().Get("state") errHandler := func(msg string) { + // text/plain so any server-controlled bytes that reach this + // callback (OAuth error_description, JSON unmarshal errors, + // etc.) cannot be rendered as HTML by the browser. + w.Header().Set("Content-Type", "text/plain; charset=utf-8") w.WriteHeader(http.StatusInternalServerError) fmt.Fprintf(w, "error: %s", msg) fmt.Printf("error: %s\n", msg) @@ -194,7 +198,14 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { var tokenData map[string]interface{} if err := json.Unmarshal(body, &tokenData); err != nil { - errHandler("could not unmarshal body from /oauth2/token: " + err.Error()) + // Surface the HTTP status when /oauth2/token returns a non-200 + // with a non-JSON body, so the underlying failure isn't hidden + // behind a generic unmarshal error. + if tokenResp.StatusCode != http.StatusOK { + errHandler(fmt.Sprintf("/oauth2/token returned HTTP %d: %q", tokenResp.StatusCode, body)) + } else { + errHandler("could not unmarshal body from /oauth2/token: " + err.Error()) + } return } From 50bf9245d060874537253db5416cda46d238d6d6 Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Fri, 5 Jun 2026 15:20:07 -0500 Subject: [PATCH 06/13] refactor: centralize safe error handling in browser-based login Replaces the per-callsite error handling in InitiateBrowserBasedLogin with a small set of invariants, addressing the class of issues raised in review (XSS via the localhost callback, terminal escape-sequence injection, unbounded reads / log flooding, and missing status checks) rather than patching each path individually: - Single fail(summary, detail) sink: the browser callback receives only `summary` -- a constant, developer-authored string, never server-controlled data -- always as text/plain. No callsite writes to the ResponseWriter directly, so injected markup can never be reflected, regardless of which path fails. - safeDetail() truncates (to 1 KiB) and strconv.Quote()s every untrusted value before it reaches the log or returned error, so control characters can't be interpreted by a terminal and a hostile body can't flood output. - readResponseBody() bounds every response read to 1 MiB via io.LimitReader, preventing memory amplification at the source. - Both /oauth2/token and /userinfo are status-checked. Co-Authored-By: Claude Opus 4.7 --- notehub/auth.go | 98 +++++++++++++++++++++++++++++++------------------ 1 file changed, 63 insertions(+), 35 deletions(-) diff --git a/notehub/auth.go b/notehub/auth.go index 3bd555c..4caca2b 100644 --- a/notehub/auth.go +++ b/notehub/auth.go @@ -17,10 +17,40 @@ import ( "os/exec" "os/signal" "runtime" + "strconv" "strings" "time" ) +const ( + // maxResponseBytes bounds how much of an OAuth/OIDC HTTP response body we + // read, so a hostile or misconfigured endpoint cannot exhaust memory. It + // is far larger than any legitimate token or userinfo response. + maxResponseBytes = 1 << 20 // 1 MiB + + // maxDetailBytes bounds how much of an (untrusted) value is embedded into + // a log line or returned error. + maxDetailBytes = 1024 +) + +// readResponseBody reads up to maxResponseBytes from an HTTP response body. +func readResponseBody(resp *http.Response) ([]byte, error) { + return io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes)) +} + +// safeDetail renders an untrusted value for inclusion in logs and returned +// errors. It truncates to maxDetailBytes and quotes the result so control +// characters (such as terminal escape sequences) are shown as printable +// escapes rather than interpreted by a terminal. +func safeDetail(s string) string { + suffix := "" + if len(s) > maxDetailBytes { + s = s[:maxDetailBytes] + suffix = " (truncated)" + } + return strconv.Quote(s) + suffix +} + type AccessToken struct { Host string Email string @@ -149,19 +179,28 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { authorizationCode := r.URL.Query().Get("code") callbackState := r.URL.Query().Get("state") - errHandler := func(msg string) { - // text/plain so any server-controlled bytes that reach this - // callback (OAuth error_description, JSON unmarshal errors, - // etc.) cannot be rendered as HTML by the browser. + // fail records an authentication failure exactly one way for every + // error path in this handler. The browser callback only ever receives + // `summary` -- a constant, developer-authored string, never server- + // controlled data -- and always as text/plain, so the callback cannot + // be used to reflect injected markup or scripts. `detail` (already + // passed through safeDetail by the caller) carries diagnostics to the + // local log and the returned Go error only. + fail := func(summary, detail string) { w.Header().Set("Content-Type", "text/plain; charset=utf-8") w.WriteHeader(http.StatusInternalServerError) - fmt.Fprintf(w, "error: %s", msg) + fmt.Fprintf(w, "error: %s", summary) + + msg := summary + if detail != "" { + msg = summary + ": " + detail + } fmt.Printf("error: %s\n", msg) accessTokenErr = errors.New(msg) } if callbackState != state { - errHandler("state mismatch") + fail("state mismatch", "") return } @@ -185,42 +224,41 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { }.Encode()), ) if err != nil { - errHandler("error on /oauth2/token: " + err.Error()) + fail("could not reach /oauth2/token", safeDetail(err.Error())) return } defer tokenResp.Body.Close() - body, err := io.ReadAll(tokenResp.Body) + body, err := readResponseBody(tokenResp) if err != nil { - errHandler("could not read body from /oauth2/token: " + err.Error()) + fail("could not read /oauth2/token response", safeDetail(err.Error())) return } var tokenData map[string]interface{} if err := json.Unmarshal(body, &tokenData); err != nil { - // Surface the HTTP status when /oauth2/token returns a non-200 - // with a non-JSON body, so the underlying failure isn't hidden - // behind a generic unmarshal error. + // A non-200 with a non-JSON body would otherwise be hidden behind + // a generic unmarshal error, so surface the HTTP status too. if tokenResp.StatusCode != http.StatusOK { - errHandler(fmt.Sprintf("/oauth2/token returned HTTP %d: %q", tokenResp.StatusCode, body)) + fail(fmt.Sprintf("/oauth2/token returned HTTP %d", tokenResp.StatusCode), safeDetail(string(body))) } else { - errHandler("could not unmarshal body from /oauth2/token: " + err.Error()) + fail("could not parse /oauth2/token response", safeDetail(string(body))) } return } if errCode, ok := tokenData["error"].(string); ok { + detail := errCode if errDescription, ok2 := tokenData["error_description"].(string); ok2 { - errHandler(fmt.Sprintf("%s: %s", errCode, errDescription)) - } else { - errHandler(errCode) + detail = errCode + ": " + errDescription } + fail("/oauth2/token returned an error", safeDetail(detail)) return } accessTokenString, ok := tokenData["access_token"].(string) if !ok { - errHandler("unexpected error: no access token returned") + fail("no access token in /oauth2/token response", "") return } @@ -241,41 +279,31 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("https://%s/userinfo", notehubApiHost), nil) if err != nil { - errHandler("could not create request for /userinfo: " + err.Error()) + fail("could not create /userinfo request", safeDetail(err.Error())) return } req.Header.Set("Authorization", "Bearer "+accessTokenString) userinfoResp, err := http.DefaultClient.Do(req) if err != nil { - errHandler("could not get userinfo: " + err.Error()) + fail("could not reach /userinfo", safeDetail(err.Error())) return } defer userinfoResp.Body.Close() - userinfoBody, err := io.ReadAll(userinfoResp.Body) + userinfoBody, err := readResponseBody(userinfoResp) if err != nil { - errHandler("could not read body from /userinfo: " + err.Error()) + fail("could not read /userinfo response", safeDetail(err.Error())) return } if userinfoResp.StatusCode != http.StatusOK { - // Keep the raw response body out of the browser-facing message - // (the localhost callback page renders as HTML by default and - // could otherwise execute injected markup from a hostile or - // misconfigured /userinfo response). The body is still - // captured in the returned Go error and the local log so the - // caller has the detail needed to diagnose the failure. - detail := fmt.Sprintf("/userinfo returned HTTP %d: %q", userinfoResp.StatusCode, userinfoBody) - accessTokenErr = errors.New(detail) - fmt.Printf("error: %s\n", detail) - w.WriteHeader(http.StatusInternalServerError) - fmt.Fprintf(w, "error: /userinfo returned HTTP %d", userinfoResp.StatusCode) + fail(fmt.Sprintf("/userinfo returned HTTP %d", userinfoResp.StatusCode), safeDetail(string(userinfoBody))) return } var userinfoData map[string]interface{} if err := json.Unmarshal(userinfoBody, &userinfoData); err != nil { - errHandler("could not unmarshal body from /userinfo: " + err.Error()) + fail("could not parse /userinfo response", safeDetail(string(userinfoBody))) return } @@ -286,7 +314,7 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { if email == "" { sub, _ := userinfoData["sub"].(string) if sub == "" { - errHandler("/userinfo response missing both email and sub") + fail("/userinfo response missing both email and sub", "") return } email = sub From ba03a945359bad91b8e06a96fb6e4e8ccbbbae0d Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Fri, 5 Jun 2026 15:35:11 -0500 Subject: [PATCH 07/13] fix: treat any non-200 /oauth2/token response as a hard failure Addresses PR review feedback. The status check was only reached when JSON unmarshalling failed, so a non-200 response with a valid JSON body that lacked both "error" and "access_token" would slip past it and fall through to the generic "no access token" error, hiding the real HTTP status -- and a non-200 response that did contain a token would have been consumed. Check the status immediately after reading the body, before consuming any fields, mirroring the /userinfo path. Both endpoints now share the same read -> status-check -> parse -> consume structure. Co-Authored-By: Claude Opus 4.7 --- notehub/auth.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/notehub/auth.go b/notehub/auth.go index 4caca2b..e6b7f13 100644 --- a/notehub/auth.go +++ b/notehub/auth.go @@ -235,15 +235,18 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { return } + // Treat any non-200 as a hard failure before consuming any fields, so + // we never accept an access token from -- or hide the status of -- an + // unsuccessful response, regardless of whether its body happens to + // parse as JSON. + if tokenResp.StatusCode != http.StatusOK { + fail(fmt.Sprintf("/oauth2/token returned HTTP %d", tokenResp.StatusCode), safeDetail(string(body))) + return + } + var tokenData map[string]interface{} if err := json.Unmarshal(body, &tokenData); err != nil { - // A non-200 with a non-JSON body would otherwise be hidden behind - // a generic unmarshal error, so surface the HTTP status too. - if tokenResp.StatusCode != http.StatusOK { - fail(fmt.Sprintf("/oauth2/token returned HTTP %d", tokenResp.StatusCode), safeDetail(string(body))) - } else { - fail("could not parse /oauth2/token response", safeDetail(string(body))) - } + fail("could not parse /oauth2/token response", safeDetail(string(body))) return } From d2d38bc7c01c07905ffe1d4b9e1d709757076ca1 Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Fri, 5 Jun 2026 15:52:37 -0500 Subject: [PATCH 08/13] docs+diag: clarify fail summary comment and include parse errors Addresses PR review feedback: - Correct the fail() doc comment: `summary` is not purely a constant string -- some call sites include the numeric HTTP status code -- so describe it as developer-authored text plus non-injectable scalars rather than implying it never derives from the response at all. - On /oauth2/token and /userinfo JSON parse failures, include the unmarshal error alongside the response body so malformed or empty responses are easier to diagnose. The combined detail still goes through safeDetail, since Go's json errors can echo input bytes. Co-Authored-By: Claude Opus 4.7 --- notehub/auth.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/notehub/auth.go b/notehub/auth.go index e6b7f13..ae9f289 100644 --- a/notehub/auth.go +++ b/notehub/auth.go @@ -181,11 +181,13 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { // fail records an authentication failure exactly one way for every // error path in this handler. The browser callback only ever receives - // `summary` -- a constant, developer-authored string, never server- - // controlled data -- and always as text/plain, so the callback cannot - // be used to reflect injected markup or scripts. `detail` (already - // passed through safeDetail by the caller) carries diagnostics to the - // local log and the returned Go error only. + // `summary`, which is composed solely of developer-authored text and + // non-injectable scalars such as the numeric HTTP status code -- never + // free-form server-controlled bytes -- and is always written as + // text/plain, so the callback cannot be used to reflect injected + // markup or scripts. `detail` (already passed through safeDetail by + // the caller) carries diagnostics to the local log and the returned + // Go error only. fail := func(summary, detail string) { w.Header().Set("Content-Type", "text/plain; charset=utf-8") w.WriteHeader(http.StatusInternalServerError) @@ -246,7 +248,7 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { var tokenData map[string]interface{} if err := json.Unmarshal(body, &tokenData); err != nil { - fail("could not parse /oauth2/token response", safeDetail(string(body))) + fail("could not parse /oauth2/token response", safeDetail(err.Error()+": "+string(body))) return } @@ -306,7 +308,7 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { var userinfoData map[string]interface{} if err := json.Unmarshal(userinfoBody, &userinfoData); err != nil { - fail("could not parse /userinfo response", safeDetail(string(userinfoBody))) + fail("could not parse /userinfo response", safeDetail(err.Error()+": "+string(userinfoBody))) return } From ff7897df270d9b750eccef9e2f2aedeacee75fa2 Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Fri, 5 Jun 2026 16:05:48 -0500 Subject: [PATCH 09/13] fix: shut down on error paths and redact tokens from diagnostics Addresses PR review feedback. Deadlock: fail() set accessTokenErr but never signaled the shutdown goroutine, so InitiateBrowserBasedLogin blocked forever on <-done for any error path -- the server only stopped when quit received a signal, which happened only on the success path. fail() now does a non-blocking send to quit so an unsuccessful callback shuts the server down and the function returns its error. (This is the hang that previously required a manual ^C on any sign-in failure.) Credential leak: the /oauth2/token error paths logged/returned the raw response body, which for a token endpoint can contain access_token / refresh_token / id_token if an IdP ever returns them on an error status. redactSensitive() masks known credential fields in a JSON body before it reaches the log or returned error. Applied to the /userinfo paths too for uniformity (a no-op there, since userinfo carries no tokens). Co-Authored-By: Claude Opus 4.7 --- notehub/auth.go | 47 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/notehub/auth.go b/notehub/auth.go index ae9f289..b3ee8d1 100644 --- a/notehub/auth.go +++ b/notehub/auth.go @@ -51,6 +51,36 @@ func safeDetail(s string) string { return strconv.Quote(s) + suffix } +// sensitiveResponseFields are response fields that may carry credentials and +// must never be written to logs or returned errors. +var sensitiveResponseFields = []string{"access_token", "refresh_token", "id_token"} + +// redactSensitive returns a representation of an HTTP response body that is +// safe to log: when the body is a JSON object, values of known credential- +// bearing fields are replaced with a placeholder; otherwise the body is +// returned unchanged (token endpoints return credentials as JSON, so a +// non-JSON body has no field to target). +func redactSensitive(body []byte) string { + var obj map[string]interface{} + if err := json.Unmarshal(body, &obj); err != nil { + return string(body) + } + redacted := false + for _, field := range sensitiveResponseFields { + if _, ok := obj[field]; ok { + obj[field] = "[REDACTED]" + redacted = true + } + } + if !redacted { + return string(body) + } + if out, err := json.Marshal(obj); err == nil { + return string(out) + } + return string(body) +} + type AccessToken struct { Host string Email string @@ -199,6 +229,15 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { } fmt.Printf("error: %s\n", msg) accessTokenErr = errors.New(msg) + + // Signal the server to shut down so InitiateBrowserBasedLogin does + // not block on <-done waiting for a success that will never come. + // Non-blocking: the buffered channel may already hold a signal + // (e.g. an OS interrupt or a prior callback). + select { + case quit <- os.Interrupt: + default: + } } if callbackState != state { @@ -242,13 +281,13 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { // unsuccessful response, regardless of whether its body happens to // parse as JSON. if tokenResp.StatusCode != http.StatusOK { - fail(fmt.Sprintf("/oauth2/token returned HTTP %d", tokenResp.StatusCode), safeDetail(string(body))) + fail(fmt.Sprintf("/oauth2/token returned HTTP %d", tokenResp.StatusCode), safeDetail(redactSensitive(body))) return } var tokenData map[string]interface{} if err := json.Unmarshal(body, &tokenData); err != nil { - fail("could not parse /oauth2/token response", safeDetail(err.Error()+": "+string(body))) + fail("could not parse /oauth2/token response", safeDetail(err.Error()+": "+redactSensitive(body))) return } @@ -302,13 +341,13 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { } if userinfoResp.StatusCode != http.StatusOK { - fail(fmt.Sprintf("/userinfo returned HTTP %d", userinfoResp.StatusCode), safeDetail(string(userinfoBody))) + fail(fmt.Sprintf("/userinfo returned HTTP %d", userinfoResp.StatusCode), safeDetail(redactSensitive(userinfoBody))) return } var userinfoData map[string]interface{} if err := json.Unmarshal(userinfoBody, &userinfoData); err != nil { - fail("could not parse /userinfo response", safeDetail(err.Error()+": "+string(userinfoBody))) + fail("could not parse /userinfo response", safeDetail(err.Error()+": "+redactSensitive(userinfoBody))) return } From 44e8f8d246d088446c0d854fc71a35538c028c9f Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Fri, 5 Jun 2026 17:17:41 -0500 Subject: [PATCH 10/13] fix: process exactly one OAuth callback and ignore stray requests A review of the callback handler surfaced three robustness bugs beyond the deadlock fixed earlier: 1. The handler is registered on "/", so it matched any request -- favicon, prefetch, a bare visit to the root. Such a request carries no valid state, hit the "state mismatch" path, and (now that error paths trigger shutdown) tore the flow down before the real redirect arrived. classifyCallback() now ignores any request without an authorization code, answering it with 204 and leaving the flow running. 2. Concurrent or duplicate callbacks each wrote the shared result and signaled shutdown. A sync.Once now admits exactly one callback; duplicates get a benign response. 3. An OS interrupt during sign-in shut the server down with neither result set, so InitiateBrowserBasedLogin returned (nil, nil) and the caller dereferenced a nil token. It now returns an error when no callback completed. Co-Authored-By: Claude Opus 4.7 --- notehub/auth.go | 66 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/notehub/auth.go b/notehub/auth.go index b3ee8d1..e193154 100644 --- a/notehub/auth.go +++ b/notehub/auth.go @@ -19,6 +19,7 @@ import ( "runtime" "strconv" "strings" + "sync" "time" ) @@ -81,6 +82,35 @@ func redactSensitive(body []byte) string { return string(body) } +// callbackAction classifies an inbound request to the local OAuth callback +// server. +type callbackAction int + +const ( + // callbackIgnore: not the OAuth redirect (e.g. favicon, prefetch, a bare + // visit to the root). Must be answered benignly without affecting the flow. + callbackIgnore callbackAction = iota + // callbackStateMismatch: carries an authorization code but the wrong state + // (a CSRF attempt or a stale redirect). Must fail closed. + callbackStateMismatch + // callbackProceed: a well-formed redirect whose state matches. + callbackProceed +) + +// classifyCallback decides how to treat a request to the callback server. A +// request without an authorization code is not the OAuth redirect at all and +// is ignored; one with a code is honored only if its state matches the value +// the flow generated. +func classifyCallback(r *http.Request, expectedState string) callbackAction { + if r.URL.Query().Get("code") == "" { + return callbackIgnore + } + if r.URL.Query().Get("state") != expectedState { + return callbackStateMismatch + } + return callbackProceed +} + type AccessToken struct { Host string Email string @@ -198,6 +228,11 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { signal.Notify(quit, os.Interrupt) defer signal.Reset(os.Interrupt) + // Ensures exactly one OAuth callback is processed; spurious or duplicate + // requests to the callback server are answered benignly without touching + // the shared result or triggering shutdown. + var once sync.Once + router := http.NewServeMux() // We'll fill this after we pick a port but declare it now so the handler can close over it. @@ -206,8 +241,28 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { // The browser will be redirected to this endpoint with an authorization code // and then this endpoint will exchange that authorization code for an access token router.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + action := classifyCallback(r, state) + + // A request that isn't the OAuth redirect (favicon, prefetch, a bare + // visit to the root) must not be mistaken for a failed sign-in nor tear + // the flow down before the real redirect arrives. + if action == callbackIgnore { + w.WriteHeader(http.StatusNoContent) + return + } + + // Handle exactly one callback; answer any duplicate benignly so a + // second request cannot race the shared result or signal shutdown twice. + handled := false + once.Do(func() { handled = true }) + if !handled { + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, "authentication already completed; you may close this window") + return + } + authorizationCode := r.URL.Query().Get("code") - callbackState := r.URL.Query().Get("state") // fail records an authentication failure exactly one way for every // error path in this handler. The browser callback only ever receives @@ -240,7 +295,7 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { } } - if callbackState != state { + if action == callbackStateMismatch { fail("state mismatch", "") return } @@ -441,5 +496,12 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { // Wait for exchange to finish <-done + + // A shutdown with neither result set means the flow was interrupted (e.g. + // an OS signal) before any callback completed. Return an error rather than + // a nil token and nil error, which a caller would dereference. + if accessToken == nil && accessTokenErr == nil { + accessTokenErr = errors.New("authentication canceled before completion") + } return accessToken, accessTokenErr } From 45afd13efd4d70601ac029c811a60973ea600152 Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Fri, 5 Jun 2026 17:19:07 -0500 Subject: [PATCH 11/13] fix: don't leak credentials or silently truncate oversized responses Addresses PR review feedback on the diagnostic helpers: - redactSensitive: when the body cannot be parsed as JSON it can no longer be redacted field-by-field, yet a truncated/malformed token response may still contain a secret. Suppress the whole body if it mentions any credential-bearing field name rather than returning it raw, honoring the "must never be written" guarantee. - readResponseBody: read one byte past the cap and return an explicit "response exceeds limit" error instead of silently truncating, which otherwise surfaces later as a misleading parse failure and hides the real cause (too large vs. malformed). Co-Authored-By: Claude Opus 4.7 --- notehub/auth.go | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/notehub/auth.go b/notehub/auth.go index e193154..7db2cea 100644 --- a/notehub/auth.go +++ b/notehub/auth.go @@ -1,6 +1,7 @@ package notehub import ( + "bytes" "context" "crypto/sha256" "encoding/base64" @@ -34,9 +35,20 @@ const ( maxDetailBytes = 1024 ) -// readResponseBody reads up to maxResponseBytes from an HTTP response body. +// readResponseBody reads an HTTP response body, capping it at maxResponseBytes +// so a hostile or misconfigured endpoint cannot exhaust memory. Reading one +// byte past the cap lets it distinguish "too large" from "exactly the limit" +// and report an explicit error rather than silently truncating (which would +// later surface as a misleading parse failure). func readResponseBody(resp *http.Response) ([]byte, error) { - return io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes)) + body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes+1)) + if err != nil { + return nil, err + } + if len(body) > maxResponseBytes { + return nil, fmt.Errorf("response exceeds %d-byte limit", maxResponseBytes) + } + return body, nil } // safeDetail renders an untrusted value for inclusion in logs and returned @@ -64,6 +76,15 @@ var sensitiveResponseFields = []string{"access_token", "refresh_token", "id_toke func redactSensitive(body []byte) string { var obj map[string]interface{} if err := json.Unmarshal(body, &obj); err != nil { + // The body did not parse, so it cannot be redacted field-by-field. If + // it nonetheless mentions a credential-bearing field (e.g. a truncated + // token response), suppress it entirely rather than risk leaking a + // secret; otherwise return it unchanged. + for _, field := range sensitiveResponseFields { + if bytes.Contains(body, []byte(field)) { + return "[unparseable response suppressed: may contain credentials]" + } + } return string(body) } redacted := false From 4fd2647d8286722144583e2153cfac83518f6ae4 Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Fri, 5 Jun 2026 17:47:38 -0500 Subject: [PATCH 12/13] fix: report OAuth error redirects instead of hanging classifyCallback treated any request without an authorization code as "not the callback" and ignored it (204, no shutdown). But per RFC 6749 an authorization failure -- most commonly the user denying consent -- redirects to the callback with an "error" parameter and no code. That redirect was therefore ignored, the awaited code never arrived, and InitiateBrowserBasedLogin blocked until the user interrupted it, without ever reporting why. Classify a request as the callback when it carries either a code or an error, validate state in both cases, and surface the provider's error (and error_description) via fail() so the flow ends promptly with a meaningful message. Co-Authored-By: Claude Opus 4.7 --- notehub/auth.go | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/notehub/auth.go b/notehub/auth.go index 7db2cea..884ca54 100644 --- a/notehub/auth.go +++ b/notehub/auth.go @@ -111,24 +111,36 @@ const ( // callbackIgnore: not the OAuth redirect (e.g. favicon, prefetch, a bare // visit to the root). Must be answered benignly without affecting the flow. callbackIgnore callbackAction = iota - // callbackStateMismatch: carries an authorization code but the wrong state - // (a CSRF attempt or a stale redirect). Must fail closed. + // callbackStateMismatch: a redirect whose state doesn't match the value the + // flow generated (a CSRF attempt or a stale redirect). Must fail closed. callbackStateMismatch - // callbackProceed: a well-formed redirect whose state matches. + // callbackError: the provider reported an authorization failure (e.g. the + // user denied consent), redirecting with an "error" parameter and no code. + callbackError + // callbackProceed: a well-formed redirect carrying a code whose state + // matches. callbackProceed ) // classifyCallback decides how to treat a request to the callback server. A -// request without an authorization code is not the OAuth redirect at all and -// is ignored; one with a code is honored only if its state matches the value -// the flow generated. +// request carrying neither an authorization code nor an OAuth error is not the +// redirect at all and is ignored. Anything that is the redirect is honored only +// if its state matches the value the flow generated; an "error" parameter (with +// no code) signals that authorization was refused or failed. func classifyCallback(r *http.Request, expectedState string) callbackAction { - if r.URL.Query().Get("code") == "" { + q := r.URL.Query() + code := q.Get("code") + oauthErr := q.Get("error") + + if code == "" && oauthErr == "" { return callbackIgnore } - if r.URL.Query().Get("state") != expectedState { + if q.Get("state") != expectedState { return callbackStateMismatch } + if oauthErr != "" { + return callbackError + } return callbackProceed } @@ -321,6 +333,19 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { return } + // The provider refused or failed the authorization (e.g. the user + // denied consent). Report it instead of waiting for a code that will + // never arrive. + if action == callbackError { + oauthErr := r.URL.Query().Get("error") + detail := oauthErr + if desc := r.URL.Query().Get("error_description"); desc != "" { + detail = oauthErr + ": " + desc + } + fail("authorization was not granted", safeDetail(detail)) + return + } + /////////////////////////////////////////// // Exchange code for access token /////////////////////////////////////////// From b2bede4d3229de69fe59ac50cc3d9011768ddcef Mon Sep 17 00:00:00 2001 From: "Zachary J. Fields" Date: Fri, 5 Jun 2026 18:55:12 -0500 Subject: [PATCH 13/13] fix: don't let a state-mismatched request cancel an in-progress login A callback whose state didn't match was routed through fail(), which consumed the single-callback slot and signaled shutdown. Because the callback ports are a predictable, hard-coded list, any local process -- or a webpage issuing requests to http://localhost:/?... -- could reliably abort a legitimate sign-in (or burn the sync.Once so the real redirect got the "already completed" response). That's a local denial of service on the login flow. Handle a state mismatch before the sync.Once gate and respond benignly (400, no error recorded, no shutdown), so only a redirect whose state matches the value this attempt generated can consume the slot or end the flow. Co-Authored-By: Claude Opus 4.7 --- notehub/auth.go | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/notehub/auth.go b/notehub/auth.go index 884ca54..9e97363 100644 --- a/notehub/auth.go +++ b/notehub/auth.go @@ -276,16 +276,26 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { router.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { action := classifyCallback(r, state) - // A request that isn't the OAuth redirect (favicon, prefetch, a bare - // visit to the root) must not be mistaken for a failed sign-in nor tear - // the flow down before the real redirect arrives. - if action == callbackIgnore { + switch action { + case callbackIgnore: + // Not the OAuth redirect (favicon, prefetch, a bare visit to the + // root); ignore it without affecting the flow. w.WriteHeader(http.StatusNoContent) return + case callbackStateMismatch: + // A request whose state doesn't match this attempt is unrelated to + // it -- a stray local request, another browser tab, or (since the + // callback ports are a predictable, hard-coded list) a webpage + // probing localhost. Reject it benignly: it must neither abort the + // in-progress login nor consume the single-callback slot below, so + // it cannot be used to deny service to a legitimate sign-in. + w.WriteHeader(http.StatusBadRequest) + return } - // Handle exactly one callback; answer any duplicate benignly so a - // second request cannot race the shared result or signal shutdown twice. + // Only a redirect whose state matches reaches here. Handle exactly one; + // answer any duplicate benignly so a second request cannot race the + // shared result or signal shutdown twice. handled := false once.Do(func() { handled = true }) if !handled { @@ -328,11 +338,6 @@ func InitiateBrowserBasedLogin(notehubApiHost string) (*AccessToken, error) { } } - if action == callbackStateMismatch { - fail("state mismatch", "") - return - } - // The provider refused or failed the authorization (e.g. the user // denied consent). Report it instead of waiting for a code that will // never arrive.