From d970b6da3d1d447c3689dca69152eb24dbc45968 Mon Sep 17 00:00:00 2001 From: Alex Dunmow Date: Wed, 19 Aug 2026 18:10:27 +0800 Subject: [PATCH] fix(frontend): scope non-proto fetch allowances --- .../0002-scope-non-proto-fetch-allowances.md | 44 +++++ frontend.go | 132 +++++++++++---- frontend_test.go | 153 ++++++++++++++++++ 3 files changed, 301 insertions(+), 28 deletions(-) create mode 100644 docs/adr/0002-scope-non-proto-fetch-allowances.md diff --git a/docs/adr/0002-scope-non-proto-fetch-allowances.md b/docs/adr/0002-scope-non-proto-fetch-allowances.md new file mode 100644 index 0000000..1bfe292 --- /dev/null +++ b/docs/adr/0002-scope-non-proto-fetch-allowances.md @@ -0,0 +1,44 @@ +# Scope non-proto fetch allowances by file and exact target + +Decided 2026-08-19. Check 5 enforces generated ConnectRPC hooks for frontend +API access. Its `knownNonProtoFetches` map matched broad URL prefixes and +reported every recognized exception as a warning. The CMS and orchestrator +therefore produced eleven permanent warnings for transport contracts that +cannot use generated unary hooks: multipart uploads, an SSE response stream, +the authenticated CMS support proxy, and the browser leg of MCP device +authorization. The warnings added no actionable signal, while prefixes such as +`/api/support/` and `/api/mcp/` could classify unrelated future calls as known. + +## Decision + +- Replace URL-prefix recognition with `allowedNonProtoFetches`, whose entries + bind one exact literal fetch target to one source file. +- Require every allowance to carry a rationale explaining why generated + ConnectRPC hooks cannot express the transport or protocol. +- Permit documented matches without a warning. Continue failing every + undocumented `/api/` fetch, including an allowed endpoint copied to another + file or an unreviewed sibling endpoint added to an allowed file. +- Parse the literal first argument to `fetch()` before matching so an exact + `/api/support/tickets` allowance cannot also admit a longer route by prefix. +- Cover all eleven current exceptions and both scope boundaries with unit + tests. + +Keeping permanent warnings was rejected because a clean run could never reach +zero warnings and new actionable warnings were hidden in expected noise. +Allowlisting entire files was rejected because it would also bypass the manual +client, transport, and unrelated fetch checks. Retaining broad endpoint +prefixes without warnings was rejected because future REST calls could evade +review merely by sharing a namespace. + +## Consequences + +- Check 5 reports cleanly for the reviewed backup, helpdesk, AI streaming, + support, MCP device, and plugin upload calls. +- New non-proto calls require an explicit file-and-target decision with a + written rationale; otherwise the safety run fails. +- Renaming a route expression or moving a caller deliberately invalidates its + allowance and forces review. +- `frontend.go` owns the allowance policy and exact-target matcher; + `frontend_test.go` is the executable inventory and boundary regression suite. + +Keywords: check-safety, check 5, frontend.go, frontend_test.go, knownNonProtoFetches, allowedNonProtoFetches, nonProtoFetchAllowed, literalFetchTarget, fetch-non-proto-api, no-fetch-api, ConnectRPC, multipart, FormData, SSE, /api/push/upload, /api/helpdesk/upload, /api/ai/chat/stream, /api/support, /api/mcp/device, /api/plugins/upload diff --git a/frontend.go b/frontend.go index 6aec982..403a5f1 100644 --- a/frontend.go +++ b/frontend.go @@ -71,19 +71,72 @@ var allowedPluginRESTFiles = map[string]bool{ "plugins/judgefestblock/web/editor.tsx": true, // Block editor reads plugin /events-picker via HTTPHandler routes } -// Specific fetch paths that are non-proto REST endpoints (no ConnectRPC equivalent) -// These get a WARN, not a FAIL -var knownNonProtoFetches = map[string]bool{ - "/api/plugins/": true, // Plugin REST APIs - "/api/mcp/": true, // MCP device auth flow - "/api/lists/subscribe": true, // Public subscribe endpoint - "/api/helpdesk/": true, // Helpdesk attachment multipart upload/download (ConnectRPC doesn't support multipart) - "/preview/": true, // Preview HTML endpoints - "/api/ai/chat/stream": true, // AI chat SSE streaming (EventSource/fetch — ConnectRPC doesn't support SSE) - "/api/support/": true, // CMS helpdesk widget: multipart attachment upload to the Chi proxy — connect-query can't send FormData - "/api/helpdesk/upload": true, // Orchestrator helpdesk: multipart attachment upload — same FormData limitation - "/api/push/upload": true, // Orchestrator push/restore: multipart backup-zip upload — same FormData limitation - "/.well-known/skills/": true, // Public well-known skill discovery index (instance-derived skill name) — no proto service +type nonProtoFetchAllowance struct { + file string + target string + reason string +} + +// allowedNonProtoFetches is intentionally scoped by both source file and exact +// fetch target. A broad path prefix would let unrelated API calls bypass the +// generated-hook rule. Add an entry only after confirming that no generated +// ConnectRPC hook can carry the endpoint's transport contract. +var allowedNonProtoFetches = []nonProtoFetchAllowance{ + { + file: "components/admin/backups/restore-new-instance-dialog.tsx", + target: "/api/push/upload?account_id=${encodeURIComponent(accountId)}", + reason: "multipart backup ZIP upload; ConnectRPC messages cannot carry FormData", + }, + { + file: "components/helpdesk/helpers.ts", + target: "/api/helpdesk/upload", + reason: "multipart helpdesk attachment upload; ConnectRPC messages cannot carry FormData", + }, + { + file: "components/ai-agents/chat-page.tsx", + target: "/api/ai/chat/stream", + reason: "SSE response stream; generated Connect Query hooks are unary", + }, + { + file: "components/ai-agents/chat-widget.tsx", + target: "/api/ai/chat/stream", + reason: "SSE response stream; generated Connect Query hooks are unary", + }, + { + file: "components/bug-report/bug-report-dialog.tsx", + target: "/api/support/bug-reports", + reason: "multipart bug report forwarded by the authenticated CMS support proxy", + }, + { + file: "components/support/help-widget.tsx", + target: "/api/support/tickets/${ticketId}/messages/${messageId}/attachments", + reason: "multipart attachment forwarded by the authenticated CMS support proxy", + }, + { + file: "lib/support/availability.ts", + target: "/api/support/tickets", + reason: "CMS support availability probe targets the authenticated orchestrator proxy, not a CMS RPC", + }, + { + file: "routes/admin/authorize-device.tsx", + target: "/api/mcp/device/status", + reason: "browser leg of the MCP device authorization protocol, outside the admin RPC surface", + }, + { + file: "routes/admin/authorize-device.tsx", + target: "/api/mcp/device/authorize", + reason: "browser leg of the MCP device authorization protocol, outside the admin RPC surface", + }, + { + file: "routes/admin/authorize-device.tsx", + target: "/api/mcp/device/deny", + reason: "browser leg of the MCP device authorization protocol, outside the admin RPC surface", + }, + { + file: "routes/admin/plugins.tsx", + target: "/api/plugins/upload?token=${encodeURIComponent(token)}", + reason: "multipart BNP upload authorized by a one-time token minted through PluginsService", + }, } var ( @@ -99,7 +152,8 @@ var ( // Anti-pattern: createClient from connectrpc (should use generated hooks) reCreateClient = regexp.MustCompile(`createClient\s*\(`) - // Anti-pattern: fetch() to /api/ or proto paths (should use ConnectRPC) + // Anti-pattern: fetch() to /api/ or proto paths (should use ConnectRPC). + // The literal first argument is parsed separately for narrow REST allowances. reFetchAPI = regexp.MustCompile(`fetch\s*\(\s*['\x60"/]`) // Anti-pattern: createConnectTransport (only allowed in transport.ts) @@ -257,20 +311,8 @@ func checkFrontend(webSrcDir string) (violations []frontendViolation, warnings [ snippet: strings.TrimSpace(line), }) } else if isAPIFetch { - // Check if this is a known non-proto endpoint (warn, not fail) - isKnownNonProto := false - for prefix := range knownNonProtoFetches { - if strings.Contains(line, prefix) { - isKnownNonProto = true - break - } - } - if isKnownNonProto { - warnings = append(warnings, frontendViolation{ - file: relPath, line: lineNum, rule: "fetch-non-proto-api", - snippet: strings.TrimSpace(line), - }) - } else { + target, literal := literalFetchTarget(line) + if !literal || !nonProtoFetchAllowed(relPath, target) { violations = append(violations, frontendViolation{ file: relPath, line: lineNum, rule: "no-fetch-api", snippet: strings.TrimSpace(line), @@ -293,6 +335,40 @@ func checkFrontend(webSrcDir string) (violations []frontendViolation, warnings [ return violations, warnings } +func literalFetchTarget(line string) (string, bool) { + match := reFetchAPI.FindStringIndex(line) + if match == nil { + return "", false + } + + call := line[match[0]:] + open := strings.IndexByte(call, '(') + if open < 0 { + return "", false + } + rest := strings.TrimLeft(call[open+1:], " \t") + if len(rest) < 2 || (rest[0] != '\'' && rest[0] != '"' && rest[0] != '`') { + return "", false + } + + quote := rest[0] + end := strings.IndexByte(rest[1:], quote) + if end < 0 { + return "", false + } + return rest[1 : end+1], true +} + +func nonProtoFetchAllowed(relPath, target string) bool { + relPath = filepath.ToSlash(relPath) + for _, allowance := range allowedNonProtoFetches { + if relPath == allowance.file && target == allowance.target { + return true + } + } + return false +} + // checkPluginPages scans plugin web page directories for plugin-specific frontend // anti-patterns. Plugins must use useQuery/useMutation with { transport } options // (AGENT-GUIDE §7.2c) — createClient+useMemo is forbidden. Plugins must also diff --git a/frontend_test.go b/frontend_test.go index a55dd09..4df9c6f 100644 --- a/frontend_test.go +++ b/frontend_test.go @@ -3,9 +3,162 @@ package main import ( "os" "path/filepath" + "strings" "testing" ) +func TestCheckFrontendAllowsDocumentedNonProtoFetches(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + file string + target string + }{ + { + name: "backup multipart upload", + file: "components/admin/backups/restore-new-instance-dialog.tsx", + target: "/api/push/upload?account_id=${encodeURIComponent(accountId)}", + }, + { + name: "helpdesk multipart upload", + file: "components/helpdesk/helpers.ts", + target: "/api/helpdesk/upload", + }, + { + name: "AI chat page stream", + file: "components/ai-agents/chat-page.tsx", + target: "/api/ai/chat/stream", + }, + { + name: "AI chat widget stream", + file: "components/ai-agents/chat-widget.tsx", + target: "/api/ai/chat/stream", + }, + { + name: "support bug report multipart upload", + file: "components/bug-report/bug-report-dialog.tsx", + target: "/api/support/bug-reports", + }, + { + name: "support attachment multipart upload", + file: "components/support/help-widget.tsx", + target: "/api/support/tickets/${ticketId}/messages/${messageId}/attachments", + }, + { + name: "support availability probe", + file: "lib/support/availability.ts", + target: "/api/support/tickets", + }, + { + name: "MCP device status", + file: "routes/admin/authorize-device.tsx", + target: "/api/mcp/device/status", + }, + { + name: "MCP device authorization", + file: "routes/admin/authorize-device.tsx", + target: "/api/mcp/device/authorize", + }, + { + name: "MCP device denial", + file: "routes/admin/authorize-device.tsx", + target: "/api/mcp/device/deny", + }, + { + name: "plugin multipart upload", + file: "routes/admin/plugins.tsx", + target: "/api/plugins/upload?token=${encodeURIComponent(token)}", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + path := filepath.Join(dir, filepath.FromSlash(tt.file)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + source := "const response = await fetch(`" + tt.target + "`, { method: \"POST\" })\n" + if err := os.WriteFile(path, []byte(source), 0o644); err != nil { + t.Fatal(err) + } + + violations, warnings := checkFrontend(dir) + if len(violations) != 0 || len(warnings) != 0 { + t.Fatalf("documented REST fetch returned violations=%#v warnings=%#v", violations, warnings) + } + }) + } +} + +func TestCheckFrontendRejectsUndocumentedNonProtoFetches(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + file string + target string + }{ + { + name: "unknown endpoint in an allowed file", + file: "routes/admin/authorize-device.tsx", + target: "/api/mcp/device/revoke-all", + }, + { + name: "allowed endpoint in an unknown file", + file: "routes/admin/unknown.tsx", + target: "/api/mcp/device/status", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + path := filepath.Join(dir, filepath.FromSlash(tt.file)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + source := "const response = await fetch(`" + tt.target + "`)\n" + if err := os.WriteFile(path, []byte(source), 0o644); err != nil { + t.Fatal(err) + } + + violations, warnings := checkFrontend(dir) + if len(warnings) != 0 { + t.Fatalf("unexpected warning for undocumented REST fetch: %#v", warnings) + } + if len(violations) != 1 || violations[0].rule != "no-fetch-api" || + !strings.Contains(violations[0].snippet, tt.target) { + t.Fatalf("violations = %#v, want one no-fetch-api finding for %q", violations, tt.target) + } + }) + } +} + +func TestAllowedNonProtoFetchesAreNarrowAndDocumented(t *testing.T) { + t.Parallel() + + seen := make(map[string]bool, len(allowedNonProtoFetches)) + for _, allowance := range allowedNonProtoFetches { + if allowance.file == "" || allowance.target == "" || allowance.reason == "" { + t.Fatalf("incomplete non-proto fetch allowance: %#v", allowance) + } + if strings.HasSuffix(allowance.target, "/") { + t.Fatalf("non-proto fetch allowance must name an exact target, got %q", allowance.target) + } + key := allowance.file + "\x00" + allowance.target + if seen[key] { + t.Fatalf("duplicate non-proto fetch allowance for %s %s", allowance.file, allowance.target) + } + seen[key] = true + } +} + func TestCheckPluginPagesFlagsQueryClientProvider(t *testing.T) { dir := t.TempDir() file := filepath.Join(dir, "editor.tsx")