Hold overlay pushes to the same write check as every other fleet action

A PiP overlay renders across a live screen — an arbitrary web page, at full resolution, for as long
as the operator wants. That is a fleet-affecting write, but the three routes that perform it carried
only requireScope('full'), which gates API tokens and is a deliberate pass-through for dashboard
sessions. The file's own comment says so ("No-op for JWT sessions"), on the assumption that
something else covered that case. Nothing did.

Every sibling route pairs the two checks — device-groups.js gates POST /:id/command with
`requireScope('full'), requireGroupWrite`. These had only the half that does nothing for a logged-in
user, so a member who is refused on every other device mutation was accepted here.

requireFleetWrite restores the pairing on POST /, POST /clear and DELETE /, resolving the caller's
context against the workspace the same way the rest of the codebase does.

5 tests pin both directions: refused for a read-only member on all three routes and for an
unauthenticated caller, still allowed for a workspace_editor and for an org owner acting into the
workspace (actingAs, whose workspaceRole is null and must not read as a viewer).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL

server/routes/pip.js

@@ -6,6 +6,24 @@ const { db } = require('../db/database');
 // full-trust (a `web` overlay renders an arbitrary page in the player), so — like the
 // group command route — it requires the 'full' token scope. No-op for JWT sessions.
 const { requireScope } = require('../middleware/apiToken');
+const { accessContext } = require('../lib/tenancy');
+
+// requireScope('full') gates API TOKENS and is a deliberate pass-through for JWT sessions
+// (middleware/apiToken.js: `if (!req.viaToken) return next()`). It was the ONLY guard on these
+// routes, so a dashboard session carried no write check at all here — every sibling
+// fleet-affecting route pairs the scope check with a role check (see device-groups.js, where
+// POST /:id/command is `requireScope('full'), requireGroupWrite`). This restores that pairing:
+// a read-only member is refused, exactly as they are on every other device mutation.
+function requireFleetWrite(req, res, next) {
+  if (!req.workspaceId) return res.status(403).json({ error: 'No workspace context' });
+  const ws = db.prepare('SELECT * FROM workspaces WHERE id = ?').get(req.workspaceId);
+  const ctx = ws && accessContext(req.user.id, req.user.role, ws);
+  if (!ctx) return res.status(403).json({ error: 'Access denied' });
+  if (!ctx.actingAs && ctx.workspaceRole === 'workspace_viewer') {
+    return res.status(403).json({ error: 'Read-only access' });
+  }
+  next();
+}
 
 // Reuse the existing 6-hex color contract (#RRGGBB). Overlay transparency is expressed
 // via the separate `opacity` field, so no alpha channel is accepted here.
@@ -82,7 +100,7 @@ function summarize(results) {
 }
 
 // POST /api/pip — show an overlay on a device or group.
