mirror of
https://github.com/affaan-m/everything-claude-code.git
synced 2026-06-16 16:36:53 +08:00
fix(security): add host/origin allowlist + validate git refs + quote workflow input (#2185)
Three defense-in-depth fixes around untrusted input flowing to subprocess execution:
1. **Control-pane HTTP server (scripts/lib/control-pane/server.js)**
The local control-pane API binds to 127.0.0.1 but had no Host or Origin
validation, so a DNS-rebinding attack from a malicious website could pivot
into the loopback endpoints — including POST /api/actions/:id, which spawns
'cargo run -- graph ...' with caller-supplied query strings. Add a hostname
allowlist (loopback variants plus the explicitly configured --host) and
reject mismatched Host (421) or non-loopback Origin (403) before any route
handler runs.
2. **OpenCode git-summary tool (.opencode/tools/git-summary.ts)**
The tool was building 'git diff ${baseBranch}...HEAD --stat' with execSync
and a raw model-supplied baseBranch string. Switch run() to execFileSync
with an args array (no shell), validate baseBranch against a conservative
git-ref allowlist (rejects shell metacharacters, leading -, embedded ..),
and clamp the depth arg to a small positive integer before interpolating
into 'git log --oneline -<N>'.
3. **Reusable test workflow (.github/workflows/reusable-test.yml)**
The 'Install dependencies' step interpolated ${{ inputs.package-manager }}
directly into a bash 'case' and into an echo, so a downstream caller that
forwarded attacker-controllable input could inject into the runner. Move
the input into a PACKAGE_MANAGER env var and reference $PACKAGE_MANAGER
inside the script per the GitHub script-injection guidance.
Detected by Aeon + semgrep p/security-audit (host check via threat-model
manual-review axis; git-summary via detect-child-process; workflow via
run-shell-injection).
Verification: node tests/run-all.js — 2686/2687 pre-existing tests pass; the
one failure (observe.sh legacy output fallback) reproduces on main without
this branch applied. Added 2 new control-pane tests covering the allowlist
classifier and the DNS-rebinding-gate behavior end-to-end.
---
Filed by [Aeon](https://github.com/aaronjmars/aeon-aaron).
Co-authored-by: aeonframework <aeon@aaronjmars.com>
This commit is contained in:
parent
41065bc0b2
commit
1c3280dc0d
5
.github/workflows/reusable-test.yml
vendored
5
.github/workflows/reusable-test.yml
vendored
@ -69,8 +69,9 @@ jobs:
|
|||||||
COREPACK_ENABLE_STRICT: '0'
|
COREPACK_ENABLE_STRICT: '0'
|
||||||
npm_config_ignore_scripts: 'true'
|
npm_config_ignore_scripts: 'true'
|
||||||
YARN_ENABLE_SCRIPTS: 'false'
|
YARN_ENABLE_SCRIPTS: 'false'
|
||||||
|
PACKAGE_MANAGER: ${{ inputs.package-manager }}
|
||||||
run: |
|
run: |
|
||||||
case "${{ inputs.package-manager }}" in
|
case "$PACKAGE_MANAGER" in
|
||||||
npm) npm ci --ignore-scripts ;;
|
npm) npm ci --ignore-scripts ;;
|
||||||
# pnpm v10 can fail CI on ignored native build scripts
|
# pnpm v10 can fail CI on ignored native build scripts
|
||||||
# (for example msgpackr-extract) even though this repo is Yarn-native
|
# (for example msgpackr-extract) even though this repo is Yarn-native
|
||||||
@ -79,7 +80,7 @@ jobs:
|
|||||||
# Yarn Berry (v4+) removed --ignore-engines; engine checking is no longer a core feature
|
# Yarn Berry (v4+) removed --ignore-engines; engine checking is no longer a core feature
|
||||||
yarn) yarn install --mode=skip-build ;;
|
yarn) yarn install --mode=skip-build ;;
|
||||||
bun) bun install --ignore-scripts ;;
|
bun) bun install --ignore-scripts ;;
|
||||||
*) echo "Unsupported package manager: ${{ inputs.package-manager }}" && exit 1 ;;
|
*) echo "Unsupported package manager: $PACKAGE_MANAGER" && exit 1 ;;
|
||||||
esac
|
esac
|
||||||
|
|
||||||
- name: Run tests
|
- name: Run tests
|
||||||
|
|||||||
@ -5,7 +5,24 @@
|
|||||||
*/
|
*/
|
||||||
|
|
||||||
import { tool, type ToolDefinition } from "@opencode-ai/plugin/tool"
|
import { tool, type ToolDefinition } from "@opencode-ai/plugin/tool"
|
||||||
import { execSync } from "child_process"
|
import { execFileSync } from "child_process"
|
||||||
|
|
||||||
|
// Conservative subset of git's allowed ref-name characters. Rejects shell
|
||||||
|
// metacharacters and option-like leading `-` so a model-supplied baseBranch
|
||||||
|
// cannot inject into the shell command line built below.
|
||||||
|
const SAFE_GIT_REF = /^[A-Za-z0-9._/-]+$/
|
||||||
|
|
||||||
|
function isSafeRef(ref: string): boolean {
|
||||||
|
if (typeof ref !== "string" || ref.length === 0 || ref.length > 200) return false
|
||||||
|
if (!SAFE_GIT_REF.test(ref)) return false
|
||||||
|
if (ref.startsWith("-") || ref.startsWith(".") || ref.startsWith("/")) return false
|
||||||
|
if (ref.includes("..") || ref.includes("//")) return false
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
|
||||||
|
function isSafeDepth(value: unknown): value is number {
|
||||||
|
return typeof value === "number" && Number.isInteger(value) && value > 0 && value <= 1000
|
||||||
|
}
|
||||||
|
|
||||||
const gitSummaryTool: ToolDefinition = tool({
|
const gitSummaryTool: ToolDefinition = tool({
|
||||||
description:
|
description:
|
||||||
@ -26,19 +43,22 @@ const gitSummaryTool: ToolDefinition = tool({
|
|||||||
},
|
},
|
||||||
async execute(args, context) {
|
async execute(args, context) {
|
||||||
const cwd = context.worktree || context.directory
|
const cwd = context.worktree || context.directory
|
||||||
const depth = args.depth ?? 5
|
const depth = isSafeDepth(args.depth) ? args.depth : 5
|
||||||
const includeDiff = args.includeDiff ?? true
|
const includeDiff = args.includeDiff ?? true
|
||||||
const baseBranch = args.baseBranch ?? "main"
|
const baseBranch = args.baseBranch ?? "main"
|
||||||
|
|
||||||
const result: Record<string, string> = {
|
const result: Record<string, string> = {
|
||||||
branch: run("git branch --show-current", cwd) || "unknown",
|
branch: runArgs(["branch", "--show-current"], cwd) || "unknown",
|
||||||
status: run("git status --short", cwd) || "clean",
|
status: runArgs(["status", "--short"], cwd) || "clean",
|
||||||
log: run(`git log --oneline -${depth}`, cwd) || "no commits found",
|
log: runArgs(["log", "--oneline", `-${depth}`], cwd) || "no commits found",
|
||||||
}
|
}
|
||||||
|
|
||||||
if (includeDiff) {
|
if (includeDiff) {
|
||||||
result.stagedDiff = run("git diff --cached --stat", cwd) || ""
|
result.stagedDiff = runArgs(["diff", "--cached", "--stat"], cwd) || ""
|
||||||
result.branchDiff = run(`git diff ${baseBranch}...HEAD --stat`, cwd) || `unable to diff against ${baseBranch}`
|
result.branchDiff = isSafeRef(baseBranch)
|
||||||
|
? runArgs(["diff", `${baseBranch}...HEAD`, "--stat"], cwd) ||
|
||||||
|
`unable to diff against ${baseBranch}`
|
||||||
|
: `unable to diff against ${baseBranch} (invalid ref)`
|
||||||
}
|
}
|
||||||
|
|
||||||
return JSON.stringify(result)
|
return JSON.stringify(result)
|
||||||
@ -47,9 +67,9 @@ const gitSummaryTool: ToolDefinition = tool({
|
|||||||
|
|
||||||
export default gitSummaryTool
|
export default gitSummaryTool
|
||||||
|
|
||||||
function run(command: string, cwd: string): string {
|
function runArgs(args: string[], cwd: string): string {
|
||||||
try {
|
try {
|
||||||
return execSync(command, { cwd, encoding: "utf-8", stdio: ["ignore", "pipe", "pipe"] }).trim()
|
return execFileSync("git", args, { cwd, encoding: "utf-8", stdio: ["ignore", "pipe", "pipe"] }).trim()
|
||||||
} catch {
|
} catch {
|
||||||
return ""
|
return ""
|
||||||
}
|
}
|
||||||
|
|||||||
@ -9,6 +9,43 @@ const { buildControlPaneAction } = require('./actions');
|
|||||||
const { buildControlPaneSnapshot, resolveControlPaneConfig } = require('./state');
|
const { buildControlPaneSnapshot, resolveControlPaneConfig } = require('./state');
|
||||||
const { renderControlPaneHtml } = require('./ui');
|
const { renderControlPaneHtml } = require('./ui');
|
||||||
|
|
||||||
|
const LOOPBACK_HOSTNAMES = new Set(['127.0.0.1', 'localhost', '[::1]', '::1']);
|
||||||
|
|
||||||
|
// Extract the hostname portion of an HTTP Host header value, stripping any
|
||||||
|
// port. Returns null when the header is missing or malformed. Used to gate
|
||||||
|
// requests against a local-only allowlist so DNS-rebinding cannot pivot a
|
||||||
|
// browser tab into the loopback control-pane API.
|
||||||
|
function parseHostHeader(value) {
|
||||||
|
if (!value || typeof value !== 'string') return null;
|
||||||
|
const trimmed = value.trim();
|
||||||
|
if (!trimmed) return null;
|
||||||
|
const match = trimmed.match(/^(\[[^\]]+\]|[^:]+)(?::\d+)?$/);
|
||||||
|
if (!match) return null;
|
||||||
|
return match[1].toLowerCase();
|
||||||
|
}
|
||||||
|
|
||||||
|
function buildAllowedHostnames(configuredHost) {
|
||||||
|
const set = new Set(LOOPBACK_HOSTNAMES);
|
||||||
|
if (configuredHost) set.add(String(configuredHost).toLowerCase());
|
||||||
|
return set;
|
||||||
|
}
|
||||||
|
|
||||||
|
function isAllowedHostHeader(hostHeader, allowedHostnames) {
|
||||||
|
const hostname = parseHostHeader(hostHeader);
|
||||||
|
if (!hostname) return false;
|
||||||
|
return allowedHostnames.has(hostname);
|
||||||
|
}
|
||||||
|
|
||||||
|
function isAllowedOrigin(originHeader, allowedHostnames) {
|
||||||
|
if (!originHeader || typeof originHeader !== 'string') return true;
|
||||||
|
try {
|
||||||
|
const url = new URL(originHeader);
|
||||||
|
return allowedHostnames.has(url.hostname.toLowerCase());
|
||||||
|
} catch {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
function usage() {
|
function usage() {
|
||||||
return [
|
return [
|
||||||
'Usage:',
|
'Usage:',
|
||||||
@ -159,9 +196,19 @@ function createControlPaneServer(options = {}) {
|
|||||||
env: options.env || process.env,
|
env: options.env || process.env,
|
||||||
});
|
});
|
||||||
const baseQuery = options.query || '';
|
const baseQuery = options.query || '';
|
||||||
|
const allowedHostnames = buildAllowedHostnames(host);
|
||||||
|
|
||||||
const server = http.createServer(async (req, res) => {
|
const server = http.createServer(async (req, res) => {
|
||||||
try {
|
try {
|
||||||
|
if (!isAllowedHostHeader(req.headers.host, allowedHostnames)) {
|
||||||
|
sendJson(res, 421, { ok: false, error: 'Misdirected request' });
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
if (!isAllowedOrigin(req.headers.origin, allowedHostnames)) {
|
||||||
|
sendJson(res, 403, { ok: false, error: 'Forbidden origin' });
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
const requestUrl = new URL(req.url, `http://${host}:${port || 0}`);
|
const requestUrl = new URL(req.url, `http://${host}:${port || 0}`);
|
||||||
|
|
||||||
if (req.method === 'GET' && requestUrl.pathname === '/') {
|
if (req.method === 'GET' && requestUrl.pathname === '/') {
|
||||||
@ -280,5 +327,8 @@ module.exports = {
|
|||||||
createControlPaneServer,
|
createControlPaneServer,
|
||||||
parseArgs,
|
parseArgs,
|
||||||
runAction,
|
runAction,
|
||||||
|
isAllowedHostHeader,
|
||||||
|
isAllowedOrigin,
|
||||||
|
buildAllowedHostnames,
|
||||||
usage,
|
usage,
|
||||||
};
|
};
|
||||||
|
|||||||
@ -4,6 +4,7 @@
|
|||||||
|
|
||||||
const assert = require('assert');
|
const assert = require('assert');
|
||||||
const fs = require('fs');
|
const fs = require('fs');
|
||||||
|
const http = require('http');
|
||||||
const os = require('os');
|
const os = require('os');
|
||||||
const path = require('path');
|
const path = require('path');
|
||||||
const { spawn, spawnSync } = require('child_process');
|
const { spawn, spawnSync } = require('child_process');
|
||||||
@ -14,6 +15,9 @@ const {
|
|||||||
createControlPaneServer,
|
createControlPaneServer,
|
||||||
parseArgs,
|
parseArgs,
|
||||||
runAction,
|
runAction,
|
||||||
|
isAllowedHostHeader,
|
||||||
|
isAllowedOrigin,
|
||||||
|
buildAllowedHostnames,
|
||||||
} = require('../../scripts/lib/control-pane/server');
|
} = require('../../scripts/lib/control-pane/server');
|
||||||
const {
|
const {
|
||||||
main: runControlPaneCli,
|
main: runControlPaneCli,
|
||||||
@ -326,6 +330,92 @@ async function runTests() {
|
|||||||
}
|
}
|
||||||
})) passed++; else failed++;
|
})) passed++; else failed++;
|
||||||
|
|
||||||
|
if (await test('classifies Host and Origin headers against the loopback allowlist', async () => {
|
||||||
|
const allowed = buildAllowedHostnames('127.0.0.1');
|
||||||
|
assert.strictEqual(isAllowedHostHeader('127.0.0.1:8765', allowed), true);
|
||||||
|
assert.strictEqual(isAllowedHostHeader('localhost:8765', allowed), true);
|
||||||
|
assert.strictEqual(isAllowedHostHeader('LOCALHOST:8765', allowed), true);
|
||||||
|
assert.strictEqual(isAllowedHostHeader('[::1]:8765', allowed), true);
|
||||||
|
assert.strictEqual(isAllowedHostHeader('attacker.example.com:8765', allowed), false);
|
||||||
|
assert.strictEqual(isAllowedHostHeader('rebind.dnsbin.io', allowed), false);
|
||||||
|
assert.strictEqual(isAllowedHostHeader('', allowed), false);
|
||||||
|
assert.strictEqual(isAllowedHostHeader(undefined, allowed), false);
|
||||||
|
|
||||||
|
// Origin is optional; absence is allowed for non-browser clients.
|
||||||
|
assert.strictEqual(isAllowedOrigin(undefined, allowed), true);
|
||||||
|
assert.strictEqual(isAllowedOrigin('', allowed), true);
|
||||||
|
assert.strictEqual(isAllowedOrigin('http://127.0.0.1:8765', allowed), true);
|
||||||
|
assert.strictEqual(isAllowedOrigin('http://localhost', allowed), true);
|
||||||
|
assert.strictEqual(isAllowedOrigin('http://attacker.example.com', allowed), false);
|
||||||
|
assert.strictEqual(isAllowedOrigin('not-a-url', allowed), false);
|
||||||
|
|
||||||
|
// A non-default configured host should still admit loopback variants.
|
||||||
|
const lan = buildAllowedHostnames('192.168.1.10');
|
||||||
|
assert.strictEqual(isAllowedHostHeader('192.168.1.10:8765', lan), true);
|
||||||
|
assert.strictEqual(isAllowedHostHeader('127.0.0.1:8765', lan), true);
|
||||||
|
assert.strictEqual(isAllowedHostHeader('attacker.example.com:8765', lan), false);
|
||||||
|
})) passed++; else failed++;
|
||||||
|
|
||||||
|
if (await test('rejects requests forged with a non-loopback Host header (DNS rebinding gate)', async () => {
|
||||||
|
const app = await createControlPaneServer({
|
||||||
|
host: '127.0.0.1',
|
||||||
|
port: 0,
|
||||||
|
repoRoot: REPO_ROOT,
|
||||||
|
allowActions: true,
|
||||||
|
});
|
||||||
|
|
||||||
|
await app.listen();
|
||||||
|
try {
|
||||||
|
const address = app.server.address();
|
||||||
|
const actualPort = address && typeof address === 'object' ? address.port : 0;
|
||||||
|
|
||||||
|
const sendWithHeaders = (method, pathname, headers, body) =>
|
||||||
|
new Promise((resolve, reject) => {
|
||||||
|
const req = http.request(
|
||||||
|
{ host: '127.0.0.1', port: actualPort, method, path: pathname, headers },
|
||||||
|
response => {
|
||||||
|
let chunks = '';
|
||||||
|
response.on('data', chunk => {
|
||||||
|
chunks += chunk.toString('utf8');
|
||||||
|
});
|
||||||
|
response.on('end', () => {
|
||||||
|
resolve({ status: response.statusCode, body: chunks });
|
||||||
|
});
|
||||||
|
}
|
||||||
|
);
|
||||||
|
req.on('error', reject);
|
||||||
|
if (body) req.write(body);
|
||||||
|
req.end();
|
||||||
|
});
|
||||||
|
|
||||||
|
const forgedHost = await sendWithHeaders('GET', '/api/health', { Host: 'attacker.example.com:1234' });
|
||||||
|
assert.strictEqual(forgedHost.status, 421);
|
||||||
|
assert.match(forgedHost.body, /Misdirected request/);
|
||||||
|
|
||||||
|
const forgedActionHost = await sendWithHeaders(
|
||||||
|
'POST',
|
||||||
|
'/api/actions/sync-knowledge',
|
||||||
|
{ Host: 'attacker.example.com:1234', 'content-type': 'application/json' },
|
||||||
|
JSON.stringify({ query: 'rebound' })
|
||||||
|
);
|
||||||
|
assert.strictEqual(forgedActionHost.status, 421);
|
||||||
|
|
||||||
|
const forgedOrigin = await sendWithHeaders('GET', '/api/health', {
|
||||||
|
Host: '127.0.0.1:' + actualPort,
|
||||||
|
Origin: 'http://attacker.example.com',
|
||||||
|
});
|
||||||
|
assert.strictEqual(forgedOrigin.status, 403);
|
||||||
|
assert.match(forgedOrigin.body, /Forbidden origin/);
|
||||||
|
|
||||||
|
const okHost = await sendWithHeaders('GET', '/api/health', { Host: '127.0.0.1:' + actualPort });
|
||||||
|
assert.strictEqual(okHost.status, 200);
|
||||||
|
const okBody = JSON.parse(okHost.body);
|
||||||
|
assert.strictEqual(okBody.ok, true);
|
||||||
|
} finally {
|
||||||
|
await app.close();
|
||||||
|
}
|
||||||
|
})) passed++; else failed++;
|
||||||
|
|
||||||
if (await test('runAction captures success, failure, and bounded output', async () => {
|
if (await test('runAction captures success, failure, and bounded output', async () => {
|
||||||
const repoRoot = REPO_ROOT;
|
const repoRoot = REPO_ROOT;
|
||||||
const success = await runAction({
|
const success = await runAction({
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user