codeql fixes

.github/workflows/pull_requests.yml

@@ -5,9 +5,18 @@ on:
   push:
     branches: [ master ]
 
+# Default for every job below: read this repository's code and nothing else.
+permissions:
+  contents: read
+
 jobs:
   pre_job:
     runs-on: ubuntu-latest
+    # skip-duplicate-actions lists this workflow's previous runs to decide whether the
+    # GUI paths changed, which needs read access to the Actions API on top of the default.
+    permissions:
+      contents: read
+      actions: read
     outputs:
       should_skip: ${{ steps.skip_check.outputs.should_skip }}
     steps:

.github/workflows/tests.yaml

@@ -6,6 +6,10 @@ on:
   pull_request:
     branches: [ master ]
 
+# The token only ever needs to read this repository's code.
+permissions:
+  contents: read
+
 jobs:
   test:
     runs-on: ${{ matrix.os }}

lib/dns/cliHelpers.js

@@ -14,7 +14,37 @@ const collect = (value, previous) => previous.concat([value]);
 // --drop-value flags compile to case-insensitive regexes. All commands must compile
 // them identically, or compare's "pass the same patterns the migration dropped"
 // contract (TASK-1.11) silently stops matching.
-const dropValuePatterns = (values) => values.map(pattern => new RegExp(pattern, 'i'));
+//
+// The pattern is the operator's own input, but it still reaches a regex engine mid-migration,
+// so it is bounded before compiling. DNS record values are printable ASCII (international
+// names arrive as punycode), which makes that the honest limit: anything outside it cannot
+// match a record anyway, and a control character in a flag is a shell-quoting accident, not a
+// pattern. The length cap keeps the compiled program small. Deliberately NOT restricted
+// further: a narrower charset would reject legitimate patterns over TXT values, and rejecting
+// nested quantifiers would also reject bounded ones like ([0-9]{1,3}\.){3} that never blow up.
+const MAX_DROP_VALUE_PATTERN_LENGTH = 200;
+const PRINTABLE_ASCII = /^[\x20-\x7e]+$/;
+
+const compileDropValuePattern = (pattern) => {
+  if (typeof pattern !== 'string' || pattern.length === 0) {
+    throw new Error('--drop-value needs a pattern, got an empty value');
+  }
+  if (pattern.length > MAX_DROP_VALUE_PATTERN_LENGTH) {
+    throw new Error(`--drop-value pattern is ${pattern.length} characters, over the ${MAX_DROP_VALUE_PATTERN_LENGTH} character limit`);
+  }
+  if (!PRINTABLE_ASCII.test(pattern)) {
+    throw new Error(`--drop-value pattern "${pattern}" contains characters outside printable ASCII, which no DNS record value can match`);
+  }
+
+  try {
+    return new RegExp(pattern, 'i');
+  } catch (error) {
+    // Without this the operator gets a bare SyntaxError stack and no hint which flag caused it.
+    throw new Error(`--drop-value pattern "${pattern}" is not a valid regular expression: ${error.message}`);
+  }
+};
+
+const dropValuePatterns = (values) => (values || []).map(compileDropValuePattern);
 
 const filterByDomains = (items, domains, getName = (item) => item.domainName) => {
   if (!domains.length) return items;

lib/server.js

@@ -2,6 +2,7 @@ import path from 'path';
 import express from 'express';
 import bodyParser from 'body-parser';
 import multer from 'multer';
+import rateLimit from 'express-rate-limit';
 import { fileURLToPath } from 'url';
 import pkg from '../package.json' with { type: 'json' };
 
@@ -26,21 +27,43 @@ const start = (env, client) => {
     email: env.MARKETPLACE_EMAIL
   }, client);
 
