diff --git a/api-tests/admin-list-authed.yml b/api-tests/admin-list-authed.yml new file mode 100644 index 0000000..d324f4f --- /dev/null +++ b/api-tests/admin-list-authed.yml @@ -0,0 +1,30 @@ +info: + name: "admin: authenticated list of pending reports" + # GET /v1/admin/reports with a valid Bearer token returns the pending ids. + # Captures the first id into the `removeId` run variable for the remove test. + type: http + seq: 7 + +http: + method: GET + url: "{{baseUrl}}/v1/admin/reports" + headers: + - name: Authorization + value: "Bearer {{adminToken}}" + +runtime: + scripts: + - type: tests + code: |- + test("authenticated list returns 200", function () { + expect(res.getStatus()).to.equal(200); + }); + test("body has status ok and a reports array", function () { + expect(res.getBody().status).to.equal("ok"); + expect(res.getBody().reports).to.be.an("array"); + }); + test("at least one pending report is listed (the seed)", function () { + expect(res.getBody().reports.length).to.be.at.least(1); + }); + // Remember an id to remove in the next request. + bru.setVar("removeId", res.getBody().reports[0]); diff --git a/api-tests/admin-list-excludes-removed.yml b/api-tests/admin-list-excludes-removed.yml new file mode 100644 index 0000000..7249e77 --- /dev/null +++ b/api-tests/admin-list-excludes-removed.yml @@ -0,0 +1,24 @@ +info: + name: "admin: removed report is excluded from the pending list" + # Re-list after the remove: the removed id must no longer appear, proving it is + # excluded from the next weekly publish run (the ticket's acceptance criterion). + type: http + seq: 9 + +http: + method: GET + url: "{{baseUrl}}/v1/admin/reports" + headers: + - name: Authorization + value: "Bearer {{adminToken}}" + +runtime: + scripts: + - type: tests + code: |- + test("list returns 200", function () { + expect(res.getStatus()).to.equal(200); + }); + test("the removed id is no longer pending", function () { + expect(res.getBody().reports).to.not.include(bru.getVar("removeId")); + }); diff --git a/api-tests/admin-remove-authed.yml b/api-tests/admin-remove-authed.yml new file mode 100644 index 0000000..9abf1bc --- /dev/null +++ b/api-tests/admin-remove-authed.yml @@ -0,0 +1,25 @@ +info: + name: "admin: authenticated remove of a pending report" + # POST /v1/admin/reports/{id}/remove with a valid Bearer token transitions the + # report pending -> removed (#10), excluding it from the next weekly publish. + type: http + seq: 8 + +http: + method: POST + url: "{{baseUrl}}/v1/admin/reports/{{removeId}}/remove" + headers: + - name: Authorization + value: "Bearer {{adminToken}}" + +runtime: + scripts: + - type: tests + code: |- + test("authenticated remove returns 200", function () { + expect(res.getStatus()).to.equal(200); + }); + test("body confirms the removed id", function () { + expect(res.getBody().status).to.equal("removed"); + expect(res.getBody().id).to.equal(bru.getVar("removeId")); + }); diff --git a/api-tests/admin-seed-report.yml b/api-tests/admin-seed-report.yml new file mode 100644 index 0000000..1b89716 --- /dev/null +++ b/api-tests/admin-seed-report.yml @@ -0,0 +1,32 @@ +info: + name: "admin: seed a pending report to review" + # Seeds one pending report so the admin list/remove flow below is self-contained + # (it does not depend on the earlier ingest contract tests having run first). + type: http + seq: 6 + +http: + method: POST + url: "{{baseUrl}}/v1/reports" + headers: + - name: Content-Type + value: application/json + body: + type: json + data: |- + { + "appVersion": "1.4.2 (142)", + "platform": "android", + "osVersion": "Android 14", + "device": "Pixel 7", + "clientTimestamp": "2026-07-02T12:34:56Z", + "report": "seed report for the admin review/removal flow (#11)" + } + +runtime: + scripts: + - type: tests + code: |- + test("seed report is accepted (202)", function () { + expect(res.getStatus()).to.equal(202); + }); diff --git a/api-tests/admin-unauthorized-bad-token.yml b/api-tests/admin-unauthorized-bad-token.yml new file mode 100644 index 0000000..534fd81 --- /dev/null +++ b/api-tests/admin-unauthorized-bad-token.yml @@ -0,0 +1,20 @@ +info: + name: "admin: wrong token is rejected (401)" + # A well-formed Bearer header carrying the wrong secret -> rejected. + type: http + seq: 11 + +http: + method: GET + url: "{{baseUrl}}/v1/admin/reports" + headers: + - name: Authorization + value: "Bearer not-the-admin-token" + +runtime: + scripts: + - type: tests + code: |- + test("a request with the wrong bearer token returns 401", function () { + expect(res.getStatus()).to.equal(401); + }); diff --git a/api-tests/admin-unauthorized-no-token.yml b/api-tests/admin-unauthorized-no-token.yml new file mode 100644 index 0000000..4d5a63c --- /dev/null +++ b/api-tests/admin-unauthorized-no-token.yml @@ -0,0 +1,20 @@ +info: + name: "admin: missing token is rejected (401)" + # No Authorization header -> the admin API rejects the request. + type: http + seq: 10 + +http: + method: GET + url: "{{baseUrl}}/v1/admin/reports" + +runtime: + scripts: + - type: tests + code: |- + test("a request with no bearer token returns 401", function () { + expect(res.getStatus()).to.equal(401); + }); + test("401 advertises the Bearer challenge", function () { + expect(res.getHeader("www-authenticate")).to.equal("Bearer"); + }); diff --git a/api-tests/environments/local.yml b/api-tests/environments/local.yml index 0b7e23c..9593963 100644 --- a/api-tests/environments/local.yml +++ b/api-tests/environments/local.yml @@ -3,3 +3,9 @@ name: local variables: - name: baseUrl value: http://localhost:8787 + # Shared secret for the maintainer admin API (#11). The dev server must be + # started with ADMIN_TOKEN set to this same value (see package.json + # "test:api"): `ADMIN_TOKEN=local-dev-admin-token go run ./cmd/devserver`. + # This is a throwaway local-only token, never a production secret. + - name: adminToken + value: local-dev-admin-token diff --git a/cmd/devserver/main.go b/cmd/devserver/main.go index 5364c3f..8cd0fa6 100644 --- a/cmd/devserver/main.go +++ b/cmd/devserver/main.go @@ -10,6 +10,13 @@ // by an in-memory object store and a throwaway per-run AES-256 key, so a POST // /v1/reports exercises the full pipeline locally. Stored objects live only for // the process lifetime. +// +// The maintainer admin API (#11) is wired over a lifecycle.Manager sharing that +// same in-memory store, so reports ingested via POST /v1/reports are immediately +// listable and removable under /v1/admin/reports. Its shared-secret bearer token +// comes from the ADMIN_TOKEN env var; if unset, the admin routes fail closed +// (every request 401s), matching production's fail-closed behaviour. Set +// ADMIN_TOKEN=... to exercise the admin API (the Bruno api-tests do this). package main import ( @@ -19,6 +26,7 @@ import ( "github.com/JMR-dev/LibreMail-Bug-Report-Ingest/internal/crypto" "github.com/JMR-dev/LibreMail-Bug-Report-Ingest/internal/handler" + "github.com/JMR-dev/LibreMail-Bug-Report-Ingest/internal/lifecycle" "github.com/JMR-dev/LibreMail-Bug-Report-Ingest/internal/storage" ) @@ -38,10 +46,21 @@ func main() { if err != nil { log.Fatalf("devserver: build keyring: %v", err) } - sink := storage.NewSink(storage.NewMemoryStore(), keyring) + // One shared in-memory store backs both the ingest Sink and the admin + // lifecycle Manager, so an ingested report is visible to the admin API. + store := storage.NewMemoryStore() + sink := storage.NewSink(store, keyring) + adminToken := os.Getenv("ADMIN_TOKEN") + admin := handler.NewManagerBackend(lifecycle.New(store), adminToken) + + if adminToken == "" { + log.Print("devserver: ADMIN_TOKEN is unset; /v1/admin routes fail closed (401). Set ADMIN_TOKEN to enable them.") + } else { + log.Print("devserver: admin API enabled at /v1/admin/reports (bearer token from ADMIN_TOKEN)") + } log.Printf("devserver listening on %s (try GET / and GET /healthz)", addr) - if err := http.ListenAndServe(addr, handler.New(sink)); err != nil { + if err := http.ListenAndServe(addr, handler.New(sink, admin)); err != nil { log.Fatalf("devserver: %v", err) } } diff --git a/docs/decisions/admin-auth.md b/docs/decisions/admin-auth.md new file mode 100644 index 0000000..cecedfe --- /dev/null +++ b/docs/decisions/admin-auth.md @@ -0,0 +1,76 @@ +# ADR 0003: Authentication for the maintainer admin API + +- **Status:** Accepted +- **Date:** 2026-07-02 +- **Deciders:** Maintainer (single-maintainer project) +- **Ticket:** [#11](https://github.com/JMR-dev/LibreMail-Bug-Report-Ingest/issues/11) — Manual review/removal path for maintainers +- **Depends on:** [#10](https://github.com/JMR-dev/LibreMail-Bug-Report-Ingest/issues/10) (report lifecycle: `MarkRemoved`) +- **Related:** [#13](https://github.com/JMR-dev/LibreMail-Bug-Report-Ingest/issues/13) (weekly publish reads `ListPending`; a removed report is excluded) + +--- + +## Context + +The weekly job publishes every still-pending report as a GitHub issue. Before that +run the single maintainer needs an authenticated way to (a) list the pending +reports and (b) remove a specific one so it is never published. This adds two +admin endpoints to the existing Worker HTTP handler: + +``` +GET /v1/admin/reports -> 200 {"status":"ok","reports":[...]} +POST /v1/admin/reports/{id}/remove -> 200 {"status":"removed","id":} +DELETE /v1/admin/reports/{id} -> 200 (REST alias of the POST above) +``` + +`remove` calls `lifecycle.Manager.MarkRemoved` (#10), transitioning the report +`pending -> removed`; #13's `ListPending` then no longer returns it, so it is +excluded from the next publish run. These endpoints are destructive and expose +report ids, so they must be authenticated. The endpoints run in a Cloudflare +Worker (Go/TinyGo/Wasm) with secrets in Cloudflare Secrets Store. + +## Decision + +**Authenticate with a shared-secret Bearer token**, compared in constant time. + +- Every admin request must send `Authorization: Bearer `. +- The presented token is compared to the configured secret with + `crypto/subtle.ConstantTimeCompare`, so a wrong guess leaks no timing signal. +- The scheme (`Bearer`) is matched case-insensitively per RFC 7235; anything else + (missing header, wrong scheme, wrong token) returns **401** with a + `WWW-Authenticate: Bearer` challenge. A wrong method on an admin path returns + **405**; an unknown/never-pending id returns **404**. +- **Fail closed:** if the server has no secret configured (unset or empty), every + admin request is rejected with 401 regardless of what the client sends. A + missing secret binding can therefore never silently disable authentication. +- **Secret custody:** in production the secret is the Cloudflare Secrets Store + binding `ADMIN_TOKEN` (wrangler.jsonc `secrets_store_secrets`), read per request + (like the encryption keyring in ADR #5) and never logged or echoed. The dev + server and tests inject the token directly (env var `ADMIN_TOKEN` for the dev + server), so the exact same handler is exercised locally. + +## Alternatives considered + +- **Cloudflare Access (Zero Trust) in front of the route.** Strong (SSO, device + posture, short-lived JWTs, per-request audit) and requires no app-side secret. + **Rejected for v1:** it needs a Zero Trust org, an application, and an access + policy to be provisioned and maintained, and it complicates scripted/`curl`/CI + access — disproportionate for a single maintainer removing the occasional + report. It remains the natural upgrade if the maintainer set grows or richer + audit is wanted; it can be layered in front of the Bearer check later without + changing the handler. +- **mTLS / client certificates.** Operationally heavy (cert issuance, rotation, + client provisioning) for one operator. Rejected. +- **No dedicated auth, rely on an unguessable URL.** Rejected: not real + authentication, leaks via logs/history, and cannot be rotated cleanly. + +## Consequences + +- **Positive:** minimal moving parts; one secret to rotate (rotate the Secrets + Store value); trivially callable from `curl`, scripts, or CI; the auth is plain, + build-tag-free Go that is fully host-testable (`httptest`) and exercised end to + end by the Bruno API tests against the dev server. +- **Negative / limitations:** a single shared secret has no per-user identity or + built-in audit trail, and if leaked it grants full admin until rotated. Mitigated + by constant-time comparison, fail-closed behaviour, never logging the token, and + HTTPS-only transport (the Worker is HTTPS). Revisit with Cloudflare Access if the + maintainer set grows or per-actor audit becomes a requirement. diff --git a/internal/handler/admin.go b/internal/handler/admin.go new file mode 100644 index 0000000..5a198ff --- /dev/null +++ b/internal/handler/admin.go @@ -0,0 +1,224 @@ +package handler + +// Maintainer admin API (#11): an authenticated path for the single maintainer to +// review the pending queue and pull a report before the weekly publish run. +// +// GET /v1/admin/reports -> 200 {"status":"ok","reports":[...]} +// POST /v1/admin/reports/{id}/remove -> 200 {"status":"removed","id":} +// DELETE /v1/admin/reports/{id} -> 200 {"status":"removed","id":} (alias) +// +// Removing a report transitions it pending -> removed via the lifecycle Manager +// (#10); #13's ListPending no longer returns it, so it is excluded from the next +// Friday publish. Remove is idempotent (removing an already-removed report still +// succeeds); an id that was never pending returns 404. +// +// # Authentication +// +// A shared-secret bearer token, chosen over Cloudflare Access for a +// single-maintainer, low-volume tool: it needs no Zero Trust org/policy setup, +// is trivially callable from curl/scripts/CI, and injects cleanly for tests and +// the dev server. See docs/decisions/admin-auth.md. +// +// Every admin request must carry `Authorization: Bearer `. The presented +// token is compared to the configured secret with crypto/subtle.ConstantTimeCompare +// so a wrong token cannot be recovered by timing. The endpoint fails closed: if +// the server has no secret configured (unset/empty), every request is rejected +// with 401 regardless of what the client sends, so a missing binding can never +// silently disable auth. Missing/malformed/wrong credentials also return 401 with +// a `WWW-Authenticate: Bearer` challenge. + +import ( + "context" + "crypto/subtle" + "encoding/json" + "errors" + "net/http" + "strings" + + "github.com/JMR-dev/LibreMail-Bug-Report-Ingest/internal/lifecycle" +) + +// AdminBackend supplies the admin API's dependencies. It is an interface, not a +// concrete *lifecycle.Manager, so the same routes serve both deployment targets: +// the host (dev server, tests) wires a static Manager + injected token via +// NewManagerBackend, while the Cloudflare Worker wires a lazy implementation that +// reads the token from Secrets Store and builds an R2-backed Manager per request +// (the token and the R2 binding are only available inside a request there). +type AdminBackend interface { + // AdminToken returns the configured shared secret, or "" if none is set (in + // which case auth fails closed). ctx-scoped so the Worker can read Secrets + // Store per request. A non-nil error (e.g. a failed secret load) yields 503. + AdminToken(ctx context.Context) (string, error) + // ListPending returns the ids of all pending reports (lifecycle.Manager.ListPending). + ListPending(ctx context.Context) ([]string, error) + // MarkRemoved transitions a pending report to removed (lifecycle.Manager.MarkRemoved), + // returning lifecycle.ErrUnknownReport if the id is not pending. + MarkRemoved(ctx context.Context, id string) error +} + +// managerBackend adapts a ready *lifecycle.Manager and a fixed shared secret to +// AdminBackend. It is the host wiring (dev server, tests); the Worker supplies +// its own lazy backend. +type managerBackend struct { + mgr *lifecycle.Manager + token string +} + +// NewManagerBackend wires an injected *lifecycle.Manager and shared secret into +// an AdminBackend for the dev server and tests. An empty token leaves the admin +// API authenticated-but-unopenable (every request 401s), which is the intended +// fail-closed behaviour when no secret is provisioned. +func NewManagerBackend(mgr *lifecycle.Manager, token string) AdminBackend { + return managerBackend{mgr: mgr, token: token} +} + +func (b managerBackend) AdminToken(context.Context) (string, error) { return b.token, nil } + +func (b managerBackend) ListPending(ctx context.Context) ([]string, error) { + return b.mgr.ListPending(ctx) +} + +func (b managerBackend) MarkRemoved(ctx context.Context, id string) error { + return b.mgr.MarkRemoved(ctx, id) +} + +// denyAllBackend is substituted when New is called with a nil AdminBackend. It +// reports an empty secret, so authentication always fails closed (401) and the +// list/remove methods are never reached. +type denyAllBackend struct{} + +func (denyAllBackend) AdminToken(context.Context) (string, error) { return "", nil } +func (denyAllBackend) ListPending(context.Context) ([]string, error) { return nil, errNotConfigured } +func (denyAllBackend) MarkRemoved(context.Context, string) error { return errNotConfigured } + +var errNotConfigured = errors.New("handler: admin backend not configured") + +// adminAPI holds the admin route handlers over an AdminBackend. +type adminAPI struct { + backend AdminBackend +} + +// register mounts the admin routes on mux. Patterns are method-agnostic and each +// handler dispatches on the method itself, matching the rest of this package +// (isGet, the ingest handler): a broad "/" catch-all is also registered on the +// same mux, and it would otherwise absorb a method-mismatched request before the +// mux's own 405 logic could fire, so the method check lives in-handler. +func (a *adminAPI) register(mux *http.ServeMux) { + mux.HandleFunc("/v1/admin/reports", a.reports) + mux.HandleFunc("/v1/admin/reports/{id}/remove", a.removeViaPost) + mux.HandleFunc("/v1/admin/reports/{id}", a.removeViaDelete) +} + +// reports serves /v1/admin/reports: GET lists the pending report ids. +func (a *adminAPI) reports(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + methodNotAllowed(w, http.MethodGet) + return + } + if !a.authorize(w, r) { + return + } + ids, err := a.backend.ListPending(r.Context()) + if err != nil { + writeAdmin(w, http.StatusServiceUnavailable, adminResponse{Status: "error", Error: "admin backend unavailable"}) + return + } + if ids == nil { + ids = []string{} // marshal an empty JSON array, never null + } + writeAdmin(w, http.StatusOK, adminList{Status: "ok", Reports: ids}) +} + +// removeViaPost serves POST /v1/admin/reports/{id}/remove. +func (a *adminAPI) removeViaPost(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + methodNotAllowed(w, http.MethodPost) + return + } + a.remove(w, r) +} + +// removeViaDelete serves DELETE /v1/admin/reports/{id}, the REST-style alias for +// removeViaPost. +func (a *adminAPI) removeViaDelete(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodDelete { + methodNotAllowed(w, http.MethodDelete) + return + } + a.remove(w, r) +} + +// remove transitions the {id} report pending -> removed, after authenticating. +func (a *adminAPI) remove(w http.ResponseWriter, r *http.Request) { + if !a.authorize(w, r) { + return + } + id := r.PathValue("id") + err := a.backend.MarkRemoved(r.Context(), id) + switch { + case err == nil: + writeAdmin(w, http.StatusOK, adminResponse{Status: "removed", ID: id}) + case errors.Is(err, lifecycle.ErrUnknownReport): + writeAdmin(w, http.StatusNotFound, adminResponse{Status: "error", Error: "unknown report"}) + default: + writeAdmin(w, http.StatusServiceUnavailable, adminResponse{Status: "error", Error: "admin backend unavailable"}) + } +} + +// methodNotAllowed writes a 405 with the Allow header advertising the one method +// the admin route accepts. +func methodNotAllowed(w http.ResponseWriter, allow string) { + w.Header().Set("Allow", allow) + writeAdmin(w, http.StatusMethodNotAllowed, adminResponse{Status: "error", Error: "method not allowed"}) +} + +// authorize enforces the shared-secret bearer token. It returns true when the +// request is authenticated; otherwise it writes a 401 (missing/bad token or unset +// server secret) or 503 (secret load failed) and returns false. The comparison is +// constant-time and the endpoint fails closed on an empty configured secret. +func (a *adminAPI) authorize(w http.ResponseWriter, r *http.Request) bool { + secret, err := a.backend.AdminToken(r.Context()) + if err != nil { + writeAdmin(w, http.StatusServiceUnavailable, adminResponse{Status: "error", Error: "admin backend unavailable"}) + return false + } + if secret == "" || !bearerMatches(r.Header.Get("Authorization"), secret) { + w.Header().Set("WWW-Authenticate", "Bearer") + writeAdmin(w, http.StatusUnauthorized, adminResponse{Status: "error", Error: "unauthorized"}) + return false + } + return true +} + +// bearerMatches reports whether the Authorization header carries a Bearer token +// equal to secret. The scheme is matched case-insensitively (per RFC 7235); the +// token is compared in constant time. secret is assumed non-empty (the caller +// fails closed on an empty secret before calling this). +func bearerMatches(header, secret string) bool { + scheme, token, found := strings.Cut(header, " ") + if !found || !strings.EqualFold(scheme, "Bearer") { + return false + } + return subtle.ConstantTimeCompare([]byte(token), []byte(secret)) == 1 +} + +// adminList is the GET /v1/admin/reports success body. Reports has no omitempty, +// so an empty pending set still marshals as {"reports":[]} (an array, never null), +// which clients and the Bruno tests rely on. +type adminList struct { + Status string `json:"status"` + Reports []string `json:"reports"` +} + +// adminResponse is the JSON body shape for the remove and error responses. +type adminResponse struct { + Status string `json:"status"` + ID string `json:"id,omitempty"` + Error string `json:"error,omitempty"` +} + +func writeAdmin(w http.ResponseWriter, status int, body any) { + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(body) +} diff --git a/internal/handler/admin_test.go b/internal/handler/admin_test.go new file mode 100644 index 0000000..6216e8b --- /dev/null +++ b/internal/handler/admin_test.go @@ -0,0 +1,268 @@ +package handler + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "slices" + "strings" + "testing" + + "github.com/JMR-dev/LibreMail-Bug-Report-Ingest/internal/lifecycle" + "github.com/JMR-dev/LibreMail-Bug-Report-Ingest/internal/storage" +) + +const testToken = "s3cret-admin-token" + +// adminHarness builds a handler whose admin API is backed by an in-memory store +// seeded with the given pending report ids, authenticated by token. It returns +// the handler and the store so tests can assert post-conditions. +func adminHarness(t *testing.T, token string, pendingIDs ...string) (http.Handler, storage.ObjectStore) { + t.Helper() + store := storage.NewMemoryStore() + for _, id := range pendingIDs { + // The admin API only lists keys and moves opaque bytes, so a placeholder + // ciphertext is enough; no real crypto is needed to exercise it. + if err := store.Put(context.Background(), storage.ReportKey(storage.StatusPending, id), []byte("frame:"+id)); err != nil { + t.Fatalf("seed pending %q: %v", id, err) + } + } + h := New(nil, NewManagerBackend(lifecycle.New(store), token)) + return h, store +} + +// adminReq issues one admin request with an optional bearer token ("" sends no +// Authorization header) and returns the recorder. +func adminReq(t *testing.T, h http.Handler, method, target, bearer string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(method, target, nil) + if bearer != "" { + req.Header.Set("Authorization", "Bearer "+bearer) + } + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + return rec +} + +// adminBody is a decode target spanning every admin response field (list, remove, +// and error bodies), so one helper can parse any admin response in the tests. +type adminBody struct { + Status string `json:"status"` + Reports []string `json:"reports"` + ID string `json:"id"` + Error string `json:"error"` +} + +// decodeAdmin parses an admin JSON response body. +func decodeAdmin(t *testing.T, rec *httptest.ResponseRecorder) adminBody { + t.Helper() + var got adminBody + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("admin response is not valid JSON: %v (body=%q)", err, rec.Body.String()) + } + return got +} + +// TestAdminListReturnsPendingIDs is the acceptance path: an authenticated +// maintainer lists exactly the pending report ids. +func TestAdminListReturnsPendingIDs(t *testing.T) { + h, _ := adminHarness(t, testToken, "id-a", "id-b", "id-c") + + rec := adminReq(t, h, http.MethodGet, "/v1/admin/reports", testToken) + if rec.Code != http.StatusOK { + t.Fatalf("GET list status = %d, want 200 (body=%q)", rec.Code, rec.Body.String()) + } + if ct := rec.Header().Get("Content-Type"); ct != "application/json; charset=utf-8" { + t.Errorf("Content-Type = %q, want application/json; charset=utf-8", ct) + } + got := decodeAdmin(t, rec) + if got.Status != "ok" { + t.Errorf("status field = %q, want ok", got.Status) + } + want := []string{"id-a", "id-b", "id-c"} // sorted, MemoryStore lists ascending + if !slices.Equal(got.Reports, want) { + t.Errorf("reports = %v, want %v", got.Reports, want) + } +} + +// TestAdminListEmptyIsArray confirms an empty pending set marshals as [] (not null), +// which the API consumers (and the Bruno tests) rely on. +func TestAdminListEmptyIsArray(t *testing.T) { + h, _ := adminHarness(t, testToken) + rec := adminReq(t, h, http.MethodGet, "/v1/admin/reports", testToken) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + if body := rec.Body.String(); !strings.Contains(body, `"reports":[]`) { + t.Errorf("empty list body = %q, want it to contain \"reports\":[]", body) + } +} + +// TestAdminRemoveExcludesFromPending is the ticket's core acceptance: a removed +// report transitions to removed and disappears from the pending list, so the next +// publish run will not see it. +func TestAdminRemoveExcludesFromPending(t *testing.T) { + h, store := adminHarness(t, testToken, "keep-1", "drop-me", "keep-2") + + rec := adminReq(t, h, http.MethodPost, "/v1/admin/reports/drop-me/remove", testToken) + if rec.Code != http.StatusOK { + t.Fatalf("remove status = %d, want 200 (body=%q)", rec.Code, rec.Body.String()) + } + if got := decodeAdmin(t, rec); got.Status != "removed" || got.ID != "drop-me" { + t.Errorf("remove body = %+v, want {status:removed id:drop-me}", got) + } + + // It is gone from the pending list... + list := adminReq(t, h, http.MethodGet, "/v1/admin/reports", testToken) + if got := decodeAdmin(t, list).Reports; slices.Contains(got, "drop-me") { + t.Errorf("pending list still contains drop-me: %v", got) + } else if !slices.Equal(got, []string{"keep-1", "keep-2"}) { + t.Errorf("pending list = %v, want [keep-1 keep-2]", got) + } + + // ...and it now lives under the removed prefix (excluded from publishing). + removed, _ := store.List(context.Background(), storage.StatusPrefix(storage.StatusRemoved)) + if !slices.Equal(removed, []string{storage.ReportKey(storage.StatusRemoved, "drop-me")}) { + t.Errorf("removed keys = %v, want the single removed report", removed) + } +} + +// TestAdminRemoveViaDelete checks the DELETE alias behaves like POST .../remove. +func TestAdminRemoveViaDelete(t *testing.T) { + h, _ := adminHarness(t, testToken, "gone") + rec := adminReq(t, h, http.MethodDelete, "/v1/admin/reports/gone", testToken) + if rec.Code != http.StatusOK { + t.Fatalf("DELETE status = %d, want 200 (body=%q)", rec.Code, rec.Body.String()) + } + list := adminReq(t, h, http.MethodGet, "/v1/admin/reports", testToken) + if got := decodeAdmin(t, list).Reports; len(got) != 0 { + t.Errorf("pending after DELETE = %v, want empty", got) + } +} + +// TestAdminRemoveIdempotent proves removing the same report twice still succeeds +// (200), matching the lifecycle Manager's idempotent transition. +func TestAdminRemoveIdempotent(t *testing.T) { + h, _ := adminHarness(t, testToken, "twice") + if rec := adminReq(t, h, http.MethodPost, "/v1/admin/reports/twice/remove", testToken); rec.Code != http.StatusOK { + t.Fatalf("first remove status = %d, want 200", rec.Code) + } + if rec := adminReq(t, h, http.MethodPost, "/v1/admin/reports/twice/remove", testToken); rec.Code != http.StatusOK { + t.Errorf("second remove status = %d, want 200 (idempotent)", rec.Code) + } +} + +// TestAdminRemoveUnknownIs404 covers an id that was never pending. +func TestAdminRemoveUnknownIs404(t *testing.T) { + h, _ := adminHarness(t, testToken, "real") + rec := adminReq(t, h, http.MethodPost, "/v1/admin/reports/ghost/remove", testToken) + if rec.Code != http.StatusNotFound { + t.Fatalf("remove unknown status = %d, want 404 (body=%q)", rec.Code, rec.Body.String()) + } + if got := decodeAdmin(t, rec); got.Status != "error" { + t.Errorf("404 status field = %q, want error", got.Status) + } +} + +// TestAdminMissingTokenIs401 covers a request with no Authorization header. +func TestAdminMissingTokenIs401(t *testing.T) { + h, _ := adminHarness(t, testToken, "id-a") + for _, tc := range []struct { + name, method, target string + }{ + {"list", http.MethodGet, "/v1/admin/reports"}, + {"remove", http.MethodPost, "/v1/admin/reports/id-a/remove"}, + {"delete", http.MethodDelete, "/v1/admin/reports/id-a"}, + } { + t.Run(tc.name, func(t *testing.T) { + rec := adminReq(t, h, tc.method, tc.target, "") + if rec.Code != http.StatusUnauthorized { + t.Fatalf("no-token status = %d, want 401", rec.Code) + } + if wa := rec.Header().Get("WWW-Authenticate"); wa != "Bearer" { + t.Errorf("WWW-Authenticate = %q, want Bearer", wa) + } + }) + } + // A no-op check that the guarded report is still pending (auth blocked the write). + list := adminReq(t, h, http.MethodGet, "/v1/admin/reports", testToken) + if got := decodeAdmin(t, list).Reports; !slices.Contains(got, "id-a") { + t.Errorf("report was mutated despite unauthorized requests: %v", got) + } +} + +// TestAdminInvalidTokenIs401 covers a well-formed header carrying the wrong token. +func TestAdminInvalidTokenIs401(t *testing.T) { + h, _ := adminHarness(t, testToken, "id-a") + rec := adminReq(t, h, http.MethodGet, "/v1/admin/reports", "wrong-token") + if rec.Code != http.StatusUnauthorized { + t.Fatalf("bad-token status = %d, want 401", rec.Code) + } +} + +// TestAdminMalformedAuthHeaderIs401 covers non-Bearer / malformed schemes. +func TestAdminMalformedAuthHeaderIs401(t *testing.T) { + h, _ := adminHarness(t, testToken, "id-a") + for _, hdr := range []string{"Basic abc123", "Bearer", "token " + testToken, testToken} { + req := httptest.NewRequest(http.MethodGet, "/v1/admin/reports", nil) + req.Header.Set("Authorization", hdr) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + if rec.Code != http.StatusUnauthorized { + t.Errorf("Authorization %q: status = %d, want 401", hdr, rec.Code) + } + } +} + +// TestAdminUnsetSecretFailsClosed is the fail-closed requirement: with no server +// secret configured, every request is rejected even when the client presents a +// (any) bearer token, so a missing binding can never leave the API open. +func TestAdminUnsetSecretFailsClosed(t *testing.T) { + h, _ := adminHarness(t, "" /* no server secret */, "id-a") + + // Client presents a plausible token; the server has none, so 401. + if rec := adminReq(t, h, http.MethodGet, "/v1/admin/reports", "any-token"); rec.Code != http.StatusUnauthorized { + t.Errorf("list with unset server secret: status = %d, want 401", rec.Code) + } + if rec := adminReq(t, h, http.MethodPost, "/v1/admin/reports/id-a/remove", "any-token"); rec.Code != http.StatusUnauthorized { + t.Errorf("remove with unset server secret: status = %d, want 401", rec.Code) + } + // The empty string as a token must not authenticate against an unset secret. + if rec := adminReq(t, h, http.MethodGet, "/v1/admin/reports", ""); rec.Code != http.StatusUnauthorized { + t.Errorf("list with no token and unset secret: status = %d, want 401", rec.Code) + } +} + +// TestAdminNilBackendFailsClosed proves New(sink, nil) still fails closed (401), +// never panics, and never allows access. +func TestAdminNilBackendFailsClosed(t *testing.T) { + h := New(nil, nil) + if rec := adminReq(t, h, http.MethodGet, "/v1/admin/reports", "any-token"); rec.Code != http.StatusUnauthorized { + t.Errorf("nil backend list: status = %d, want 401", rec.Code) + } +} + +// TestAdminWrongMethodIs405 checks the mux answers 405 for a wrong method on a +// known admin path, with an Allow header, before auth runs. +func TestAdminWrongMethodIs405(t *testing.T) { + h, _ := adminHarness(t, testToken, "id-a") + for _, tc := range []struct { + name, method, target, allow string + }{ + {"put-list", http.MethodPut, "/v1/admin/reports", http.MethodGet}, + {"delete-list", http.MethodDelete, "/v1/admin/reports", http.MethodGet}, + {"get-remove", http.MethodGet, "/v1/admin/reports/id-a/remove", http.MethodPost}, + {"put-report", http.MethodPut, "/v1/admin/reports/id-a", http.MethodDelete}, + } { + t.Run(tc.name, func(t *testing.T) { + rec := adminReq(t, h, tc.method, tc.target, testToken) + if rec.Code != http.StatusMethodNotAllowed { + t.Fatalf("status = %d, want 405", rec.Code) + } + if allow := rec.Header().Get("Allow"); allow != tc.allow { + t.Errorf("Allow = %q, want %q", allow, tc.allow) + } + }) + } +} diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 16c38bb..12cfe9c 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -20,21 +20,34 @@ const serviceName = "libremail-bug-report-ingest" // New returns an http.Handler serving the ingest Worker's endpoints: // -// GET / -> 200, JSON service/status/message -// GET /healthz -> 200, JSON {"status":"ok"} -// POST /v1/reports -> 202 on accept; 400/413/415/405/503 per the ingest contract +// GET / -> 200, JSON service/status/message +// GET /healthz -> 200, JSON {"status":"ok"} +// POST /v1/reports -> 202 on accept; 400/413/415/405/503 per the ingest contract +// GET /v1/admin/reports -> 200 list of pending report ids (authenticated, #11) +// POST /v1/admin/reports/{id}/remove -> 200 remove a pending report (authenticated, #11) +// DELETE /v1/admin/reports/{id} -> 200 remove a pending report (authenticated alias, #11) // // Any other path returns 404. On the health/hello endpoints any non-GET method -// returns 405; on /v1/reports any non-POST method returns 405 (Allow: POST). +// returns 405; on /v1/reports any non-POST method returns 405 (Allow: POST); on +// the admin routes a wrong method returns 405 (Allow header from the mux). // // sink is the storage backend for accepted reports (scrub + encrypt + R2, #9). // It is injected so the deployed Worker supplies the real R2/Secrets-Store sink // while cmd/devserver and tests supply an in-memory one. A nil sink defaults to // ingest.NopSink, which enforces the full HTTP contract but discards bodies. -func New(sink ingest.Sink) http.Handler { +// +// admin is the maintainer admin API backend (#11): the lifecycle Manager plus the +// shared-secret token, injected the same way. A nil admin registers the admin +// routes but fails every request closed with 401, so the endpoints' shape is +// always present and can never be silently left unauthenticated. +func New(sink ingest.Sink, admin AdminBackend) http.Handler { + if admin == nil { + admin = denyAllBackend{} + } mux := http.NewServeMux() mux.HandleFunc("/healthz", healthz) mux.Handle("/v1/reports", ingest.NewHandler(sink)) + (&adminAPI{backend: admin}).register(mux) mux.HandleFunc("/", root) return mux } diff --git a/internal/handler/handler_test.go b/internal/handler/handler_test.go index 7c4737b..d1c62e6 100644 --- a/internal/handler/handler_test.go +++ b/internal/handler/handler_test.go @@ -15,7 +15,7 @@ func doRequest(t *testing.T, method, target string) *httptest.ResponseRecorder { t.Helper() req := httptest.NewRequest(method, target, nil) rec := httptest.NewRecorder() - New(nil).ServeHTTP(rec, req) + New(nil, nil).ServeHTTP(rec, req) return rec } @@ -78,7 +78,7 @@ func TestUnknownPathReturns404(t *testing.T) { // POST /v1/reports (the #9 storage seam), returning 202 and storing the report. func TestReportsRoutedToInjectedSink(t *testing.T) { sink := &ingest.MemorySink{} - h := New(sink) + h := New(sink, nil) body := `{"appVersion":"1.0.0","platform":"android","report":"boom"}` req := httptest.NewRequest(http.MethodPost, "/v1/reports", strings.NewReader(body)) diff --git a/internal/storage/store.go b/internal/storage/store.go index 67a3376..e7d2e12 100644 --- a/internal/storage/store.go +++ b/internal/storage/store.go @@ -31,6 +31,10 @@ const ( // KeyringBinding is the Cloudflare Secrets Store binding holding the JSON // keyring secret, named per ADR #5. KeyringBinding = "BUGREPORT_ENC_KEYRING" + // AdminTokenBinding is the Cloudflare Secrets Store binding holding the + // maintainer admin API shared secret (#11). Read per request via ReadSecret; + // never logged or echoed. + AdminTokenBinding = "ADMIN_TOKEN" ) // ObjectStore is the seam for the opaque object backend. Implementations only diff --git a/internal/storage/worker_sink_wasm.go b/internal/storage/worker_sink_wasm.go index f80bd96..fbccb63 100644 --- a/internal/storage/worker_sink_wasm.go +++ b/internal/storage/worker_sink_wasm.go @@ -78,6 +78,14 @@ func (s *WorkerSink) loadKeyring() (*crypto.Keyring, error) { return kr, nil } +// ReadSecret reads a Cloudflare Secrets Store secret by binding name, for use +// outside this package (e.g. the Worker admin backend reading AdminTokenBinding, +// #11). Like getSecret it must be called within a request handler, and the value +// must never be logged or echoed. It errors if the binding is unbound or empty. +func ReadSecret(binding string) ([]byte, error) { + return getSecret(binding) +} + // getSecret reads a Cloudflare Secrets Store secret via `await binding.get()`. // The returned value must never be logged or echoed (ADR #5, Key custody). func getSecret(binding string) ([]byte, error) { diff --git a/worker/admin.go b/worker/admin.go new file mode 100644 index 0000000..25926da --- /dev/null +++ b/worker/admin.go @@ -0,0 +1,66 @@ +//go:build js && wasm + +package main + +// workerAdminBackend is the production handler.AdminBackend for the maintainer +// admin API (#11) in the Cloudflare Worker. +// +// The admin secret and the R2 bucket binding are only available inside a request +// on the Workers runtime (the Secrets Store get() is async and per-request; the +// R2 binding resolves from the request context), so — unlike the dev server, +// which injects a ready Manager + token at startup — this backend resolves both +// lazily on each call. The token is read from Secrets Store (AdminTokenBinding) +// and the lifecycle Manager is built over a fresh R2-backed store per operation. +// This mirrors how WorkerSink loads its keyring lazily. +// +// Compiled only into the js/wasm Worker; excluded from host builds and tests. + +import ( + "context" + + "github.com/JMR-dev/LibreMail-Bug-Report-Ingest/internal/lifecycle" + "github.com/JMR-dev/LibreMail-Bug-Report-Ingest/internal/storage" +) + +type workerAdminBackend struct{} + +// AdminToken reads the shared admin secret from Cloudflare Secrets Store. A load +// failure (unbound/empty binding) returns an error, which the handler surfaces as +// 503; the handler independently fails closed (401) on an empty secret value. +func (workerAdminBackend) AdminToken(context.Context) (string, error) { + tok, err := storage.ReadSecret(storage.AdminTokenBinding) + if err != nil { + return "", err + } + return string(tok), nil +} + +// ListPending builds an R2-backed lifecycle Manager for this request and returns +// the pending report ids. +func (workerAdminBackend) ListPending(ctx context.Context) ([]string, error) { + mgr, err := managerForRequest() + if err != nil { + return nil, err + } + return mgr.ListPending(ctx) +} + +// MarkRemoved builds an R2-backed lifecycle Manager for this request and +// transitions the report pending -> removed. +func (workerAdminBackend) MarkRemoved(ctx context.Context, id string) error { + mgr, err := managerForRequest() + if err != nil { + return err + } + return mgr.MarkRemoved(ctx, id) +} + +// managerForRequest resolves the R2 bucket binding (available within a request) +// and wraps it in a lifecycle Manager. +func managerForRequest() (*lifecycle.Manager, error) { + store, err := storage.NewR2Store(storage.BucketBinding) + if err != nil { + return nil, err + } + return lifecycle.New(store), nil +} diff --git a/worker/main.go b/worker/main.go index cdb3b25..b0f654a 100644 --- a/worker/main.go +++ b/worker/main.go @@ -21,5 +21,9 @@ func main() { // The real storage Sink (#9): scrub -> AES-256-GCM encrypt -> R2 put, with the // keyring loaded from Cloudflare Secrets Store on first request. Bindings // (REPORTS_BUCKET, BUGREPORT_ENC_KEYRING) are declared in wrangler.jsonc. - workers.Serve(handler.New(storage.NewWorkerSink())) + // + // The maintainer admin API (#11) is wired with workerAdminBackend, which reads + // the admin shared secret (ADMIN_TOKEN) from Secrets Store and drives the + // lifecycle Manager over R2 per request (see admin.go). + workers.Serve(handler.New(storage.NewWorkerSink(), workerAdminBackend{})) } diff --git a/wrangler.jsonc b/wrangler.jsonc index b228540..d740116 100644 --- a/wrangler.jsonc +++ b/wrangler.jsonc @@ -40,6 +40,19 @@ "binding": "BUGREPORT_ENC_KEYRING", "store_id": "", "secret_name": "bugreport-enc-keyring" + }, + // Maintainer admin API shared secret (issue #11). The Worker reads it per + // request via env.ADMIN_TOKEN.get() (internal/storage.AdminTokenBinding) to + // authenticate GET /v1/admin/reports and POST /v1/admin/reports/{id}/remove + // with a constant-time-compared Bearer token. Fail-closed: if this binding is + // absent or empty the admin routes reject every request (401/503). Replace + // "" with the account's Secrets Store id at deploy time; not needed + // for `pnpm run build` (Wasm compile) or the devserver (which uses ADMIN_TOKEN + // from the environment instead). + { + "binding": "ADMIN_TOKEN", + "store_id": "", + "secret_name": "bugreport-admin-token" } ],