Skip to content

Commit bc4ca1d

Browse files
betegonclaude
andcommitted
fix: address PR review issues in interactive and local-ops
- Guard against empty multiselect options when only errorMonitoring is available - Use purpose field for example detection in handleConfirm with string fallback - Block glob (*, ?) and brace ({, }) expansion in shell metacharacter validation - Improve metacharacter ordering docs and add Unix shell scope comment Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 39e4b5a commit bc4ca1d

File tree

4 files changed

+93
-3
lines changed

4 files changed

+93
-3
lines changed

src/lib/init/interactive.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,13 @@ async function handleMultiSelect(
9696

9797
const optional = available.filter((f) => f !== REQUIRED_FEATURE);
9898

99+
if (optional.length === 0) {
100+
if (hasRequired) {
101+
log.info(`${featureLabel(REQUIRED_FEATURE)} is always included.`);
102+
}
103+
return { features: hasRequired ? [REQUIRED_FEATURE] : [] };
104+
}
105+
99106
const hints: string[] = [];
100107
if (hasRequired) {
101108
hints.push(
@@ -127,8 +134,11 @@ async function handleConfirm(
127134
payload: InteractivePayload,
128135
options: WizardOptions
129136
): Promise<Record<string, unknown>> {
137+
const isExample =
138+
payload.purpose === "add-example" || payload.prompt.includes("example");
139+
130140
if (options.yes) {
131-
if (payload.prompt.includes("example")) {
141+
if (isExample) {
132142
log.info("Auto-confirmed: adding example trigger");
133143
return { addExample: true };
134144
}
@@ -143,7 +153,7 @@ async function handleConfirm(
143153

144154
const value = abortIfCancelled(confirmed);
145155

146-
if (payload.prompt.includes("example")) {
156+
if (isExample) {
147157
return { addExample: value };
148158
}
149159
return { action: value ? "continue" : "stop" };

src/lib/init/local-ops.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,15 @@ import type {
2727
/**
2828
* Shell metacharacters that enable chaining, piping, substitution, or redirection.
2929
* All legitimate install commands are simple single commands that don't need these.
30+
*
31+
* Ordering matters for error-message accuracy (not correctness): multi-character
32+
* operators like `&&` and `||` are checked before their single-character prefixes
33+
* (`&`, `|`) so the reported label describes the actual construct the user wrote.
3034
*/
3135
const SHELL_METACHARACTER_PATTERNS: Array<{ pattern: string; label: string }> =
3236
[
3337
{ pattern: ";", label: "command chaining (;)" },
34-
// Check multi-char operators before single `|` so labels are accurate
38+
// Check multi-char operators before single `|` / `&` so labels are accurate
3539
{ pattern: "&&", label: "command chaining (&&)" },
3640
{ pattern: "||", label: "command chaining (||)" },
3741
{ pattern: "|", label: "piping (|)" },
@@ -48,6 +52,12 @@ const SHELL_METACHARACTER_PATTERNS: Array<{ pattern: string; label: string }> =
4852
{ pattern: ">", label: "redirection (>)" },
4953
{ pattern: "<", label: "redirection (<)" },
5054
{ pattern: "&", label: "background execution (&)" },
55+
// Glob and brace expansion — brace expansion is the real risk
56+
// (e.g. `npm install {evil,@sentry/node}`)
57+
{ pattern: "{", label: "brace expansion ({)" },
58+
{ pattern: "}", label: "brace expansion (})" },
59+
{ pattern: "*", label: "glob expansion (*)" },
60+
{ pattern: "?", label: "glob expansion (?)" },
5161
];
5262

5363
const WHITESPACE_RE = /\s+/;
@@ -336,6 +346,8 @@ async function runCommands(
336346
return { ok: true, data: { results } };
337347
}
338348

349+
// Note: shell: true targets Unix shells. Windows cmd.exe metacharacters
350+
// (%, ^) are not blocked; the CLI assumes a Unix Node.js environment.
339351
function runSingleCommand(
340352
command: string,
341353
cwd: string,

test/lib/init/interactive.test.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,21 @@ describe("handleMultiSelect", () => {
252252
).rejects.toThrow("Setup cancelled");
253253
});
254254

255+
test("returns required feature without calling multiselect when only errorMonitoring available", async () => {
256+
const result = await handleInteractive(
257+
{
258+
type: "interactive",
259+
prompt: "Select features",
260+
kind: "multi-select",
261+
availableFeatures: ["errorMonitoring"],
262+
},
263+
makeOptions({ yes: false })
264+
);
265+
266+
expect(result).toEqual({ features: ["errorMonitoring"] });
267+
expect(multiselectSpy).not.toHaveBeenCalled();
268+
});
269+
255270
test("excludes errorMonitoring from multiselect options (always included)", async () => {
256271
multiselectSpy.mockImplementation(
257272
() => Promise.resolve(["performanceMonitoring"]) as any
@@ -336,6 +351,49 @@ describe("handleConfirm", () => {
336351
).rejects.toThrow("Setup cancelled");
337352
});
338353

354+
test("returns addExample: true via purpose field with --yes", async () => {
355+
const result = await handleInteractive(
356+
{
357+
type: "interactive",
358+
prompt: "Would you like to add a trigger?",
359+
kind: "confirm",
360+
purpose: "add-example",
361+
},
362+
makeOptions({ yes: true })
363+
);
364+
365+
expect(result).toEqual({ addExample: true });
366+
});
367+
368+
test("returns addExample via purpose field in interactive mode", async () => {
369+
confirmSpy.mockImplementation(() => Promise.resolve(false) as any);
370+
371+
const result = await handleInteractive(
372+
{
373+
type: "interactive",
374+
prompt: "Would you like to add a trigger?",
375+
kind: "confirm",
376+
purpose: "add-example",
377+
},
378+
makeOptions({ yes: false })
379+
);
380+
381+
expect(result).toEqual({ addExample: false });
382+
});
383+
384+
test("falls back to prompt string match when no purpose field", async () => {
385+
const result = await handleInteractive(
386+
{
387+
type: "interactive",
388+
prompt: "Add an example error trigger?",
389+
kind: "confirm",
390+
},
391+
makeOptions({ yes: true })
392+
);
393+
394+
expect(result).toEqual({ addExample: true });
395+
});
396+
339397
test("returns action: stop when user declines non-example prompt", async () => {
340398
confirmSpy.mockImplementation(() => Promise.resolve(false) as any);
341399

test/lib/init/local-ops.test.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,16 @@ describe("validateCommand", () => {
8686
}
8787
});
8888

89+
test("blocks glob and brace expansion characters", () => {
90+
for (const cmd of [
91+
"npm install {evil,@sentry/node}",
92+
"npm install sentry-*",
93+
"npm install sentry-?.js",
94+
]) {
95+
expect(validateCommand(cmd)).toContain("Blocked command");
96+
}
97+
});
98+
8999
test("blocks dangerous executables", () => {
90100
for (const cmd of [
91101
"rm -rf /",

0 commit comments

Comments
 (0)