[security] Close tg12 auth-bypass chain #249, #254, #255 (#263)

External audit by @tg12 found three coupled vulnerabilities in the
Next.js admin-auth surface that together let any webpage the operator
visits trigger arbitrary privileged backend calls:

  #249/#254 — Cross-origin webpages can have process.env.ADMIN_KEY
              injected into their forwarded backend requests just by
              issuing fetch('http://localhost:3000/api/wormhole/...')
              from a browser tab the operator has open. Full
              identity-takeover CSRF.

  #255      — When ADMIN_KEY is unset on the server (the default in
              .env.example), the admin session route fell through to
              GET /api/settings/privacy-profile to "verify" the user-
              supplied key. That endpoint is public; it always returns
              200 for any X-Admin-Key value. So arbitrary attacker
              keys minted full admin session cookies on default
              installs.

Both fixes preserve every legitimate UX path. Origin-header gating is
transparent to browser tabs on the dashboard's own host, transparent
to Tauri/native shells (no Origin), and transparent to server-to-
server callers (no Origin). Only cross-origin browser fetches with a
foreign Origin lose the injection.

  frontend/src/app/api/[...path]/route.ts
    Adds isSameOriginOrNonBrowser() — checks the Origin header against
    the request's own Host. Allow if no Origin (native/server-to-
    server), allow if Origin host == Host host (same-origin), reject
    otherwise. The admin-key injection now requires EITHER a valid
    session cookie (auth) OR same-origin-or-non-browser (CSRF guard).

  frontend/src/app/api/admin/session/route.ts
    verifyAdminKey() simplified to local-only string comparison. When
    ADMIN_KEY is configured, the supplied key must match exactly.
    When ADMIN_KEY is unset, minting is refused entirely with a clear
    message pointing the operator at the backend's auto-trust-loopback
    behavior (SHADOWBROKER_TRUST_DOCKER_BRIDGE_LOCAL_OPERATOR=1, the
    Docker default — local users keep working without a session).

    The previous round-trip to /api/settings/privacy-profile was both
    the source of the bug AND useless on its own merits (the endpoint
    is public). Removing it makes the validation honest about what
    it's checking.

Tests:
  frontend/src/__tests__/proxy/proxyAuthBypassChain.test.ts (new, 12)
    Cross-origin fetch to sensitive route → no admin-key injection
    Cross-origin POST to sensitive route → no admin-key injection
    Same-origin fetch → admin-key injection works
    No-Origin (server-to-server / native) → admin-key injection works
    Valid session cookie on cross-origin → cookie auth wins
    Malformed Origin → conservative reject
    Non-sensitive routes unaffected
    Mint with ADMIN_KEY unset → refused (no fetch happens)
    Empty key → 400
    Mint with matching ADMIN_KEY → success
    Mint with mismatched key → 403
    Mint never round-trips to the backend (local-only validation)

  frontend/src/__tests__/desktop/adminSessionBoundary.test.ts (updated)
    Three tests updated to reflect the new local-only validation
    contract. The previous tests asserted fetchMock.toHaveBeenCalled
    which validated the now-removed (and broken) backend round-trip.

Full frontend suite: 707 passed, 72 files. No regressions.

Credit: @tg12 for the report. The cross-origin CSRF angle was
non-obvious — they specifically called out that the proxy's
admin-key injection was an open door for any page running in the
operator's browser, which is exactly the right framing.

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
Shadowbroker
2026-05-20 20:59:40 -06:00
committed by GitHub
co-authored by Claude Opus 4.7
parent 729ea78cb2
commit bcc2d036b3
4 changed files with 446 additions and 56 deletions
@@ -45,12 +45,12 @@ describe('admin/session boundary hardening', () => {
});
it('accepts a verified admin key and reports the minted session as present', async () => {
const fetchMock = vi.fn().mockResolvedValue(
new Response(JSON.stringify({ ok: true }), {
status: 200,
headers: { 'Content-Type': 'application/json' },
}),
);
// Issue #255 fix: the route no longer round-trips to the backend
// to "verify" the key (the previous implementation called a public
// endpoint that always returned 200, so any key was accepted when
// ADMIN_KEY was unset). Local string comparison is the only
// validation, so we don't mock fetch and don't assert it was called.
const fetchMock = vi.fn();
vi.stubGlobal('fetch', fetchMock);
const req = new NextRequest('http://localhost/api/admin/session', {
@@ -65,7 +65,8 @@ describe('admin/session boundary hardening', () => {
expect(res.status).toBe(200);
expect(cookie).toContain('sb_admin_session=');
expect(res.headers.get('cache-control')).toContain('no-store');
expect(fetchMock).toHaveBeenCalledTimes(1);
// Validation is local-only — no backend round-trip should happen.
expect(fetchMock).not.toHaveBeenCalled();
const getReq = new NextRequest('http://localhost/api/admin/session', {
method: 'GET',
@@ -88,12 +89,8 @@ describe('admin/session boundary hardening', () => {
});
it('invalidates the previous admin session token when a new one is minted', async () => {
const fetchMock = vi.fn().mockResolvedValue(
new Response(JSON.stringify({ ok: true }), {
status: 200,
headers: { 'Content-Type': 'application/json' },
}),
);
// Issue #255 fix: no backend round-trip. Validation is local-only.
const fetchMock = vi.fn();
vi.stubGlobal('fetch', fetchMock);
const firstReq = new NextRequest('http://localhost/api/admin/session', {
@@ -135,21 +132,25 @@ describe('admin/session boundary hardening', () => {
);
const newBody = await newSessionCheck.json();
expect(newBody.hasSession).toBe(true);
expect(fetchMock).toHaveBeenCalledTimes(2);
// Local validation only — backend should not be called during minting.
expect(fetchMock).not.toHaveBeenCalled();
});
it('rejects session minting when frontend admin key is set but backend has no configured admin key', async () => {
const fetchMock = vi.fn().mockResolvedValue(
new Response(JSON.stringify({ detail: 'Forbidden — admin key not configured' }), {
status: 403,
headers: { 'Content-Type': 'application/json' },
}),
);
it('refuses session minting when frontend ADMIN_KEY env var is unset (#255)', async () => {
// Issue #255 (tg12): previously, when ADMIN_KEY was unset the route
// fell through to a public backend endpoint that always returned
// 200, so any user-supplied key minted a full admin session. The
// fix is to refuse minting entirely when ADMIN_KEY is unconfigured
// and surface a clear message pointing the operator at the
// backend's auto-trust-loopback behavior.
process.env.ADMIN_KEY = '';
const fetchMock = vi.fn();
vi.stubGlobal('fetch', fetchMock);
const req = new NextRequest('http://localhost/api/admin/session', {
method: 'POST',
body: JSON.stringify({ adminKey: 'top-secret' }),
body: JSON.stringify({ adminKey: 'any-key-an-attacker-supplies' }),
headers: { 'Content-Type': 'application/json' },
});
@@ -158,8 +159,11 @@ describe('admin/session boundary hardening', () => {
expect(res.status).toBe(403);
expect(body.ok).toBe(false);
expect(body.detail).toBe('Forbidden — admin key not configured');
expect(String(body.detail)).toMatch(/no admin key configured/i);
expect(res.headers.get('set-cookie')).toBeNull();
// Crucially: no backend round-trip happens. The previous broken
// verifyAgainstBackend() call must NOT be re-introduced.
expect(fetchMock).not.toHaveBeenCalled();
});
it('does not forward raw x-admin-key headers through the sensitive proxy path', async () => {