+  // Gateway rejections are Error instances, so `res.send(error)` serialised them to `{}`
+  // under a 200 — the GUI saw a successful but empty response and then threw on the fields
+  // it expected. Forward the upstream status with the upstream body instead, and always as
+  // JSON so no error text is ever handed back for a browser to parse as HTML.
+  const sendError = (res, error) => {
+    const status = error?.statusCode || error?.response?.statusCode || 502;
+    const body = error?.response?.body;
+
+    if (body && typeof body === 'object') return res.status(status).json(body);
+    return res.status(status).json({ error: (typeof body === 'string' && body) || error?.message || 'Request failed' });
+  };
+
   const graphqlRouting = (req, res) => {
     gateway
       .graph(req.body)
       .then(body => res.send(body))
-      .catch(error => res.send(error));
+      .catch(error => sendError(res, error));
   };
 
   const liquidRouting = (req, res) => {
     gateway
       .liquid(req.body)
       .then(body => res.send(body))
-      .catch(error => res.send(error));
+      .catch(error => sendError(res, error));
   };
   app.use(bodyParser.json());
 
+  // A ceiling on abuse, not a throttle on normal use: one GUI page load pulls dozens of
+  // static assets through the catch-all route, so the limit has to sit far above anything
+  // a developer clicking around can reach.
+  app.use(rateLimit({
+    windowMs: 60 * 1000,
+    limit: 2000,
+    standardHeaders: true,
+    legacyHeaders: false
+  }));
+
   app.use((req, res, next) => {
     res.header('Access-Control-Allow-Origin', '*');
     res.header('Access-Control-Allow-Headers', 'Origin, X-Requested-With, Content-Type, Accept');
@@ -65,7 +88,7 @@ const start = (env, client) => {
     gateway
       .logs({ lastId: req.query.lastId })
       .then(body => res.send(body))
-      .catch(error => res.send(error));
+      .catch(error => sendError(res, error));
   });
 
   app.get('/api/logsv2', (req, res) => {
@@ -75,7 +98,7 @@ const start = (env, client) => {
         res.send(body);
       })
       .catch(error => {
-        logger.Debug(error); res.send(error);
+        logger.Debug(error); sendError(res, error);
       });
   });
 
@@ -86,7 +109,7 @@ const start = (env, client) => {
         res.send(body);
       })
       .catch(error => {
-        logger.Debug(error); res.send(error);
+        logger.Debug(error); sendError(res, error);
       });
   });
 
@@ -106,7 +129,7 @@ const start = (env, client) => {
       gateway
         .sync(formData)
         .then(body => res.send(body))
-        .catch(error => res.send(error));
+        .catch(error => sendError(res, error));
     }
   );
 

mcp-min/http-server.js

@@ -1,5 +1,6 @@
 import express from 'express';
 import bodyParser from 'body-parser';
+import { randomUUID } from 'crypto';
 import tools from './tools.js';
 import { sseHandler, writeSSE } from './sse.js';
 import { DEBUG } from './config.js';
@@ -8,8 +9,12 @@ import log from './log.js';
 // SSE sessions keyed by Mcp-Session-Id. Supports multiple concurrent clients.
 const sseSessions = new Map();
 
