fix: prevent path traversal in functions:create command [security]#8207
fix: prevent path traversal in functions:create command [security]#8207NiteshCodes7 wants to merge 7 commits into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe patch updates function creation and download logic in a TypeScript command file to prevent path traversal and unsafe paths. For URL-based downloads the fallback filename is sanitized with Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/functions/functions-create.ts (1)
374-400:⚠️ Potential issue | 🔴 CriticalPath traversal still reachable via
--url+--name/positional argument.
nameToUsereturned bygetNameFromArgsis unvalidated when it comes from a flag or positional arg (validation ingetNameFromArgsruns only for the interactive prompt). SincedownloadFromURLis not routed throughensureFunctionPathIsOk, the following still escapes the functions directory:
netlify functions:create --url <gh-url> --name "../../evil"→fnFolder = path.join(functionsDir, "../../evil")on Line 379, and${nameToUse}.json Line 399 expands to../../evil.js.netlify functions:create --url <gh-url> "../../evil"→ same.The GitHub-filename sanitization alone does not close the original vulnerability described in the PR; the
nameitself is the primary attacker-controlled input. Recommend enforcing containment at the root cause (either ingetNameFromArgsor at the start ofdownloadFromURL) so both scaffold and URL-download paths are covered.🔒 Proposed fix — validate the name at the entry of
downloadFromURLconst downloadFromURL = async function (command, options, argumentName, functionsDir) { const folderContents = await readRepoURL(options.url) const [functionName] = options.url.split('/').slice(-1) const nameToUse = await getNameFromArgs(argumentName, options, functionName) - const fnFolder = path.join(functionsDir, nameToUse) + const fnFolder = ensureFunctionPathIsOk(functionsDir, nameToUse)Alternatively, centralize validation in
getNameFromArgsso any caller is safe by construction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/functions/functions-create.ts` around lines 374 - 400, The downloadFromURL function accepts an unvalidated nameToUse from getNameFromArgs which allows path traversal; fix by validating/sanitizing nameToUse at the start of downloadFromURL (or update getNameFromArgs) so it cannot contain path separators or traversal segments (e.g., strip/deny "../", "/", Windows backslashes, and empty names) and/or resolve fnFolder and assert it is contained inside functionsDir (using path.resolve and comparing prefixes) before creating directories or writing files; use the validated/sanitized nameToUse for fnFolder and final file names (references: downloadFromURL, getNameFromArgs, functionsDir, fnFolder, nameToUse, ensureFunctionPathIsOk).
🧹 Nitpick comments (3)
src/commands/functions/functions-create.ts (3)
397-400: Remove explanatory comments (guideline) and simplify the ternary.
// SAFE:and// ← strip any directory componentsdescribe what the code does and should be removed in favor of self-explanatory code. Note also that both branches now callpath.basename(name, ...), so the ternary can be simplified. Combined with the fix proposed above (validatingnameToUseviaensureFunctionPathIsOkbefore we reach this point), this becomes:✂️ Proposed diff
- // SAFE: - const finalName = path.basename(name, '.js') === functionName - ? `${nameToUse}.js` - : path.basename(name) // ← strip any directory components + const safeBaseName = path.basename(name) + const finalName = + path.basename(safeBaseName, '.js') === functionName ? `${nameToUse}.js` : safeBaseNameAs per coding guidelines: "Never write comments on what the code does; make the code clean and self-explanatory instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/functions/functions-create.ts` around lines 397 - 400, Remove the explanatory comments and simplify the ternary that computes finalName: validate nameToUse earlier via ensureFunctionPathIsOk, then replace the current ternary using redundant path.basename calls with a single expression that always uses path.basename(name, '.js') when appropriate and falls back to path.basename(name) only once; update the code referencing finalName, functionName, name, nameToUse and path.basename to use the simplified expression and delete the comments "// SAFE:" and "// ← strip any directory components".
751-751: Drop the inline comment per coding guidelines.- // Prevent path traversal: reject names like "../../evil" const resolvedFunctionsDir = path.resolve(functionsDir)As per coding guidelines: "Never write comments on what the code does; make the code clean and self-explanatory instead". The function name
ensureFunctionPathIsOkplus a clear error message already convey intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/functions/functions-create.ts` at line 751, Remove the inline comment "// Prevent path traversal: reject names like "../../evil"" near the ensureFunctionPathIsOk function; the function name and its error messages are self-explanatory per guidelines, so delete that comment (and any other comments that merely restate what ensureFunctionPathIsOk does) to comply with the coding standard.
59-83: Root-cause opportunity: centralize name validation ingetNameFromArgs.Today, validation only fires inside the inquirer
validatecallback (Line 77), so values fromoptions.name(Line 64) andargumentName(Line 68) bypass it entirely — this is the exact bypass called out in the PR description. Adding the regex check once here makes every caller (includingdownloadFromURL) safe by construction and removes the need to sprinkle containment checks across call sites.🛡️ Proposed fix
const getNameFromArgs = async function (argumentName, options, defaultName) { + const isValid = (val: unknown): val is string => typeof val === 'string' && /^[\w.-]+$/i.test(val) + if (options.name) { if (argumentName) { throw new Error('function name specified in both flag and arg format, pick one') } + if (!isValid(options.name)) { + return logAndThrowError(`Invalid function name: "${options.name as string}"`) + } return options.name } if (argumentName) { + if (!isValid(argumentName)) { + return logAndThrowError(`Invalid function name: "${argumentName as string}"`) + } return argumentName } const { name } = await inquirer.prompt([ { name: 'name', message: 'Name your function:', default: defaultName, type: 'input', - validate: (val) => Boolean(val) && /^[\w.-]+$/i.test(val), + validate: (val) => Boolean(val) && isValid(val), }, ]) return name }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/functions/functions-create.ts` around lines 59 - 83, Centralize name validation in getNameFromArgs: after deciding the candidate name (from options.name or argumentName) and before returning it, validate it against the same regex used in the inquirer validate callback (/^[\w.-]+$/i) and throw a clear Error if it fails; also ensure when options.name and argumentName are both provided you still throw the existing duplicate-name Error, and keep the inquirer validate as-is for interactive input. This ensures any name returned by getNameFromArgs (including options.name and argumentName paths and callers like downloadFromURL) is validated consistently.
🤖 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/commands/functions/functions-create.ts`:
- Around line 750-763: Move the ensureFunctionPathIsOk declaration to column 0
to match other top-level functions; replace the startsWith containment check
with a robust containment test using path.relative(resolvedFunctionsDir,
functionPath) and reject when the relative path starts with '..' or is absolute
(or when relative === '' and name resolves to the same dir in an unexpected
way), and replace both process.exit(1) calls with calls to logAndThrowError
(preserving existing log messages via log and including
NETLIFYDEVERR/NETLIFYDEVLOG constants) so errors are thrown instead of exiting;
keep fs.existsSync to detect existing path and return functionPath on success.
---
Outside diff comments:
In `@src/commands/functions/functions-create.ts`:
- Around line 374-400: The downloadFromURL function accepts an unvalidated
nameToUse from getNameFromArgs which allows path traversal; fix by
validating/sanitizing nameToUse at the start of downloadFromURL (or update
getNameFromArgs) so it cannot contain path separators or traversal segments
(e.g., strip/deny "../", "/", Windows backslashes, and empty names) and/or
resolve fnFolder and assert it is contained inside functionsDir (using
path.resolve and comparing prefixes) before creating directories or writing
files; use the validated/sanitized nameToUse for fnFolder and final file names
(references: downloadFromURL, getNameFromArgs, functionsDir, fnFolder,
nameToUse, ensureFunctionPathIsOk).
---
Nitpick comments:
In `@src/commands/functions/functions-create.ts`:
- Around line 397-400: Remove the explanatory comments and simplify the ternary
that computes finalName: validate nameToUse earlier via ensureFunctionPathIsOk,
then replace the current ternary using redundant path.basename calls with a
single expression that always uses path.basename(name, '.js') when appropriate
and falls back to path.basename(name) only once; update the code referencing
finalName, functionName, name, nameToUse and path.basename to use the simplified
expression and delete the comments "// SAFE:" and "// ← strip any directory
components".
- Line 751: Remove the inline comment "// Prevent path traversal: reject names
like "../../evil"" near the ensureFunctionPathIsOk function; the function name
and its error messages are self-explanatory per guidelines, so delete that
comment (and any other comments that merely restate what ensureFunctionPathIsOk
does) to comply with the coding standard.
- Around line 59-83: Centralize name validation in getNameFromArgs: after
deciding the candidate name (from options.name or argumentName) and before
returning it, validate it against the same regex used in the inquirer validate
callback (/^[\w.-]+$/i) and throw a clear Error if it fails; also ensure when
options.name and argumentName are both provided you still throw the existing
duplicate-name Error, and keep the inquirer validate as-is for interactive
input. This ensures any name returned by getNameFromArgs (including options.name
and argumentName paths and callers like downloadFromURL) is validated
consistently.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09ee38e3-23c4-4259-8cee-bd033bb734b1
📒 Files selected for processing (1)
src/commands/functions/functions-create.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/commands/functions/functions-create.ts (1)
749-762:⚠️ Potential issue | 🟠 MajorPreviously‑raised issues in
ensureFunctionPathIsOkare still unaddressed.Re-flagging so this doesn't ship as-is:
- Indentation / formatting. The declaration is indented two spaces while every other top-level function (
ensureEdgeFuncDirExists,downloadFromURL,scaffoldFromTemplate, …) starts at column 0. Prettier will reject this.- Fragile containment check.
functionPath.startsWith(resolvedFunctionsDir + path.sep)is case-sensitive, so on case-insensitive filesystems (macOS default, Windows) a name with differing casing can bypass or false-trip the guard.path.relativeis the idiomatic containment test.- Inconsistent error handling. The rest of this file routes failures through
logAndThrowError; usingprocess.exit(1)here makes the command harder to test and bypasses any upstream cleanup.- Explanatory comment. Line 750 describes what the code does. Per the coding guideline, the code should be self-explanatory instead.
♻️ Proposed refactor
- const ensureFunctionPathIsOk = function (functionsDir, name) { - // Prevent path traversal: reject names like "../../evil" - const resolvedFunctionsDir = path.resolve(functionsDir) - const functionPath = path.join(resolvedFunctionsDir, name) - if (!functionPath.startsWith(resolvedFunctionsDir + path.sep)) { - log(`${NETLIFYDEVERR} Invalid function name: "${name}" resolves outside the functions directory.`) - process.exit(1) - } - if (fs.existsSync(functionPath)) { - log(`${NETLIFYDEVLOG} Function ${functionPath} already exists, cancelling...`) - process.exit(1) - } - return functionPath - } +const ensureFunctionPathIsOk = function (functionsDir, name) { + const resolvedFunctionsDir = path.resolve(functionsDir) + const functionPath = path.resolve(resolvedFunctionsDir, name) + const rel = path.relative(resolvedFunctionsDir, functionPath) + if (rel === '' || rel.startsWith('..') || path.isAbsolute(rel)) { + return logAndThrowError( + `${NETLIFYDEVERR} Invalid function name: "${name}" resolves outside the functions directory.`, + ) + } + if (fs.existsSync(functionPath)) { + log(`${NETLIFYDEVLOG} Function ${functionPath} already exists, cancelling...`) + process.exit(1) + } + return functionPath +}As per coding guidelines: "Never write comments on what the code does; make the code clean and self-explanatory instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/functions/functions-create.ts` around lines 749 - 762, The ensureFunctionPathIsOk function should be unindented to top-level (match other functions like ensureEdgeFuncDirExists), remove the explanatory comment, use path.relative to check containment (e.g. compute path.relative(resolvedFunctionsDir, functionPath) and ensure it doesn't start with '..' or equal '..') to avoid case/fs issues, and replace process.exit(1) calls with the module's standard error flow by invoking logAndThrowError (or the existing logAndThrowError helper) with a clear message when the name resolves outside the functions dir or when the path already exists so callers can handle errors and tests can clean up.
🧹 Nitpick comments (1)
src/commands/functions/functions-create.ts (1)
397-399: Drop the trailing explanatory comment.The
// ← strip any directory componentscomment describes what the code does. Rename the variable or rely onpath.basenamebeing self-evident instead.♻️ Proposed refactor
- const finalName = path.basename(name, '.js') === functionName - ? `${nameToUse}.js` - : path.basename(name) // ← strip any directory components + const sanitizedName = path.basename(name) + const finalName = path.basename(sanitizedName, '.js') === functionName ? `${nameToUse}.js` : sanitizedNameAs per coding guidelines: "Never write comments on what the code does; make the code clean and self-explanatory instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/functions/functions-create.ts` around lines 397 - 399, Remove the trailing explanatory comment after the path.basename call and make the code self-descriptive: drop the "// ← strip any directory components" comment and, if helpful for clarity, consider renaming variables (e.g., finalName, nameToUse, functionName) or adding a small, clear helper function name so the intent of path.basename(name) is obvious without commentary.
🤖 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/commands/functions/functions-create.ts`:
- Around line 749-762: The ensureFunctionPathIsOk function should be unindented
to top-level (match other functions like ensureEdgeFuncDirExists), remove the
explanatory comment, use path.relative to check containment (e.g. compute
path.relative(resolvedFunctionsDir, functionPath) and ensure it doesn't start
with '..' or equal '..') to avoid case/fs issues, and replace process.exit(1)
calls with the module's standard error flow by invoking logAndThrowError (or the
existing logAndThrowError helper) with a clear message when the name resolves
outside the functions dir or when the path already exists so callers can handle
errors and tests can clean up.
---
Nitpick comments:
In `@src/commands/functions/functions-create.ts`:
- Around line 397-399: Remove the trailing explanatory comment after the
path.basename call and make the code self-descriptive: drop the "// ← strip any
directory components" comment and, if helpful for clarity, consider renaming
variables (e.g., finalName, nameToUse, functionName) or adding a small, clear
helper function name so the intent of path.basename(name) is obvious without
commentary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b5b06e5-e22f-4698-8505-401817bc28d7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/commands/functions/functions-create.ts
NiteshCodes7
changed the title
|
Thanks for tackling this @NiteshCodes7 The ensureFunctionPathIsOk guard is a good improvement for template scaffolding! I think there’s still a path traversal gap in the --url flow: getNameFromArgs() returns --name / positional arg without validation. We should also add an integration test that verifies traversal is rejected for the URL flow (e.g. functions:create --url ... --name ../../evil exits with an error and does not create files outside the functions dir). Also can we drop unrelated package-lock.json churn from this PR to keep the security fix focused and easier to review? |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Changed dependency from '@netlify/db' to '@netlify/database' in integration tests. - Removed experimental flag for database availability in functions. - Updated dev server tests to use dynamic port allocation. - Enhanced redirects tests to support dynamic port configuration. - Improved functions serve command tests with dynamic port handling. - Updated help command snapshots to reflect database command name change. - Enhanced framework detection tests to support dynamic port allocation. - Added diagnostic report generation for hung dev servers. - Refactored site builder to use random UUID for test site names. - Updated database migration tests to handle interactive and non-interactive modes. - Added tests for edge functions watch ignore functionality. - Introduced shell command utility tests for command handling.
NiteshCodes7
left a comment
NiteshCodes7
left a comment
There was a problem hiding this comment.
Updated! Here is what changed:
-
getNameFromArgs() now validates --name flag and positional
args with the same regex used by the interactive prompt. -
downloadFromURL() now has an explicit path traversal check
before path.join() is called. -
Added integration test covering the --url + --name ../../evil
flow — verifies non-zero exit and no files written outside
functions dir. -
Removed package-lock.json from the PR.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/commands/functions-create/functions-create.test.ts`:
- Around line 20-39: The test tests path traversal by calling execa('netlify',
['functions:create', ...]) against a live GitHub URL which can fail for
unrelated reasons; replace this with the fixture-based integration workflow (use
withSiteBuilder / the integration helpers used across tests) to create a local
site and invoke the CLI command via the test runner, and assert a
traversal-specific failure (match expected error text or exit code from the
functions:create handler) rather than simply checking reject: false; update the
test in functions-create.test.ts to stop using temporaryDirectory() + execa
directly and instead use the site builder helper, trigger the same flags (--name
'../../evil' --url ...) and assert the CLI outputs the specific traversal
rejection message/code.
- Line 41: Remove the explanatory inline comments in the test file
tests/integration/commands/functions-create/functions-create.test.ts (e.g. the
"// Should exit with error" comment at the location around line 41 and the
similar comment at line 44); leave the assertions and test code unchanged so the
behavior is expressed by the test code itself (no changes to test names or
assertion calls like those in the surrounding it/describe blocks).
- Line 45: The test's outside-path assertion uses the wrong target
(path.join(tmpDir, '..', '..', 'evil')) and can miss a traversal; update the
assertion to check the actual resolved location from functionsDir for input
'../../evil' (one level above tmpDir) by replacing the expect check to use
path.join(tmpDir, '..', 'evil') (reference the functionsDir variable and the
expect line in functions-create.test.ts).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b830612-9e35-48ee-bcc2-b02afd6209d1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/commands/functions/functions-create.tstests/integration/commands/functions-create/functions-create.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/functions/functions-create.ts
2 participants
Description
There is a path traversal vulnerability in
functions:create. The interactiveprompt correctly validates function names with
/^[\w.-]+$/i, but this validationis completely bypassed when the name is supplied via the
--nameflag or as apositional argument.
Affected File
src/commands/functions/functions-create.tsReproduction Steps
Run either of the following:
netlify functions:create --name "../../evil"
netlify functions:create "../../evil"
This causes files to be written outside the intended functions directory.
Root Cause
In
getNameFromArgs, validation only runs on the interactive prompt:const { name } = await inquirer.prompt([
{
validate: (val) => Boolean(val) && /^[\w.-]+$/i.test(val), // ✅ validated
},
])
But when --name or a positional arg is passed, the raw unvalidated value goes
directly into:
const functionPath = path.join(functionsDir, name) // ❌ no validation
Impact
An attacker or a malicious script can write files to arbitrary locations on the
filesystem outside the functions directory.
Proposed Fix
Add a containment check in
ensureFunctionPathIsOk:const ensureFunctionPathIsOk = function (functionsDir, name) {
const functionPath = path.join(functionsDir, name)
if (!functionPath.startsWith(path.resolve(functionsDir) + path.sep)) {
log(
Invalid function name: "${name}" resolves outside the functions directory.)process.exit(1)
}
if (fs.existsSync(functionPath)) {
log(
Function ${functionPath} already exists, cancelling...)process.exit(1)
}
return functionPath
}
Additional Note
A second related issue exists in
downloadFromURLwhere filenames from theGitHub API response are not sanitized before being passed to
path.join.Fix: wrap
namewithpath.basename(name)before use.