Skip to content

Commit 937e488

Browse files
committed
fix(security): refuse to overwrite an existing release draft
Expose local draft persistence with an explicit repository path and exclusive file creation. Prepare data before changing branches and check for an existing draft again after checkout.
1 parent b4c7544 commit 937e488

5 files changed

Lines changed: 235 additions & 19 deletions

File tree

‎docs/git-node.md‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -506,7 +506,9 @@ the command creates a tracking branch from that ref. Fetch first when remote
506506
state may have changed.
507507

508508
Security release commits reject unrelated staged changes before staging their
509-
own files. Commit or unstage those changes before continuing.
509+
own files. Commit or unstage those changes before continuing. `--start` refuses
510+
to overwrite an existing draft, including one found after switching branches.
511+
Use `--sync`, `--add-report`, or `--remove-report` to update that release.
510512

511513
#### Preparing release data without the CLI
512514

@@ -538,6 +540,15 @@ metadata is listed for follow-up; a draft with no reports can still contain
538540
dependency updates. These checks prepare a draft, not a final-release approval.
539541
The caller owns selection, human review, persistence, and publication.
540542

543+
After reviewing the prepared data, local tools can use
544+
`writeSecurityReleaseDraft(directory, release)` from
545+
`lib/security-release/draft.js`. The directory is an explicit security-release
546+
repository path. The helper validates the draft and creates
547+
`security-release/next-security-release/vulnerabilities.json` with an exclusive
548+
write, so an existing file cannot be overwritten. It does not change Git state
549+
or publish anything. Review and authorization belong to the calling tool; the
550+
CLI retains its directory and file-write confirmations.
551+
541552
### `git node security --apply-patches`
542553

543554
This command fetches the list of reports and the list of PRs labelled for the

