fix: prevent path traversal in functions:create command [security]#8207
Open
NiteshCodes7 wants to merge 2 commits intonetlify:mainfrom
Open
fix: prevent path traversal in functions:create command [security]#8207NiteshCodes7 wants to merge 2 commits intonetlify:mainfrom
NiteshCodes7 wants to merge 2 commits intonetlify:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThe 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 🚥 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}
Contributor
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
Contributor
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
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
1 participant
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.
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.