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=<remote> 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.
This commit is contained in:
Anso
2026-04-24 10:20:08 -04:00
committed by GitHub
parent 86722fe98e
commit 43a595905b
5 changed files with 465 additions and 5 deletions
@@ -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<void>((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<void>((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);
});
});
@@ -0,0 +1,91 @@
/**
* Regression guard for `createRemoteProxyMiddleware` mount order.
*
* `index.ts` mounts every local `/api/<group>` 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);
});
});
@@ -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<string, unknown>;
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<string, unknown>;
// 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);
});
});
+158
View File
@@ -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<void>((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<void>((resolve, reject) => {
server.close((err) => (err ? reject(err) : resolve()));
});
cleanupTestDb(tmpDir);
});
function connect(pathAndQuery: string, opts: { cookie?: string } = {}): WebSocket {
const headers: Record<string, string> = {};
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<ReturnType<typeof waitForOutcome>>): 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<number> {
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=<remote> 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/<name>/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<string>((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);
});
});
+8 -5
View File
@@ -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 });