test(security): add regression tests for deploy() instance name validation (#575) - #1815
Closed
ColinM-sys wants to merge 2 commits into
Closed
test(security): add regression tests for deploy() instance name validation (#575)#1815ColinM-sys wants to merge 2 commits into
ColinM-sys wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdded a new test suite for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/deploy.test.ts (1)
150-168: MockedvalidateNamemisses important production branches; add required/length coverage.The mock in Line 161-168 only checks regex, while the real validator also checks required/type and max length. Add cases for missing/empty and
>63chars so this suite locks the full contract.Proposed patch
- function makeMockOpts(instanceName: string) { + function makeMockOpts(instanceName?: string) { @@ - validateName: (name: string, label: string) => { + validateName: (name: string, label: string) => { + if (!name || typeof name !== "string") { + throw new Error(`${label} is required`); + } + if (name.length > 63) { + throw new Error(`${label} too long (max 63 chars): '${name.slice(0, 20)}...'`); + } if (!/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/.test(name)) { throw new Error( `Invalid ${label}: '${name}'. Must be lowercase alphanumeric with optional internal hyphens.`, ); } return name; },const maliciousNames = [ @@ "has spaces", + "", + "a".repeat(64), ];Also applies to: 193-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/deploy.test.ts` around lines 150 - 168, The mock validateName in makeMockOpts only enforces the regex but omits the production validator's required/type and max-length checks; update validateName (in makeMockOpts) to throw if name is null/undefined or not a string, throw for empty string, enforce the >63-character limit (throw an Error with same message shape), and keep the existing regex check; apply the same fixes to the other mock validateName instance used later in the test file so the tests cover required/type, empty, regex and max-length branches like production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/deploy.test.ts`:
- Around line 170-177: The test's fake command runner only records
run/runInteractive calls so checks for "no commands executed" miss execFileSync
and spawnSync; update the mock in deploy.test.ts to push identifiable entries
into the same calls array from execFileSync and spawnSync (e.g.,
calls.push(`execFileSync:${file} ${args.join(' ')}`) and
calls.push(`spawnSync:${command}`)) and ensure spawnSync returns a shape the
test expects (not undefined) so all command invocation paths (run,
runInteractive, execFileSync, spawnSync) are tracked for assertions.
- Around line 225-234: The test currently swallows all errors from executeDeploy
and only conditionally checks getExitCode, allowing an "Invalid instance name"
error to be ignored; modify the try/catch around executeDeploy in the test so
that you only catch the expected provider/brev CLI errors (e.g., check error
message or type) and rethrow any other errors (like "Invalid instance name"), or
remove the broad catch and instead assert that getExitCode() === 1
unconditionally after calling executeDeploy; reference executeDeploy and
getExitCode to locate and update the test logic accordingly.
---
Nitpick comments:
In `@src/lib/deploy.test.ts`:
- Around line 150-168: The mock validateName in makeMockOpts only enforces the
regex but omits the production validator's required/type and max-length checks;
update validateName (in makeMockOpts) to throw if name is null/undefined or not
a string, throw for empty string, enforce the >63-character limit (throw an
Error with same message shape), and keep the existing regex check; apply the
same fixes to the other mock validateName instance used later in the test file
so the tests cover required/type, empty, regex and max-length branches like
production.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d0f46d9-b242-4aa5-8a35-47120bfcfe48
📒 Files selected for processing (1)
src/lib/deploy.test.ts
Comment on lines
+170
to
+177
| run: (command: string) => { | ||
| calls.push(`run:${command}`); | ||
| }, | ||
| runInteractive: (command: string) => { | ||
| calls.push(`runInteractive:${command}`); | ||
| }, | ||
| execFileSync: (_file: string, _args: string[], _opts?: Record<string, unknown>) => "", | ||
| spawnSync: () => {}, |
Contributor
There was a problem hiding this comment.
Track all command paths; current “no commands executed” check is incomplete.
Line 215 only proves run/runInteractive were not called. execFileSync and spawnSync (Line 176-177) are also command-execution surfaces but currently untracked, so a regression there would be missed.
Proposed patch
- execFileSync: (_file: string, _args: string[], _opts?: Record<string, unknown>) => "",
- spawnSync: () => {},
+ execFileSync: (file: string, args: string[], _opts?: Record<string, unknown>) => {
+ calls.push(`execFileSync:${file} ${args.join(" ")}`);
+ return "";
+ },
+ spawnSync: (file: string, args: string[], _opts?: Record<string, unknown>) => {
+ calls.push(`spawnSync:${file} ${args.join(" ")}`);
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| run: (command: string) => { | |
| calls.push(`run:${command}`); | |
| }, | |
| runInteractive: (command: string) => { | |
| calls.push(`runInteractive:${command}`); | |
| }, | |
| execFileSync: (_file: string, _args: string[], _opts?: Record<string, unknown>) => "", | |
| spawnSync: () => {}, | |
| run: (command: string) => { | |
| calls.push(`run:${command}`); | |
| }, | |
| runInteractive: (command: string) => { | |
| calls.push(`runInteractive:${command}`); | |
| }, | |
| execFileSync: (file: string, args: string[], _opts?: Record<string, unknown>) => { | |
| calls.push(`execFileSync:${file} ${args.join(" ")}`); | |
| return ""; | |
| }, | |
| spawnSync: (file: string, args: string[], _opts?: Record<string, unknown>) => { | |
| calls.push(`spawnSync:${file} ${args.join(" ")}`); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/deploy.test.ts` around lines 170 - 177, The test's fake command
runner only records run/runInteractive calls so checks for "no commands
executed" miss execFileSync and spawnSync; update the mock in deploy.test.ts to
push identifiable entries into the same calls array from execFileSync and
spawnSync (e.g., calls.push(`execFileSync:${file} ${args.join(' ')}`) and
calls.push(`spawnSync:${command}`)) and ensure spawnSync returns a shape the
test expects (not undefined) so all command invocation paths (run,
runInteractive, execFileSync, spawnSync) are tracked for assertions.
wscurran
added
enhancement
Contributor
✨ Thanks for submitting this PR, which proposes an enhancement to the testing infrastructure.Possibly related open issues: Possibly related open issues: |
ColinM-sys
force-pushed
the
fix/575-validate-instance-name
branch
from
April 16, 2026 15:22
3c7d0b3 to
d7baac3
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/lib/deploy.test.ts (2)
225-234:⚠️ Potential issue | 🟠 MajorThe valid-name test still allows false positives.
Line 225-Line 234 swallows all thrown errors and only asserts conditionally; an
Invalid instance namefailure can still pass this test.Proposed patch
- try { - await executeDeploy(opts); - } catch { - // Expected: either exit(1) from missing provider or brev CLI not found - } - - // If it exited, it should be because of missing provider/brev, not name validation - if (getExitCode() !== undefined) { - expect(getExitCode()).toBe(1); - } + const caught = await executeDeploy(opts).then(() => null).catch((error: unknown) => error); + if (caught) { + expect(String(caught)).not.toMatch(/Invalid instance name|instance name is required/i); + } + if (getExitCode() !== undefined) { + expect(getExitCode()).toBe(1); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/deploy.test.ts` around lines 225 - 234, The test currently swallows all errors from executeDeploy(opts) which allows an "Invalid instance name" failure to be hidden; change the catch to inspect the thrown error and rethrow unexpected ones. Specifically, in src/lib/deploy.test.ts replace the bare catch with catch(e) { if (e && typeof e.message === 'string' && e.message.includes('Invalid instance name')) throw e; /* or rethrow for any non-expected error */ } or alternatively assert on e.message to ensure the error is the expected missing-provider/CLI error; keep the existing getExitCode() assertion for exit(1) cases. Reference executeDeploy and getExitCode in your change.
176-177:⚠️ Potential issue | 🟠 MajorTrack all command-execution surfaces in
calls.Line 176 and Line 177 currently do not record invocations, so the “no shell commands executed” assertion can miss regressions through
execFileSync/spawnSync.Proposed patch
- execFileSync: (_file: string, _args: string[], _opts?: Record<string, unknown>) => "", - spawnSync: () => {}, + execFileSync: (file: string, args: string[], _opts?: Record<string, unknown>) => { + calls.push(`execFileSync:${file} ${args.join(" ")}`); + return ""; + }, + spawnSync: (file: string, args: string[], _opts?: Record<string, unknown>) => { + calls.push(`spawnSync:${file} ${args.join(" ")}`); + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/deploy.test.ts` around lines 176 - 177, The test stubs for execFileSync and spawnSync don't record calls, so update their mock implementations to push a descriptive record into the existing calls array whenever they're invoked (include the function name like "execFileSync" or "spawnSync", the passed args and opts/stdio info). Keep the original return behavior (empty string for execFileSync and an empty object for spawnSync) but ensure each invocation appends something like { fn: "execFileSync", file, args, opts } and { fn: "spawnSync", cmd, args, opts } to calls so the "no shell commands executed" assertion sees these invocations.
🧹 Nitpick comments (1)
src/lib/deploy.test.ts (1)
161-167: Keep testvalidateNamemock behavior aligned with production.The mock at Line 161-Line 167 only checks regex shape. Production
validateName(insrc/lib/runner.ts:288-301) also enforces required and max-length paths; mirroring that here will prevent drift in security regression coverage.Suggested alignment
validateName: (name: string, label: string) => { + if (!name || typeof name !== "string") { + throw new Error(`${label} is required`); + } + if (name.length > 63) { + throw new Error(`${label} too long (max 63 chars): '${name.slice(0, 20)}...'`); + } if (!/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/.test(name)) { throw new Error( `Invalid ${label}: '${name}'. Must be lowercase alphanumeric with optional internal hyphens.`, ); } return name; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/deploy.test.ts` around lines 161 - 167, The mock validateName only checks the regex; update it to match production validateName behavior by also enforcing non-empty (required) and maximum-length checks before running the regex. Specifically, in the validateName mock (function validateName) add a check that throws when name is falsy/empty, a check that throws when name.length exceeds the same max length used in production (e.g., 63 chars), then run the existing /^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/ test and return the name; ensure thrown error messages mirror production wording for consistency with tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/deploy.test.ts`:
- Around line 225-234: The test currently swallows all errors from
executeDeploy(opts) which allows an "Invalid instance name" failure to be
hidden; change the catch to inspect the thrown error and rethrow unexpected
ones. Specifically, in src/lib/deploy.test.ts replace the bare catch with
catch(e) { if (e && typeof e.message === 'string' && e.message.includes('Invalid
instance name')) throw e; /* or rethrow for any non-expected error */ } or
alternatively assert on e.message to ensure the error is the expected
missing-provider/CLI error; keep the existing getExitCode() assertion for
exit(1) cases. Reference executeDeploy and getExitCode in your change.
- Around line 176-177: The test stubs for execFileSync and spawnSync don't
record calls, so update their mock implementations to push a descriptive record
into the existing calls array whenever they're invoked (include the function
name like "execFileSync" or "spawnSync", the passed args and opts/stdio info).
Keep the original return behavior (empty string for execFileSync and an empty
object for spawnSync) but ensure each invocation appends something like { fn:
"execFileSync", file, args, opts } and { fn: "spawnSync", cmd, args, opts } to
calls so the "no shell commands executed" assertion sees these invocations.
---
Nitpick comments:
In `@src/lib/deploy.test.ts`:
- Around line 161-167: The mock validateName only checks the regex; update it to
match production validateName behavior by also enforcing non-empty (required)
and maximum-length checks before running the regex. Specifically, in the
validateName mock (function validateName) add a check that throws when name is
falsy/empty, a check that throws when name.length exceeds the same max length
used in production (e.g., 63 chars), then run the existing
/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/ test and return the name; ensure thrown error
messages mirror production wording for consistency with tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 019a526e-1f33-4313-b93f-1365bc73cb30
📒 Files selected for processing (1)
src/lib/deploy.test.ts
ColinM-sys
added a commit
to ColinM-sys/NemoClaw
that referenced
this pull request
Apr 16, 2026
… errors Per CodeRabbit feedback on NVIDIA#1815: - Stub `execFileSync`/`spawnSync` now record invocations into `calls` so the "no shell commands executed" assertion catches regressions through either surface, not just run/runInteractive. - The valid-name test no longer swallows all errors silently. It now captures any thrown error and asserts it is NOT a name-validation failure, so an "Invalid instance name" regression can't slip past this test. Signed-off-by: ColinM-sys <cmcdonough@50words.com>
) The deploy() function already validates instance names via validateName() (RFC 1123 regex) and escapes them via shellQuote() in all shell command interpolations. This commit adds explicit test coverage proving that names containing shell metacharacters (;, |, $(), backticks, &&, quotes, path traversal, uppercase, spaces) are rejected before any shell command executes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… errors Per CodeRabbit feedback on NVIDIA#1815: - Stub `execFileSync`/`spawnSync` now record invocations into `calls` so the "no shell commands executed" assertion catches regressions through either surface, not just run/runInteractive. - The valid-name test no longer swallows all errors silently. It now captures any thrown error and asserts it is NOT a name-validation failure, so an "Invalid instance name" regression can't slip past this test. Signed-off-by: ColinM-sys <cmcdonough@50words.com>
ColinM-sys
force-pushed
the
fix/575-validate-instance-name
branch
from
April 27, 2026 06:40
e36ad96 to
42d4831
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/deploy.test.ts`:
- Around line 161-167: The test mock validateName in src/lib/deploy.test.ts
diverges from production validation (allowing leading digits and omitting
max-length) so update the mock to mirror the real validation in
src/lib/runner.ts: require a leading letter, allow only lowercase letters,
digits and internal hyphens, and enforce the 63-character max; keep the same
error message shape and throw an Error when validation fails so tests exercise
the same rejection paths as the runner's validateName.
- Around line 226-244: The test currently allows a vacuous success if
executeDeploy resolves; ensure it actually progressed past name validation by
asserting the deploy failed: after calling executeDeploy (using makeMockOpts)
add a mandatory assertion like expect(caught).not.toBeNull() or
expect(caught).toBeTruthy() (and/or assert getExitCode() === 1) so the test
fails if executeDeploy returns successfully instead of erroring at provider
detection; place this check right after you compute `caught` and before the
existing name-error exclusion and exit-code checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 35fd13f1-d421-40ca-9024-f3d631adf485
📒 Files selected for processing (1)
src/lib/deploy.test.ts
Comment on lines
+161
to
+167
| validateName: (name: string, label: string) => { | ||
| if (!/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/.test(name)) { | ||
| throw new Error( | ||
| `Invalid ${label}: '${name}'. Must be lowercase alphanumeric with optional internal hyphens.`, | ||
| ); | ||
| } | ||
| return name; |
Contributor
There was a problem hiding this comment.
Mock validateName diverges from production rules and can mask regressions.
At Line 162, the test helper allows names starting with digits and omits the max-length guard, while src/lib/runner.ts requires a leading letter and enforces 63-char max. This weakens regression fidelity.
Suggested patch
validateName: (name: string, label: string) => {
- if (!/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/.test(name)) {
+ if (!name) {
+ throw new Error(`${label} is required`);
+ }
+ if (name.length > 63) {
+ throw new Error(`${label} too long (max 63 chars): '${name.slice(0, 20)}...'`);
+ }
+ if (!/^[a-z]([a-z0-9-]*[a-z0-9])?$/.test(name)) {
throw new Error(
- `Invalid ${label}: '${name}'. Must be lowercase alphanumeric with optional internal hyphens.`,
+ `Invalid ${label}: '${name}'. Must start with a letter and contain only lowercase alphanumerics with optional internal hyphens.`,
);
}
return name;
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| validateName: (name: string, label: string) => { | |
| if (!/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/.test(name)) { | |
| throw new Error( | |
| `Invalid ${label}: '${name}'. Must be lowercase alphanumeric with optional internal hyphens.`, | |
| ); | |
| } | |
| return name; | |
| validateName: (name: string, label: string) => { | |
| if (!name) { | |
| throw new Error(`${label} is required`); | |
| } | |
| if (name.length > 63) { | |
| throw new Error(`${label} too long (max 63 chars): '${name.slice(0, 20)}...'`); | |
| } | |
| if (!/^[a-z]([a-z0-9-]*[a-z0-9])?$/.test(name)) { | |
| throw new Error( | |
| `Invalid ${label}: '${name}'. Must start with a letter and contain only lowercase alphanumerics with optional internal hyphens.`, | |
| ); | |
| } | |
| return name; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/deploy.test.ts` around lines 161 - 167, The test mock validateName in
src/lib/deploy.test.ts diverges from production validation (allowing leading
digits and omitting max-length) so update the mock to mirror the real validation
in src/lib/runner.ts: require a leading letter, allow only lowercase letters,
digits and internal hyphens, and enforce the 63-character max; keep the same
error message shape and throw an Error when validation fails so tests exercise
the same rejection paths as the runner's validateName.
Comment on lines
+226
to
+244
| const { opts, getExitCode } = makeMockOpts("my-valid-instance"); | ||
|
|
||
| // This will fail at the provider detection step (no credentials), | ||
| // but it should NOT fail at validation — proving the name was accepted. | ||
| // Catch any thrown error and explicitly assert it wasn't a name-validation | ||
| // failure, so a future regression can't silently pass this test. | ||
| const caught = await executeDeploy(opts) | ||
| .then(() => null) | ||
| .catch((error: unknown) => error); | ||
| if (caught) { | ||
| expect(String(caught)).not.toMatch( | ||
| /Invalid instance name|instance name is required/i, | ||
| ); | ||
| } | ||
|
|
||
| // If it exited, it should be because of missing provider/brev, not name validation | ||
| if (getExitCode() !== undefined) { | ||
| expect(getExitCode()).toBe(1); | ||
| } |
Contributor
There was a problem hiding this comment.
The valid-name test still has a vacuous success path.
If executeDeploy returns without throwing, exiting, or executing commands, this test still passes. Add one mandatory post-condition proving it advanced beyond validation.
Suggested patch
- const { opts, getExitCode } = makeMockOpts("my-valid-instance");
+ const { opts, calls, getExitCode } = makeMockOpts("my-valid-instance");
@@
- // If it exited, it should be because of missing provider/brev, not name validation
+ // Ensure it progressed beyond validation (either exited downstream or attempted a command path)
+ expect(getExitCode() !== undefined || calls.length > 0).toBe(true);
+ // If it exited, it should be because of missing provider/brev, not name validation
if (getExitCode() !== undefined) {
expect(getExitCode()).toBe(1);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/deploy.test.ts` around lines 226 - 244, The test currently allows a
vacuous success if executeDeploy resolves; ensure it actually progressed past
name validation by asserting the deploy failed: after calling executeDeploy
(using makeMockOpts) add a mandatory assertion like
expect(caught).not.toBeNull() or expect(caught).toBeTruthy() (and/or assert
getExitCode() === 1) so the test fails if executeDeploy returns successfully
instead of erroring at provider detection; place this check right after you
compute `caught` and before the existing name-error exclusion and exit-code
checks.
Collaborator
|
Thanks for the regression test PR. I’m closing this as stale/duplicative because the underlying #575 deploy instance-name command-injection issue is already addressed on Evidence:
Also, this PR is now merge-dirty and targets the old Closing as already fixed on |
wscurran
added
area: e2e
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #575.
The
deploy()function insrc/lib/deploy.tsalready validates instance names viavalidateName()and escapes viashellQuote()— both applied before any shell interpolation. This was fixed in a prior refactor but had zero test coverage.This PR adds 11 regression tests to lock in the security behavior:
executeDeploy()rejects malicious names and executes zero shell commands:foo;rm -rf /(semicolon injection)foo|cat /etc/passwd(pipe injection)$(whoami)(command substitution)foo&&cat /etc/shadow(chained commands)../../../etc/passwd(path traversal)UPPERCASE(invalid format)foo bar(spaces)my-valid-instance) passes validationTest plan
vitest run)🤖 Generated with Claude Code
Summary by CodeRabbit
Signed-off-by: ColinM-sys cmcdonough@50words.com