Open
Conversation
📝 WalkthroughWalkthroughAdded HTTP(S) code download support to the deployment flow: 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: temp file path
Download-->>Upload: return temp file path
Upload->>Packager: package/process temp file
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)
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.
| } | ||
| logger.debug(`Code path absolute path: ${zipPath}`); | ||
|
|
||
| const needZip = this._assertNeedZip(codeUri); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use resolved local path for zip-type detection, not raw URL string.
Line 291 uses codeUri for _assertNeedZip(). For URLs like https://host/pkg.zip?token=..., this returns the wrong result and can trigger unnecessary re-zipping.
Proposed fix
- const needZip = this._assertNeedZip(codeUri);
+ const needZip = this._assertNeedZip(zipPath);🤖 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 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.
| * 从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.
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.
fix: code support http url
Summary by CodeRabbit