From 675ed230f38b44ee182c93aaecf7e064b80fc543 Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Thu, 30 Jul 2026 21:50:13 +0200 Subject: [PATCH] fix(admin): accept same-origin first-run setup requests (#1280) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(admin): accept same-origin first-run setup requests A freshly generated config.yaml leaves allowed_origins commented out, so the list is empty. The setup handler's CSRF guard assumed "no Origin header means same-origin", but browsers send Origin on same-origin POSTs too — Chrome and Edge always, Firefox since 70. The admin panel's own setup call is one of those POSTs, so every new install hit "cross-origin setup request blocked" and could never create an owner account. The guard now accepts a request whose Origin names the same host:port as the request's own Host header, falling back to the allowlist otherwise. That is what the original comment intended. CSRF protection is unaffected: a cross-site attacker cannot set Origin, the browser does, and a foreign origin still needs an explicit allowlist entry. Scheme is not compared. Nothing in this server derives the external scheme (no r.TLS or X-Forwarded-Proto handling exists anywhere), so a scheme check would reject legitimate requests behind a TLS-terminating proxy. Tests: isSameOrigin table covering port/host/suffix/schemeless/opaque-origin cases, plus two handler-level tests pinning both halves — same-origin succeeds against an empty allowlist, a foreign origin still 403s and creates no user. Co-Authored-By: Claude Opus 5 (1M context) * feat(identity): implement identity keypair caching and error handling --------- Co-authored-by: Claude Opus 5 (1M context) --- Client/tauri-client/src/lib/identity.ts | 58 +++++++++++- .../tauri-client/tests/unit/identity.test.ts | 80 ++++++++++++++++- Server/admin/setup_handler.go | 35 +++++++- Server/admin/setup_handler_test.go | 57 ++++++++++++ Server/admin/setup_origin_test.go | 88 +++++++++++++++++++ 5 files changed, 309 insertions(+), 9 deletions(-) diff --git a/Client/tauri-client/src/lib/identity.ts b/Client/tauri-client/src/lib/identity.ts index a89fc510..f85c3663 100644 --- a/Client/tauri-client/src/lib/identity.ts +++ b/Client/tauri-client/src/lib/identity.ts @@ -119,12 +119,53 @@ export async function getIdentityPin(host: string, userId: string): Promise>(); + /** * Load this host's identity keypair from the keyring, generating and saving a * fresh one on first login (or when the stored blob is corrupt). In non-Tauri * environments the keypair is in-memory only (not persisted). + * + * Stable for the lifetime of the process: repeat callers get the same keypair + * even when the keyring is unavailable (see `identityKeyPairCache`). */ -export async function getOrCreateIdentityKeyPair(host: string): Promise { +export function getOrCreateIdentityKeyPair(host: string): Promise { + let pending = identityKeyPairCache.get(host); + if (pending === undefined) { + // A rejected load must not be cached, or the host is poisoned for the + // rest of the session; drop it so the next caller can retry. + pending = loadOrGenerateIdentityKeyPair(host).catch((err: unknown) => { + identityKeyPairCache.delete(host); + throw err; + }); + identityKeyPairCache.set(host, pending); + } + return pending; +} + +/** Test-only: drop the per-host keypair memo so each case starts clean. */ +export function resetIdentityKeyPairCache(): void { + identityKeyPairCache.clear(); +} + +async function loadOrGenerateIdentityKeyPair(host: string): Promise { const stored = await loadIdentityKey(host); if (stored) { try { @@ -134,7 +175,20 @@ export async function getOrCreateIdentityKeyPair(host: string): Promise ({ invokeMock: vi.fn() })); +const { invokeMock, logMock } = vi.hoisted(() => ({ + invokeMock: vi.fn(), + logMock: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }, +})); vi.mock("@tauri-apps/api/core", () => ({ invoke: invokeMock })); -vi.mock("@lib/logger", () => ({ - createLogger: () => ({ debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }), -})); +vi.mock("@lib/logger", () => ({ createLogger: () => logMock })); import { saveIdentityKey, @@ -14,6 +15,7 @@ import { storeIdentityPin, getIdentityPin, getOrCreateIdentityKeyPair, + resetIdentityKeyPairCache, publishIdentityKey, ensureIdentityKeyPublished, } from "@lib/identity"; @@ -21,6 +23,11 @@ import { generateIdentityKeyPair, exportPublicKey } from "@lib/e2eeCrypto"; beforeEach(() => { invokeMock.mockReset(); + logMock.error.mockReset(); + logMock.warn.mockReset(); + // The keypair memo is process-wide by design; without this, one case's + // cached pair would satisfy the next case's keyring assertions. + resetIdentityKeyPairCache(); }); describe("identity keyring wrappers", () => { @@ -110,6 +117,9 @@ describe("getOrCreateIdentityKeyPair", () => { const firstPub = await exportPublicKey(first.publicKey); // Second login: keyring returns the saved blob → same public key, no save. + // Drop the memo first, or this asserts nothing about the keyring — a new + // login is a new process, which is exactly what the reload path is for. + resetIdentityKeyPairCache(); invokeMock.mockReset(); invokeMock.mockImplementation((cmd: string) => { if (cmd === "load_identity_key") return Promise.resolve(savedBlob); @@ -129,6 +139,68 @@ describe("getOrCreateIdentityKeyPair", () => { expect(kp.publicKey).toBeDefined(); expect(invokeMock.mock.calls.some((c) => c[0] === "save_identity_key")).toBe(true); }); + + it("hands every caller the same keypair when the keyring never persists", async () => { + // A store that accepts the write and returns nothing on the next read. + // Before the memo, the ready hook (publishes the public half) and the voice + // session (signs announces with the private half) each generated their own + // keypair here — so the published key was never the key that signed, and + // peers rejected every announce as a forged signature. + invokeMock.mockImplementation((cmd: string) => { + if (cmd === "load_identity_key") return Promise.resolve(null); + return Promise.resolve(undefined); + }); + + const [publishPair, signingPair] = await Promise.all([ + getOrCreateIdentityKeyPair("chat.example"), + getOrCreateIdentityKeyPair("chat.example"), + ]); + const laterPair = await getOrCreateIdentityKeyPair("chat.example"); + + expect(signingPair).toBe(publishPair); + expect(laterPair).toBe(publishPair); + // One generation, not one per caller. + expect(invokeMock.mock.calls.filter((c) => c[0] === "save_identity_key")).toHaveLength(1); + }); + + it("keeps the memo per host", async () => { + invokeMock.mockImplementation((cmd: string) => { + if (cmd === "load_identity_key") return Promise.resolve(null); + return Promise.resolve(undefined); + }); + const a = await getOrCreateIdentityKeyPair("chat.example"); + const b = await getOrCreateIdentityKeyPair("other.example"); + expect(await exportPublicKey(b.publicKey)).not.toBe(await exportPublicKey(a.publicKey)); + }); + + it("reports a credential store that accepts the write but drops the value", async () => { + invokeMock.mockImplementation((cmd: string) => { + if (cmd === "load_identity_key") return Promise.resolve(null); + return Promise.resolve(undefined); // save_identity_key "succeeds" + }); + + await getOrCreateIdentityKeyPair("chat.example"); + + expect(logMock.error).toHaveBeenCalledWith(expect.stringContaining("did not persist"), { + host: "chat.example", + }); + }); + + it("stays quiet when the store round-trips the key", async () => { + let savedBlob: string | undefined; + invokeMock.mockImplementation((cmd: string, args?: Record) => { + if (cmd === "load_identity_key") return Promise.resolve(savedBlob ?? null); + if (cmd === "save_identity_key") { + savedBlob = args!.key as string; + return Promise.resolve(undefined); + } + return Promise.resolve(undefined); + }); + + await getOrCreateIdentityKeyPair("chat.example"); + + expect(logMock.error).not.toHaveBeenCalled(); + }); }); describe("publishIdentityKey", () => { diff --git a/Server/admin/setup_handler.go b/Server/admin/setup_handler.go index db342cca..09e3cd5b 100644 --- a/Server/admin/setup_handler.go +++ b/Server/admin/setup_handler.go @@ -7,6 +7,7 @@ import ( "log/slog" "net" "net/http" + "net/url" "strings" "time" @@ -57,10 +58,10 @@ func handleSetupStatus(database *db.DB) http.HandlerFunc { func handleSetup(database *db.DB, limiter *auth.RateLimiter, allowedOrigins []string) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { // CSRF protection: reject cross-origin requests (BUG-097). - // If Origin is present and doesn't match allowed origins, deny. - // No Origin header = same-origin or non-browser client (allow). + // A request is accepted when it is same-origin, or when its Origin is + // explicitly allowlisted. Absent Origin = non-browser client (allow). if origin := r.Header.Get("Origin"); origin != "" { - if !isSetupOriginAllowed(origin, allowedOrigins) { + if !isSameOrigin(origin, r.Host) && !isSetupOriginAllowed(origin, allowedOrigins) { writeErr(w, http.StatusForbidden, "FORBIDDEN", "cross-origin setup request blocked") return } @@ -165,6 +166,34 @@ func handleSetup(database *db.DB, limiter *auth.RateLimiter, allowedOrigins []st } } +// isSameOrigin reports whether a browser-supplied Origin names this same +// server, by comparing its host:port against the request's Host header. +// +// Browsers send Origin on same-origin POSTs too (Chrome and Edge always, +// Firefox since 70), so the admin panel's own first-run setup call arrives +// carrying one. Without this check it is measured against allowed_origins, +// which is empty in a freshly generated config — so setup failed with +// "cross-origin setup request blocked" on every new install. +// +// Scheme is deliberately not compared. Nothing in this server derives the +// external scheme (there is no r.TLS or X-Forwarded-Proto handling anywhere), +// so a TLS-terminating proxy in front would make a scheme check reject +// legitimate requests. Matching host:port is enough: forging it requires +// already serving content on this exact host and port, at which point the +// origin is not the attacker's to borrow. Cross-site attackers cannot set +// Origin at all — the browser does. +func isSameOrigin(origin, host string) bool { + if host == "" { + return false + } + u, err := url.Parse(origin) + // Require a scheme so a schemeless "//host:port" cannot pass as same-origin. + if err != nil || u.Scheme == "" || u.Host == "" { + return false + } + return strings.EqualFold(u.Host, host) +} + // isSetupOriginAllowed checks if the given origin is permitted by the // configured allowed_origins list. Wildcard "*" allows any origin. // An empty list denies all cross-origin requests (safe default). diff --git a/Server/admin/setup_handler_test.go b/Server/admin/setup_handler_test.go index eeb19317..8d98143b 100644 --- a/Server/admin/setup_handler_test.go +++ b/Server/admin/setup_handler_test.go @@ -1,10 +1,12 @@ package admin_test import ( + "bytes" "context" "encoding/json" "fmt" "net/http" + "net/http/httptest" "testing" "github.com/owncord/server/admin" @@ -194,3 +196,58 @@ func TestSetup_ConcurrentRace(t *testing.T) { t.Errorf("user count = %d, want 1", count) } } + +// A freshly generated config leaves allowed_origins empty, and browsers send an +// Origin header on same-origin POSTs. Before isSameOrigin existed, that pairing +// made first-run setup fail on every new install with "cross-origin setup +// request blocked". These two tests pin both halves: same-origin gets through +// on an empty allowlist, and a foreign origin still does not. +func TestSetup_SameOriginAllowedWithEmptyAllowlist(t *testing.T) { + database := openAdminTestDB(t) + handler := admin.NewAdminAPI(database, "1.0.0", nil, nil, nil, nil, nil, newTestModService(database)) + + body, err := json.Marshal(map[string]string{"username": "owner", "password": "correct-horse"}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + req := httptest.NewRequest("POST", "/setup", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + // httptest.NewRequest sets Host to example.com; the browser would send the + // matching Origin for a page served from this same server. + req.Header.Set("Origin", "https://"+req.Host) + + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + if rr.Code != http.StatusCreated { + t.Fatalf("POST /setup with same-origin Origin = %d, want 201; body: %s", rr.Code, rr.Body.String()) + } +} + +func TestSetup_ForeignOriginStillBlocked(t *testing.T) { + database := openAdminTestDB(t) + handler := admin.NewAdminAPI(database, "1.0.0", nil, nil, nil, nil, nil, newTestModService(database)) + + body, err := json.Marshal(map[string]string{"username": "owner", "password": "correct-horse"}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + req := httptest.NewRequest("POST", "/setup", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Origin", "https://evil.example") + + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + if rr.Code != http.StatusForbidden { + t.Fatalf("POST /setup from a foreign origin = %d, want 403", rr.Code) + } + + count, err := database.UserCount(context.Background()) + if err != nil { + t.Fatalf("UserCount: %v", err) + } + if count != 0 { + t.Fatalf("owner account created from a foreign origin: user count = %d, want 0", count) + } +} diff --git a/Server/admin/setup_origin_test.go b/Server/admin/setup_origin_test.go index 78150c36..a1ed1338 100644 --- a/Server/admin/setup_origin_test.go +++ b/Server/admin/setup_origin_test.go @@ -90,3 +90,91 @@ func TestIsSetupOriginAllowed(t *testing.T) { }) } } + +// isSameOrigin is what lets the admin panel's own setup call through on a +// default config, where allowed_origins is empty. It must not become a hole: +// only an Origin naming this exact host:port may pass. +func TestIsSameOrigin(t *testing.T) { + tests := []struct { + name string + origin string + host string + want bool + }{ + { + name: "admin panel on the default port is same-origin", + origin: "https://localhost:8443", + host: "localhost:8443", + want: true, + }, + { + name: "host comparison is case-insensitive", + origin: "https://LocalHost:8443", + host: "localhost:8443", + want: true, + }, + { + name: "plain http against a proxied host still matches", + origin: "http://chat.example", + host: "chat.example", + want: true, + }, + { + name: "a different host is not same-origin", + origin: "https://evil.example", + host: "localhost:8443", + want: false, + }, + { + name: "a different port is not same-origin", + origin: "https://localhost:9999", + host: "localhost:8443", + want: false, + }, + { + name: "loopback by IP does not match loopback by name", + origin: "https://127.0.0.1:8443", + host: "localhost:8443", + want: false, + }, + { + name: "a suffix of the host is not same-origin", + origin: "https://evil-localhost:8443", + host: "localhost:8443", + want: false, + }, + { + name: "schemeless origin cannot pass as same-origin", + origin: "//localhost:8443", + host: "localhost:8443", + want: false, + }, + { + name: "opaque origin (sandboxed iframe) is denied", + origin: "null", + host: "localhost:8443", + want: false, + }, + { + name: "empty origin is denied", + origin: "", + host: "localhost:8443", + want: false, + }, + { + name: "empty host denies rather than matching an empty origin host", + origin: "https://localhost:8443", + host: "", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isSameOrigin(tt.origin, tt.host); got != tt.want { + t.Errorf("isSameOrigin(%q, %q) = %v, want %v", + tt.origin, tt.host, got, tt.want) + } + }) + } +}