Skip to content

Commit d83423e

Browse files
authored
chore: 2nd-pass adversarial audit — fix Critical bugs hidden by Major findings (#25)
Previously declared "done" by PRs #20/#21/#23. Re-audited with the same model treating prior self-assessment as a verification target. Found 4 Critical, 3 Major, 2 Minor. All Critical + Major fixed; Minor as inline TODOs. Critical (silent failure modes — direct reproduction in commit message): C1. scripts/bump-version.cjs: pre-release versions write "1.2.NaN" to package.json silently. Reproduced: `1.2.3-alpha` + patch → version becomes string "1.2.NaN" on disk. Now refuses non-numeric major.minor.patch with exit 1, leaving the previous version untouched. 7 tests added covering happy paths + pre-release + build-metadata + garbage + unknown level. C2. .github/workflows/cd.yml: actions/attest-build-provenance pinned to @v2, but the latest release is v4.1.0 (verified via gh api). The CD workflow is workflow_dispatch only so GH never ran the step — bug would have surfaced at first manual deploy. Bumped @v2@v4. C3. tests/headers.test.js: previous CSP regex `Content-Security-Policy:.*frame-ancestors 'none'` passes vacuously if the `frame-ancestors` directive is deleted entirely (since `.*` matches the rest of the CSP line that has other directives). Same hole for `default-src`, `script-src 'unsafe-inline'`. Rewrote tests to extract the CSP line, parse named directives, and assert presence first then constrain values. New helper cspDirective(name) returns null for absent directives so "absent" is unambiguously distinct from "present but safe." C4. tests/headers.test.js: HSTS regex `max-age=\d{7,}` counts digits, not seconds. `max-age=0000001` (1 second, 7 digits) was passing. Now parses the value and asserts >= 31536000 (1 year). Major (README/docs drift caught by `ls tests/` vs README tree): M1. README.md "What's Included" tree showed only functions.test.js + visits.test.js; reality has 4 (added headers.test.js, bump-version.test.js). M2. README.ko.md mirror of M1. M3. AGENTS.md listed only hello.js + functions.test.js. Updated to list both Pages Functions, all 4 test files, both scripts, and BRANCH_PROTECTION.md. Process improvement (root cause of drift): - package.json test script enumerated test files explicitly, so adding a new test file required two edits — and a missed edit silently lost test coverage. Switched to `node --test 'tests/*.test.js'` so new files auto-discover. This was the structural cause of the headers.test.js / README drift in the first place. Minor (inline TODO(2nd-pass-audit-2026-05-21)): - tests/visits.test.js: 'count' literal duplicated from COUNTER_KEY (drift risk). - functions/api/visits.js: parseInt('123abc',10)→123 silently tolerates partial-numeric corruption. Verification - npm test: 26/26 (up from 19/19; +7 bump-version, headers tests restructured) - npm run lint: clean - npm audit: 0 vulnerabilities
1 parent 9d028e2 commit d83423e

10 files changed

Lines changed: 199 additions & 26 deletions

File tree

.github/workflows/cd.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ jobs:
5858
# Pages does not yet verify attestations server-side, but downstream
5959
# consumers can: `gh attestation verify <file> --owner <org>`.
6060
- name: Attest build provenance for deployed bundle
61-
uses: actions/attest-build-provenance@v2
61+
uses: actions/attest-build-provenance@v4
6262
with:
6363
subject-path: 'src/**/*'
6464

AGENTS.md

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,22 @@ Static site with Cloudflare Pages deployment and GitHub Actions CI/CD.
88
src/
99
index.html → Site entry point
1010
style.css → Styles
11-
main.js → JavaScript (calls /api/hello demo)
11+
main.js → JavaScript (calls /api/hello + /api/visits demos)
1212
functions/
13-
api/hello.js → Example Pages Function
13+
api/hello.js → Example Pages Function (GET-only, 405 on other verbs)
14+
api/visits.js → KV-backed visit counter (GET-only, eventual consistency)
15+
_headers → CSP / HSTS / Permissions-Policy (Cloudflare native)
1416
tests/
15-
functions.test.js → node:test units for Pages Functions
17+
functions.test.js → /api/hello unit tests
18+
visits.test.js → /api/visits unit tests with mock KV
19+
headers.test.js → _headers regression guard
20+
bump-version.test.js → bump-version.cjs behavior + pre-release refusal
1621
scripts/
17-
bump-version.cjs → Version bumping
22+
bump-version.cjs → Version bumping (strict semver only)
23+
check-placeholders.cjs → postinstall warning when name/repo placeholders remain
1824
docs/
1925
CLOUDFLARE_PAGES_SETUP.md → Deployment setup guide
26+
BRANCH_PROTECTION.md → Recommended main ruleset + gh api payload
2027
```
2128

2229
## CI/CD Pipeline

README.ko.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,9 @@ cd my-site && npm install && npm run dev
8383
│ └── visits.js # KV 기반 방문 카운터 → GET /api/visits
8484
├── tests/
8585
│ ├── functions.test.js # /api/hello용 node:test 유닛 테스트
86-
│ └── visits.test.js # /api/visits용 KV 목 테스트
86+
│ ├── visits.test.js # /api/visits용 KV 목 테스트
87+
│ ├── headers.test.js # _headers 회귀 가드 (CSP/HSTS/Permissions-Policy)
88+
│ └── bump-version.test.js # version-bump 스크립트 동작 + pre-release 거부 검증
8789
├── wrangler.toml # Pages 설정 + 주석 처리된 KV 바인딩 예시
8890
├── .github/
8991
│ ├── workflows/

README.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,9 @@ cd my-site && npm install && npm run dev
8383
│ └── visits.js # KV-backed visit counter → GET /api/visits
8484
├── tests/
8585
│ ├── functions.test.js # node:test unit tests for /api/hello
86-
│ └── visits.test.js # Unit tests for /api/visits with mock KV
86+
│ ├── visits.test.js # Unit tests for /api/visits with mock KV
87+
│ ├── headers.test.js # _headers regression guard (CSP/HSTS/Permissions-Policy)
88+
│ └── bump-version.test.js # version-bump script behavior + pre-release refusal
8789
├── wrangler.toml # Pages config + commented KV binding example
8890
├── .github/
8991
│ ├── workflows/

functions/api/visits.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ export async function onRequest(context) {
4040
// warning and surface the recovery via a header so Cloudflare logs and
4141
// monitoring can spot silent counter loss.
4242
const raw = await env.VISITS.get(COUNTER_KEY);
43+
// TODO(2nd-pass-audit-2026-05-21): parseInt('123abc',10) → 123 silently
44+
// accepts partial-numeric corruption. Tighten with /^-?\d+$/.test(raw)
45+
// gate if KV ever holds anything other than counter integers.
4346
const current = parseInt(raw ?? '0', 10);
4447
const recovered = !Number.isFinite(current);
4548
if (recovered) {

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
"scripts": {
88
"dev": "wrangler pages dev src --port 3000",
99
"lint": "eslint --cache src/ functions/ tests/",
10-
"test": "node --test tests/functions.test.js tests/visits.test.js tests/headers.test.js",
10+
"test": "node --test 'tests/*.test.js'",
1111
"deploy": "wrangler pages deploy src --project-name $npm_package_name",
1212
"version:patch": "node scripts/bump-version.cjs patch",
1313
"version:minor": "node scripts/bump-version.cjs minor",

scripts/bump-version.cjs

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,24 @@ const pkgPath = path.resolve(__dirname, '..', 'package.json');
1313
const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8'));
1414

1515
const previous = pkg.version;
16-
const [major, minor, patch] = previous.split('.').map(Number);
16+
17+
// Strict semver: numeric major.minor.patch only. Pre-release / build-metadata
18+
// tags drop information when bumped (e.g. "1.2.3-alpha" + patch → ambiguous),
19+
// so refuse instead of writing a silently-broken version like "1.2.NaN".
20+
const STRICT_SEMVER = /^(\d+)\.(\d+)\.(\d+)$/;
21+
const m = STRICT_SEMVER.exec(previous);
22+
if (!m) {
23+
console.error(
24+
`bump-version: refusing to bump non-numeric version "${previous}". ` +
25+
`Expected major.minor.patch (e.g. 1.2.3). Strip any pre-release / build ` +
26+
`metadata before bumping.`,
27+
);
28+
process.exit(1);
29+
}
30+
const major = Number(m[1]);
31+
const minor = Number(m[2]);
32+
const patch = Number(m[3]);
33+
1734
const next =
1835
level === 'major' ? `${major + 1}.0.0` :
1936
level === 'minor' ? `${major}.${minor + 1}.0` :

tests/bump-version.test.js

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
import { test } from 'node:test';
2+
import assert from 'node:assert/strict';
3+
import { execFileSync } from 'node:child_process';
4+
import { mkdtempSync, writeFileSync, readFileSync, rmSync, cpSync } from 'node:fs';
5+
import { tmpdir } from 'node:os';
6+
import { join, resolve, dirname } from 'node:path';
7+
import { fileURLToPath } from 'node:url';
8+
9+
const HERE = dirname(fileURLToPath(import.meta.url));
10+
const SCRIPT = resolve(HERE, '..', 'scripts', 'bump-version.cjs');
11+
12+
function withTempPkg(version, fn) {
13+
const dir = mkdtempSync(join(tmpdir(), 'bump-version-'));
14+
try {
15+
// Mirror the on-disk layout the script expects: <repo>/scripts/<file>
16+
// and <repo>/package.json, where __dirname is <repo>/scripts.
17+
const scriptsDir = join(dir, 'scripts');
18+
cpSync(dirname(SCRIPT), scriptsDir, { recursive: true });
19+
writeFileSync(join(dir, 'package.json'), JSON.stringify({ name: 't', version }, null, 2) + '\n');
20+
return fn(dir);
21+
} finally {
22+
rmSync(dir, { recursive: true, force: true });
23+
}
24+
}
25+
26+
function run(dir, level) {
27+
try {
28+
const out = execFileSync('node', [join(dir, 'scripts', 'bump-version.cjs'), level], {
29+
encoding: 'utf8',
30+
stdio: ['ignore', 'pipe', 'pipe'],
31+
});
32+
return { code: 0, stdout: out, stderr: '' };
33+
} catch (e) {
34+
return { code: e.status, stdout: e.stdout?.toString() ?? '', stderr: e.stderr?.toString() ?? '' };
35+
}
36+
}
37+
38+
function versionOf(dir) {
39+
return JSON.parse(readFileSync(join(dir, 'package.json'), 'utf8')).version;
40+
}
41+
42+
test('patch bumps the last segment', () => {
43+
withTempPkg('1.2.3', (d) => {
44+
const r = run(d, 'patch');
45+
assert.equal(r.code, 0);
46+
assert.equal(versionOf(d), '1.2.4');
47+
});
48+
});
49+
50+
test('minor bumps middle and zeros patch', () => {
51+
withTempPkg('1.2.3', (d) => {
52+
const r = run(d, 'minor');
53+
assert.equal(r.code, 0);
54+
assert.equal(versionOf(d), '1.3.0');
55+
});
56+
});
57+
58+
test('major bumps first and zeros the rest', () => {
59+
withTempPkg('1.2.3', (d) => {
60+
const r = run(d, 'major');
61+
assert.equal(r.code, 0);
62+
assert.equal(versionOf(d), '2.0.0');
63+
});
64+
});
65+
66+
test('refuses pre-release version instead of writing 1.2.NaN', () => {
67+
withTempPkg('1.2.3-alpha', (d) => {
68+
const r = run(d, 'patch');
69+
assert.equal(r.code, 1, 'must exit non-zero');
70+
assert.match(r.stderr, /refusing to bump/);
71+
// Crucially: the original version is preserved on disk.
72+
assert.equal(versionOf(d), '1.2.3-alpha');
73+
});
74+
});
75+
76+
test('refuses build-metadata version', () => {
77+
withTempPkg('1.2.3+build.5', (d) => {
78+
const r = run(d, 'minor');
79+
assert.equal(r.code, 1);
80+
assert.equal(versionOf(d), '1.2.3+build.5');
81+
});
82+
});
83+
84+
test('refuses garbage version', () => {
85+
withTempPkg('not-a-version', (d) => {
86+
const r = run(d, 'patch');
87+
assert.equal(r.code, 1);
88+
assert.equal(versionOf(d), 'not-a-version');
89+
});
90+
});
91+
92+
test('refuses unknown level argument', () => {
93+
withTempPkg('1.2.3', (d) => {
94+
const r = run(d, 'huge');
95+
assert.equal(r.code, 1);
96+
assert.match(r.stderr, /Usage/);
97+
assert.equal(versionOf(d), '1.2.3');
98+
});
99+
});

tests/headers.test.js

Lines changed: 57 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,37 @@ import { dirname, resolve } from 'node:path';
1111
const HERE = dirname(fileURLToPath(import.meta.url));
1212
const headers = readFileSync(resolve(HERE, '..', '_headers'), 'utf8');
1313

14-
test('declares strict HSTS for /*', () => {
15-
assert.match(headers, /^\s*Strict-Transport-Security:.*max-age=\d{7,}/m);
16-
assert.match(headers, /includeSubDomains/);
14+
// Extract the single Content-Security-Policy directive line once.
15+
// Cloudflare _headers uses one line per header, so the CSP is a single
16+
// physical line. CSP directives are separated by `;`.
17+
function getCSP() {
18+
const m = headers.match(/Content-Security-Policy:[^\n]*/);
19+
return m ? m[0] : null;
20+
}
21+
22+
// Find a single CSP directive's source list (e.g. cspDirective('script-src')
23+
// returns "'self'" if the directive declares `script-src 'self';`).
24+
// Returns null if the directive is absent. This is the key adversarial
25+
// affordance: `null` lets a test distinguish "directive missing" (which
26+
// silently falls back to default-src) from "directive present but loose."
27+
function cspDirective(name) {
28+
const csp = getCSP();
29+
if (!csp) return null;
30+
// Match `<name> <values>` terminated by `;` or end-of-line.
31+
const re = new RegExp(`\\b${name}\\s+([^;]+?)(?:;|$)`);
32+
const m = csp.match(re);
33+
return m ? m[1].trim() : null;
34+
}
35+
36+
test('declares strict HSTS with at least a 1-year max-age', () => {
37+
const m = headers.match(/^\s*Strict-Transport-Security:\s*max-age=(\d+)([^\n]*)/m);
38+
assert.ok(m, 'HSTS header should be present');
39+
const seconds = Number(m[1]);
40+
// 31536000 = 365 days. Anything shorter is a degradation regardless of
41+
// how many digits the literal has — `\d{7,}` alone would accept
42+
// `max-age=0000001`.
43+
assert.ok(seconds >= 31536000, `HSTS max-age must be >= 1y, got ${seconds}s`);
44+
assert.match(m[2], /includeSubDomains/, 'HSTS must include subdomains');
1745
});
1846

1947
test('blocks framing globally', () => {
@@ -35,23 +63,35 @@ test('disables sensitive Permissions-Policy features by default', () => {
3563
}
3664
});
3765

38-
test('CSP forbids frame-ancestors and remote scripts', () => {
39-
assert.match(headers, /^\s*Content-Security-Policy:.*frame-ancestors 'none'/m);
40-
assert.match(headers, /^\s*Content-Security-Policy:.*default-src 'self'/m);
41-
assert.doesNotMatch(headers, /'unsafe-eval'/);
66+
test('CSP declares the directives we depend on (absence ≠ safe)', () => {
67+
// Adversarial: the previous version of this test used
68+
// `assert.match(headers, /.*frame-ancestors 'none'/)`, which passes
69+
// vacuously if the directive is deleted entirely. Assert presence
70+
// first; THEN constrain the value.
71+
assert.ok(getCSP(), 'Content-Security-Policy header must be present');
72+
73+
const defaultSrc = cspDirective('default-src');
74+
assert.ok(defaultSrc !== null, "CSP must declare default-src ('self' fallback)");
75+
assert.match(defaultSrc, /'self'/, "default-src must include 'self'");
76+
77+
const frameAncestors = cspDirective('frame-ancestors');
78+
assert.ok(frameAncestors !== null, 'CSP must declare frame-ancestors');
79+
assert.match(frameAncestors, /'none'/, "frame-ancestors must be 'none'");
80+
81+
const scriptSrc = cspDirective('script-src');
82+
assert.ok(scriptSrc !== null, "CSP must declare script-src explicitly (not rely on default-src fallback)");
4283
});
4384

44-
test("CSP script-src never allows 'unsafe-inline'", () => {
85+
test('CSP forbids unsafe-eval and unsafe-inline scripts', () => {
86+
const csp = getCSP();
87+
assert.doesNotMatch(csp, /'unsafe-eval'/, "CSP must not allow 'unsafe-eval' anywhere");
88+
4589
// style-src 'unsafe-inline' is deliberately permitted for the demo;
46-
// script-src 'unsafe-inline' is not. Pin the inverse so a careless
47-
// CSP edit that adds it to scripts fails CI before deploy.
48-
const cspMatch = headers.match(/Content-Security-Policy:[^\n]*/);
49-
assert.ok(cspMatch, 'CSP header line should be present');
50-
assert.doesNotMatch(
51-
cspMatch[0],
52-
/script-src[^;]*'unsafe-inline'/,
53-
"script-src must not allow 'unsafe-inline'",
54-
);
90+
// script-src 'unsafe-inline' is not. Pin both: the directive must
91+
// exist AND must not include 'unsafe-inline'.
92+
const scriptSrc = cspDirective('script-src');
93+
assert.ok(scriptSrc !== null, "script-src must be declared (asserted above; re-asserting locally for failure clarity)");
94+
assert.doesNotMatch(scriptSrc, /'unsafe-inline'/, "script-src must not allow 'unsafe-inline'");
5595
});
5696

5797
test('/api/* is marked Cache-Control: no-store', () => {

tests/visits.test.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@ import { test } from 'node:test';
22
import assert from 'node:assert/strict';
33
import { onRequest } from '../functions/api/visits.js';
44

5+
// TODO(2nd-pass-audit-2026-05-21): import COUNTER_KEY from ../functions/api/visits.js
6+
// instead of hardcoding 'count' here. Currently the test silently passes if the
7+
// implementation renames the key — drift waiting to happen.
58
function makeMockKV(initial = null) {
69
const store = new Map();
710
if (initial !== null) store.set('count', initial);

0 commit comments

Comments
 (0)