Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds HTTP(S) codeUri support: Changes
Sequence DiagramsequenceDiagram
participant Deploy as Deploy Process
participant Upload as _uploadCode()
participant Download as _downloadFromUrl()
participant HTTP as HTTP Client (axios)
participant FS as File System
participant Packager as Packaging Logic
Deploy->>Upload: provide codeUri
Upload->>Upload: detect if codeUri starts with http(s)
alt codeUri is URL
Upload->>Download: downloadFromUrl(url)
Download->>HTTP: GET url (stream)
HTTP-->>Download: response stream
Download->>FS: create temp dir & write file
FS-->>Download: return temp file path
Download-->>Upload: return temp file path
Upload->>Packager: package/process temp file (zipPath)
Packager->>FS: upload packaged artifact
Packager->>FS: cleanup temp file (delete)
else codeUri is local
Upload->>FS: resolve absolute/local path
Upload->>Packager: package/process local path
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 (2)
src/subCommands/deploy/impl/function.ts (2)
282-349:⚠️ Potential issue | 🟠 MajorAlways clean downloaded temp artifacts (including failure paths).
Downloaded files/dirs created for URL sources are never removed, and current cleanup is not in a
finallypath. This can leak temp storage over repeated deploys or failed uploads.💡 Proposed fix
- let zipPath: string; + let zipPath: string; + let downloadedPath = ''; + let downloadedTempDir = ''; ... - zipPath = await this._downloadFromUrl(codeUri); + zipPath = await this._downloadFromUrl(codeUri); + downloadedPath = zipPath; + downloadedTempDir = path.dirname(downloadedPath); ... - const ossConfig = await this.fcSdk.uploadCodeToTmpOss(zipPath); - logger.debug('ossConfig: ', ossConfig); - _.set(this.local, 'code', ossConfig); - - if (generateZipFilePath) { - try { - fs.rmSync(generateZipFilePath); - } catch (ex) { - logger.debug(`Unable to remove zip file: ${zipPath}`); - } - } + try { + const ossConfig = await this.fcSdk.uploadCodeToTmpOss(zipPath); + logger.debug('ossConfig: ', ossConfig); + _.set(this.local, 'code', ossConfig); + } finally { + if (generateZipFilePath) { + try { + fs.rmSync(generateZipFilePath, { force: true }); + } catch (ex) { + logger.debug(`Unable to remove zip file: ${generateZipFilePath}`); + } + } + if (downloadedTempDir) { + try { + fs.rmSync(downloadedTempDir, { recursive: true, force: true }); + } catch (ex) { + logger.debug(`Unable to remove downloaded temp dir: ${downloadedTempDir}`); + } + } + }Also applies to: 354-366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` around lines 282 - 349, The code currently only removes generated zip files in the success path and when codeChecksum matches, but never guarantees cleanup of downloaded temp artifacts (from _downloadFromUrl) on failures; update the upload flow in the function (references: _downloadFromUrl, needZip/_assertNeedZip, generateZipFilePath, and the cleanup blocks that call fs.rmSync) to always remove any temp files/dirs created (both the downloaded path and generatedZipFilePath) by moving cleanup into a finally block or equivalent, ensure both success and error paths call the same remover, handle and swallow fs.rmSync errors with debug logging, and apply the same change to the similar cleanup at the other location noted (around the code at 354-366).
284-292:⚠️ Potential issue | 🟠 MajorFix zip/packaging decision for URL inputs.
Line 291 uses
this._assertNeedZip(codeUri). For URLs likehttps://x/a.zip?token=..., suffix checks fail and can trigger incorrect re-packaging. Base the decision on the URL pathname (or resolved download filename), not the raw URL string.💡 Proposed fix
- const needZip = this._assertNeedZip(codeUri); + const zipDecisionSource = + codeUri.startsWith('http://') || codeUri.startsWith('https://') + ? new URL(codeUri).pathname + : codeUri; + const needZip = this._assertNeedZip(zipDecisionSource);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` around lines 284 - 292, The packaging decision incorrectly calls this._assertNeedZip(codeUri) using the raw URL string; update the logic so when codeUri is an HTTP(S) URL you determine the filename from the URL (e.g. new URL(codeUri).pathname) or from the resolved download filename returned by this._downloadFromUrl and pass that filename/path into this._assertNeedZip (or call _assertNeedZip after download using zipPath) so suffix checks operate on the actual filename/path rather than the full query-string URL.
🤖 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/subCommands/deploy/impl/function.ts`:
- Around line 368-372: The axios download calls lack timeout and payload limits;
update the axios options in _downloadFromUrl() (in
src/subCommands/deploy/impl/function.ts) and in downloadFile() (in
src/utils/index.ts) to include a timeout and content/body size caps (e.g.,
timeout: 30000, maxContentLength and maxBodyLength set to a reasonable limit
such as 50 * 1024 * 1024) so slow or very large responses fail fast and avoid
exhausting disk/memory; ensure the same keys (timeout, maxContentLength,
maxBodyLength) are added to the existing axios({...}) call objects used for
downloads.
---
Outside diff comments:
In `@src/subCommands/deploy/impl/function.ts`:
- Around line 282-349: The code currently only removes generated zip files in
the success path and when codeChecksum matches, but never guarantees cleanup of
downloaded temp artifacts (from _downloadFromUrl) on failures; update the upload
flow in the function (references: _downloadFromUrl, needZip/_assertNeedZip,
generateZipFilePath, and the cleanup blocks that call fs.rmSync) to always
remove any temp files/dirs created (both the downloaded path and
generatedZipFilePath) by moving cleanup into a finally block or equivalent,
ensure both success and error paths call the same remover, handle and swallow
fs.rmSync errors with debug logging, and apply the same change to the similar
cleanup at the other location noted (around the code at 354-366).
- Around line 284-292: The packaging decision incorrectly calls
this._assertNeedZip(codeUri) using the raw URL string; update the logic so when
codeUri is an HTTP(S) URL you determine the filename from the URL (e.g. new
URL(codeUri).pathname) or from the resolved download filename returned by
this._downloadFromUrl and pass that filename/path into this._assertNeedZip (or
call _assertNeedZip after download using zipPath) so suffix checks operate on
the actual filename/path rather than the full query-string URL.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.jsonsrc/subCommands/deploy/impl/function.ts
Comment on lines
368
to
372
| const response = await axios({ | ||
| method: 'GET', | ||
| url, | ||
| responseType: 'stream', | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the file and check the specific lines mentioned
cat -n src/subCommands/deploy/impl/function.ts | sed -n '365,375p'Repository: devsapp/fc3
Length of output: 410
🏁 Script executed:
# Check for axios calls with timeout/body limit configurations in the entire file
rg -n -C5 --type=ts 'axios\(\{' src/subCommands/deploy/impl/function.tsRepository: devsapp/fc3
Length of output: 413
🏁 Script executed:
# Search for timeout and content length configurations in the codebase
rg -n --type=ts 'timeout|maxContentLength|maxBodyLength' src/subCommands/deploy/impl/function.tsRepository: devsapp/fc3
Length of output: 37
🏁 Script executed:
# Check if axios instance is created with defaults elsewhere
rg -n --type=ts 'axios\.create|axios\.defaults' src/Repository: devsapp/fc3
Length of output: 37
🏁 Script executed:
# Look for all axios calls in the codebase to see if any have timeout/limits
rg -n --type=ts 'axios\(' . --max-count=20Repository: devsapp/fc3
Length of output: 196
🏁 Script executed:
# Check if this function has any error handling or resource limits
cat -n src/subCommands/deploy/impl/function.ts | sed -n '340,385p'Repository: devsapp/fc3
Length of output: 1576
🏁 Script executed:
# Search for any axios configuration or interceptors that might set defaults
rg -n --type=ts -B2 -A2 'interceptor' src/Repository: devsapp/fc3
Length of output: 37
🏁 Script executed:
# Check the other axios call in utils/index.ts
cat -n src/utils/index.ts | sed -n '12,30p'Repository: devsapp/fc3
Length of output: 695
🏁 Script executed:
# Get more context on the download function to see if there's any error handling for the stream
cat -n src/subCommands/deploy/impl/function.ts | sed -n '385,410p'Repository: devsapp/fc3
Length of output: 1273
🏁 Script executed:
# Check if there are any limits set on the file system operations
rg -n --type=ts 'createWriteStream|highWaterMark' src/Repository: devsapp/fc3
Length of output: 316
Add timeout and payload limits to axios download requests.
The axios calls for remote downloads lack timeout and body size limits. This affects both _downloadFromUrl() in src/subCommands/deploy/impl/function.ts:368 and downloadFile() in src/utils/index.ts:17. A slow or unbounded response can hang deployment or exhaust local disk/memory. Configure timeout, maxContentLength, and maxBodyLength to prevent resource exhaustion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/subCommands/deploy/impl/function.ts` around lines 368 - 372, The axios
download calls lack timeout and payload limits; update the axios options in
_downloadFromUrl() (in src/subCommands/deploy/impl/function.ts) and in
downloadFile() (in src/utils/index.ts) to include a timeout and content/body
size caps (e.g., timeout: 30000, maxContentLength and maxBodyLength set to a
reasonable limit such as 50 * 1024 * 1024) so slow or very large responses fail
fast and avoid exhausting disk/memory; ensure the same keys (timeout,
maxContentLength, maxBodyLength) are added to the existing axios({...}) call
objects used for downloads.
mozhou52
force-pushed
the
support-http
branch
from
7c108dd to
48f731c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/subCommands/deploy/impl/function.ts`:
- Around line 282-289: The downloaded temp artifact assigned to zipPath when
codeUri is HTTP(S) is never removed; update the function (the block that calls
this._downloadFromUrl to produce zipPath) to ensure cleanup of the downloaded
file/dir in all cases by wrapping usage in a try/finally (or equivalent) and
deleting the temp artifact in the finally block; apply the same pattern to the
other similar blocks mentioned (lines around the other occurrences) so that any
temp path created by _downloadFromUrl is removed via fs.unlink/fs.rm or a
recursive remover before returning or throwing.
- Line 355: The current logger.warn call logs the full download URL
(logger.warn(`Downloading code from URL: ${url}`)), which can leak sensitive
query parameters; change it to log a redacted form instead by parsing the url
(new URL(url)) and either stripping search params or replacing them with a
placeholder (e.g., show origin+pathname or origin+pathname+"?REDACTED") before
passing to logger.warn so no tokens/signatures appear in logs.
- Line 291: The call to this._assertNeedZip(codeUri) uses the raw codeUri (which
may be a remote URL with query params) causing incorrect zip detection; change
the code to compute the resolved local filesystem path first (e.g., resolve or
download/normalize codeUri into a localPath/resolvedCodeUri variable) and pass
that resolved local path into this._assertNeedZip instead of the raw codeUri so
detection operates on the actual filesystem path.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.jsonsrc/subCommands/deploy/impl/function.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
Comment on lines
282
to
289
| let zipPath: string; | ||
| // 处理不同类型的 codeUri | ||
| if (codeUri.startsWith('http://') || codeUri.startsWith('https://')) { | ||
| zipPath = await this._downloadFromUrl(codeUri); | ||
| } else { | ||
| zipPath = path.isAbsolute(codeUri) ? codeUri : path.join(this.inputs.baseDir, codeUri); | ||
| } | ||
| logger.debug(`Code path absolute path: ${zipPath}`); |
There was a problem hiding this comment.
Downloaded temp artifacts are not cleaned up, causing disk leaks.
When codeUri is HTTP(S), the downloaded file/temp dir is never removed on either success or failure paths. Over repeated deploys, this will leak files under OS temp.
Proposed fix
private async _uploadCode(): Promise<boolean> {
const codeUri = this.local.code;
@@
- let zipPath: string;
+ let zipPath: string;
+ let downloadedTempDir = '';
@@
if (codeUri.startsWith('http://') || codeUri.startsWith('https://')) {
zipPath = await this._downloadFromUrl(codeUri);
+ downloadedTempDir = path.dirname(zipPath);
} else {
zipPath = path.isAbsolute(codeUri) ? codeUri : path.join(this.inputs.baseDir, codeUri);
}
@@
- if (generateZipFilePath) {
- try {
- fs.rmSync(generateZipFilePath);
- } catch (ex) {
- logger.debug(`Unable to remove zip file: ${zipPath}`);
- }
- }
+ if (generateZipFilePath) {
+ try {
+ fs.rmSync(generateZipFilePath, { force: true });
+ } catch (ex) {
+ logger.debug(`Unable to remove zip file: ${zipPath}`);
+ }
+ }
+ if (downloadedTempDir) {
+ try {
+ fs.rmSync(downloadedTempDir, { recursive: true, force: true });
+ } catch (ex) {
+ logger.debug(`Unable to remove temp download dir: ${downloadedTempDir}`);
+ }
+ }Also applies to: 322-346, 354-389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/subCommands/deploy/impl/function.ts` around lines 282 - 289, The
downloaded temp artifact assigned to zipPath when codeUri is HTTP(S) is never
removed; update the function (the block that calls this._downloadFromUrl to
produce zipPath) to ensure cleanup of the downloaded file/dir in all cases by
wrapping usage in a try/finally (or equivalent) and deleting the temp artifact
in the finally block; apply the same pattern to the other similar blocks
mentioned (lines around the other occurrences) so that any temp path created by
_downloadFromUrl is removed via fs.unlink/fs.rm or a recursive remover before
returning or throwing.
| * 从URL下载文件到本地临时目录 | ||
| */ | ||
| private async _downloadFromUrl(url: string): Promise<string> { | ||
| logger.warn(`Downloading code from URL: ${url}`); |
There was a problem hiding this comment.
Avoid logging full download URL (may leak signed query credentials).
Line 355 logs the full URL. Pre-signed URLs commonly carry sensitive query params (token, signature, etc.), which should not enter logs.
Proposed fix
- logger.warn(`Downloading code from URL: ${url}`);
+ const parsed = new URL(url);
+ logger.warn(`Downloading code from URL: ${parsed.origin}${parsed.pathname}`);📝 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
| logger.warn(`Downloading code from URL: ${url}`); | |
| const parsed = new URL(url); | |
| logger.warn(`Downloading code from URL: ${parsed.origin}${parsed.pathname}`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/subCommands/deploy/impl/function.ts` at line 355, The current logger.warn
call logs the full download URL (logger.warn(`Downloading code from URL:
${url}`)), which can leak sensitive query parameters; change it to log a
redacted form instead by parsing the url (new URL(url)) and either stripping
search params or replacing them with a placeholder (e.g., show origin+pathname
or origin+pathname+"?REDACTED") before passing to logger.warn so no
tokens/signatures appear in logs.
61061ae to
1754b59
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/subCommands/deploy/impl/function.ts (3)
282-348:⚠️ Potential issue | 🟠 MajorDownloaded HTTP temp artifacts are still not cleaned in all paths.
Line 285 creates a downloaded temp artifact, but Line 322 and Line 340 only clean generated zip files. This still leaks temp files/dirs for URL-based deploys and on mid-flow exceptions.
Proposed fix
- let zipPath: string; + let zipPath: string; + let downloadedTempDir = ''; @@ if (codeUri.startsWith('http://') || codeUri.startsWith('https://')) { zipPath = await this._downloadFromUrl(codeUri); + downloadedTempDir = path.dirname(zipPath); } else { zipPath = path.isAbsolute(codeUri) ? codeUri : path.join(this.inputs.baseDir, codeUri); } @@ - let generateZipFilePath = ''; - if (needZip) { + let generateZipFilePath = ''; + try { + if (needZip) { ... - } + } @@ - if (generateZipFilePath) { - try { - fs.rmSync(generateZipFilePath); - } catch (ex) { - logger.debug(`Unable to remove zip file: ${zipPath}`); - } - } - - return true; + return true; + } finally { + if (generateZipFilePath) { + try { + fs.rmSync(generateZipFilePath, { force: true }); + } catch (ex) { + logger.debug(`Unable to remove zip file: ${generateZipFilePath}`); + } + } + if (downloadedTempDir) { + try { + fs.rmSync(downloadedTempDir, { recursive: true, force: true }); + } catch (ex) { + logger.debug(`Unable to remove temp download dir: ${downloadedTempDir}`); + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` around lines 282 - 348, The code downloads remote artifacts via _downloadFromUrl (assigning zipPath) but only removes generateZipFilePath, leaking the original downloaded temp file and leaving it on early returns or exceptions; wrap the download+processing/upload flow in a try/finally (or track a downloadedTempPath flag returned by _downloadFromUrl) and ensure both generateZipFilePath and the original downloaded zipPath/tempDownloadPath are removed in the finally block (use fs.rmSync inside guarded try/catch), and update the early-return branch (where codeChecksum matches) to remove the downloaded temp artifact before returning; reference functions/vars: _downloadFromUrl, zip, calculateCRC64, uploadCodeToTmpOss, zipPath, generateZipFilePath, and fs.rmSync.
355-355:⚠️ Potential issue | 🟠 MajorRedact query credentials from download URL logs.
Line 355 logs the full URL and can expose signed tokens/signatures.
Proposed fix
- logger.warn(`Downloading code from URL: ${url}`); + try { + const parsed = new URL(url); + logger.warn(`Downloading code from URL: ${parsed.origin}${parsed.pathname}`); + } catch { + logger.warn('Downloading code from URL'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` at line 355, The current logger.warn call prints the full download URL (logger.warn(`Downloading code from URL: ${url}`)), which may expose signed tokens in query params; update the logging to redact or remove query parameters before logging by parsing the url (e.g., new URL(url)) and logging only origin+pathname or a masked version of the search (e.g., replace each query value with "***" or omit search entirely) so that logger.warn no longer emits sensitive credentials.
369-373:⚠️ Potential issue | 🟠 MajorAdd timeout and payload caps to axios download request.
Line 369 has no timeout/body limits, so slow or oversized responses can hang or exhaust resources.
Proposed fix
const response = await axios({ method: 'GET', url, responseType: 'stream', + timeout: 30_000, + maxContentLength: 50 * 1024 * 1024, + maxBodyLength: 50 * 1024 * 1024, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` around lines 369 - 373, The download axios call that creates `response` lacks timeout and payload caps; modify the axios request (the object passed to axios where `responseType: 'stream'` is set) to include a sensible `timeout` (e.g. 30s) and set `maxContentLength` and `maxBodyLength` (e.g. 10MB or an appropriate cap) to prevent hanging or oversized responses, and ensure any timeout/overflow errors from this call are allowed to surface or are caught where `response` is awaited so the caller can handle them.
🤖 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/subCommands/deploy/impl/function.ts`:
- Around line 351-354: The JSDoc/comment and method declaration for
_downloadFromUrl are mis-indented; adjust the indentation so the /** ... */
comment and the private async _downloadFromUrl(url: string): Promise<string> {
signature align with the surrounding class members (same indent level as other
private methods in this class), ensuring the opening comment, asterisks, and the
method line use consistent spaces/tabs to satisfy the formatter and pipeline
checks.
- Around line 375-381: The download promise currently only listens to the
destination stream (writer) and ignores errors from the source stream
(response.data), which can cause the download to hang; update the promise around
the pipe so it also attaches an 'error' handler to response.data to reject the
promise and destroy the writer (e.g., response.data.on('error', err => {
writer.destroy(err); reject(err); })), and ensure reciprocal cleanup by handling
writer.on('error', err => { response.data.destroy?.(); reject(err); }) so both
source (response.data) and destination (writer) errors are handled and the file
at downloadPath is not left in an inconsistent state.
---
Duplicate comments:
In `@src/subCommands/deploy/impl/function.ts`:
- Around line 282-348: The code downloads remote artifacts via _downloadFromUrl
(assigning zipPath) but only removes generateZipFilePath, leaking the original
downloaded temp file and leaving it on early returns or exceptions; wrap the
download+processing/upload flow in a try/finally (or track a downloadedTempPath
flag returned by _downloadFromUrl) and ensure both generateZipFilePath and the
original downloaded zipPath/tempDownloadPath are removed in the finally block
(use fs.rmSync inside guarded try/catch), and update the early-return branch
(where codeChecksum matches) to remove the downloaded temp artifact before
returning; reference functions/vars: _downloadFromUrl, zip, calculateCRC64,
uploadCodeToTmpOss, zipPath, generateZipFilePath, and fs.rmSync.
- Line 355: The current logger.warn call prints the full download URL
(logger.warn(`Downloading code from URL: ${url}`)), which may expose signed
tokens in query params; update the logging to redact or remove query parameters
before logging by parsing the url (e.g., new URL(url)) and logging only
origin+pathname or a masked version of the search (e.g., replace each query
value with "***" or omit search entirely) so that logger.warn no longer emits
sensitive credentials.
- Around line 369-373: The download axios call that creates `response` lacks
timeout and payload caps; modify the axios request (the object passed to axios
where `responseType: 'stream'` is set) to include a sensible `timeout` (e.g.
30s) and set `maxContentLength` and `maxBodyLength` (e.g. 10MB or an appropriate
cap) to prevent hanging or oversized responses, and ensure any timeout/overflow
errors from this call are allowed to surface or are caught where `response` is
awaited so the caller can handle them.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.jsonsrc/subCommands/deploy/impl/function.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/subCommands/deploy/impl/function.ts (4)
282-288:⚠️ Potential issue | 🟠 MajorClean up downloaded temp artifacts in
_uploadCodeviafinally.
_downloadFromUrl()returns a file inside a temp directory, but_uploadCode()only cleansgenerateZipFilePath. On successful upload and checksum-skip paths, downloaded temp files/dirs are left behind.♻️ Proposed fix
private async _uploadCode(): Promise<boolean> { const codeUri = this.local.code; @@ - let zipPath: string; + let zipPath: string; + let downloadedTempDir = ''; @@ - if (codeUri.startsWith('http://') || codeUri.startsWith('https://')) { + if (codeUri.startsWith('http://') || codeUri.startsWith('https://')) { zipPath = await this._downloadFromUrl(codeUri); + downloadedTempDir = path.dirname(zipPath); } else { zipPath = path.isAbsolute(codeUri) ? codeUri : path.join(this.inputs.baseDir, codeUri); } @@ - let generateZipFilePath = ''; - if (needZip) { + let generateZipFilePath = ''; + try { + if (needZip) { setNodeModulesBinPermissions(zipPath); @@ - zipPath = generateZipFilePath; - } + zipPath = generateZipFilePath; + } @@ - if (this.codeChecksum) { + if (this.codeChecksum) { @@ - // 清理临时文件 - if (generateZipFilePath) { - try { - fs.rmSync(generateZipFilePath); - } catch (ex) { - logger.debug(`Unable to remove zip file: ${zipPath}`); - } - } - - return false; + return false; @@ - const ossConfig = await this.fcSdk.uploadCodeToTmpOss(zipPath); - logger.debug('ossConfig: ', ossConfig); - _.set(this.local, 'code', ossConfig); - - if (generateZipFilePath) { - try { - fs.rmSync(generateZipFilePath); - } catch (ex) { - logger.debug(`Unable to remove zip file: ${zipPath}`); - } - } - - return true; + const ossConfig = await this.fcSdk.uploadCodeToTmpOss(zipPath); + logger.debug('ossConfig: ', ossConfig); + _.set(this.local, 'code', ossConfig); + return true; + } finally { + if (generateZipFilePath) { + try { + fs.rmSync(generateZipFilePath, { force: true }); + } catch (ex) { + logger.debug(`Unable to remove zip file: ${generateZipFilePath}`); + } + } + if (downloadedTempDir) { + try { + fs.rmSync(downloadedTempDir, { recursive: true, force: true }); + } catch (ex) { + logger.debug(`Unable to remove temp download dir: ${downloadedTempDir}`); + } + } + } }Also applies to: 322-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` around lines 282 - 288, The downloaded temp files returned by _downloadFromUrl (used when codeUri is an http/https URL) are not being cleaned up because _uploadCode only removes generateZipFilePath; modify _uploadCode to track the path(s) returned by _downloadFromUrl and ensure all temp artifacts (the downloaded file and its parent temp dir if applicable) are removed in a finally block so cleanup runs on success, checksum-skip, and error paths; update the same cleanup logic for the other similar code path around the 322-346 region so both upload flows always remove the downloaded temp files.
375-381:⚠️ Potential issue | 🟠 MajorHandle both source and destination stream errors during download.
pipe()+ writer-only handlers can miss source stream failures and hang. Usepipeline()to guarantee propagation and cleanup behavior.🔧 Proposed fix
+import { pipeline } from 'stream/promises'; @@ - const writer = fs.createWriteStream(downloadPath); - response.data.pipe(writer); - - await new Promise<void>((resolve, reject) => { - writer.on('finish', resolve); - writer.on('error', reject); - }); + const writer = fs.createWriteStream(downloadPath); + await pipeline(response.data, writer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` around lines 375 - 381, The current download uses response.data.pipe(writer) and only listens for writer events, which can miss errors from the source stream; replace the pipe+manual Promise with a proper stream pipeline to propagate source and dest errors and ensure cleanup (e.g., import pipeline from "stream/promises" and await pipeline(response.data, writer) or use util.promisify(stream.pipeline) to create an awaited pipeline). Update the code where response.data and writer are used (the download writer creation and await block) to use pipeline(response.data, writer) so both source (response.data) and destination (writer) errors are handled.
355-355:⚠️ Potential issue | 🟠 MajorRedact query credentials before logging download URLs.
Line 355 logs the full URL. Pre-signed URLs often carry sensitive query params; this can leak credentials into logs.
🔐 Proposed fix
- logger.warn(`Downloading code from URL: ${url}`); + const parsedUrl = new URL(url); + logger.warn(`Downloading code from URL: ${parsedUrl.origin}${parsedUrl.pathname}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` at line 355, The warning logs the full pre-signed URL (logger.warn(`Downloading code from URL: ${url}`)) which can leak credentials in query params; change it to log a redacted URL by removing or masking the query string before logging (use the URL parser to build a safeUrl from the input url variable — e.g., log url.origin + url.pathname or append "?[REDACTED]" — and replace the logger.warn call to use that safeUrl), keeping all other behavior in the function intact.
369-373:⚠️ Potential issue | 🟠 MajorAdd request timeout and payload limits to axios download.
The current axios download request is unbounded. Slow or oversized responses can stall deploy or exhaust resources.
🛡️ Proposed fix
const response = await axios({ method: 'GET', url, responseType: 'stream', + timeout: 30_000, + maxContentLength: 50 * 1024 * 1024, + maxBodyLength: 50 * 1024 * 1024, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` around lines 369 - 373, The axios GET used to create the streaming download (the call that assigns to variable response) is unbounded; add a request timeout and payload-size limits and pre-check Content-Length before streaming to avoid stalls or OOMs. Update the axios(...) call that creates response to include a timeout (e.g. 30_000 ms) and maxContentLength/maxBodyLength limits (e.g. 50MB), and before piping/consuming the response check response.headers['content-length'] (or the request head prior to download) and throw/abort if it exceeds the chosen limit so oversized responses are rejected early.
🤖 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/subCommands/deploy/impl/function.ts`:
- Around line 282-288: The downloaded temp files returned by _downloadFromUrl
(used when codeUri is an http/https URL) are not being cleaned up because
_uploadCode only removes generateZipFilePath; modify _uploadCode to track the
path(s) returned by _downloadFromUrl and ensure all temp artifacts (the
downloaded file and its parent temp dir if applicable) are removed in a finally
block so cleanup runs on success, checksum-skip, and error paths; update the
same cleanup logic for the other similar code path around the 322-346 region so
both upload flows always remove the downloaded temp files.
- Around line 375-381: The current download uses response.data.pipe(writer) and
only listens for writer events, which can miss errors from the source stream;
replace the pipe+manual Promise with a proper stream pipeline to propagate
source and dest errors and ensure cleanup (e.g., import pipeline from
"stream/promises" and await pipeline(response.data, writer) or use
util.promisify(stream.pipeline) to create an awaited pipeline). Update the code
where response.data and writer are used (the download writer creation and await
block) to use pipeline(response.data, writer) so both source (response.data) and
destination (writer) errors are handled.
- Line 355: The warning logs the full pre-signed URL (logger.warn(`Downloading
code from URL: ${url}`)) which can leak credentials in query params; change it
to log a redacted URL by removing or masking the query string before logging
(use the URL parser to build a safeUrl from the input url variable — e.g., log
url.origin + url.pathname or append "?[REDACTED]" — and replace the logger.warn
call to use that safeUrl), keeping all other behavior in the function intact.
- Around line 369-373: The axios GET used to create the streaming download (the
call that assigns to variable response) is unbounded; add a request timeout and
payload-size limits and pre-check Content-Length before streaming to avoid
stalls or OOMs. Update the axios(...) call that creates response to include a
timeout (e.g. 30_000 ms) and maxContentLength/maxBodyLength limits (e.g. 50MB),
and before piping/consuming the response check
response.headers['content-length'] (or the request head prior to download) and
throw/abort if it exceeds the chosen limit so oversized responses are rejected
early.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.jsonsrc/subCommands/deploy/impl/function.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/subCommands/deploy/impl/function.ts (3)
369-373:⚠️ Potential issue | 🟠 MajorAdd axios timeout and payload caps for downloads.
The download request has no
timeout,maxContentLength, ormaxBodyLength, so slow/oversized responses can stall deploys or consume excessive resources.Proposed fix
const response = await axios({ method: 'GET', url, responseType: 'stream', + timeout: 30_000, + maxContentLength: 50 * 1024 * 1024, + maxBodyLength: 50 * 1024 * 1024, });#!/bin/bash # Verify current axios download options in this file. rg -n -C4 "axios\\(\\{" src/subCommands/deploy/impl/function.ts rg -n "timeout|maxContentLength|maxBodyLength" src/subCommands/deploy/impl/function.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` around lines 369 - 373, The axios GET call that creates the download stream (the const response = await axios({...}) call) lacks timeout and payload size limits; update that axios request options to include a sensible timeout and limits (e.g., timeout, maxContentLength, maxBodyLength) to prevent stalled or oversized downloads, using values from an existing config or set conservative defaults, and ensure the same option names are passed into the axios call that returns the stream so response handling (and any consumers of response.data) benefit from the enforced limits.
355-355:⚠️ Potential issue | 🟠 MajorRedact URL query parameters before logging.
Line 355 logs full URLs, which may include signed tokens in query params.
Proposed fix
- logger.warn(`Downloading code from URL: ${url}`); + const parsed = new URL(url); + logger.warn(`Downloading code from URL: ${parsed.origin}${parsed.pathname}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` at line 355, The call to logger.warn currently logs the full variable url (logger.warn(`Downloading code from URL: ${url}`)), which can expose sensitive query parameters; update the logging to redact or remove URL query/search params before logging by parsing url (e.g., via the URL constructor) and logging only origin + pathname or replacing search with a constant like "?REDACTED", and fall back to a safe placeholder if parsing fails; apply this change where logger.warn and the url variable are used in the deploy function implementation so no signed tokens or query strings are emitted.
282-349:⚠️ Potential issue | 🟠 MajorClean up downloaded URL artifacts in a single
finallypath.Line 285 creates a temp-backed download path, but only
generateZipFilePathis conditionally removed. Downloaded temp files/dirs can leak on normal return and exception paths.Proposed fix
private async _uploadCode(): Promise<boolean> { const codeUri = this.local.code; @@ - let zipPath: string; + let zipPath: string; + let downloadedTempDir = ''; + let generateZipFilePath = ''; @@ if (codeUri.startsWith('http://') || codeUri.startsWith('https://')) { zipPath = await this._downloadFromUrl(codeUri); + downloadedTempDir = path.dirname(zipPath); } else { zipPath = path.isAbsolute(codeUri) ? codeUri : path.join(this.inputs.baseDir, codeUri); } @@ - let generateZipFilePath = ''; - if (needZip) { + try { + if (needZip) { setNodeModulesBinPermissions(zipPath); @@ - zipPath = generateZipFilePath; - } + zipPath = generateZipFilePath; + } @@ - if (this.codeChecksum) { + if (this.codeChecksum) { @@ - if (generateZipFilePath) { - try { - fs.rmSync(generateZipFilePath); - } catch (ex) { - logger.debug(`Unable to remove zip file: ${zipPath}`); - } - } - - return false; + return false; } else { @@ - const ossConfig = await this.fcSdk.uploadCodeToTmpOss(zipPath); - logger.debug('ossConfig: ', ossConfig); - _.set(this.local, 'code', ossConfig); - - if (generateZipFilePath) { - try { - fs.rmSync(generateZipFilePath); - } catch (ex) { - logger.debug(`Unable to remove zip file: ${zipPath}`); - } - } - - return true; + const ossConfig = await this.fcSdk.uploadCodeToTmpOss(zipPath); + logger.debug('ossConfig: ', ossConfig); + _.set(this.local, 'code', ossConfig); + return true; + } finally { + if (generateZipFilePath) { + try { + fs.rmSync(generateZipFilePath, { force: true }); + } catch (ex) { + logger.debug(`Unable to remove zip file: ${generateZipFilePath}`); + } + } + if (downloadedTempDir) { + try { + fs.rmSync(downloadedTempDir, { recursive: true, force: true }); + } catch (ex) { + logger.debug(`Unable to remove temp download dir: ${downloadedTempDir}`); + } + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/subCommands/deploy/impl/function.ts` around lines 282 - 349, The code currently only removes generateZipFilePath but can leak temporary download artifacts created by this._downloadFromUrl; wrap the main uploadCode routine (the block that sets zipPath, computes CRC, uploads to OSS and returns) in a try/finally and ensure any temp paths are tracked and removed in the finally: capture the value returned by this._downloadFromUrl into a distinct tempDownloadPath variable (instead of overwriting zipPath), use zipPath for processing, and in the finally remove both generateZipFilePath and tempDownloadPath (fs.rmSync with try/catch and debug logs) so downloaded URL artifacts are always cleaned whether you return early or throw; update references to generateZipFilePath and tempDownloadPath in the cleanup code accordingly.
🤖 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/subCommands/deploy/impl/function.ts`:
- Around line 369-373: The axios GET call that creates the download stream (the
const response = await axios({...}) call) lacks timeout and payload size limits;
update that axios request options to include a sensible timeout and limits
(e.g., timeout, maxContentLength, maxBodyLength) to prevent stalled or oversized
downloads, using values from an existing config or set conservative defaults,
and ensure the same option names are passed into the axios call that returns the
stream so response handling (and any consumers of response.data) benefit from
the enforced limits.
- Line 355: The call to logger.warn currently logs the full variable url
(logger.warn(`Downloading code from URL: ${url}`)), which can expose sensitive
query parameters; update the logging to redact or remove URL query/search params
before logging by parsing url (e.g., via the URL constructor) and logging only
origin + pathname or replacing search with a constant like "?REDACTED", and fall
back to a safe placeholder if parsing fails; apply this change where logger.warn
and the url variable are used in the deploy function implementation so no signed
tokens or query strings are emitted.
- Around line 282-349: The code currently only removes generateZipFilePath but
can leak temporary download artifacts created by this._downloadFromUrl; wrap the
main uploadCode routine (the block that sets zipPath, computes CRC, uploads to
OSS and returns) in a try/finally and ensure any temp paths are tracked and
removed in the finally: capture the value returned by this._downloadFromUrl into
a distinct tempDownloadPath variable (instead of overwriting zipPath), use
zipPath for processing, and in the finally remove both generateZipFilePath and
tempDownloadPath (fs.rmSync with try/catch and debug logs) so downloaded URL
artifacts are always cleaned whether you return early or throw; update
references to generateZipFilePath and tempDownloadPath in the cleanup code
accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.jsonsrc/subCommands/deploy/impl/function.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
rsonghuster
reviewed
Mar 2, 2026
| * 从URL下载文件到本地临时目录 | ||
| */ | ||
| private async _downloadFromUrl(url: string): Promise<string> { | ||
| logger.warn(`Downloading code from URL: ${url}`); |
rsonghuster
reviewed
Mar 2, 2026
| /** | ||
| * 从URL下载文件到本地临时目录 | ||
| */ | ||
| private async _downloadFromUrl(url: string): Promise<string> { |
Contributor
There was a problem hiding this comment.
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
2 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.
fix: code support http url
Summary by CodeRabbit
New Features
Bug Fixes
Chores