‎lib/prepare_security.js‎

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,12 @@
1-
import fs from 'node:fs';
21
import path from 'node:path';
32
import auth from './auth.js';
43
import Request from './request.js';
54
import { parsePRFromURL } from './links.js';
5+
import {
6+
assertNewSecurityRelease,
7+
getSecurityReleaseDraftPath,
8+
writeSecurityReleaseDraft
9+
} from './security-release/draft.js';
610
import {
711
buildIncludedTriagedReport,
812
getReportPRURL,
@@ -14,7 +18,6 @@ import {
1418

1519
import {
1620
NEXT_SECURITY_RELEASE_BRANCH,
17-
NEXT_SECURITY_RELEASE_FOLDER,
1821
checkoutOnSecurityReleaseBranch,
1922
commitAndPushVulnerabilitiesJSON,
2023
validateDate,
@@ -23,7 +26,6 @@ import {
2326
getReportSeverity,
2427
pickReport,
2528
confirmSecurityStep,
26-
writeSecurityFile,
2729
SecurityRelease
2830
} from './security-release/security-release.js';
2931

@@ -100,6 +102,7 @@ export default class PrepareSecurityRelease extends SecurityRelease {
100102
title = 'Next Security Release';
101103

102104
async start() {
105+
assertNewSecurityRelease(process.cwd());
103106
const credentials = await auth({
104107
github: true,
105108
h1: true
@@ -200,17 +203,22 @@ export default class PrepareSecurityRelease extends SecurityRelease {
200203
reportSelectionMode = 'review',
201204
candidates
202205
) {
203-
// checkout on the next-security-release branch
204-
await checkoutOnSecurityReleaseBranch(this.cli, this.repository);
206+
assertNewSecurityRelease(process.cwd());
205207

206208
// choose the reports to include in the security release
207209
const reports = reportSelectionMode === 'include-all'
208210
? await this.includeAllTriagedReports(excludedReports, candidates)
209211
: await this.chooseReports(excludedReports, candidates);
210212
const deps = await this.getDependencyUpdates();
213+
const { release } = prepareSecurityRelease({ releaseDate, reports, dependencies: deps });
211214

212-
// create the vulnerabilities.json file in the security-release repo
213-
const filePath = await this.createVulnerabilitiesJSON(reports, deps, releaseDate);
215+
// Prepare all data before changing Git state. An existing branch may contain
216+
// a draft that was not present on the branch where this command started.
217+
await checkoutOnSecurityReleaseBranch(this.cli, this.repository);
218+
assertNewSecurityRelease(process.cwd());
219+
220+
const filePath = await this.createVulnerabilitiesJSON(
221+
release.reports, release.dependencies, release.releaseDate);
214222

215223
// review the vulnerabilities.json file
216224
const review = await this.promptReviewVulnerabilitiesJSON();
@@ -444,26 +452,23 @@ export default class PrepareSecurityRelease extends SecurityRelease {
444452
}
445453

446454
async createVulnerabilitiesJSON(reports, dependencies, releaseDate) {
447-
this.cli.startSpinner('Creating vulnerabilities.json...');
448455
const { release } = prepareSecurityRelease({ releaseDate, reports, dependencies });
449-
const fileContent = JSON.stringify(release, null, 2) + '\n';
450-
451-
const folderPath = path.resolve(NEXT_SECURITY_RELEASE_FOLDER);
452-
const fullPath = path.join(folderPath, 'vulnerabilities.json');
456+
const directory = process.cwd();
457+
const fullPath = getSecurityReleaseDraftPath(directory);
458+
assertNewSecurityRelease(directory);
453459
await confirmSecurityStep(
454460
this.cli,
455-
`create directory \`${folderPath}\``,
461+
`create directory \`${path.dirname(fullPath)}\``,
456462
'This creates the security release folder if it does not already exist.'
457463
);
458-
await fs.promises.mkdir(folderPath, { recursive: true });
459-
await writeSecurityFile(
464+
await confirmSecurityStep(
460465
this.cli,
461-
fullPath,
462-
fileContent,
466+
`write \`${fullPath}\``,
463467
'This creates vulnerabilities.json for the next security release.'
464468
);
469+
this.cli.startSpinner('Creating vulnerabilities.json...');
470+
writeSecurityReleaseDraft(directory, release);
465471
this.cli.stopSpinner(`Created ${fullPath}`);
466-
467472
return fullPath;
468473
}
469474

‎lib/security-release/draft.js‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
import fs from 'node:fs';
2+
import path from 'node:path';
3+
4+
import { NEXT_SECURITY_RELEASE_FOLDER } from './security-release.js';
5+
import { prepareSecurityRelease } from './preparation.js';
6+
7+
export function getSecurityReleaseDraftPath(directory) {
8+
if (typeof directory !== 'string' || !directory.trim()) {
9+
throw new Error('Security release repository directory is required');
10+
}
11+
return path.resolve(directory, NEXT_SECURITY_RELEASE_FOLDER, 'vulnerabilities.json');
12+
}
13+
14+
export function assertNewSecurityRelease(directory) {
15+
const file = getSecurityReleaseDraftPath(directory);
16+
if (fs.existsSync(file)) {
17+
throw new Error(
18+
`Security release draft already exists: ${file}. ` +
19+
'Use --sync, --add-report, or --remove-report to update the existing release.'
20+
);
21+
}
22+
}
23+
24+
// Local persistence only. The caller handles review, Git, and publication.
25+
export function writeSecurityReleaseDraft(directory, draft) {
26+
const { release } = prepareSecurityRelease(draft);
27+
const file = getSecurityReleaseDraftPath(directory);
28+
assertNewSecurityRelease(directory);
29+
fs.mkdirSync(path.dirname(file), { recursive: true });
30+
fs.writeFileSync(file, JSON.stringify(release, null, 2) + '\n', { flag: 'wx' });
31+
return file;
32+
}

‎test/unit/security_draft.test.js‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
import { describe, it } from 'node:test';
2+
import assert from 'node:assert';
3+
import fs from 'node:fs';
4+
import os from 'node:os';
5+
import path from 'node:path';
6+
7+
import {
8+
assertNewSecurityRelease,
9+
getSecurityReleaseDraftPath,
10+
writeSecurityReleaseDraft
11+
} from '../../lib/security-release/draft.js';
12+
import PrepareSecurityRelease from '../../lib/prepare_security.js';
13+
14+
function directory(t) {
15+
const previous = process.cwd();
16+
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ncu-security-draft-'));
17+
t.after(() => {
18+
process.chdir(previous);
19+
fs.rmSync(dir, { recursive: true, force: true });
20+
});
21+
return dir;
22+
}
23+
24+
describe('security release draft persistence', () => {
25+
it('writes a normalized draft only in the explicit repository directory', (t) => {
26+
const dir = directory(t);
27+
const file = writeSecurityReleaseDraft(dir, {
28+
releaseDate: '2026/10/06', reports: [], dependencies: {}
29+
});
30+
assert.strictEqual(file, path.join(dir,
31+
'security-release', 'next-security-release', 'vulnerabilities.json'));
32+
assert.deepStrictEqual(JSON.parse(fs.readFileSync(file, 'utf8')), {
33+
releaseDate: '2026-10-06', reports: [], dependencies: {}
34+
});
35+
assert.deepStrictEqual(fs.readdirSync(dir), ['security-release']);
36+
});
37+
38+
it('preserves an existing draft byte for byte', (t) => {
39+
const dir = directory(t);
40+
const draft = { releaseDate: 'TBD', reports: [] };
41+
const file = writeSecurityReleaseDraft(dir, draft);
42+
const before = fs.readFileSync(file);
43+
assert.throws(() => assertNewSecurityRelease(dir), /draft already exists/);
44+
assert.throws(() => writeSecurityReleaseDraft(dir, {
45+
releaseDate: '2026-10-06', reports: []
46+
}), /draft already exists/);
47+
assert.deepStrictEqual(fs.readFileSync(file), before);
48+
});
49+
50+
it('uses exclusive creation even if the file appears after preflight', (t) => {
51+
const dir = directory(t);
52+
const file = getSecurityReleaseDraftPath(dir);
53+
const mkdir = fs.mkdirSync;
54+
t.mock.method(fs, 'mkdirSync', (...args) => {
55+
mkdir(...args);
56+
fs.writeFileSync(file, 'Another session\n');
57+
});
58+
assert.throws(() => writeSecurityReleaseDraft(dir, {
59+
releaseDate: 'TBD', reports: []
60+
}), { code: 'EEXIST' });
61+
assert.strictEqual(fs.readFileSync(file, 'utf8'), 'Another session\n');
62+
});
63+
64+
it('validates inputs before creating directories', (t) => {
65+
const dir = directory(t);
66+
assert.throws(() => writeSecurityReleaseDraft(dir, {
67+
releaseDate: '2026-02-30', reports: []
68+
}), /Invalid release date/);
69+
assert.deepStrictEqual(fs.readdirSync(dir), []);
70+
assert.throws(() => getSecurityReleaseDraftPath(), /directory is required/);
71+
});
72+
73+
it('does not write a draft when the file-write confirmation is declined', async(t) => {
74+
const dir = directory(t);
75+
const previous = process.cwd();
76+
t.after(() => process.chdir(previous));
77+
process.chdir(dir);
78+
let prompts = 0;
79+
const release = new PrepareSecurityRelease({
80+
async prompt() {
81+
return ++prompts === 1;
82+
}
83+
});
84+
await assert.rejects(release.createVulnerabilitiesJSON([], {}, 'TBD'), /Aborted: write/);
85+
assert.strictEqual(prompts, 2);
86+
assert.deepStrictEqual(fs.readdirSync(dir), []);
87+
});
88+
});

‎test/unit/security_git.test.js‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@ import os from 'node:os';
55
import path from 'node:path';
66
import { execFileSync } from 'node:child_process';
77

8+
import PrepareSecurityRelease from '../../lib/prepare_security.js';
9+
import { writeSecurityReleaseDraft } from '../../lib/security-release/draft.js';
10+
811
import {
912
checkoutOnSecurityReleaseBranch,
1013
commitAndPushVulnerabilitiesJSON,
@@ -118,4 +121,81 @@ describe('security release git state', { concurrency: false }, () => {
118121
assert.match(prompts[0], /git commit/);
119122
assert.strictEqual(git('write-tree'), index);
120123
});
124+
125+
it('rejects an existing draft before prompting or fetching reports', async(t) => {
126+
const dir = repository(t);
127+
const file = writeSecurityReleaseDraft(dir, { releaseDate: 'TBD', reports: [] });
128+
const before = fs.readFileSync(file);
129+
const release = new PrepareSecurityRelease({
130+
prompt() { assert.fail('Existing releases must not start again'); }
131+
});
132+
await assert.rejects(release.start(), /draft already exists/);
133+
assert.strictEqual(git('branch', '--show-current'), 'main');
134+
assert.deepStrictEqual(fs.readFileSync(file), before);
135+
});
136+
137+
it('preserves Git state if draft preparation fails', async(t) => {
138+
repository(t);
139+
const release = new PrepareSecurityRelease(cli);
140+
release.chooseReports = async() => [];
141+
release.getDependencyUpdates = async() => {
142+
throw new Error('Preparation interrupted');
143+
};
144+
await assert.rejects(
145+
release.startVulnerabilitiesJSONCreation('TBD', 'Release'), /Preparation interrupted/);
146+
assert.strictEqual(git('branch', '--show-current'), 'main');
147+
assert.strictEqual(git('branch', '--list', 'next-security-release'), '');
148+
assert.strictEqual(git('status', '--porcelain'), '');
149+
});
150+
151+
it('preserves a draft discovered on the existing release branch', async(t) => {
152+
const dir = repository(t);
153+
git('checkout', '-b', 'next-security-release');
154+
const file = writeSecurityReleaseDraft(dir, { releaseDate: 'TBD', reports: [] });
155+
const before = fs.readFileSync(file);
156+
git('add', 'security-release');
157+
git('commit', '-m', 'Existing release');
158+
const head = git('rev-parse', 'HEAD');
159+
git('checkout', 'main');
160+
assert.ok(!fs.existsSync(file));
161+
162+
const release = new PrepareSecurityRelease(cli);
163+
release.chooseReports = async() => [];
164+
release.getDependencyUpdates = async() => ({});
165+
await assert.rejects(
166+
release.startVulnerabilitiesJSONCreation('2026-10-06', 'Release'), /draft already exists/);
167+
168+
assert.deepStrictEqual(fs.readFileSync(file), before);
169+
assert.strictEqual(git('rev-parse', 'HEAD'), head);
170+
assert.strictEqual(git('status', '--porcelain'), '');
171+
});
172+
173+
it('creates a local draft without committing when publication is declined', async(t) => {
174+
const dir = repository(t);
175+
fs.writeFileSync('user-notes.txt', 'Staged user work\n');
176+
git('add', 'user-notes.txt');
177+
const index = git('write-tree');
178+
const head = git('rev-parse', 'HEAD');
179+
const release = new PrepareSecurityRelease({
180+
...cli, startSpinner() {}, stopSpinner() {}
181+
});
182+
release.chooseReports = async() => [];
183+
release.getDependencyUpdates = async() => ({
184+
undici: { affectedVersions: { '24.x': 'https://github.com/nodejs/node/pull/1' } }
185+
});
186+
release.promptReviewVulnerabilitiesJSON = async() => false;
187+
release.createPullRequest = async() => assert.fail('Do not publish a local draft');
188+
189+
await release.startVulnerabilitiesJSONCreation('2026/10/06', 'Release');
190+
191+
const file = path.join(dir, 'security-release/next-security-release/vulnerabilities.json');
192+
const draft = JSON.parse(fs.readFileSync(file, 'utf8'));
193+
assert.strictEqual(draft.releaseDate, '2026-10-06');
194+
assert.deepStrictEqual(draft.reports, []);
195+
assert.strictEqual(draft.dependencies.undici.affectedVersions['24.x'],
196+
'https://github.com/nodejs/node/pull/1');
197+
assert.strictEqual(git('write-tree'), index);
198+
assert.strictEqual(git('rev-parse', 'HEAD'), head);
199+
assert.strictEqual(git('branch', '--show-current'), 'next-security-release');
200+
});
121201
});

0 commit comments

Comments
 (0)