Fix five P0/P1 security & correctness findings from the full-eval
- Arbitrary file write (P0): validate import keys in /api/library/import via a now-exported safeFilename(); a ../../ key is skipped, not written out of the scope dir. - SSRF (P0): guard downloadPodcastAudio — reject non-HTTP(S) schemes, block IP-literal and DNS-resolved private/link-local/loopback/reserved/multicast and embedded-IPv4 IPv6 targets (closes DNS rebinding), cap + resolve redirects. - ESM require (P1): top-level import of randomBytes in license-purchase.js (the inner require threw on the anon purchase-settle path). - Concurrency lock (P1): skip the process-global free-tier slot in multi-mode so it no longer serializes every cloud tenant onto one job. - X-Forwarded-For bypass (P1): set Express trust proxy from RECAP_TRUSTED_PROXY_HOPS (default 1); getClientIp now reads req.ip instead of a client-spoofable XFF entry. Tests added for safeFilename, the SSRF guard, and getClientIp (119 pass). Registry blockers deferred (ROADMAP); leaked-key history purge queued.
This commit is contained in:
@@ -0,0 +1,37 @@
|
||||
// Tests for server/anon-trial.js — focused on getClientIp, which underpins
|
||||
// the per-IP trial cap. The DB-backed minting paths need a multi-mode SQLite
|
||||
// handle and are exercised by integration tests; here we lock down that the
|
||||
// client IP is taken from Express's trust-proxy-resolved req.ip, never from a
|
||||
// raw client-supplied X-Forwarded-For header.
|
||||
|
||||
import { test, describe } from "node:test";
|
||||
import { strict as assert } from "node:assert";
|
||||
import { getClientIp } from "../anon-trial.js";
|
||||
|
||||
describe("getClientIp", () => {
|
||||
test("uses req.ip (Express's trust-proxy-resolved client address)", () => {
|
||||
assert.equal(getClientIp({ ip: "203.0.113.7" }), "203.0.113.7");
|
||||
});
|
||||
|
||||
test("strips the IPv4-mapped IPv6 prefix", () => {
|
||||
assert.equal(getClientIp({ ip: "::ffff:203.0.113.7" }), "203.0.113.7");
|
||||
});
|
||||
|
||||
test("falls back to the socket address when req.ip is absent", () => {
|
||||
assert.equal(
|
||||
getClientIp({ socket: { remoteAddress: "::ffff:198.51.100.9" } }),
|
||||
"198.51.100.9",
|
||||
);
|
||||
});
|
||||
|
||||
test("does NOT trust a raw client-supplied X-Forwarded-For header", () => {
|
||||
// Express, not getClientIp, decides the client IP from trust proxy. A
|
||||
// header Express hasn't blessed must be ignored — so with no req.ip we
|
||||
// fall through to the socket address, never the spoofed header value.
|
||||
const spoofed = {
|
||||
headers: { "x-forwarded-for": "1.2.3.4" },
|
||||
socket: { remoteAddress: "203.0.113.7" },
|
||||
};
|
||||
assert.equal(getClientIp(spoofed), "203.0.113.7");
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,88 @@
|
||||
// Tests for server/audio.js — focused on the SSRF guard around
|
||||
// downloadPodcastAudio (a fully user-controlled outbound fetch). The
|
||||
// ffprobe/ffmpeg helpers need real media + binaries and aren't covered here.
|
||||
|
||||
import { test, describe } from "node:test";
|
||||
import { strict as assert } from "node:assert";
|
||||
import { isBlockedAddress, downloadPodcastAudio } from "../audio.js";
|
||||
|
||||
const SINK = "/tmp/recap-audio-test-should-not-be-written";
|
||||
|
||||
describe("isBlockedAddress", () => {
|
||||
test("blocks IPv4 loopback / private / link-local / reserved", () => {
|
||||
for (const ip of [
|
||||
"127.0.0.1", "127.1.2.3",
|
||||
"10.0.0.1", "172.16.0.1", "172.31.255.255", "192.168.1.1",
|
||||
"169.254.169.254", // cloud metadata
|
||||
"100.64.0.1", // CGNAT
|
||||
"0.0.0.0",
|
||||
"224.0.0.1", "255.255.255.255",
|
||||
]) {
|
||||
assert.equal(isBlockedAddress(ip), true, `${ip} should be blocked`);
|
||||
}
|
||||
});
|
||||
|
||||
test("allows ordinary public IPv4 (incl. 172.x boundaries)", () => {
|
||||
for (const ip of ["8.8.8.8", "1.1.1.1", "93.184.216.34", "172.15.0.1", "172.32.0.1"]) {
|
||||
assert.equal(isBlockedAddress(ip), false, `${ip} should be allowed`);
|
||||
}
|
||||
});
|
||||
|
||||
test("blocks IPv6 loopback / ULA / link-local / multicast + IPv4-mapped privates", () => {
|
||||
for (const ip of [
|
||||
"::1", "::", "fc00::1", "fd12:3456::1", "fe80::1", "ff02::1",
|
||||
"::ffff:127.0.0.1", "::ffff:169.254.169.254",
|
||||
]) {
|
||||
assert.equal(isBlockedAddress(ip), true, `${ip} should be blocked`);
|
||||
}
|
||||
});
|
||||
|
||||
test("blocks hex-encoded embedded-IPv4 IPv6 forms (mapped/SIIT/NAT64/6to4)", () => {
|
||||
for (const ip of [
|
||||
"::ffff:7f00:1", // IPv4-mapped 127.0.0.1, hex form
|
||||
"::ffff:0:7f00:1", // SIIT 127.0.0.1
|
||||
"64:ff9b::7f00:1", // NAT64 well-known prefix of 127.0.0.1
|
||||
"2002:7f00:1::", // 6to4 of 127.0.0.1
|
||||
]) {
|
||||
assert.equal(isBlockedAddress(ip), true, `${ip} should be blocked`);
|
||||
}
|
||||
});
|
||||
|
||||
test("allows ordinary public IPv6", () => {
|
||||
assert.equal(isBlockedAddress("2606:4700:4700::1111"), false);
|
||||
assert.equal(isBlockedAddress("::ffff:8.8.8.8"), false);
|
||||
});
|
||||
|
||||
test("blocks junk / empty / non-strings", () => {
|
||||
assert.equal(isBlockedAddress(""), true);
|
||||
assert.equal(isBlockedAddress(null), true);
|
||||
assert.equal(isBlockedAddress("not-an-ip"), true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("downloadPodcastAudio SSRF guard", () => {
|
||||
test("rejects non-HTTP(S) schemes", async () => {
|
||||
await assert.rejects(downloadPodcastAudio("file:///etc/passwd", SINK), /non-HTTP/);
|
||||
await assert.rejects(downloadPodcastAudio("ftp://example.com/x", SINK), /non-HTTP/);
|
||||
});
|
||||
|
||||
test("rejects internal / private destinations before connecting", async () => {
|
||||
// 127.0.0.1:1 would refuse instantly if we connected; we must reject at
|
||||
// the DNS-guard step instead (proving the guard fires before connect).
|
||||
await assert.rejects(
|
||||
downloadPodcastAudio("http://127.0.0.1:1/x", SINK),
|
||||
/disallowed address/,
|
||||
);
|
||||
await assert.rejects(
|
||||
downloadPodcastAudio("http://169.254.169.254/latest/meta-data/", SINK),
|
||||
/disallowed address/,
|
||||
);
|
||||
});
|
||||
|
||||
test("rejects a malformed URL", async () => {
|
||||
await assert.rejects(
|
||||
downloadPodcastAudio("not a url at all", SINK),
|
||||
/invalid podcast audio URL/,
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -138,3 +138,24 @@ describe("loadMeta + saveMeta", () => {
|
||||
assert.deepEqual(loaded.uncategorized, []);
|
||||
});
|
||||
});
|
||||
|
||||
describe("safeFilename", () => {
|
||||
// Exported so callers writing user content to disk (e.g. library import)
|
||||
// can share the one guard instead of rolling their own.
|
||||
test("returns valid ids unchanged", () => {
|
||||
for (const id of ["abc123", "a_b-c", "VIDEOid123", "0"]) {
|
||||
assert.equal(history.safeFilename(id), id);
|
||||
}
|
||||
});
|
||||
|
||||
test("rejects traversal, separators, and other unsafe chars", () => {
|
||||
for (const bad of ["../../evil", "..", "a/b", "a\\b", "a.json", "foo bar", "", "a:b"]) {
|
||||
assert.throws(() => history.safeFilename(bad), /invalid_session_id/);
|
||||
}
|
||||
});
|
||||
|
||||
test("rejects non-strings", () => {
|
||||
assert.throws(() => history.safeFilename(123), /invalid_session_id/);
|
||||
assert.throws(() => history.safeFilename(null), /invalid_session_id/);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user