From 43bfd6e888a67e40d6c5ffcb3abbb26865b2cc19 Mon Sep 17 00:00:00 2001 From: Jonathon Leight Date: Sun, 5 Jul 2026 18:31:35 -0400 Subject: [PATCH] Fix logging gap --- internal/auth/handlers.go | 12 +++-- internal/core/auth_error_leak_test.go | 76 +++++++++++++++++++++++++++ internal/core/confirm.go | 8 ++- internal/core/console.go | 4 ++ 4 files changed, 95 insertions(+), 5 deletions(-) create mode 100644 internal/core/auth_error_leak_test.go diff --git a/internal/auth/handlers.go b/internal/auth/handlers.go index 865fe12..7865707 100644 --- a/internal/auth/handlers.go +++ b/internal/auth/handlers.go @@ -156,10 +156,13 @@ func (s *Service) RegisterFinish(w http.ResponseWriter, r *http.Request) { } } - // Verify the credential before writing anything durable. + // Verify the credential before writing anything durable. A failure here is a + // client/ceremony error (400), but the underlying go-webauthn detail can leak + // internals, so log it server-side and return only a generic message. cred, err := s.wa.FinishRegistration(waUser, *sessionData, r) if err != nil { - httpError(w, r, http.StatusBadRequest, "registration failed: "+err.Error(), nil) + web.LogError(r, "webauthn: finish registration", err) + httpError(w, r, http.StatusBadRequest, "registration failed", nil) return } blob, err := json.Marshal(cred) @@ -254,9 +257,12 @@ func (s *Service) LoginFinish(w http.ResponseWriter, r *http.Request) { return } + // Assertion failure is a client error (401), but the go-webauthn detail can + // leak internals — log it server-side and return only a generic message. cred, err := s.wa.FinishLogin(waUser, *sessionData, r) if err != nil { - httpError(w, r, http.StatusUnauthorized, "login failed: "+err.Error(), nil) + web.LogError(r, "webauthn: finish login", err) + httpError(w, r, http.StatusUnauthorized, "login failed", nil) return } // Persist the updated sign counter / clone-warning state. diff --git a/internal/core/auth_error_leak_test.go b/internal/core/auth_error_leak_test.go new file mode 100644 index 0000000..aaca36a --- /dev/null +++ b/internal/core/auth_error_leak_test.go @@ -0,0 +1,76 @@ +package core + +import ( + "encoding/json" + "io" + "net/http" + "net/http/cookiejar" + "net/http/httptest" + "strings" + "testing" + + "github.com/go-webauthn/webauthn/webauthn" +) + +// TestLoginFinishDoesNotLeakWebAuthnError drives a passkey assertion to failure +// and asserts the client sees only a generic message — the go-webauthn internal +// detail must be logged server-side, not returned. Regression for the error-leak +// audit (auth/handlers.go LoginFinish). +func TestLoginFinishDoesNotLeakWebAuthnError(t *testing.T) { + t.Parallel() + st, ctx, ts, h := splitServer(t) + + user, err := st.CreateUser(ctx, "wauser", "") + if err != nil { + t.Fatalf("create user: %v", err) + } + // A minimal stored credential so LoginBegin has something to challenge; its key + // material is never exercised because the assertion body below fails to parse. + blob, err := json.Marshal(webauthn.Credential{ID: []byte("cred-1")}) + if err != nil { + t.Fatalf("marshal credential: %v", err) + } + if err := st.AddCredential(ctx, user.ID, []byte("cred-1"), blob, "key"); err != nil { + t.Fatalf("add credential: %v", err) + } + + // One client with a cookie jar so the ceremony stashed at /begin is present at + // /finish (both carry the same session cookie). + jar, _ := cookiejar.New(nil) + client := &http.Client{Jar: jar, CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }} + + begin := authJSON(t, client, ts, h.auth, "/api/login/begin", `{"username":"wauser"}`) + begin.Body.Close() + if begin.StatusCode != http.StatusOK { + t.Fatalf("login begin status = %d, want 200", begin.StatusCode) + } + + // A malformed assertion body makes FinishLogin fail. The response must be the + // generic message only. + finish := authJSON(t, client, ts, h.auth, "/api/login/finish", `{}`) + body, _ := io.ReadAll(finish.Body) + finish.Body.Close() + if finish.StatusCode != http.StatusUnauthorized { + t.Fatalf("login finish status = %d, want 401 (body %s)", finish.StatusCode, body) + } + got := strings.TrimSpace(string(body)) + if got != `{"error":"login failed"}` { + t.Fatalf("login finish body = %s, want generic {\"error\":\"login failed\"} with no leaked detail", got) + } +} + +// authJSON POSTs a JSON body to the given host/path using the provided client. +func authJSON(t *testing.T, client *http.Client, ts *httptest.Server, host, path, jsonBody string) *http.Response { + t.Helper() + req, err := http.NewRequest(http.MethodPost, ts.URL+path, strings.NewReader(jsonBody)) + if err != nil { + t.Fatalf("request: %v", err) + } + req.Host = host + req.Header.Set("Content-Type", "application/json") + resp, err := client.Do(req) + if err != nil { + t.Fatalf("post %s%s: %v", host, path, err) + } + return resp +} diff --git a/internal/core/confirm.go b/internal/core/confirm.go index 4488c95..c012bee 100644 --- a/internal/core/confirm.go +++ b/internal/core/confirm.go @@ -134,6 +134,9 @@ func (s *Handlers) wsConfirm(w http.ResponseWriter, r *http.Request) { } if err := modem.Connect(ctx); err != nil { + // The user's own local modem — the detail helps them troubleshoot, so keep it + // in the status frame; also log it so operators see connection failures. + web.LogError(r, "confirm: modem connect", err, "repeater_id", id) _ = bridge.Status("error", "modem connect: "+err.Error()) return } @@ -180,6 +183,7 @@ func (s *Handlers) wsConfirm(w http.ResponseWriter, r *http.Request) { SF: uint8(rep.RadioSF), //nolint:gosec // G115: radio config value is bounded (preset-constrained) CR: uint8(rep.RadioCR), //nolint:gosec // G115: radio config value is bounded (preset-constrained) }); err != nil { + web.LogError(r, "confirm: set radio", err, "repeater_id", id) _ = bridge.Status("error", "set radio: "+err.Error()) return } @@ -205,7 +209,7 @@ func (s *Handlers) wsConfirm(w http.ResponseWriter, r *http.Request) { if err := s.Store.SetRepeaterConfirmed(ctx, id, uid, lr.IsAdmin, int16(lr.Permissions)); err != nil { web.LogError(r, "confirm: save confirmation", err, "repeater_id", id) - _ = bridge.Status("error", "could not save confirmation: "+err.Error()) + _ = bridge.Status("error", "Could not save the confirmation — please try again.") return } if debug { @@ -268,7 +272,7 @@ func (s *Handlers) fetchAndStoreLocation(ctx context.Context, r *http.Request, e } if err := s.Store.SetRepeaterLocation(ctx, id, lat, lon); err != nil { web.LogError(r, "confirm: store location", err, "repeater_id", id) - _ = bridge.Status("error", "could not store location: "+err.Error()) + _ = bridge.Status("error", "Could not store the location — please try again.") return 0, 0, false } _ = bridge.Status("info", fmt.Sprintf("Stored location: %.5f, %.5f", lat, lon)) diff --git a/internal/core/console.go b/internal/core/console.go index 0e7adfe..0b453b1 100644 --- a/internal/core/console.go +++ b/internal/core/console.go @@ -226,6 +226,9 @@ func (s *Handlers) wsConsole(w http.ResponseWriter, r *http.Request) { }() if err := modem.Connect(ctx); err != nil { + // The user's own local modem — keep the detail for troubleshooting, and log + // it so operators see connection failures. + web.LogError(r, "console: modem connect", err, "repeater_id", id) _ = bridge.Status("error", "modem connect: "+err.Error()) return } @@ -289,6 +292,7 @@ func (s *Handlers) wsConsole(w http.ResponseWriter, r *http.Request) { SF: uint8(rep.RadioSF), //nolint:gosec // G115: radio config value is bounded (preset-constrained) CR: uint8(rep.RadioCR), //nolint:gosec // G115: radio config value is bounded (preset-constrained) }); err != nil { + web.LogError(r, "console: set radio", err, "repeater_id", id) _ = bridge.Status("error", "set radio: "+err.Error()) return }