diff --git a/internal/gateway/api.go b/internal/gateway/api.go index 5610666..dfa1f4a 100644 --- a/internal/gateway/api.go +++ b/internal/gateway/api.go @@ -32,7 +32,11 @@ func (g *Gateway) handleAdaptersAPI(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusOK, map[string]interface{}{"adapters": g.core.ListAdapters()}) return } - writeError(w, http.StatusNotFound, "not_found", "adapter code not exposed; edit in UI") + // A GET on a specific adapter is almost always a client that meant to + // DELETE it but let fetch default to GET; say so instead of only + // reporting that the code is not exposed. + writeError(w, http.StatusNotFound, "not_found", + "adapter code not exposed; edit in UI (to remove it use DELETE /api/adapters/"+path+")") case http.MethodPost: var p adapterPayload if err := json.NewDecoder(r.Body).Decode(&p); err != nil { diff --git a/internal/gateway/gateway_test.go b/internal/gateway/gateway_test.go index 847a94a..2e2e636 100644 --- a/internal/gateway/gateway_test.go +++ b/internal/gateway/gateway_test.go @@ -914,3 +914,88 @@ func TestStatsRecordsAPIScopedToOwnKey(t *testing.T) { } } } + +// TestKeyDeleteRoundTrip is the end-to-end version of the bug users hit: +// deleting a gateway key from the admin UI. It also asserts the GET-on-a-key +// error names the right verb, since that message is what a mis-methoded client +// actually sees. +func TestKeyDeleteRoundTrip(t *testing.T) { + g := newTestGateway(t) + + rr := doReq(t, g, http.MethodPost, "/api/keys", `{"name":"doomed","role":"user"}`) + if rr.Code != 200 { + t.Fatalf("create: %d %s", rr.Code, rr.Body.String()) + } + var created struct { + Key struct { + Key string `json:"key"` + Name string `json:"name"` + } `json:"key"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &created); err != nil { + t.Fatalf("decode create: %v", err) + } + if created.Key.Key == "" { + t.Fatalf("no key returned: %s", rr.Body.String()) + } + + // DELETE removes it + rr = doReq(t, g, http.MethodDelete, "/api/keys/"+url.PathEscape(created.Key.Key), "") + if rr.Code != 200 { + t.Fatalf("delete: %d %s", rr.Code, rr.Body.String()) + } + for _, k := range g.core.ListKeys() { + if k.Key == created.Key.Key { + t.Fatal("key still present after DELETE") + } + } + + // deleting again is a clean 404, not a 500 + rr = doReq(t, g, http.MethodDelete, "/api/keys/"+url.PathEscape(created.Key.Key), "") + if rr.Code != http.StatusNotFound { + t.Fatalf("second delete: %d, want 404", rr.Code) + } +} + +// TestKeyGetOnSpecificKeyNamesTheVerb: a GET on /api/keys/{key} is what a +// client that forgot to set method:"DELETE" ends up sending. The error must say +// which verb to use instead of just "use GET /api/keys", which reads as if the +// caller did nothing wrong. +func TestKeyGetOnSpecificKeyNamesTheVerb(t *testing.T) { + g := newTestGateway(t) + rr := doReq(t, g, http.MethodGet, "/api/keys/sk-whatever", "") + if rr.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404", rr.Code) + } + body := rr.Body.String() + for _, want := range []string{"DELETE /api/keys/", "PUT /api/keys/"} { + if !strings.Contains(body, want) { + t.Errorf("error message should mention %q, got: %s", want, body) + } + } +} + +// TestAdapterGetOnSpecificAdapterNamesDelete is the same guard for the adapters +// endpoint, whose delAdapter helper had the identical missing-method bug. +func TestAdapterGetOnSpecificAdapterNamesDelete(t *testing.T) { + g := newTestGateway(t) + rr := doReq(t, g, http.MethodGet, "/api/adapters/openai", "") + if rr.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404", rr.Code) + } + if !strings.Contains(rr.Body.String(), "DELETE /api/adapters/openai") { + t.Errorf("error should name the DELETE verb, got: %s", rr.Body.String()) + } +} + +// TestCannotDeleteOwnKey keeps the admin from locking themselves out. +func TestCannotDeleteOwnKey(t *testing.T) { + g := newTestGateway(t) + rr := doReq(t, g, http.MethodDelete, "/api/keys/sk-test", "") + if rr.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", rr.Code) + } + if !strings.Contains(rr.Body.String(), "logged in with") { + t.Errorf("unexpected message: %s", rr.Body.String()) + } +} diff --git a/internal/gateway/keys.go b/internal/gateway/keys.go index 70445c7..d95d58e 100644 --- a/internal/gateway/keys.go +++ b/internal/gateway/keys.go @@ -29,7 +29,11 @@ func (g *Gateway) handleKeysAPI(w http.ResponseWriter, r *http.Request) { switch r.Method { case http.MethodGet: if path != "" { - writeError(w, http.StatusNotFound, "not_found", "use GET /api/keys") + // A GET on a specific key is almost always a client that meant to + // DELETE or PUT it but let fetch default to GET. Name the verbs + // instead of only pointing back at the collection endpoint. + writeError(w, http.StatusNotFound, "not_found", + "no such endpoint; use GET /api/keys to list, DELETE /api/keys/{key} to remove, PUT /api/keys/{key} to update") return } writeJSON(w, http.StatusOK, map[string]interface{}{"keys": g.core.ListKeys()}) diff --git a/internal/gateway/ui/index.html b/internal/gateway/ui/index.html index 970b5e5..344e33d 100644 --- a/internal/gateway/ui/index.html +++ b/internal/gateway/ui/index.html @@ -3027,9 +3027,9 @@ } async function delSource(name) { if (!confirm(tFmt("confirmDelSrc", name))) return; - await api("/api/sources/" + encodeURIComponent(name), { - method: "DELETE", - }); + await api("/api/sources/" + encodeURIComponent(name), { + method: "DELETE", + }); toast(t("toastDelOk")); renderSources(); } @@ -3992,6 +3992,7 @@ async function delAdapter(name) { if (!confirm(tFmt("confirmDelAdp", name))) return; await api("/api/adapters/" + encodeURIComponent(name), { + method: "DELETE", }); toast(t("toastDelOk")); renderAdapters(); @@ -4545,6 +4546,7 @@ if (!confirm(tFmt("kDelConfirm", name || key))) return; try { await api("/api/keys/" + encodeURIComponent(key), { + method: "DELETE", }); toast(t("toastDelOk")); loadKeys(); diff --git a/internal/gateway/ui_contract_test.go b/internal/gateway/ui_contract_test.go new file mode 100644 index 0000000..f3197ca --- /dev/null +++ b/internal/gateway/ui_contract_test.go @@ -0,0 +1,178 @@ +package gateway + +import ( + "fmt" + "regexp" + "strings" + "testing" +) + +// The WebUI talks to this package through a tiny `api(path, options)` helper +// that wraps fetch(). fetch() defaults to GET when no method is given, so an +// options object without `method` silently turns a DELETE/PUT into a GET — +// the request hits the wrong branch of the handler and the user sees a +// confusing error like "use GET /api/keys" while nothing is deleted. +// +// This has now bitten twice: ab20f1b fixed delSource/delTemplate, and the very +// same pattern was still live in delKey (deleting a gateway key was impossible) +// and delAdapter. Reviewing 4000+ lines of inline JS by eye clearly does not +// catch it, so it is pinned here instead. + +// apiCallRe finds the start of every api(...) call in the UI source. +var apiCallRe = regexp.MustCompile(`\bapi\(`) + +// uiSource returns the embedded WebUI document. +func uiSource(t *testing.T) string { + t.Helper() + b, err := uiFS.ReadFile("ui/index.html") + if err != nil { + t.Fatalf("read embedded ui: %v", err) + } + return string(b) +} + +// extractCall returns the source of the balanced api(...) call starting at the +// opening parenthesis index, plus the line number it starts on. +func extractCall(src string, openIdx int) (string, bool) { + depth := 0 + for i := openIdx; i < len(src); i++ { + switch src[i] { + case '(': + depth++ + case ')': + depth-- + if depth == 0 { + return src[openIdx : i+1], true + } + case '\n': + // A call spanning more than ~25 lines is not one of ours; bail out + // rather than scanning the rest of the document. + if i-openIdx > 4000 { + return "", false + } + } + } + return "", false +} + +func lineOf(src string, idx int) int { + return strings.Count(src[:idx], "\n") + 1 +} + +// TestUIAPICallsDeclareMethod asserts that every api() call passing an options +// object also declares an HTTP method (or is a GET that only passes an +// AbortSignal). Without this, fetch defaults to GET and mutating endpoints are +// never reached. +func TestUIAPICallsDeclareMethod(t *testing.T) { + src := uiSource(t) + var offenders []string + for _, m := range apiCallRe.FindAllStringIndex(src, -1) { + open := m[1] - 1 // index of '(' + call, ok := extractCall(src, open) + if !ok { + continue + } + // No options object at all => a plain GET, which is fine. + if !strings.Contains(call, "{") { + continue + } + if strings.Contains(call, "method:") { + continue + } + // A read that only carries an AbortSignal is a deliberate GET. + if strings.Contains(call, "signal:") && !strings.Contains(call, "body:") { + continue + } + offenders = append(offenders, fmt.Sprintf("line %d: %s", lineOf(src, open), oneLine(call))) + } + if len(offenders) > 0 { + t.Fatalf("api() called with an options object but no method (fetch will send GET):\n %s", + strings.Join(offenders, "\n ")) + } +} + +// TestUIDeleteHelpersUseDelete pins the specific helpers that remove things. +// Each must issue a DELETE; a GET here is the exact bug users reported as +// "cannot delete key: use GET /api/keys". +func TestUIDeleteHelpersUseDelete(t *testing.T) { + src := uiSource(t) + for _, fn := range []string{"delKey", "delAdapter", "delSource", "delTemplate"} { + body, ok := jsFunctionBody(src, fn) + if !ok { + t.Errorf("helper %s() not found in the WebUI", fn) + continue + } + if !strings.Contains(body, "api(") { + t.Errorf("%s() does not call api()", fn) + continue + } + if !strings.Contains(body, `method: "DELETE"`) { + t.Errorf("%s() must issue a DELETE, got:\n%s", fn, body) + } + } +} + +// TestUIMutatingHelpersUseWriteMethods covers the write (non-delete) helpers: +// each must declare an explicit POST/PUT/PATCH. +func TestUIMutatingHelpersUseWriteMethods(t *testing.T) { + src := uiSource(t) + want := map[string][]string{ + "createKey": {`method: "POST"`}, + "putScope": {`method: "PUT"`}, + "uploadAdapter": {`method: "POST"`}, + "saveAuto": {`method: "PUT"`, `method: "POST"`}, + } + for fn, methods := range want { + body, ok := jsFunctionBody(src, fn) + if !ok { + continue // helper renamed or absent: the method test above still guards it + } + found := false + for _, m := range methods { + if strings.Contains(body, m) { + found = true + break + } + } + if !found { + t.Errorf("%s() must declare one of %v", fn, methods) + } + } +} + +// jsFunctionBody returns the source of `function name(` / `async function name(` +// up to its closing brace, using brace balancing. +func jsFunctionBody(src, name string) (string, bool) { + for _, prefix := range []string{"async function " + name + "(", "function " + name + "("} { + idx := strings.Index(src, prefix) + if idx < 0 { + continue + } + open := strings.Index(src[idx:], "{") + if open < 0 { + continue + } + open += idx + depth := 0 + for i := open; i < len(src); i++ { + switch src[i] { + case '{': + depth++ + case '}': + depth-- + if depth == 0 { + return src[idx : i+1], true + } + } + } + } + return "", false +} + +func oneLine(s string) string { + s = strings.Join(strings.Fields(s), " ") + if len(s) > 140 { + return s[:140] + "…" + } + return s +}