fix(webui): send DELETE when removing keys and adapters

Deleting a gateway key from the admin UI did nothing and reported
"use GET /api/keys": delKey() called api() with an empty options object, so
fetch defaulted to GET and the request landed in the GET branch of
handleKeysAPI. delAdapter() had the identical bug and reported
"adapter code not exposed; edit in UI".

This is the third instance of the same mistake — ab20f1b fixed delSource and
delTemplate, missing these two — so it is now pinned by tests instead of by
review:

  * TestUIAPICallsDeclareMethod walks every api() call in the embedded
    index.html and fails if one passes an options object without a method
    (an AbortSignal-only read is allowed, being a deliberate GET).
  * TestUIDeleteHelpersUseDelete / TestUIMutatingHelpersUseWriteMethods pin the
    verb of each removal and write helper by name.
  * TestKeyDeleteRoundTrip covers create -> DELETE -> gone -> second DELETE is a
    clean 404, and TestCannotDeleteOwnKey keeps the lockout guard.

The 404 bodies for GET /api/keys/{key} and GET /api/adapters/{name} now name the
verb to use ("DELETE /api/keys/{key} to remove"), because that message is what a
mis-methoded client actually shows its user; "use GET /api/keys" read as though
the caller had done nothing wrong.

delSource's indentation, broken by ab20f1b, is also straightened out.
This commit is contained in:
JianFeeeee
2026-08-30 09:07:05 +08:00
parent 813de19bd0
commit 882288f67f
5 changed files with 278 additions and 5 deletions

View File

@ -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()}) writeJSON(w, http.StatusOK, map[string]interface{}{"adapters": g.core.ListAdapters()})
return 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: case http.MethodPost:
var p adapterPayload var p adapterPayload
if err := json.NewDecoder(r.Body).Decode(&p); err != nil { if err := json.NewDecoder(r.Body).Decode(&p); err != nil {

View File

@ -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())
}
}

View File

@ -29,7 +29,11 @@ func (g *Gateway) handleKeysAPI(w http.ResponseWriter, r *http.Request) {
switch r.Method { switch r.Method {
case http.MethodGet: case http.MethodGet:
if path != "" { 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 return
} }
writeJSON(w, http.StatusOK, map[string]interface{}{"keys": g.core.ListKeys()}) writeJSON(w, http.StatusOK, map[string]interface{}{"keys": g.core.ListKeys()})

View File

@ -3027,9 +3027,9 @@
} }
async function delSource(name) { async function delSource(name) {
if (!confirm(tFmt("confirmDelSrc", name))) return; if (!confirm(tFmt("confirmDelSrc", name))) return;
await api("/api/sources/" + encodeURIComponent(name), { await api("/api/sources/" + encodeURIComponent(name), {
method: "DELETE", method: "DELETE",
}); });
toast(t("toastDelOk")); toast(t("toastDelOk"));
renderSources(); renderSources();
} }
@ -3992,6 +3992,7 @@
async function delAdapter(name) { async function delAdapter(name) {
if (!confirm(tFmt("confirmDelAdp", name))) return; if (!confirm(tFmt("confirmDelAdp", name))) return;
await api("/api/adapters/" + encodeURIComponent(name), { await api("/api/adapters/" + encodeURIComponent(name), {
method: "DELETE",
}); });
toast(t("toastDelOk")); toast(t("toastDelOk"));
renderAdapters(); renderAdapters();
@ -4545,6 +4546,7 @@
if (!confirm(tFmt("kDelConfirm", name || key))) return; if (!confirm(tFmt("kDelConfirm", name || key))) return;
try { try {
await api("/api/keys/" + encodeURIComponent(key), { await api("/api/keys/" + encodeURIComponent(key), {
method: "DELETE",
}); });
toast(t("toastDelOk")); toast(t("toastDelOk"));
loadKeys(); loadKeys();

View File

@ -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
}