From 1cd8557b58a221b8cadb770babc6029a3f125536 Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Thu, 17 Sep 2026 23:09:02 -0400 Subject: [PATCH] fix(devices): re-approving a known device rotates its token instead of 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit App reinstalls that preserve data (Android Studio installDebug over an existing install) re-register with the same device_identifier, but the devices row from the previous install still exists — device_identifier is UNIQUE, so ApproveDevice's blind INSERT failed with a unique violation and returned 500 'failed to create device' (reproduced via curl: second approve with the same identifier = instant 500; the ~98s in the original report was app-side retry/polling, not server wait). ApproveDevice is now idempotent: look the device up by identifier first; a row owned by the approving user gets its auth token rotated via UpdateDeviceAuthToken (row id unchanged, so synced highlights/ bookmarks/progress anchored to it stay valid; fresh install = fresh credentials, old token invalidated); a row owned by another user gets 409; unknown identifiers INSERT as before, with the 23505 race falling through to the rotate path. DB failures are logged (they were silent). Also guard the in-memory pendingRegistrations map with a mutex — register/approve/reject/status/list all touch it from HTTP goroutines, and a racing write is a Go runtime fatal, not an error. The approver's credential publication and the status poller's approved-branch snapshot now run under the lock so the token can never be read half-written. Regression test: TestApproveDeviceReapprovalRotatesToken — register → approve → re-register same identifier → approve (must be 200) → token rotated, exactly one devices row, row carries the new token. --- cmd/server/tests/device_test.go | 82 ++++++++++++++++++ internal/handlers/devices.go | 142 +++++++++++++++++++++++++++----- 2 files changed, 204 insertions(+), 20 deletions(-) diff --git a/cmd/server/tests/device_test.go b/cmd/server/tests/device_test.go index 5372314..5c51b02 100644 --- a/cmd/server/tests/device_test.go +++ b/cmd/server/tests/device_test.go @@ -252,6 +252,88 @@ func TestListPendingRegistrations(t *testing.T) { assert.NotNil(t, pending, "Pending registrations should not be nil") } +// Regression test for the "approve returns 500 after app reinstall" bug: +// an app reinstall that preserves data (e.g. Android Studio installDebug +// over an existing install) re-registers with the SAME device_identifier, +// and the blind INSERT in ApproveDevice hit the UNIQUE(device_identifier) +// constraint. Re-approval must be idempotent: same row (id unchanged), +// rotated token, old token invalidated. +func TestApproveDeviceReapprovalRotatesToken(t *testing.T) { + setup := setupTestServer(t) + userID := getTestUserID(t, setup.DB) + + identifier := fmt.Sprintf("repro-reinstall-%s", uuid.New().String()) + + registerAndApprove := func() string { + regRequest := map[string]interface{}{ + "device_name": "Reinstall Device", + "device_type": "mobile", + "device_identifier": identifier, + } + regBody, _ := json.Marshal(regRequest) + + req := httptest.NewRequest("POST", "/api/devices/register", bytes.NewReader(regBody)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + setup.Server.Config.Handler.ServeHTTP(rec, req) + require.Equal(t, http.StatusCreated, rec.Code, "Should initiate registration") + + var regResponse map[string]interface{} + json.Unmarshal(rec.Body.Bytes(), ®Response) + registrationID, ok := regResponse["registration_id"].(string) + require.True(t, ok, "Should have registration_id") + + req = httptest.NewRequest("GET", fmt.Sprintf("/api/devices/approve/%s", registrationID), nil) + req.Header.Set("Authorization", "Bearer "+setup.Token) + rec = httptest.NewRecorder() + setup.Server.Config.Handler.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code, + "Approve must succeed even when the identifier already exists (was the reinstall 500)") + + // The status response is single-use and carries the credentials. + statusBody, _ := json.Marshal(map[string]string{"registration_id": registrationID}) + req = httptest.NewRequest("POST", "/api/devices/register/status", bytes.NewReader(statusBody)) + req.Header.Set("Content-Type", "application/json") + rec = httptest.NewRecorder() + setup.Server.Config.Handler.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code) + + var statusResponse map[string]interface{} + json.Unmarshal(rec.Body.Bytes(), &statusResponse) + require.Equal(t, "approved", statusResponse["status"]) + token, ok := statusResponse["auth_token"].(string) + require.True(t, ok, "Should have auth_token") + require.NotEmpty(t, token) + return token + } + + token1 := registerAndApprove() + token2 := registerAndApprove() + + assert.NotEqual(t, token1, token2, "Re-approval must rotate the auth token (fresh install = fresh credentials)") + + devices, err := setup.DB.ListDevicesByUser(context.Background(), + pgtype.UUID{Bytes: [16]byte(userID), Valid: true}) + require.NoError(t, err) + rows := 0 + for _, d := range devices { + if d.DeviceIdentifier == identifier { + rows++ + } + } + assert.Equal(t, 1, rows, "Re-approval must reuse the existing devices row, not duplicate it") + + // The rotated-in token must be the live one. + var live *database.Devices + for i := range devices { + if devices[i].DeviceIdentifier == identifier { + live = &devices[i] + } + } + require.NotNil(t, live) + assert.Equal(t, token2, live.AuthToken, "The devices row must carry the newly rotated token") +} + func TestApproveDeviceRegistration(t *testing.T) { setup := setupTestServer(t) diff --git a/internal/handlers/devices.go b/internal/handlers/devices.go index f62d45e..324eace 100644 --- a/internal/handlers/devices.go +++ b/internal/handlers/devices.go @@ -6,11 +6,16 @@ import ( "crypto/rand" "encoding/base64" "encoding/json" + "errors" "fmt" + "log/slog" "net/http" + "sync" "time" "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" "github.com/jackc/pgx/v5/pgtype" "github.com/labstack/echo/v5" "github.com/skip2/go-qrcode" @@ -106,7 +111,14 @@ type PendingRegistration struct { SyncEndpoints map[string]string } -var pendingRegistrations = make(map[string]*PendingRegistration) +// pendingRegistrations holds in-flight (unapproved) device registrations. +// HTTP handlers touch it from multiple goroutines — every access must hold +// pendingMu (Go maps are not safe for concurrent use; a racing write is a +// runtime fatal, not an error). +var ( + pendingRegistrations = make(map[string]*PendingRegistration) + pendingMu sync.Mutex +) func (h *DeviceHandler) InitiateRegistration(c *echo.Context) error { req := DeviceRegistrationRequest{} @@ -130,7 +142,9 @@ func (h *DeviceHandler) InitiateRegistration(c *echo.Context) error { CreatedAt: time.Now(), } + pendingMu.Lock() pendingRegistrations[registrationID] = registration + pendingMu.Unlock() authURL := fmt.Sprintf("%s/devices/approve/%s", h.cfg.BaseURL, registrationID) @@ -167,25 +181,32 @@ func (h *DeviceHandler) CheckRegistrationStatus(c *echo.Context) error { return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid request format"}) } + pendingMu.Lock() registration, exists := pendingRegistrations[req.RegistrationID] if !exists { + pendingMu.Unlock() return c.JSON(http.StatusNotFound, map[string]string{"error": "registration not found"}) } if time.Now().After(registration.ExpiresAt) { delete(pendingRegistrations, req.RegistrationID) + pendingMu.Unlock() return c.JSON(http.StatusGone, map[string]string{"error": "registration expired"}) } if registration.Approved { delete(pendingRegistrations, req.RegistrationID) - - return c.JSON(http.StatusOK, DeviceAuthStatusResponse{ + // Snapshot under the lock: the approver wrote these fields, and + // the row is gone from the map — no other reader/writer remains. + resp := DeviceAuthStatusResponse{ Status: "approved", AuthToken: registration.AuthToken, DeviceID: registration.DeviceID, SyncEndpoints: registration.SyncEndpoints, - }) + } + pendingMu.Unlock() + + return c.JSON(http.StatusOK, resp) } return c.JSON(http.StatusOK, DeviceAuthStatusResponse{ @@ -552,17 +573,21 @@ func (h *DeviceHandler) ApproveDevice(c *echo.Context) error { registrationID := c.Param("registration_id") + pendingMu.Lock() registration, exists := pendingRegistrations[registrationID] if !exists { + pendingMu.Unlock() return c.JSON(http.StatusNotFound, map[string]string{"error": "registration not found"}) } if time.Now().After(registration.ExpiresAt) { delete(pendingRegistrations, registrationID) + pendingMu.Unlock() return c.JSON(http.StatusGone, map[string]string{"error": "registration expired"}) } if registration.Approved { + pendingMu.Unlock() return c.JSON(http.StatusOK, map[string]interface{}{ "message": "device already approved", "device_name": registration.DeviceName, @@ -570,6 +595,7 @@ func (h *DeviceHandler) ApproveDevice(c *echo.Context) error { "approved": true, }) } + pendingMu.Unlock() authToken, err := generateDeviceToken() if err != nil { @@ -577,23 +603,87 @@ func (h *DeviceHandler) ApproveDevice(c *echo.Context) error { } pgUserID := pgtype.UUID{Bytes: [16]byte(userUUID), Valid: true} - syncEnabled := pgtype.Bool{Bool: true, Valid: true} - autoSync := pgtype.Bool{Bool: true, Valid: true} - syncFreq := pgtype.Int4{Int32: 5, Valid: true} - device, err := h.db.CreateDevice(c.Request().Context(), database.CreateDeviceParams{ - UserID: pgUserID, - DeviceName: registration.DeviceName, - DeviceType: registration.DeviceType, - DeviceIdentifier: registration.DeviceIdentifier, - AuthToken: authToken, - SyncEnabled: syncEnabled, - AutoSync: autoSync, - SyncFrequencyMinutes: syncFreq, - DeviceMetadata: []byte("{}"), - }) - if err != nil { - return c.JSON(http.StatusInternalServerError, map[string]string{"error": "failed to create device"}) + // Re-approval: app reinstalls that preserve data (e.g. Android Studio + // installDebug over an existing install) re-register with the SAME + // device_identifier, but the devices row from the previous install + // still exists — device_identifier is UNIQUE, so a blind INSERT fails + // with a unique violation (the historical "approve returns 500 after + // reinstall" bug). Look the device up first and rotate its token + // instead; a fresh install MUST get fresh credentials, so the old + // token is invalidated either way. + rotateExisting := func(existing database.Devices) (database.Devices, bool, error) { + // The identifier is globally unique; a row owned by another user + // means cross-account identifier reuse — refuse it. + if !existing.UserID.Valid || existing.UserID.Bytes != pgUserID.Bytes { + return existing, false, nil + } + updated, err := h.db.UpdateDeviceAuthToken(c.Request().Context(), + database.UpdateDeviceAuthTokenParams{ + ID: pgtype.UUID{Bytes: existing.ID.Bytes, Valid: true}, + AuthToken: authToken, + }) + return updated, true, err + } + + existing, err := h.db.GetDeviceByIdentifier(c.Request().Context(), + registration.DeviceIdentifier) + var device database.Devices + switch { + case err == nil: + // Known device: rotate the token, keep the row (id unchanged — + // highlights/bookmarks/progress anchored to it stay valid). + var ok bool + device, ok, err = rotateExisting(existing) + if err != nil { + slog.Error("device re-approval failed", + "registration_id", registrationID, "error", err) + return c.JSON(http.StatusInternalServerError, + map[string]string{"error": "failed to approve device"}) + } + if !ok { + return c.JSON(http.StatusConflict, + map[string]string{"error": "device identifier already registered to another user"}) + } + case errors.Is(err, pgx.ErrNoRows): + device, err = h.db.CreateDevice(c.Request().Context(), database.CreateDeviceParams{ + UserID: pgUserID, + DeviceName: registration.DeviceName, + DeviceType: registration.DeviceType, + DeviceIdentifier: registration.DeviceIdentifier, + AuthToken: authToken, + SyncEnabled: pgtype.Bool{Bool: true, Valid: true}, + AutoSync: pgtype.Bool{Bool: true, Valid: true}, + SyncFrequencyMinutes: pgtype.Int4{Int32: 5, Valid: true}, + DeviceMetadata: []byte("{}"), + }) + if err != nil { + // Concurrent approve racing us onto the unique index: fall + // through to the rotate path for the winner's row. + var pgErr *pgconn.PgError + if asErr := errors.As(err, &pgErr); asErr && pgErr.Code == "23505" { + if winner, lerr := h.db.GetDeviceByIdentifier( + c.Request().Context(), registration.DeviceIdentifier); lerr == nil { + var ok bool + device, ok, err = rotateExisting(winner) + if err == nil && !ok { + return c.JSON(http.StatusConflict, + map[string]string{"error": "device identifier already registered to another user"}) + } + } + } + } + if err != nil { + slog.Error("device creation failed", + "registration_id", registrationID, "error", err) + return c.JSON(http.StatusInternalServerError, + map[string]string{"error": "failed to create device"}) + } + default: + slog.Error("device lookup failed", + "registration_id", registrationID, "error", err) + return c.JSON(http.StatusInternalServerError, + map[string]string{"error": "failed to look up device"}) } syncEndpoints := map[string]string{} @@ -607,11 +697,16 @@ func (h *DeviceHandler) ApproveDevice(c *echo.Context) error { syncEndpoints["library"] = fmt.Sprintf("%s/api/sync/kobo/library", h.cfg.BaseURL) } + // Publish the credentials under the map lock: the status poller + // snapshots these fields only after Approved flips, so the token can + // never be read half-written. + pendingMu.Lock() registration.UserID = userUUID registration.Approved = true registration.AuthToken = authToken registration.DeviceID = device.ID.Bytes registration.SyncEndpoints = syncEndpoints + pendingMu.Unlock() return c.JSON(http.StatusOK, map[string]interface{}{ "message": "device approved successfully", @@ -625,12 +720,15 @@ func (h *DeviceHandler) ApproveDevice(c *echo.Context) error { func (h *DeviceHandler) RejectDevice(c *echo.Context) error { registrationID := c.Param("registration_id") + pendingMu.Lock() _, exists := pendingRegistrations[registrationID] if !exists { + pendingMu.Unlock() return c.JSON(http.StatusNotFound, map[string]string{"error": "registration not found"}) } delete(pendingRegistrations, registrationID) + pendingMu.Unlock() return c.JSON(http.StatusOK, map[string]string{ "message": "device registration rejected", @@ -639,6 +737,7 @@ func (h *DeviceHandler) RejectDevice(c *echo.Context) error { func (h *DeviceHandler) GetPendingRegistrationsData(c *echo.Context) ([]map[string]interface{}, error) { registrations := []map[string]interface{}{} + pendingMu.Lock() for _, reg := range pendingRegistrations { if reg.UserID == (uuid.UUID{}) { registrations = append(registrations, map[string]interface{}{ @@ -652,6 +751,7 @@ func (h *DeviceHandler) GetPendingRegistrationsData(c *echo.Context) ([]map[stri }) } } + pendingMu.Unlock() return registrations, nil } @@ -664,6 +764,7 @@ func (h *DeviceHandler) ListPendingRegistrations(c *echo.Context) error { } registrations := []map[string]interface{}{} + pendingMu.Lock() for _, reg := range pendingRegistrations { if reg.UserID == userUUID || reg.UserID == (uuid.UUID{}) { registrations = append(registrations, map[string]interface{}{ @@ -677,6 +778,7 @@ func (h *DeviceHandler) ListPendingRegistrations(c *echo.Context) error { }) } } + pendingMu.Unlock() return c.JSON(http.StatusOK, map[string]interface{}{ "registrations": registrations,