+// A session id is the only thing separating one SSE client's stream from another's, so it
+// has to be unguessable: Math.random() is seeded per process and its output is predictable
+// from a couple of prior samples, which would let a local caller attach to someone else's
+// session by supplying the Mcp-Session-Id it derived.
 function generateSessionId() {
-  return `mcpmin-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`;
+  return `mcpmin-${randomUUID()}`;
 }
 
 export default async function startHttp({ port = 5910 } = {}) {

package.json

@@ -54,6 +54,7 @@
     "email-validator": "^2.0.4",
     "execa": "^10.0.1",
     "express": "^5.2.1",
+    "express-rate-limit": "^8.6.2",
     "fast-glob": "^3.3.3",
     "ignore": "^7.0.5",
     "inquirer": "^14.2.0",

test/global-setup.js

@@ -1,24 +1,27 @@
-import { execSync } from 'child_process';
+import { execFileSync } from 'child_process';
 import path from 'path';
 import { fileURLToPath } from 'url';
 import dotenv from 'dotenv';
+import { isExampleUrl } from './utils/credentials.js';
 
 const __filename = fileURLToPath(import.meta.url);
 const __dirname = path.dirname(__filename);
 
-const cliPath = `node ${path.join(__dirname, '../bin/pos-cli.js')}`;
+const cliScript = path.join(__dirname, '../bin/pos-cli.js');
 
 export async function setup() {
   dotenv.config();
 
   const { MPKIT_URL, MPKIT_TOKEN, MPKIT_EMAIL } = process.env;
-  if (!MPKIT_URL || !MPKIT_TOKEN || !MPKIT_EMAIL || MPKIT_URL.includes('example.com')) {
+  if (!MPKIT_URL || !MPKIT_TOKEN || !MPKIT_EMAIL || isExampleUrl(MPKIT_URL)) {
     console.log('[Global Setup] No real credentials found, skipping instance cleanup');
     return;
   }
 
   console.log(`[Global Setup] Cleaning instance: ${MPKIT_URL}`);
-  execSync(`${cliPath} data clean --include-schema --auto-confirm`, {
+  // execFileSync, not execSync: the repo path goes into argv, and a shell would split it on
+  // spaces and re-read any metacharacter in it before node ever saw the script name.
+  execFileSync(process.execPath, [cliScript, 'data', 'clean', '--include-schema', '--auto-confirm'], {
     env: process.env,
     stdio: 'inherit'
   });

test/integration/ai.test.js

@@ -2,8 +2,7 @@ import { describe, test, expect, vi, beforeEach, afterEach } from 'vitest';
 import fs from 'fs';
 import os from 'os';
 import path from 'path';
-import exec from '#test/utils/exec';
-import cliPath from '#test/utils/cliPath';
+import cli from '#test/utils/exec';
 
 vi.setConfig({ testTimeout: 60000 });
 
@@ -17,7 +16,7 @@ afterEach(() => {
   fs.rmSync(tmpDir, { recursive: true, force: true });
 });
 
-const run = (options = '') => exec(`${cliPath} ai init ${options}`, { cwd: tmpDir });
+const run = (options = '') => cli(`ai init ${options}`, { cwd: tmpDir });
 
 describe('pos-cli ai init', () => {
   test('--tool claude creates .mcp.json with both servers', async () => {
@@ -48,12 +47,12 @@ describe('pos-cli ai init', () => {
   });
 
   test('help lists the ai command and its init subcommand', async () => {
-    const aiHelp = await exec(`${cliPath} ai --help`);
+    const aiHelp = await cli('ai --help');
     expect(aiHelp.stdout).toMatch('Usage: pos-cli ai');
     expect(aiHelp.stdout).toMatch('init');
     expect(aiHelp.stdout).toMatch('register platformOS MCP servers');
 
-    const rootHelp = await exec(`${cliPath} --help`);
+    const rootHelp = await cli('--help');
     expect(rootHelp.stdout).toMatch('configure AI tools');
   });
 });

test/integration/check.test.js

@@ -1,13 +1,12 @@
 import { describe, test, expect, vi } from 'vitest';
-import exec from '#test/utils/exec';
-import cliPath from '#test/utils/cliPath';
+import cli from '#test/utils/exec';
 import { path as checkPaths } from '@platformos/platformos-check-node';
 import path from 'path';
 
 vi.setConfig({ testTimeout: 60000 });
 
 const cwd = name => path.join(process.cwd(), 'test', 'fixtures', 'check', name);
-const run = (fixtureName, options = '') => exec(`${cliPath} check run ${options}`, { cwd: cwd(fixtureName) });
+const run = (fixtureName, options = '') => cli(`check run ${options}`, { cwd: cwd(fixtureName) });
 
 describe('pos-cli check run', () => {
   describe('Happy path', () => {
@@ -147,7 +146,7 @@ describe('pos-cli check run', () => {
       // is. `app/` is the only marker here, so the message reports an inference, not a fact.
       const root = cwd('with-issues');
       const specificPath = path.join(root, 'app/views/pages');
-      const { stdout, stderr } = await exec(`${cliPath} check run ${specificPath}`);
+      const { stdout, stderr } = await cli(['check', 'run', specificPath]);
 
       // The refusal is platformos-check's own message, and it names a path the way it keys one:
       // through a URI, which lowercases a Windows drive letter. So `D:\…` comes back as `d:\…`,
@@ -194,15 +193,15 @@ describe('pos-cli check run', () => {
   describe('Error handling', () => {
     test('Non-existent path', async () => {
       const nonExistentPath = path.join(process.cwd(), 'test', 'fixtures', 'check', 'nonexistent');
-      const { stderr, code } = await exec(`${cliPath} check run ${nonExistentPath}`);
+      const { stderr, code } = await cli(['check', 'run', nonExistentPath]);
 
       expect(code).toEqual(1);
       expect(stderr).toMatch('Path does not exist');
     });
 
     test('File instead of directory', async () => {
       const filePath = path.join(cwd('clean'), 'app/views/pages/index.liquid');
-      const { stderr, code } = await exec(`${cliPath} check run ${filePath}`);
+      const { stderr, code } = await cli(['check', 'run', filePath]);
 
       expect(code).toEqual(1);
       expect(stderr).toMatch('Path is not a directory');
@@ -219,7 +218,7 @@ describe('pos-cli check run', () => {
       fs.writeFileSync(path.join(tempDir, '.pos'), '{}');
 
       try {
-        const { stdout, code } = await exec(`${cliPath} check init ${tempDir}`);
+        const { stdout, code } = await cli(['check', 'init', tempDir]);
 
         expect(code).toEqual(0);
         expect(stdout).toMatch('Created .platformos-check.yml');
@@ -250,7 +249,7 @@ describe('pos-cli check run', () => {
       fs.writeFileSync(path.join(tempDir, '.platformos-check.yml'), 'existing config');
 
       try {
-        const { stdout, code } = await exec(`${cliPath} check init ${tempDir}`);
+        const { stdout, code } = await cli(['check', 'init', tempDir]);
 
         expect(code).toEqual(0);
         expect(stdout).toMatch('.platformos-check.yml already exists');
@@ -267,7 +266,7 @@ describe('pos-cli check run', () => {
 
   describe('Help text', () => {
     test('Check command help', async () => {
-      const { stdout } = await exec(`${cliPath} check --help`);
+      const { stdout } = await cli('check --help');
 
       expect(stdout).toMatch('Usage: pos-cli check');
       expect(stdout).toMatch('run [path]');
@@ -277,7 +276,7 @@ describe('pos-cli check run', () => {
     });
 
     test('Check run command help', async () => {
-      const { stdout } = await exec(`${cliPath} check run --help`);
+      const { stdout } = await cli('check run --help');
 
       expect(stdout).toMatch('Usage: pos-cli check run');
       expect(stdout).toMatch('-a');
@@ -290,7 +289,7 @@ describe('pos-cli check run', () => {
     });
 
     test('Check init command help', async () => {
-      const { stdout } = await exec(`${cliPath} check init --help`);
+      const { stdout } = await cli('check init --help');
 
       expect(stdout).toMatch('Usage: pos-cli check init');
       expect(stdout).toMatch('initialize .platformos-check.yml configuration file');

test/integration/deploy.test.js

@@ -1,7 +1,6 @@
 import 'dotenv/config';
 import { describe, test, expect, vi, beforeAll } from 'vitest';
-import exec from '#test/utils/exec';
-import cliPath from '#test/utils/cliPath';
+import cli from '#test/utils/exec';
 import unzip from 'unzipper';
 import fs from 'fs';
 import path from 'path';
@@ -23,7 +22,7 @@ beforeAll(() => {
   cleanupTmp();
 });
 
-const run = (fixtureName, options, env = process.env) => exec(`${cliPath} deploy ${options || ''}`, { cwd: cwd(fixtureName), env });
+const run = (fixtureName, options, env = process.env) => cli(`deploy ${options || ''}`, { cwd: cwd(fixtureName), env });
 
 const extract = async (inputPath, outputPath) => {
   return unzip.Open.file(inputPath).then(d => d.extract({ path: outputPath, concurrency: 5 }));