From 43a595905b1474908d96260f70a49dbf16812e32 Mon Sep 17 00:00:00 2001 From: Anso Date: Fri, 24 Apr 2026 10:20:08 -0400 Subject: [PATCH] fix(backend): restore remote proxy mount order before local routers (#747) The index.ts refactor inverted the proxy mount order. The pre-refactor monolith mounted `app.use('/api/', remoteNodeProxy)` before any inline route, so remote-nodeId requests short-circuited into the proxy. After the refactor the proxy was registered after every per-group router, so Express matched local routers first and remote-nodeId requests were silently handled with the control instance's local state (e.g. GET /api/stacks with x-node-id= returned local stacks rather than the remote's). Fix moves createRemoteProxyMiddleware() between enforceApiTokenScope and the first per-group router, matching middleware-order.md step 13 and restoring pre-refactor behavior. PROXY_EXEMPT_PREFIXES continues to cover gateway-level paths (auth, nodes, license, fleet, webhooks, meta) that must stay local even when x-node-id targets a remote. Add four regression guards that would have caught this: - json-parser-bypass.test.ts: asserts conditionalJsonParser leaves the request stream intact on proxy-eligible paths so http-proxy can pipe the raw body to the upstream; spins up a local echo server and verifies the bytes arrive. - proxy-mount-order.test.ts: asserts a remote-nodeId GET short-circuits into the proxy (502 from unreachable upstream) instead of matching a local router (200 from local state). - upgrade-order.test.ts: pins WebSocket dispatch order by observing handler-specific side effects for notifications, remote forwarder, logs, and pilot tunnel. - remote-console-session.test.ts: asserts the HTTP console-token route mints a JWT with the same claim shape as the shared mintConsoleSession helper, so gateway and WS forwarder tokens remain interchangeable. Full suite: 73 files, 1,358 tests, all passing. --- .../src/__tests__/json-parser-bypass.test.ts | 111 ++++++++++++ .../src/__tests__/proxy-mount-order.test.ts | 91 ++++++++++ .../__tests__/remote-console-session.test.ts | 97 +++++++++++ backend/src/__tests__/upgrade-order.test.ts | 158 ++++++++++++++++++ backend/src/index.ts | 13 +- 5 files changed, 465 insertions(+), 5 deletions(-) create mode 100644 backend/src/__tests__/json-parser-bypass.test.ts create mode 100644 backend/src/__tests__/proxy-mount-order.test.ts create mode 100644 backend/src/__tests__/remote-console-session.test.ts create mode 100644 backend/src/__tests__/upgrade-order.test.ts diff --git a/backend/src/__tests__/json-parser-bypass.test.ts b/backend/src/__tests__/json-parser-bypass.test.ts new file mode 100644 index 00000000..bfaebb7e --- /dev/null +++ b/backend/src/__tests__/json-parser-bypass.test.ts @@ -0,0 +1,111 @@ +/** + * Regression guard for the `conditionalJsonParser` remote-proxy bypass. + * + * When a request targets a remote node via `x-node-id` and the path is NOT in + * `PROXY_EXEMPT_PREFIXES`, the JSON parser must leave the request stream + * untouched so `http-proxy` can pipe the raw body to the upstream Sencho + * instance. If the parser runs, `req.pipe(proxyReq)` errors with + * `ERR_HTTP_STREAM_WRITE_AFTER_END` and the remote never sees the body. + * + * This test spins up a tiny HTTP echo server, seeds a remote node pointing at + * it, and POSTs a JSON body through the proxy. The echo server asserts the + * bytes arrived intact. A second case confirms that exempt paths are handled + * locally (upstream receives nothing). + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import request from 'supertest'; +import http from 'http'; +import type { AddressInfo } from 'net'; +import jwt from 'jsonwebtoken'; +import { setupTestDb, cleanupTestDb, TEST_USERNAME, TEST_JWT_SECRET } from './helpers/setupTestDb'; + +describe('conditionalJsonParser remote-proxy bypass', () => { + let tmpDir: string; + let app: import('express').Express; + let upstream: http.Server; + let upstreamUrl: string; + let lastUpstreamBody: Buffer | null = null; + let lastUpstreamAuth: string | null = null; + let authHeader: string; + let remoteNodeId: number; + + beforeAll(async () => { + tmpDir = await setupTestDb(); + + upstream = http.createServer((req, res) => { + const chunks: Buffer[] = []; + req.on('data', (c: Buffer) => chunks.push(c)); + req.on('end', () => { + lastUpstreamBody = Buffer.concat(chunks); + lastUpstreamAuth = (req.headers['authorization'] as string | undefined) ?? null; + res.statusCode = 200; + res.setHeader('content-type', 'application/json'); + res.end('{"ok":true}'); + }); + req.on('error', () => { + if (!res.headersSent) { + res.statusCode = 500; + res.end(); + } + }); + }); + await new Promise((resolve) => upstream.listen(0, '127.0.0.1', resolve)); + const addr = upstream.address() as AddressInfo; + upstreamUrl = `http://127.0.0.1:${addr.port}`; + + ({ app } = await import('../index')); + + const { DatabaseService } = await import('../services/DatabaseService'); + remoteNodeId = DatabaseService.getInstance().addNode({ + name: 'bypass-test-remote', + type: 'remote', + compose_dir: '/tmp', + is_default: false, + api_url: upstreamUrl, + api_token: 'bypass-test-token', + }); + + const token = jwt.sign({ username: TEST_USERNAME }, TEST_JWT_SECRET, { expiresIn: '1m' }); + authHeader = `Bearer ${token}`; + }); + + afterAll(async () => { + await new Promise((resolve) => upstream.close(() => resolve())); + cleanupTestDb(tmpDir); + }); + + it('forwards the raw request body to the remote for proxy-eligible paths', async () => { + lastUpstreamBody = null; + lastUpstreamAuth = null; + + const payload = { name: 'parser-bypass-stack', content: 'services:\n web:\n image: nginx' }; + + const res = await request(app) + .post('/api/stacks') + .set('Authorization', authHeader) + .set('x-node-id', String(remoteNodeId)) + .set('Content-Type', 'application/json') + .send(payload); + + expect(res.status).toBe(200); + expect(lastUpstreamBody).not.toBeNull(); + expect(lastUpstreamBody!.length).toBeGreaterThan(0); + const parsed = JSON.parse(lastUpstreamBody!.toString('utf-8')); + expect(parsed).toEqual(payload); + expect(lastUpstreamAuth).toBe('Bearer bypass-test-token'); + }); + + it('handles proxy-exempt paths locally (upstream receives nothing)', async () => { + lastUpstreamBody = null; + lastUpstreamAuth = null; + + const res = await request(app) + .get(`/api/nodes/${remoteNodeId}`) + .set('Authorization', authHeader) + .set('x-node-id', String(remoteNodeId)); + + expect(lastUpstreamBody).toBeNull(); + expect(lastUpstreamAuth).toBeNull(); + expect([200, 404]).toContain(res.status); + }); +}); diff --git a/backend/src/__tests__/proxy-mount-order.test.ts b/backend/src/__tests__/proxy-mount-order.test.ts new file mode 100644 index 00000000..28684bb7 --- /dev/null +++ b/backend/src/__tests__/proxy-mount-order.test.ts @@ -0,0 +1,91 @@ +/** + * Regression guard for `createRemoteProxyMiddleware` mount order. + * + * `index.ts` mounts every local `/api/` router before the remote proxy + * at step 13 of the canonical middleware order. A remote-`nodeId` request + * must short-circuit into the proxy rather than match a local router. If the + * order were reversed, a GET `/api/stacks` for a remote node would return the + * control instance's local stack list instead of the remote's. + * + * The test seeds a remote node whose `api_url` points at a closed loopback + * port. The proxy forwards, the connection fails, and the middleware + * responds with a 502. A local handler on the same path would return 200 + * (or 404 / 400) with a JSON body, never 502. That distinguishing response + * proves the proxy intercepted first. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import request from 'supertest'; +import jwt from 'jsonwebtoken'; +import { setupTestDb, cleanupTestDb, TEST_USERNAME, TEST_JWT_SECRET } from './helpers/setupTestDb'; + +describe('remote proxy mount order', () => { + let tmpDir: string; + let app: import('express').Express; + let authHeader: string; + let remoteNodeId: number; + + beforeAll(async () => { + tmpDir = await setupTestDb(); + ({ app } = await import('../index')); + + const { DatabaseService } = await import('../services/DatabaseService'); + // 127.0.0.1:1 is a reserved port that no process binds; TCP connect + // fails immediately with ECONNREFUSED. The proxy surfaces that as 502. + remoteNodeId = DatabaseService.getInstance().addNode({ + name: 'mount-order-remote', + type: 'remote', + compose_dir: '/tmp', + is_default: false, + api_url: 'http://127.0.0.1:1', + api_token: 'mount-order-token', + }); + + const token = jwt.sign({ username: TEST_USERNAME }, TEST_JWT_SECRET, { expiresIn: '1m' }); + authHeader = `Bearer ${token}`; + }); + + afterAll(() => { + cleanupTestDb(tmpDir); + }); + + it('short-circuits remote-nodeId requests into the proxy before local routers match', async () => { + const res = await request(app) + .get('/api/stacks') + .set('Authorization', authHeader) + .set('x-node-id', String(remoteNodeId)); + + // 502: the proxy caught the request and the unreachable upstream reported + // the connection refusal. If a local router had matched first, we would + // have seen a 200 with a JSON array of local stacks. + expect(res.status).toBe(502); + // `x-sencho-proxy` is attached by the proxy's proxyRes callback, which + // only fires when the upstream produced a response. The unreachable + // upstream never does; absence here is consistent with a proxy error. + expect(res.headers['x-sencho-proxy']).toBeUndefined(); + expect(res.body?.error).toMatch(/unreachable/i); + }); + + it('routes local requests (no x-node-id) to the local handler', async () => { + const res = await request(app) + .get('/api/stacks') + .set('Authorization', authHeader); + + // Local handler responds with a stack list (200) or, in CI without docker, + // a 500 from DockerController. Anything other than 502 proves the local + // router matched instead of the proxy. + expect(res.status).not.toBe(502); + }); + + it('handles proxy-exempt paths locally even when x-node-id targets a remote', async () => { + // /api/nodes/:id is in PROXY_EXEMPT_PREFIXES. The proxy must never catch + // gateway-level concerns; otherwise a user whose default node is remote + // could never manage the node registry itself. + const res = await request(app) + .get(`/api/nodes/${remoteNodeId}`) + .set('Authorization', authHeader) + .set('x-node-id', String(remoteNodeId)); + + expect(res.status).not.toBe(502); + expect([200, 404]).toContain(res.status); + }); +}); diff --git a/backend/src/__tests__/remote-console-session.test.ts b/backend/src/__tests__/remote-console-session.test.ts new file mode 100644 index 00000000..8402d059 --- /dev/null +++ b/backend/src/__tests__/remote-console-session.test.ts @@ -0,0 +1,97 @@ +/** + * Regression guard for `console_session` JWT parity. + * + * A gateway that forwards an interactive WebSocket (host console or container + * exec) to a remote node calls the remote's `POST /api/system/console-token` + * endpoint and forwards the returned token in an `Authorization: Bearer` + * header during the upgrade. Meanwhile the HTTP route `/api/system/console- + * token` is what the gateway calls to mint that same token on its own remote. + * Both mint calls go through `helpers/consoleSession.ts::mintConsoleSession`. + * + * This test guards against future drift between the HTTP route and the + * helper: if somebody changes the route to mint a different claim shape, + * the remote's upgrade handler would reject one set of tokens and the + * product would silently lose remote-console support. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import request from 'supertest'; +import jwt from 'jsonwebtoken'; +import { setupTestDb, cleanupTestDb, TEST_USERNAME, TEST_JWT_SECRET, loginAsTestAdmin } from './helpers/setupTestDb'; +import { mintConsoleSession } from '../helpers/consoleSession'; + +describe('console_session token parity (HTTP route vs mint helper)', () => { + let tmpDir: string; + let app: import('express').Express; + let adminCookie: string; + + beforeAll(async () => { + tmpDir = await setupTestDb(); + ({ app } = await import('../index')); + adminCookie = await loginAsTestAdmin(app); + + // POST /api/system/console-token is Admiral-gated. Seed an active Admiral + // license so the parity assertion can observe the token the route returns. + // The license_last_validated fallback is skipped when the state key is + // absent, so we only need the two keys that drive requireAdmiral. + const { DatabaseService } = await import('../services/DatabaseService'); + DatabaseService.getInstance().setSystemState('license_status', 'active'); + DatabaseService.getInstance().setSystemState('license_variant_type', 'admiral'); + }); + + afterAll(() => { + cleanupTestDb(tmpDir); + }); + + it('POST /api/system/console-token produces a token with the same shape as mintConsoleSession()', async () => { + const directToken = mintConsoleSession(); + const directDecoded = jwt.verify(directToken, TEST_JWT_SECRET) as Record; + + const res = await request(app) + .post('/api/system/console-token') + .set('Cookie', adminCookie); + expect(res.status).toBe(200); + expect(typeof res.body.token).toBe('string'); + + const routeDecoded = jwt.verify(res.body.token, TEST_JWT_SECRET) as Record; + + // Identical scope so the remote's upgrade handler treats both the same. + expect(routeDecoded.scope).toBe('console_session'); + expect(directDecoded.scope).toBe('console_session'); + + // Same claim keys: if somebody adds or drops a claim on one path but not + // the other, remote upgrade behavior will diverge. + expect(Object.keys(routeDecoded).sort()).toEqual(Object.keys(directDecoded).sort()); + + // Same short TTL (60 seconds, plus or minus a second for test scheduling). + const directTtl = (directDecoded.exp as number) - (directDecoded.iat as number); + const routeTtl = (routeDecoded.exp as number) - (routeDecoded.iat as number); + expect(directTtl).toBe(60); + expect(routeTtl).toBe(60); + }); + + it('rejects non-admin callers of POST /api/system/console-token', async () => { + // Build a valid session JWT for a non-admin user and confirm the route + // is still admin-gated (regression: easy to drop the requireAdmin check + // when tier gates are refactored). + const viewerToken = jwt.sign({ username: 'nobody-exists' }, TEST_JWT_SECRET, { expiresIn: '1m' }); + const res = await request(app) + .post('/api/system/console-token') + .set('Authorization', `Bearer ${viewerToken}`); + // Either 401 (user not found) or 403 (role check) is acceptable; the + // critical invariant is "not 200 for a non-admin". + expect(res.status).not.toBe(200); + expect([401, 403]).toContain(res.status); + }); + + it('rejects API-token callers of POST /api/system/console-token (rejectApiTokenScope)', async () => { + // Mint a JWT that claims the api_token scope but is not backed by a real + // row in the database. The authMiddleware rejects this at the api_token + // branch before the route handler runs, which is the behavior we want: + // an API token should never be allowed to mint a console session. + const fakeApiToken = jwt.sign({ scope: 'api_token', username: TEST_USERNAME }, TEST_JWT_SECRET, { expiresIn: '1m' }); + const res = await request(app) + .post('/api/system/console-token') + .set('Authorization', `Bearer ${fakeApiToken}`); + expect(res.status).toBe(401); + }); +}); diff --git a/backend/src/__tests__/upgrade-order.test.ts b/backend/src/__tests__/upgrade-order.test.ts new file mode 100644 index 00000000..21f8db49 --- /dev/null +++ b/backend/src/__tests__/upgrade-order.test.ts @@ -0,0 +1,158 @@ +/** + * Regression guard for WebSocket upgrade dispatch order. + * + * `websocket/upgradeHandler.ts` dispatches upgrades in a first-match-wins + * ladder: pilot tunnel, then auth, then API-token scope gate, then + * notifications (local-only), then remote forwarder, then stack logs, then + * host console, then generic. Reordering the ladder silently breaks several + * product paths. This test pins each position by observing behavior that + * could only originate from the expected handler. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import WebSocket from 'ws'; +import jwt from 'jsonwebtoken'; +import type { AddressInfo } from 'net'; +import { setupTestDb, cleanupTestDb, TEST_USERNAME, TEST_JWT_SECRET } from './helpers/setupTestDb'; + +describe('WebSocket upgrade dispatch order', () => { + let tmpDir: string; + let server: import('http').Server; + let port: number; + let sessionCookie: string; + let remoteNodeId: number; + + beforeAll(async () => { + tmpDir = await setupTestDb(); + ({ server } = await import('../index')); + + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + const addr = server.address() as AddressInfo; + port = addr.port; + + const { DatabaseService } = await import('../services/DatabaseService'); + remoteNodeId = DatabaseService.getInstance().addNode({ + name: 'upgrade-order-remote', + type: 'remote', + compose_dir: '/tmp', + is_default: false, + api_url: 'http://127.0.0.1:1', + api_token: 'upgrade-order-token', + }); + + const token = jwt.sign({ username: TEST_USERNAME }, TEST_JWT_SECRET, { expiresIn: '1m' }); + sessionCookie = `sencho_token=${token}`; + }); + + afterAll(async () => { + await new Promise((resolve, reject) => { + server.close((err) => (err ? reject(err) : resolve())); + }); + cleanupTestDb(tmpDir); + }); + + function connect(pathAndQuery: string, opts: { cookie?: string } = {}): WebSocket { + const headers: Record = {}; + if (opts.cookie) headers['cookie'] = opts.cookie; + return new WebSocket(`ws://127.0.0.1:${port}${pathAndQuery}`, { headers }); + } + + /** Wait for an open or a failure; resolves with one of several outcomes. */ + function waitForOutcome(ws: WebSocket, timeoutMs = 3000): Promise< + | { kind: 'open' } + | { kind: 'unexpected'; status: number } + | { kind: 'close'; code: number } + | { kind: 'error' } + | { kind: 'timeout' } + > { + return new Promise((resolve) => { + const timer = setTimeout(() => resolve({ kind: 'timeout' }), timeoutMs); + const done = (outcome: Awaited>): void => { + clearTimeout(timer); + resolve(outcome); + }; + ws.once('open', () => done({ kind: 'open' })); + ws.once('unexpected-response', (_req, res) => done({ kind: 'unexpected', status: res.statusCode ?? 0 })); + ws.once('close', (code: number) => done({ kind: 'close', code })); + ws.once('error', () => done({ kind: 'error' })); + }); + } + + async function waitForSubscriberCount(target: number, timeoutMs = 2000): Promise { + const { NotificationService } = await import('../services/NotificationService'); + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + const count = NotificationService.getInstance().getSubscriberCount(); + if (count === target) return count; + await new Promise((r) => setTimeout(r, 25)); + } + return NotificationService.getInstance().getSubscriberCount(); + } + + it('rejects unauthenticated upgrades with HTTP 401 (default-deny)', async () => { + const ws = connect('/ws'); + const outcome = await waitForOutcome(ws); + expect(outcome.kind).toBe('unexpected'); + if (outcome.kind === 'unexpected') expect(outcome.status).toBe(401); + }); + + it('routes /ws/notifications (local, no nodeId) to the notifications handler', async () => { + const { NotificationService } = await import('../services/NotificationService'); + const before = NotificationService.getInstance().getSubscriberCount(); + + const ws = connect('/ws/notifications', { cookie: sessionCookie }); + const outcome = await waitForOutcome(ws); + expect(outcome.kind).toBe('open'); + + const afterOpen = NotificationService.getInstance().getSubscriberCount(); + expect(afterOpen).toBe(before + 1); + + ws.close(); + const afterClose = await waitForSubscriberCount(before); + expect(afterClose).toBe(before); + }); + + it('routes /ws/notifications?nodeId= to the remote forwarder, not local notifications', async () => { + const { NotificationService } = await import('../services/NotificationService'); + const before = NotificationService.getInstance().getSubscriberCount(); + + const ws = connect(`/ws/notifications?nodeId=${remoteNodeId}`, { cookie: sessionCookie }); + await waitForOutcome(ws); + + // Whichever outcome arrives, the local subscriber count MUST NOT have + // incremented. If it did, a remote-targeted upgrade is reaching the local + // notifications handler, which is exactly the bug the dispatch order + // prevents. + await new Promise((r) => setTimeout(r, 50)); + expect(NotificationService.getInstance().getSubscriberCount()).toBe(before); + try { ws.terminate(); } catch { /* ignore */ } + }); + + it('routes /api/stacks//logs to the logs handler (distinct error frame on invalid name)', async () => { + const ws = connect('/api/stacks/%2Einvalid/logs', { cookie: sessionCookie }); + + const firstMessage = await new Promise((resolve, reject) => { + const timer = setTimeout(() => reject(new Error('no frame received')), 2000); + ws.once('message', (data: WebSocket.Data) => { + clearTimeout(timer); + resolve(data.toString()); + }); + ws.once('error', (e) => { clearTimeout(timer); reject(e); }); + }); + + expect(firstMessage).toContain('Invalid stack name'); + try { ws.terminate(); } catch { /* ignore */ } + }); + + it('dispatches /api/pilot/tunnel to the pilot handler (rejects non-pilot bearer before path-based dispatch)', async () => { + // A plain session cookie is a valid *user* JWT but not a pilot JWT. The + // pilot handler runs first (before the shared cookie/Bearer auth) and + // does its own Bearer check. Because this request carries only a cookie + // and no Bearer, pilotTunnel rejects with 401. Any other outcome means + // the ladder has been reordered: either the shared auth ran first and + // let the cookie through, or the unknown-path catch-all handled it. + const ws = connect('/api/pilot/tunnel', { cookie: sessionCookie }); + const outcome = await waitForOutcome(ws); + expect(outcome.kind).toBe('unexpected'); + if (outcome.kind === 'unexpected') expect(outcome.status).toBe(401); + }); +}); diff --git a/backend/src/index.ts b/backend/src/index.ts index 928e378d..131bbb7c 100644 --- a/backend/src/index.ts +++ b/backend/src/index.ts @@ -73,6 +73,14 @@ app.use('/api', auditLog); app.use('/api', enforceApiTokenScope); +// Remote Node HTTP Proxy (see proxy/remoteNodeProxy.ts). Mounted BEFORE the +// per-group routers so a request targeting a remote node short-circuits into +// the proxy instead of hitting a local handler that would read local state. +// Gateway-level paths (auth, nodes, license, fleet, webhooks, meta) are listed +// in helpers/proxyExemptPaths.ts and bypass the proxy back to the local +// handlers below. +app.use('/api/', createRemoteProxyMiddleware()); + app.use('/api/license', licenseRouter); app.use('/api/system', systemUpdateRouter); app.use('/api/permissions', permissionsRouter); @@ -107,11 +115,6 @@ app.use('/api/ports', portsRouter); app.use('/api/nodes', nodesRouter); app.use('/api/stacks', stacksRouter); -// Remote Node HTTP Proxy (see proxy/remoteNodeProxy.ts). Mounted here after -// authGate + auditLog + apiTokenScope so local Sencho enforces auth first; -// the proxy then takes over for remote-targeted requests. -app.use('/api/', createRemoteProxyMiddleware()); - const { server, wss, pilotTunnelWss } = createServer(app); attachUpgrade(server, { wss, pilotTunnelWss });