-router.post('/', requireScope('full'), (req, res) => {
+router.post('/', requireScope('full'), requireFleetWrite, (req, res) => {
   const b = req.body || {};
 
   if (!b.device_id) return res.status(400).json({ error: 'device_id required (device or group id)' });
@@ -157,7 +175,7 @@ function handleClear(req, res) {
   res.json({ success: true, target: targets.kind, ...summary });
 }
 
-router.post('/clear', requireScope('full'), handleClear);
-router.delete('/', requireScope('full'), handleClear);
+router.post('/clear', requireScope('full'), requireFleetWrite, handleClear);
+router.delete('/', requireScope('full'), requireFleetWrite, handleClear);
 
 module.exports = router;

server/test/pip-write-authorization.test.js

@@ -0,0 +1,99 @@
+'use strict';
+
+// A PiP overlay is pushed to a live screen and can render an arbitrary web page across it, at full
+// resolution, for as long as the operator wants (duration 0 = persistent). That is a fleet-affecting
+// write and must be held to the same bar as every other one.
+//
+// It was not. The only guard was requireScope('full'), which gates API TOKENS and is a deliberate
+// pass-through for dashboard sessions (`if (!req.viaToken) return next()`). Every sibling route
+// pairs that scope check with a role check — device-groups.js gates POST /:id/command with
+// `requireScope('full'), requireGroupWrite` — but these three routes had only the half that does
+// nothing for a logged-in user. A member whose role is read-only everywhere else could push and
+// clear overlays on every screen in the workspace.
+//
+// The invariant: a read-only member cannot change what a screen displays. Pinned for all three
+// write routes, because a partial fix here is worthless.
+
+const { test } = require('node:test');
+const assert = require('node:assert/strict');
+const path = require('path');
+const fs = require('fs');
+const os = require('os');
+
+const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'st-pip-authz-'));
+process.env.DATA_DIR = tmp;
+process.env.JWT_SECRET = 'test-secret-pip-authz';
+
+const express = require('express');
+const { db } = require('../db/database');
+const { requireAuth, generateToken } = require('../middleware/auth');
+const { resolveTenancy } = require('../lib/tenancy');
+
+// One workspace, one device, three members: an owner, an editor and a viewer.
+const O = 'o-pip', WS = 'ws-pip', DEV = 'd-pip';
+db.prepare("INSERT OR IGNORE INTO users (id,email,password_hash,role) VALUES ('u-owner','owner@t.local','x','user')").run();
+db.prepare("INSERT OR IGNORE INTO users (id,email,password_hash,role) VALUES ('u-editor','editor@t.local','x','user')").run();
+db.prepare("INSERT OR IGNORE INTO users (id,email,password_hash,role) VALUES ('u-viewer','viewer@t.local','x','user')").run();
+db.prepare('INSERT OR IGNORE INTO organizations (id,name,owner_user_id) VALUES (?,?,?)').run(O, 'Org', 'u-owner');
+db.prepare('INSERT OR IGNORE INTO workspaces (id,organization_id,name) VALUES (?,?,?)').run(WS, O, 'WS');
+db.prepare("INSERT OR IGNORE INTO organization_members (organization_id,user_id,role) VALUES (?,?, 'org_owner')").run(O, 'u-owner');
+db.prepare("INSERT OR IGNORE INTO workspace_members (workspace_id,user_id,role) VALUES (?,?, 'workspace_editor')").run(WS, 'u-editor');
+db.prepare("INSERT OR IGNORE INTO workspace_members (workspace_id,user_id,role) VALUES (?,?, 'workspace_viewer')").run(WS, 'u-viewer');
+db.prepare(`INSERT OR IGNORE INTO devices (id,name,workspace_id,created_at,updated_at)
+            VALUES (?,?,?,strftime('%s','now'),strftime('%s','now'))`).run(DEV, 'Screen', WS);
+
+const app = express();
+app.use(express.json());
+// Minimal socket stub — the route emits to the device room on a successful push. The deny paths
+// never reach it, but the allow paths must not 500 on a missing io.
+const emitted = [];
+const nsp = {
+  adapter: { rooms: new Map([[DEV, new Set(['sock-1'])]]) },
+  to: () => ({ emit: (...a) => emitted.push(a) }),
+  emit: (...a) => emitted.push(a),
+};
+app.set('io', { of: () => nsp });
+app.use('/api/pip', requireAuth, resolveTenancy, require('../routes/pip'));
+const server = app.listen(0);
+
+const row = (id) => db.prepare('SELECT id, email, role FROM users WHERE id = ?').get(id);
+const tokenFor = (id) => generateToken(row(id), WS);
+
+async function call(method, pathname, who, body) {
+  await new Promise(r => (server.listening ? r() : server.once('listening', r)));
+  const res = await fetch(`http://127.0.0.1:${server.address().port}${pathname}`, {
+    method,
+    headers: { 'Content-Type': 'application/json', ...(who ? { Authorization: `Bearer ${tokenFor(who)}` } : {}) },
+    ...(body ? { body: JSON.stringify(body) } : {}),
+  });
+  return res.status;
+}
+
+const PUSH = { device_id: DEV, type: 'web', uri: 'https://example.com/', width: 800, height: 600, duration: 0 };
+
+test('THE HOLE: a read-only member cannot push an overlay to a screen', async () => {
+  assert.equal(await call('POST', '/api/pip', 'u-viewer', PUSH), 403);
+});
+
+test('a read-only member cannot clear overlays either', async () => {
+  // Both spellings of clear — a fix that covers one and not the other is not a fix.
+  assert.equal(await call('POST', '/api/pip/clear', 'u-viewer', { device_id: DEV }), 403);
+  assert.equal(await call('DELETE', '/api/pip', 'u-viewer', { device_id: DEV }), 403);
+});
+
+test('an unauthenticated caller is refused', async () => {
+  assert.equal(await call('POST', '/api/pip', null, PUSH), 401);
+});
+
+test('AND THE OTHER HALF: an editor can still push and clear', async () => {
+  // The guard must not break the feature. A workspace_editor manages content by definition.
+  assert.equal(await call('POST', '/api/pip', 'u-editor', PUSH), 200);
+  assert.equal(await call('POST', '/api/pip/clear', 'u-editor', { device_id: DEV }), 200);
+});
+
+test('an org owner acting into the workspace can still push', async () => {
+  // actingAs true, workspaceRole null — must not be mistaken for a viewer.
+  assert.equal(await call('POST', '/api/pip', 'u-owner', PUSH), 200);
+});
+
+test.after(() => { server.close(); try { fs.rmSync(tmp, { recursive: true, force: true }); } catch (_) {} });