From ea4890c25758a0c6a26047a0ba14ce2506ef1881 Mon Sep 17 00:00:00 2001 From: yatul Date: Fri, 18 Sep 2026 19:26:43 +0400 Subject: [PATCH] =?UTF-8?q?fix(pairing):=20address=20review=20=E2=80=94=20?= =?UTF-8?q?docs,=20error=20mapping,=20approve=20feedback,=20unreadable=20e?= =?UTF-8?q?xpiry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - docs: device.pair.update and the `permanent` option on approve in docs/04-gateway-protocol.md, docs/19-websocket-rpc.md and websocket-protocol.md; the paired-device TTL row in docs/09-security.md now mentions the admin opt-out. - store.ErrPairedDeviceNotFound: SetPairingPermanent wraps it in both stores. device.pair.update maps it to NOT_FOUND and any other store error to INTERNAL, so a DB failure no longer reads as "not found". - web UI: approve and make-permanent/set-expiry now toast the server error and reload the list in `finally`. A partially applied approve (paired, but the permanent write failed) shows up in the table instead of leaving the dialog dead-ended. - SQLite ListPaired: a stored expiry that fails to parse stays 0 (expires, date unknown) rather than being mistaken for permanent; the UI renders it as "--" instead of a 1970 date. Tests: gateway handler error mapping (NOT_FOUND / INTERNAL / OK), sentinel checks in the PG and SQLite store tests, SQLite unreadable-expiry case. Co-Authored-By: Claude Opus 5 --- docs/04-gateway-protocol.md | 5 +- docs/09-security.md | 2 +- docs/19-websocket-rpc.md | 7 +- internal/gateway/methods/pairing.go | 7 +- .../gateway/methods/pairing_update_test.go | 66 +++++++++++++++++++ internal/store/pairing_store.go | 12 +++- internal/store/pg/pairing.go | 2 +- internal/store/pg/pairing_permanent_test.go | 5 +- internal/store/sqlitestore/pairing.go | 4 +- .../sqlitestore/pairing_permanent_test.go | 25 +++++-- ui/web/src/i18n/locales/en/nodes.json | 4 ++ ui/web/src/i18n/locales/ko/nodes.json | 4 ++ ui/web/src/i18n/locales/ru/nodes.json | 4 ++ ui/web/src/i18n/locales/vi/nodes.json | 4 ++ ui/web/src/i18n/locales/zh/nodes.json | 4 ++ ui/web/src/pages/nodes/hooks/use-nodes.ts | 24 +++++-- ui/web/src/pages/nodes/nodes-page.tsx | 4 +- websocket-protocol.md | 1 + 18 files changed, 161 insertions(+), 23 deletions(-) create mode 100644 internal/gateway/methods/pairing_update_test.go diff --git a/docs/04-gateway-protocol.md b/docs/04-gateway-protocol.md index 7a703a04..bf6db71b 100644 --- a/docs/04-gateway-protocol.md +++ b/docs/04-gateway-protocol.md @@ -112,7 +112,7 @@ flowchart LR |------|--------------------| | viewer | `agents.list`, `config.get`, `sessions.list`, `sessions.preview`, `health`, `status`, `providers.models`, `skills.list`, `skills.get`, `channels.list`, `channels.status`, `cron.list`, `cron.status`, `cron.runs`, `usage.get`, `usage.summary` | | operator | All viewer methods plus: `chat.send`, `chat.abort`, `chat.history`, `chat.inject`, `sessions.delete`, `sessions.reset`, `sessions.patch`, `cron.create`, `cron.update`, `cron.delete`, `cron.toggle`, `cron.run`, `skills.update`, `send`, `exec.approval.list`, `exec.approval.approve`, `exec.approval.deny`, `device.pair.request`, `device.pair.list` | -| admin | All operator methods plus: `config.apply`, `config.patch`, `config.permissions.*`, `agents.create`, `agents.update`, `agents.delete`, `agents.files.*`, `teams.*`, `channels.toggle`, `device.pair.approve`, `device.pair.revoke` | +| admin | All operator methods plus: `config.apply`, `config.patch`, `config.permissions.*`, `agents.create`, `agents.update`, `agents.delete`, `agents.files.*`, `teams.*`, `channels.toggle`, `device.pair.approve`, `device.pair.revoke`, `device.pair.update` | --- @@ -233,9 +233,10 @@ flowchart TD | Method | Description | |--------|-------------| | `device.pair.request` | Request a pairing code | -| `device.pair.approve` | Approve a pairing request | +| `device.pair.approve` | Approve a pairing request (optional `permanent`) | | `device.pair.list` | List paired devices | | `device.pair.revoke` | Revoke a paired device | +| `device.pair.update` | Make a paired device permanent or restore the default TTL | | `browser.pairing.status` | Poll browser pairing approval status | ### Exec Approval diff --git a/docs/09-security.md b/docs/09-security.md index 809ce0b0..03844124 100644 --- a/docs/09-security.md +++ b/docs/09-security.md @@ -451,7 +451,7 @@ Browser pairing allows web UI clients to authenticate without full admin credent |-----------|--------| | Pairing code | 8-character alphanumeric code (A-Z, 2-9, excludes I/O/L for clarity), generated via `generatePairingCode()` in `internal/store/pg/pairing.go` | | Code TTL | 60 minutes; expired codes are auto-pruned from database | -| Paired device TTL | 30 days; provides defense-in-depth expiry (paired devices auto-cleaned if unused) | +| Paired device TTL | 30 days by default; provides defense-in-depth expiry (expired pairings are auto-cleaned). An admin can opt a single pairing out of expiry (`device.pair.approve` with `permanent`, or `device.pair.update`); such a pairing lasts until revoked | | Pending limit | Max 3 pending pairing requests per account; prevents spam/enumeration | | HTTP access | Paired browsers access HTTP APIs via `X-GoClaw-Sender-Id` header (requires `channel=browser`). Fail-closed: `IsPaired()` check blocks unpaired sessions. Logs failed HTTP pairing auth attempts for security monitoring. | | Approval flow | Requires WebSocket `device.pair.approve` method from authenticated admin session, triggered by `pairing.approve` command. Admin approval adds sender to `paired_devices` table with `paired_by` audit field. | diff --git a/docs/19-websocket-rpc.md b/docs/19-websocket-rpc.md index 9b09783a..bfbaa674 100644 --- a/docs/19-websocket-rpc.md +++ b/docs/19-websocket-rpc.md @@ -378,10 +378,11 @@ Get JSON schema for config form generation. | Method | Description | Auth | |--------|-------------|------| | `device.pair.request` | Request pairing (from device) | Unauthenticated | -| `device.pair.approve` | Approve request (from admin) | Admin | +| `device.pair.approve` | Approve request (from admin); optional `permanent: true` skips the 30-day TTL | Admin | | `device.pair.deny` | Deny request | Admin | | `device.pair.list` | List pending + paired devices | Admin | | `device.pair.revoke` | Revoke device | Admin | +| `device.pair.update` | `{senderId, channel, permanent}` — `true` clears expiry, `false` restarts the 30-day TTL from now; an expired pairing is not revived | Admin | | `browser.pairing.status` | Poll pairing status | Unauthenticated | ### Pairing Flow @@ -391,7 +392,7 @@ sequenceDiagram Device->>Gateway: device.pair.request {senderId, channel} Gateway-->>Device: {code: "A1B2C3D4"} Device->>Gateway: browser.pairing.status {sender_id} (poll) - Admin->>Gateway: device.pair.approve {code, approvedBy} + Admin->>Gateway: device.pair.approve {code, approvedBy, permanent?} Gateway-->>Device: {status: "approved"} ``` @@ -816,7 +817,7 @@ Methods are gated by role. The role is determined at `connect` time from the tok ### Admin-Only Methods -`config.apply`, `config.patch`, `agents.create`, `agents.update`, `agents.delete`, `channels.toggle`, `device.pair.approve`, `device.pair.deny`, `device.pair.revoke`, `teams.*`, `api_keys.*`, `tenants.*` +`config.apply`, `config.patch`, `agents.create`, `agents.update`, `agents.delete`, `channels.toggle`, `device.pair.approve`, `device.pair.deny`, `device.pair.revoke`, `device.pair.update`, `teams.*`, `api_keys.*`, `tenants.*` ### Write Methods (Operator+) diff --git a/internal/gateway/methods/pairing.go b/internal/gateway/methods/pairing.go index 52be455c..9eded369 100644 --- a/internal/gateway/methods/pairing.go +++ b/internal/gateway/methods/pairing.go @@ -3,6 +3,7 @@ package methods import ( "context" "encoding/json" + "errors" "log/slog" "regexp" @@ -258,7 +259,11 @@ func (m *PairingMethods) handleUpdate(ctx context.Context, client *gateway.Clien } if err := m.service.SetPairingPermanent(ctx, params.SenderID, params.Channel, *params.Permanent); err != nil { - client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrNotFound, err.Error())) + code := protocol.ErrInternal + if errors.Is(err, store.ErrPairedDeviceNotFound) { + code = protocol.ErrNotFound + } + client.SendResponse(protocol.NewErrorResponse(req.ID, code, err.Error())) return } diff --git a/internal/gateway/methods/pairing_update_test.go b/internal/gateway/methods/pairing_update_test.go new file mode 100644 index 00000000..9a526ecb --- /dev/null +++ b/internal/gateway/methods/pairing_update_test.go @@ -0,0 +1,66 @@ +package methods + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/bus" + "github.com/nextlevelbuilder/goclaw/internal/gateway" + "github.com/nextlevelbuilder/goclaw/internal/permissions" + "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/pkg/protocol" +) + +// stubPermanentPairingStore answers SetPairingPermanent with a fixed error. +type stubPermanentPairingStore struct { + store.PairingStore + err error +} + +func (s *stubPermanentPairingStore) SetPairingPermanent(context.Context, string, string, bool) error { + return s.err +} + +func callPairingUpdate(t *testing.T, storeErr error) protocol.ResponseFrame { + t.Helper() + m := NewPairingMethods(&stubPermanentPairingStore{err: storeErr}, bus.New(), nil) + client, out := gateway.NewCapturingTestClient(permissions.RoleAdmin, store.MasterTenantID, "admin-1", 1) + raw, _ := json.Marshal(map[string]any{"senderId": "u1", "channel": "telegram", "permanent": true}) + + m.handleUpdate(t.Context(), client, &protocol.RequestFrame{ID: "r1", Params: raw}) + + var frame protocol.ResponseFrame + if err := json.Unmarshal(<-out, &frame); err != nil { + t.Fatalf("decode response: %v", err) + } + return frame +} + +func TestPairingUpdate_ErrorMapping(t *testing.T) { + cases := []struct { + name string + storeErr error + wantCode string + }{ + {"not found", fmt.Errorf("%w: telegram/u1", store.ErrPairedDeviceNotFound), protocol.ErrNotFound}, + {"db failure", errors.New("connection reset"), protocol.ErrInternal}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + frame := callPairingUpdate(t, tc.storeErr) + if frame.OK || frame.Error == nil || frame.Error.Code != tc.wantCode { + t.Fatalf("want error %s, got ok=%v error=%+v", tc.wantCode, frame.OK, frame.Error) + } + }) + } +} + +func TestPairingUpdate_OK(t *testing.T) { + frame := callPairingUpdate(t, nil) + if !frame.OK { + t.Fatalf("want ok, got error %+v", frame.Error) + } +} diff --git a/internal/store/pairing_store.go b/internal/store/pairing_store.go index 32449f33..7f1fb490 100644 --- a/internal/store/pairing_store.go +++ b/internal/store/pairing_store.go @@ -1,6 +1,13 @@ package store -import "context" +import ( + "context" + "errors" +) + +// ErrPairedDeviceNotFound is returned by SetPairingPermanent when there is no +// live (non-expired) pairing for the sender/channel. +var ErrPairedDeviceNotFound = errors.New("paired device not found") // PairingRequest represents a pending pairing code. type PairingRequestData struct { @@ -15,7 +22,8 @@ type PairingRequestData struct { } // PairedDeviceData represents an approved pairing. -// ExpiresAt is Unix ms; nil means the pairing never expires. +// ExpiresAt is Unix ms; nil means the pairing never expires, 0 means it +// expires but the stored date could not be read. type PairedDeviceData struct { SenderID string `json:"sender_id" db:"sender_id"` Channel string `json:"channel" db:"channel"` diff --git a/internal/store/pg/pairing.go b/internal/store/pg/pairing.go index 81952621..e503f62b 100644 --- a/internal/store/pg/pairing.go +++ b/internal/store/pg/pairing.go @@ -173,7 +173,7 @@ func (s *PGPairingStore) SetPairingPermanent(ctx context.Context, senderID, chan } n, _ := result.RowsAffected() if n == 0 { - return fmt.Errorf("paired device not found: %s/%s", channel, senderID) + return fmt.Errorf("%w: %s/%s", store.ErrPairedDeviceNotFound, channel, senderID) } return nil } diff --git a/internal/store/pg/pairing_permanent_test.go b/internal/store/pg/pairing_permanent_test.go index e66547d5..8dadf96f 100644 --- a/internal/store/pg/pairing_permanent_test.go +++ b/internal/store/pg/pairing_permanent_test.go @@ -2,6 +2,7 @@ package pg import ( "context" + "errors" "testing" "time" @@ -92,8 +93,8 @@ func TestPGPairing_SetPermanentDoesNotReviveExpired(t *testing.T) { t.Fatalf("expire pairing: %v", err) } - if err := s.SetPairingPermanent(ctx, "u1", "telegram", true); err == nil { - t.Fatal("SetPairingPermanent on expired pairing: want error, got nil") + if err := s.SetPairingPermanent(ctx, "u1", "telegram", true); !errors.Is(err, store.ErrPairedDeviceNotFound) { + t.Fatalf("SetPairingPermanent on expired pairing: want ErrPairedDeviceNotFound, got %v", err) } if ok, _ := s.IsPaired(ctx, "u1", "telegram"); ok { t.Fatal("expired pairing was revived") diff --git a/internal/store/sqlitestore/pairing.go b/internal/store/sqlitestore/pairing.go index 11da8853..e17cabdb 100644 --- a/internal/store/sqlitestore/pairing.go +++ b/internal/store/sqlitestore/pairing.go @@ -166,7 +166,7 @@ func (s *SQLitePairingStore) SetPairingPermanent(ctx context.Context, senderID, } n, _ := result.RowsAffected() if n == 0 { - return fmt.Errorf("paired device not found: %s/%s", channel, senderID) + return fmt.Errorf("%w: %s/%s", store.ErrPairedDeviceNotFound, channel, senderID) } return nil } @@ -249,6 +249,8 @@ func (s *SQLitePairingStore) ListPaired(ctx context.Context) []store.PairedDevic } d.PairedAt = parseTimeToMillis(pairedAtStr) if expiresAtStr.Valid { + // A parse miss yields 0: the pairing still expires (SQL compares the + // raw value), only the date is unknown. Never report it as permanent. ms := parseTimeToMillis(expiresAtStr.String) d.ExpiresAt = &ms } diff --git a/internal/store/sqlitestore/pairing_permanent_test.go b/internal/store/sqlitestore/pairing_permanent_test.go index 53931acb..ba91d952 100644 --- a/internal/store/sqlitestore/pairing_permanent_test.go +++ b/internal/store/sqlitestore/pairing_permanent_test.go @@ -4,6 +4,7 @@ package sqlitestore import ( "context" + "errors" "path/filepath" "testing" "time" @@ -102,8 +103,8 @@ func TestSQLitePairing_SetPermanentDoesNotReviveExpired(t *testing.T) { t.Fatalf("expire pairing: %v", err) } - if err := s.SetPairingPermanent(ctx, "u1", "telegram", true); err == nil { - t.Fatal("SetPairingPermanent on expired pairing: want error, got nil") + if err := s.SetPairingPermanent(ctx, "u1", "telegram", true); !errors.Is(err, store.ErrPairedDeviceNotFound) { + t.Fatalf("SetPairingPermanent on expired pairing: want ErrPairedDeviceNotFound, got %v", err) } if ok, _ := s.IsPaired(ctx, "u1", "telegram"); ok { t.Fatal("expired pairing was revived") @@ -113,7 +114,23 @@ func TestSQLitePairing_SetPermanentDoesNotReviveExpired(t *testing.T) { func TestSQLitePairing_SetPermanentUnknownDevice(t *testing.T) { s, ctx := newTestSQLitePairingStore(t) - if err := s.SetPairingPermanent(ctx, "nobody", "telegram", true); err == nil { - t.Fatal("want error for unknown device, got nil") + if err := s.SetPairingPermanent(ctx, "nobody", "telegram", true); !errors.Is(err, store.ErrPairedDeviceNotFound) { + t.Fatalf("want ErrPairedDeviceNotFound for unknown device, got %v", err) + } +} + +// An expiry the parser cannot read must not turn into "never expires". +func TestSQLitePairing_UnreadableExpiryIsNotPermanent(t *testing.T) { + s, ctx := newTestSQLitePairingStore(t) + pairTestDevice(t, s, ctx, "u1") + + if _, err := s.db.ExecContext(ctx, "UPDATE paired_devices SET expires_at = ? WHERE sender_id = ?", + "9999-garbage", "u1"); err != nil { + t.Fatalf("corrupt expiry: %v", err) + } + + got := findPaired(s.ListPaired(ctx), "u1") + if got == nil || got.ExpiresAt == nil || *got.ExpiresAt != 0 { + t.Fatalf("want u1 with ExpiresAt=0 (unknown date), got %+v", got) } } diff --git a/ui/web/src/i18n/locales/en/nodes.json b/ui/web/src/i18n/locales/en/nodes.json index 35504b99..9a30b275 100644 --- a/ui/web/src/i18n/locales/en/nodes.json +++ b/ui/web/src/i18n/locales/en/nodes.json @@ -46,5 +46,9 @@ "title": "Set Pairing Expiry", "description": "Return the pairing for {{channel}}:{{senderId}} to the standard term? It will expire in 30 days from now.", "confirmLabel": "Set expiry" + }, + "toast": { + "approveFailed": "Could not approve pairing", + "updateFailed": "Could not change pairing expiry" } } diff --git a/ui/web/src/i18n/locales/ko/nodes.json b/ui/web/src/i18n/locales/ko/nodes.json index 3b99d23b..4a0da5c3 100644 --- a/ui/web/src/i18n/locales/ko/nodes.json +++ b/ui/web/src/i18n/locales/ko/nodes.json @@ -46,5 +46,9 @@ "title": "페어링 만료 설정", "description": "{{channel}}:{{senderId}} 페어링을 기본 기간으로 되돌리시겠습니까? 지금부터 30일 후 만료됩니다.", "confirmLabel": "만료 설정" + }, + "toast": { + "approveFailed": "페어링을 승인할 수 없습니다", + "updateFailed": "페어링 만료를 변경할 수 없습니다" } } diff --git a/ui/web/src/i18n/locales/ru/nodes.json b/ui/web/src/i18n/locales/ru/nodes.json index 54e6ae10..85d3bef2 100644 --- a/ui/web/src/i18n/locales/ru/nodes.json +++ b/ui/web/src/i18n/locales/ru/nodes.json @@ -46,5 +46,9 @@ "title": "Вернуть срок привязки", "description": "Вернуть привязке {{channel}}:{{senderId}} стандартный срок? Она истечёт через 30 дней, считая от сейчас.", "confirmLabel": "Вернуть срок" + }, + "toast": { + "approveFailed": "Не удалось одобрить привязку", + "updateFailed": "Не удалось изменить срок привязки" } } diff --git a/ui/web/src/i18n/locales/vi/nodes.json b/ui/web/src/i18n/locales/vi/nodes.json index 7a42d6e6..b69507a7 100644 --- a/ui/web/src/i18n/locales/vi/nodes.json +++ b/ui/web/src/i18n/locales/vi/nodes.json @@ -46,5 +46,9 @@ "title": "Đặt thời hạn ghép nối", "description": "Đưa ghép nối {{channel}}:{{senderId}} về thời hạn mặc định? Nó sẽ hết hạn sau 30 ngày kể từ bây giờ.", "confirmLabel": "Đặt thời hạn" + }, + "toast": { + "approveFailed": "Không thể phê duyệt ghép nối", + "updateFailed": "Không thể thay đổi thời hạn ghép nối" } } diff --git a/ui/web/src/i18n/locales/zh/nodes.json b/ui/web/src/i18n/locales/zh/nodes.json index 27c26788..6121e8de 100644 --- a/ui/web/src/i18n/locales/zh/nodes.json +++ b/ui/web/src/i18n/locales/zh/nodes.json @@ -46,5 +46,9 @@ "title": "设置配对期限", "description": "将 {{channel}}:{{senderId}} 的配对恢复为默认期限？将从现在起 30 天后过期。", "confirmLabel": "设置期限" + }, + "toast": { + "approveFailed": "无法批准配对", + "updateFailed": "无法更改配对期限" } } diff --git a/ui/web/src/pages/nodes/hooks/use-nodes.ts b/ui/web/src/pages/nodes/hooks/use-nodes.ts index 079ef50f..aa2e9c21 100644 --- a/ui/web/src/pages/nodes/hooks/use-nodes.ts +++ b/ui/web/src/pages/nodes/hooks/use-nodes.ts @@ -3,6 +3,8 @@ import { useWs } from "@/hooks/use-ws"; import { useAuthStore } from "@/stores/use-auth-store"; import { useWsEvent } from "@/hooks/use-ws-event"; import { Methods, Events } from "@/api/protocol"; +import { toast } from "@/stores/use-toast-store"; +import i18next from "i18next"; export interface PendingPairing { code: string; @@ -20,7 +22,7 @@ export interface PairedDevice { chat_id: string; paired_at: number; paired_by: string; - /** Unix ms; null means the pairing never expires. */ + /** Unix ms; null means the pairing never expires, 0 means it expires on an unknown date. */ expires_at: number | null; } @@ -62,8 +64,15 @@ export function useNodes() { const approvePairing = useCallback( async (code: string, permanent = false) => { - await ws.call(Methods.PAIRING_APPROVE, { code, permanent }); - load(); + try { + await ws.call(Methods.PAIRING_APPROVE, { code, permanent }); + } catch (err) { + // With permanent=true the device may already be paired with the default + // TTL when this fails; the reload below shows the actual state. + toast.error(i18next.t("nodes:toast.approveFailed"), err instanceof Error ? err.message : ""); + } finally { + load(); + } }, [ws, load], ); @@ -86,8 +95,13 @@ export function useNodes() { const setPairingPermanent = useCallback( async (senderId: string, channel: string, permanent: boolean) => { - await ws.call(Methods.PAIRING_UPDATE, { senderId, channel, permanent }); - load(); + try { + await ws.call(Methods.PAIRING_UPDATE, { senderId, channel, permanent }); + } catch (err) { + toast.error(i18next.t("nodes:toast.updateFailed"), err instanceof Error ? err.message : ""); + } finally { + load(); + } }, [ws, load], ); diff --git a/ui/web/src/pages/nodes/nodes-page.tsx b/ui/web/src/pages/nodes/nodes-page.tsx index 9fa31c1c..ba718291 100644 --- a/ui/web/src/pages/nodes/nodes-page.tsx +++ b/ui/web/src/pages/nodes/nodes-page.tsx @@ -151,8 +151,10 @@ export function NodesPage() { {t("never")} - ) : ( + ) : d.expires_at > 0 ? ( formatDate(new Date(d.expires_at)) + ) : ( + "--" )} diff --git a/websocket-protocol.md b/websocket-protocol.md index 06e3067e..3d5f8be7 100644 --- a/websocket-protocol.md +++ b/websocket-protocol.md @@ -42,6 +42,7 @@ The first request must be a `connect` handshake. Authentication supports three p | `device.pair.approve` | Approve a pairing code | | `device.pair.list` | List pending and approved pairings | | `device.pair.revoke` | Revoke a pairing | +| `device.pair.update` | Make a pairing permanent or restore the default TTL | ## Events (server push)