fix(fleet-secrets): restrict bundle management to admin hub sessions (#1274)

* fix(fleet-secrets): restrict bundle management to admin hub sessions

Fleet Secrets exposes decrypted environment-variable values and writes
credentials across the fleet, so every route now requires an admin role,
runs only on the instance you are signed into, and rejects long-lived API
tokens:

- Add an admin-role check to all secrets routes; the frontend Secrets tab
  renders only for admin users so the affordance matches the backend gate.
- Add /api/secrets/ to the hub-only path list so a request carrying a
  remote node id cannot be proxied to read another node's decrypted values.
- Reject API tokens on every secrets route (browser admin sessions only),
  matching how registry credentials are handled.

Also adds lifecycle and developer-mode diagnostic logging (never the secret
values) and tests covering the admin boundary on every endpoint, API-token
rejection, hub-only enforcement, and diagnostic gating.

* fix(fleet-secrets): require a signed-in user session for all secrets routes

The earlier API-token rejection only blocked opaque API tokens. node_proxy
and pilot_tunnel JWTs are mapped to an admin role by the auth middleware
without an API-token scope, so they still passed the admin gate and could
read decrypted bundles via GET /api/secrets/:id.

Replace the API-token check with requireUserSession, which rejects API tokens
and node_proxy / pilot_tunnel machine credentials (userId 0) on every secrets
route, returning SESSION_REQUIRED. The admin role is still enforced after.

Tests now assert SESSION_REQUIRED for a full-admin API token across all nine
routes and for node_proxy and pilot_tunnel JWTs.

* test(fleet-secrets): mint the rejection-test token via the real endpoint

The machine-credential test reconstructed an API token by sha256-hashing a
raw key inline. That duplicated a hashing sink that CodeQL's
js/insufficient-password-hash query flags (a false positive for a 256-bit
random token, but a new occurrence in the diff). Create the token through
POST /api/api-tokens instead, so the hashing stays in the production path
and the test carries none of its own. Behavior and coverage are unchanged.

backend/src/__tests__/hub-only-guard.test.ts

@@ -201,6 +201,31 @@ describe('hubOnlyGuard', () => {
     expect(res.body?.code).not.toBe('HUB_ONLY_ENDPOINT');
   });
 
+  // Regression for Fleet Secrets: bundles are stored and decrypted per instance,
+  // and GET /api/secrets/:id returns plaintext values. A proxied secrets request
+  // would read a remote node's decrypted values as the node-proxy admin, and the
+  // routes' own requireAdmin gate lives in the local handler the proxy skips.
+  // Cover the collection (no trailing slash) and a sub-path.
+  it('rejects /api/secrets with 403 when nodeId targets a remote node', async () => {
+    const res = await request(app)
+      .get('/api/secrets')
+      .set('Authorization', authHeader)
+      .set('x-node-id', String(remoteNodeId));
+
+    expect(res.status).toBe(403);
+    expect(res.body?.code).toBe('HUB_ONLY_ENDPOINT');
+  });
+
+  it('rejects a secrets sub-path (/api/secrets/5) with 403 when nodeId targets a remote node', async () => {
+    const res = await request(app)
+      .get('/api/secrets/5')
+      .set('Authorization', authHeader)
+      .set('x-node-id', String(remoteNodeId));
+
+    expect(res.status).toBe(403);
+    expect(res.body?.code).toBe('HUB_ONLY_ENDPOINT');
+  });
+
   it('does not interfere with non-hub paths even when nodeId targets a remote node', async () => {
     // /api/stacks is not hub-only and should be forwarded by the proxy.
     // The exact upstream-error status is not the contract here; what

backend/src/__tests__/secrets.test.ts

@@ -5,10 +5,12 @@
  *  - encrypt round-trip via CryptoService
  *  - DatabaseService secret + version + push CRUD
  *  - SecretsService versioning, importFromStack, executePush aggregation
- *  - Route guards (requirePaid 403, push lock 409)
+ *  - Route guards (requirePaid 403, requireAdmin 403, requireUserSession 403, push lock 409)
+ *  - Hub-only enforcement is covered in hub-only-guard.test.ts
+ *  - developer_mode diagnostics gating (and that diagnostics never log the secret value)
  *  - getAuditSummary patterns for /secrets routes
  */
-import { describe, it, expect, beforeAll, afterAll, beforeEach, vi } from 'vitest';
+import { describe, it, expect, beforeAll, afterAll, beforeEach, afterEach, vi } from 'vitest';
 import request from 'supertest';
 import path from 'path';
 import fs from 'fs';
@@ -42,6 +44,30 @@ function clearSecretsTables(): void {
     db.prepare('DELETE FROM secrets').run();
 }
 
+// Shared by the admin-role and machine-credential matrices: one representative
+// call per secrets endpoint. requireUserSession and requireAdmin both run before
+// requireBody and param parsing, so a bodyless request still surfaces the guard.
+const SECRET_ENDPOINTS: Array<[string, string]> = [
+    ['get', '/api/secrets'],
+    ['post', '/api/secrets'],
+    ['get', '/api/secrets/1'],
+    ['put', '/api/secrets/1'],
+    ['delete', '/api/secrets/1'],
+    ['get', '/api/secrets/1/versions'],
+    ['post', '/api/secrets/1/import-from-stack'],
+    ['post', '/api/secrets/1/push/preview'],
+    ['post', '/api/secrets/1/push'],
+];
+
+function callWithToken(method: string, p: string, token: string) {
+    const agent = request(app);
+    const r = method === 'get' ? agent.get(p)
+        : method === 'post' ? agent.post(p)
+        : method === 'put' ? agent.put(p)
+        : agent.delete(p);
+    return r.set('Authorization', `Bearer ${token}`);
+}
+
 beforeAll(async () => {
     tmpDir = await setupTestDb();
     ({ DatabaseService } = await import('../services/DatabaseService'));
@@ -399,3 +425,119 @@ describe('Routes /api/secrets tier gating and lock', () => {
         expect(res.status).toBe(400);
     });
 });
+
+// ---- Admin-role gating: secrets reveal decrypted values, so every route is admin-only ----
+
+describe('Routes /api/secrets admin-role gating', () => {
+    // authMiddleware resolves the role from the DB (not the JWT), so a real
+    // non-admin user must exist for the gate to see a non-admin role.
+    function viewerToken(): string {
+        const db = DatabaseService.getInstance();
+        let user = db.getUserByUsername('sec-viewer');
+        if (!user) {
+            db.addUser({ username: 'sec-viewer', password_hash: 'x', role: 'viewer' });
+            user = db.getUserByUsername('sec-viewer')!;
+        }
+        return authToken('sec-viewer', 'viewer', user.token_version);
+    }
+
+    it.each(SECRET_ENDPOINTS)('403s a non-admin paid user on %s %s', async (method, p) => {
+        const res = await callWithToken(method, p, viewerToken());
+        expect(res.status).toBe(403);
+        expect(res.body.code).toBe('ADMIN_REQUIRED');
+    });
+
+    it('lets an admin paid user list (200)', async () => {
+        const res = await request(app)
+            .get('/api/secrets')
+            .set('Authorization', `Bearer ${adminToken()}`);
+        expect(res.status).toBe(200);
+    });
+});
+
+// ---- Machine-credential rejection: secrets need a real signed-in user session ----
+
+describe('Routes /api/secrets machine-credential rejection', () => {
+    // A full-admin API token resolves to role 'admin' and would otherwise pass
+    // requireAdmin and reach the decrypted-value GET. requireUserSession runs
+    // first and blocks it. Mint the token through the real endpoint so the test
+    // exercises the production creation path and carries no hashing of its own.
+    let fullAdminToken: string;
+    beforeAll(async () => {
+        const res = await request(app)
+            .post('/api/api-tokens')
+            .set('Authorization', `Bearer ${adminToken()}`)
+            .send({ name: `secrets-rejection-${Date.now()}`, scope: 'full-admin' });
+        fullAdminToken = res.body.token as string;
+    });
+
+    // node_proxy / pilot_tunnel JWTs are signed with this instance's secret and
+    // map to { username: 'node-proxy', role: 'admin', userId: 0 } in authMiddleware.
+    // They carry no apiTokenScope, so only the userId-0 check blocks them.
+    function machineJwt(scope: 'node_proxy' | 'pilot_tunnel'): string {
+        return jwt.sign({ scope }, TEST_JWT_SECRET, { expiresIn: '1m' });
+    }
+
+    it.each(SECRET_ENDPOINTS)('403s a full-admin API token on %s %s with SESSION_REQUIRED', async (method, p) => {
+        const res = await callWithToken(method, p, fullAdminToken);
+        expect(res.status).toBe(403);
+        expect(res.body.code).toBe('SESSION_REQUIRED');
+    });
+
+    it.each([
+        ['node_proxy', '/api/secrets'],
+        ['node_proxy', '/api/secrets/1'],
+        ['pilot_tunnel', '/api/secrets/1'],
+    ] as const)('403s a %s JWT on %s with SESSION_REQUIRED', async (scope, p) => {
+        const res = await request(app)
+            .get(p)
+            .set('Authorization', `Bearer ${machineJwt(scope)}`);
+        expect(res.status).toBe(403);
+        expect(res.body.code).toBe('SESSION_REQUIRED');
+    });
+});
+
+// ---- developer_mode diagnostics gating ----
+
+describe('SecretsService.executePush developer_mode diagnostics', () => {
+    beforeEach(() => {
+        const composeDir = process.env.COMPOSE_DIR!;
+        const stackDir = path.join(composeDir, 'devmodestack');
+        fs.mkdirSync(stackDir, { recursive: true });
+        fs.writeFileSync(path.join(stackDir, '.env'), 'EXISTING=keep\n');
+        fs.writeFileSync(path.join(stackDir, 'compose.yaml'), 'services:\n  app:\n    image: nginx\n');
+    });
+
+    afterEach(() => {
+        DatabaseService.getInstance().updateGlobalSetting('developer_mode', '0');
+    });
+
+    async function runPush(name: string): Promise<void> {
+        const db = DatabaseService.getInstance();
+        const localNode = db.getNodes().find(n => n.type === 'local')!;
+        const svc = SecretsService.getInstance();
+        const { id } = svc.create({ name, kv: { TOKEN: 'supersecretvalue' }, user: TEST_USERNAME });
+        await svc.executePush(id, { type: 'nodes', ids: [localNode.id] }, 'devmodestack', '.env', TEST_USERNAME);
+    }
+
+    it('emits [Secrets:diag] only when developer_mode is on, and never the secret value', async () => {
+        const logSpy = vi.spyOn(console, 'log').mockImplementation(() => undefined);
+        try {
+            DatabaseService.getInstance().updateGlobalSetting('developer_mode', '0');
+            await runPush('devmode-off');
+            const offLogs = logSpy.mock.calls.map(c => c.join(' '));
+            expect(offLogs.some(l => l.includes('[Secrets:diag]'))).toBe(false);
+
+            logSpy.mockClear();
+            DatabaseService.getInstance().updateGlobalSetting('developer_mode', '1');
+            await runPush('devmode-on');
+            const onLogs = logSpy.mock.calls.map(c => c.join(' '));
+            expect(onLogs.some(l => l.includes('[Secrets:diag]'))).toBe(true);
+
+            // Diagnostics summarize counts only; the decrypted value must never appear.
+            expect([...offLogs, ...onLogs].some(l => l.includes('supersecretvalue'))).toBe(false);
+        } finally {
+            logSpy.mockRestore();
+        }
+    });
+});

backend/src/helpers/proxyExemptPaths.ts

@@ -32,9 +32,10 @@ export function isProxyExemptPath(path: string): boolean {
 // nodeId resolves to a remote node so a script/curl call cannot trick the proxy
 // into forwarding the request across a node boundary. This matters for the logs
 // feed (its admin gate lives in the local route handler, which the proxy would
-// skip when forwarding a remote nodeId) and for registries (a proxied registry
-// request would otherwise carry a plaintext secret to, or read stored
-// credentials from, the remote).
+// skip when forwarding a remote nodeId) and for registries and Fleet Secrets (a
+// proxied request would otherwise carry a plaintext secret to, or read stored
+// secret values from, the remote; the secrets routes' admin gate also lives in
+// the local route handler, which the proxy would skip).
 //
 // Entries are stored with a trailing slash; the matcher accepts the exact
 // collection path (without the trailing slash) AND any sub-path under it,
@@ -54,6 +55,7 @@ export const HUB_ONLY_PREFIXES: readonly string[] = [
   '/api/logs/global/',
   '/api/system/log-stream-metrics/',
   '/api/registries/',
+  '/api/secrets/',
 ];
 
 /** Returns true when the path is hub-only and must not be proxied to a remote node. */

backend/src/middleware/tierGates.ts

@@ -50,6 +50,20 @@ export const requireAdmin = (req: Request, res: Response): boolean => {
   return true;
 };
 
+/**
+ * Require a genuine signed-in user session. Rejects opaque API tokens
+ * (`req.apiTokenScope`) and node_proxy / pilot_tunnel machine credentials,
+ * which authMiddleware maps to role 'admin' with userId 0. Endpoints that
+ * expose decrypted secrets use this so a long-lived machine credential cannot
+ * reach them; the admin role itself is still enforced separately by requireAdmin.
+ */
+export const requireUserSession = (req: Request, res: Response): boolean => {
+  if (req.apiTokenScope || !req.user || req.user.userId === 0) {
+    return deny(res, 'SESSION_REQUIRED', 'This action requires a signed-in user session.');
+  }
+  return true;
+};
+
 /**
  * Accept only calls from a sibling Sencho using its node_proxy Bearer token.
  * Browser sessions, API tokens, and console tokens are all rejected.

backend/src/routes/secrets.ts

@@ -1,6 +1,6 @@
 import { Router, type Request, type Response } from 'express';
 import { authMiddleware } from '../middleware/auth';
-import { requirePaid, requireBody } from '../middleware/tierGates';
+import { requirePaid, requireAdmin, requireUserSession, requireBody } from '../middleware/tierGates';
 import { SecretsService, PushBusyError, type SecretKv } from '../services/SecretsService';
 import { DatabaseService, type BlueprintSelector } from '../services/DatabaseService';
 import { isValidStackName } from '../utils/validation';
@@ -10,6 +10,11 @@ import { sanitizeForLog } from '../utils/safeLog';
 
 export const secretsRouter = Router();
 
+// Fleet Secrets reveals decrypted values and writes credentials fleet-wide, so
+// every route requires a signed-in admin user. requireUserSession rejects API
+// tokens and node_proxy / pilot_tunnel machine credentials (authMiddleware maps
+// the latter to role 'admin'), then requireAdmin enforces the role.
+
 const NAME_PATTERN = /^[a-zA-Z0-9][a-zA-Z0-9 _.-]{0,62}[a-zA-Z0-9]$/;
 
 function isValidName(name: unknown): name is string {
@@ -60,7 +65,9 @@ function parsePushBody(body: unknown): PushBody | { error: string } {
 }
 
 secretsRouter.get('/', authMiddleware, async (req: Request, res: Response): Promise<void> => {
+    if (!requireUserSession(req, res)) return;
     if (!requirePaid(req, res)) return;
+    if (!requireAdmin(req, res)) return;
     try {
         const items = SecretsService.getInstance().list();
         res.json(items);
@@ -71,7 +78,9 @@ secretsRouter.get('/', authMiddleware, async (req: Request, res: Response): Prom
 });
 
 secretsRouter.post('/', authMiddleware, async (req: Request, res: Response): Promise<void> => {
+    if (!requireUserSession(req, res)) return;
     if (!requirePaid(req, res)) return;
+    if (!requireAdmin(req, res)) return;
     if (!requireBody(req, res)) return;
     try {
         const { name, description, kv, note } = req.body as { name?: unknown; description?: unknown; kv?: unknown; note?: unknown };
@@ -98,6 +107,7 @@ secretsRouter.post('/', authMiddleware, async (req: Request, res: Response): Pro
             user: getActor(req),
             note: typeof note === 'string' ? note : undefined,
         });
+        console.log(`[Secrets] Created bundle ${sanitizeForLog(name)} (v${result.version}) by ${sanitizeForLog(getActor(req))}`);
         res.status(201).json(result);
     } catch (err) {
         if (isSqliteUniqueViolation(err)) {
@@ -110,7 +120,9 @@ secretsRouter.post('/', authMiddleware, async (req: Request, res: Response): Pro
 });
 
 secretsRouter.get('/:id', authMiddleware, async (req: Request, res: Response): Promise<void> => {
+    if (!requireUserSession(req, res)) return;
     if (!requirePaid(req, res)) return;
+    if (!requireAdmin(req, res)) return;
     try {
         const id = parseIntParam(req, res, 'id', 'secret ID');
         if (id === null) return;
@@ -128,7 +140,9 @@ secretsRouter.get('/:id', authMiddleware, async (req: Request, res: Response): P
 });
 
 secretsRouter.put('/:id', authMiddleware, async (req: Request, res: Response): Promise<void> => {
+    if (!requireUserSession(req, res)) return;
     if (!requirePaid(req, res)) return;
+    if (!requireAdmin(req, res)) return;
     if (!requireBody(req, res)) return;
     try {
         const id = parseIntParam(req, res, 'id', 'secret ID');
@@ -157,6 +171,7 @@ secretsRouter.put('/:id', authMiddleware, async (req: Request, res: Response): P
             user: getActor(req),
             note: typeof note === 'string' ? note : undefined,
         });
+        console.log(`[Secrets] Updated bundle ${sanitizeForLog(existing.name)} to v${result.version} by ${sanitizeForLog(getActor(req))}`);
         res.json(result);
     } catch (err) {
         console.error('[Secrets] Update error:', err);
@@ -165,7 +180,9 @@ secretsRouter.put('/:id', authMiddleware, async (req: Request, res: Response): P
 });
 
 secretsRouter.delete('/:id', authMiddleware, async (req: Request, res: Response): Promise<void> => {
+    if (!requireUserSession(req, res)) return;
     if (!requirePaid(req, res)) return;
+    if (!requireAdmin(req, res)) return;
     try {
         const id = parseIntParam(req, res, 'id', 'secret ID');
         if (id === null) return;
@@ -174,6 +191,7 @@ secretsRouter.delete('/:id', authMiddleware, async (req: Request, res: Response)
             res.status(404).json({ error: 'Secret not found' });
             return;
         }
+        console.log(`[Secrets] Deleted bundle id=${id} by ${sanitizeForLog(getActor(req))}`);
         res.json({ ok: true });
     } catch (err) {
         console.error('[Secrets] Delete error:', err);
@@ -182,7 +200,9 @@ secretsRouter.delete('/:id', authMiddleware, async (req: Request, res: Response)
 });
 
 secretsRouter.get('/:id/versions', authMiddleware, async (req: Request, res: Response): Promise<void> => {
+    if (!requireUserSession(req, res)) return;
     if (!requirePaid(req, res)) return;
+    if (!requireAdmin(req, res)) return;
     try {
         const id = parseIntParam(req, res, 'id', 'secret ID');
         if (id === null) return;
@@ -198,7 +218,9 @@ secretsRouter.get('/:id/versions', authMiddleware, async (req: Request, res: Res
 });
 
 secretsRouter.post('/:id/import-from-stack', authMiddleware, async (req: Request, res: Response): Promise<void> => {
+    if (!requireUserSession(req, res)) return;
     if (!requirePaid(req, res)) return;
+    if (!requireAdmin(req, res)) return;
     if (!requireBody(req, res)) return;
     try {
         const id = parseIntParam(req, res, 'id', 'secret ID');
@@ -226,7 +248,9 @@ secretsRouter.post('/:id/import-from-stack', authMiddleware, async (req: Request
 });
 
 secretsRouter.post('/:id/push/preview', authMiddleware, async (req: Request, res: Response): Promise<void> => {
+    if (!requireUserSession(req, res)) return;
     if (!requirePaid(req, res)) return;
+    if (!requireAdmin(req, res)) return;
     if (!requireBody(req, res)) return;
     try {
         const id = parseIntParam(req, res, 'id', 'secret ID');
@@ -249,7 +273,9 @@ secretsRouter.post('/:id/push/preview', authMiddleware, async (req: Request, res
 });
 
 secretsRouter.post('/:id/push', authMiddleware, async (req: Request, res: Response): Promise<void> => {
+    if (!requireUserSession(req, res)) return;
     if (!requirePaid(req, res)) return;
+    if (!requireAdmin(req, res)) return;
     if (!requireBody(req, res)) return;
     try {
         const id = parseIntParam(req, res, 'id', 'secret ID');
@@ -266,7 +292,9 @@ secretsRouter.post('/:id/push', authMiddleware, async (req: Request, res: Respon
         }
         try {
             const result = await SecretsService.getInstance().executePush(id, parsed.selector, parsed.stackName, parsed.envFileBasename, getActor(req));
-            console.log(`[Secrets] Push ${sanitizeForLog(secret.name)} v${secret.current_version}: ${result.results.length} nodes`);
+            const failedCount = result.results.filter(r => r.status === 'failed').length;
+            console.log(`[Secrets] Push ${sanitizeForLog(secret.name)} v${secret.current_version}: ${result.results.length} node(s), ${failedCount} failed (by ${sanitizeForLog(getActor(req))})`);
+            if (failedCount > 0) console.warn(`[Secrets] Push ${sanitizeForLog(secret.name)} v${secret.current_version}: ${failedCount} node(s) failed`);
             res.json(result);
         } catch (err) {
             if (err instanceof PushBusyError) {

backend/src/services/SecretsService.ts

@@ -8,6 +8,7 @@ import { NodeRegistry } from './NodeRegistry';
 import { resolveAllEnvFilePaths } from '../routes/stacks';
 import { getErrorMessage } from '../utils/errors';
 import { formatNoTargetError } from '../utils/remoteTarget';
+import { isDebugEnabled } from '../utils/debug';
 
 export type SecretKv = Record<string, string>;
 export type DiffStatus = 'added' | 'changed' | 'removed' | 'unchanged';
@@ -427,6 +428,7 @@ export class SecretsService {
         const db = DatabaseService.getInstance();
         const overlay = this.getDecryptedKv(id);
         const matched = NodeLabelService.getInstance().matchSelector(selector, db.getNodes());
+        if (isDebugEnabled()) console.log(`[Secrets:diag] preview id=${id} targets=${matched.length}`);
 
         // Preview is read-only and per-node; fan out in parallel for snappier wizard UX.
         return Promise.all(matched.map(async (node): Promise<SecretPushPlanEntry> => {
@@ -472,6 +474,7 @@ export class SecretsService {
         const overlay = decryptKv(versionRow.encrypted_payload);
         const matched = NodeLabelService.getInstance().matchSelector(selector, db.getNodes());
         const pushId = crypto.randomUUID();
+        if (isDebugEnabled()) console.log(`[Secrets:diag] push id=${id} v${versionRow.version} targets=${matched.length}`);
         const results: SecretPushResultEntry[] = [];
         const rows: Array<Parameters<typeof db.insertSecretPushes>[0][number]> = [];
         const pushedAt = Date.now();
@@ -526,6 +529,10 @@ export class SecretsService {
         } finally {
             activePushes.delete(id);
         }
+        if (isDebugEnabled()) {
+            const okCount = results.filter(r => r.status === 'ok').length;
+            console.log(`[Secrets:diag] push id=${id} done in ${Date.now() - pushedAt}ms ok=${okCount} failed=${results.length - okCount}`);
+        }
         return { pushId, results };
     }
 }

frontend/src/components/FleetView.tsx

@@ -117,7 +117,7 @@ export function FleetView({ onNavigateToNode }: FleetViewProps) {
                                     <Wrench className="w-4 h-4 mr-1.5" />Fleet Actions
                                 </TabsTrigger>
                             </TabsHighlightItem>
-                            {isPaid && (
+                            {isPaid && isAdmin && (
                                 <TabsHighlightItem value="secrets">
                                     <TabsTrigger value="secrets">
                                         <KeyRound className="w-4 h-4 mr-1.5" />Secrets
@@ -224,7 +224,7 @@ export function FleetView({ onNavigateToNode }: FleetViewProps) {
                 <TabsContent value="actions">
                     <FleetActionsTab nodes={overview.allNodes} />
                 </TabsContent>
-                {isPaid && (
+                {isPaid && isAdmin && (
                     <TabsContent value="secrets">
                         <SecretsTab />
                     </TabsContent>