fix(frontend): scope non-proto fetch allowances
This commit is contained in:
Alex Dunmow
parent
a159bddf0b
commit
d970b6da3d
44
docs/adr/0002-scope-non-proto-fetch-allowances.md
Normal file
44
docs/adr/0002-scope-non-proto-fetch-allowances.md
Normal file
@ -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
|
||||
132
frontend.go
132
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
|
||||
|
||||
153
frontend_test.go
153
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")
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user