Conversation
This commit introduces several security enhancements to the zip extraction process within the CDN download strategy. Previously, there was a potential for path traversal vulnerabilities if a malicious zip file contained entries with paths that attempted to escape the designated extraction directory. The following changes were implemented: - Added a check to ensure the resolved file path of an entry is strictly within the base extraction directory. If it escapes, the entry is logged and skipped. - Explicitly ensured that the parent directory for each file being extracted exists before attempting to write the file. This prevents errors and potential issues with nested directories within zip archives. - Refactored `isSafePath` to more robustly handle Windows path separators, disallow absolute paths and drive letters, and ensure the resolved target path starts with the resolved root directory, preventing traversal.
WalkthroughPer-entry zip extraction hardened with path normalization/validation, per-entry async writes awaited, and manifest detection integrated; cspell vocabulary updated (removed "Zrdm", added "backoff", "autodrain", "pluginname"); category slug normalization tightened; one subscription error log switched to parameterized format. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant CDN as CDN/Network
participant Unzip as Unzipper
participant FS as Filesystem
Note over Unzip,FS: For each archive entry: normalize path → validate → ensure parent dirs → create write stream → track finished promise
Client->>CDN: request archive stream
CDN-->>Client: archive stream
Client->>Unzip: pipe stream into unzipper
Unzip->>Unzip: read entry header (entryPath, size)
alt unsafe path (absolute / drive / .. / Windows-sep)
Unzip->>Unzip: drain/skip entry and log rejection
else safe path
Unzip->>FS: mkdir -p parent directories
Unzip->>FS: open write stream for entry
Unzip->>Unzip: register finished(writeStream) as pending promise
Unzip->>Unzip: check per-file and total size limits
alt size exceeded
Unzip->>Client: abort extraction with error
end
Unzip->>Unzip: detect manifest.json → record pluginDir
end
Unzip->>Unzip: on stream close/finish → await all pending write promises
Unzip-->>Client: resolve with extracted result or propagate error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
Greptile SummaryThis PR addresses AI-generated suggestions from issue #9298, implementing three types of fixes across the codebase. Changes Made
Critical Issues FoundThe security enhancements in
These issues undermine the security improvements this PR intended to provide. Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant CdnDownloadStrategy
participant FileSystem
participant Unzipper
Client->>CdnDownloadStrategy: unzip(filePath, extractDir)
CdnDownloadStrategy->>CdnDownloadStrategy: resolve baseExtractPath
CdnDownloadStrategy->>FileSystem: createReadStream(filePath)
FileSystem->>Unzipper: pipe to unzipper.Parse()
loop For each entry in zip
Unzipper->>CdnDownloadStrategy: on('entry', {path, type, size})
CdnDownloadStrategy->>CdnDownloadStrategy: isSafePath(extractDir, entryPath)
alt Path is unsafe
CdnDownloadStrategy->>Unzipper: entry.autodrain()
else File too large
CdnDownloadStrategy->>Unzipper: entry.autodrain()
else Total size exceeded
CdnDownloadStrategy-->>Client: reject(Error)
else Valid entry
CdnDownloadStrategy->>CdnDownloadStrategy: resolve fullPath and validate
alt Path escapes directory
CdnDownloadStrategy->>Unzipper: entry.autodrain()
else Directory type
CdnDownloadStrategy->>Unzipper: entry.autodrain()
else File type
CdnDownloadStrategy->>FileSystem: mkdir(dirname, {recursive: true})
FileSystem-->>CdnDownloadStrategy: directory created
CdnDownloadStrategy->>FileSystem: entry.pipe(createWriteStream(resolvedFullPath))
end
end
end
Unzipper->>CdnDownloadStrategy: on('close')
CdnDownloadStrategy-->>Client: resolve(extractDir)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Outdated
Show resolved
Hide resolved
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In
@packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts:
- Around line 344-357: The extraction may finish before async mkdir/pipe
complete because fs.mkdir(...).then(...) runs asynchronously and the surrounding
'close' handler (at the stream's 'close' event) can fire earlier; fix by turning
the mkdir+write into a promise the extractor waits for: use
fs.promises.mkdir(resolvedFullPathDir, { recursive: true }) and then use a
stream-to-promise helper (e.g., pipeline or stream.promises.finished) for
entry.pipe(createWriteStream(resolvedFullPath)) so you can await it, and expose
these per-entry promises (e.g., push to a pendingWrites array) so the 'close'
handler waits for Promise.all(pendingWrites) before concluding; update locations
referencing entry.pipe, fs.mkdir and the 'close' event to use the new
promise-based flow.
In
@packages/plugins/registry/src/lib/domain/services/plugin-category.service.ts:
- Around line 173-176: The slug normalization in generateUniqueSlug computes
baseSlug from name and can become empty for inputs like "!!!", leading to
invalid slugs; add a defensive check after computing baseSlug in
PluginCategoryService.generateUniqueSlug (or the equivalent method) that throws
a clear error (e.g., "Name must contain at least one alphanumeric character")
when baseSlug is empty, so the service refuses names with no alphanumeric
characters instead of producing "", "-1", etc.; alternatively, add a DTO
validation rule (e.g., ensure name matches /[a-z0-9]/i) to prevent such names
earlier, but implement at least the in-service check to be defensive.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts (3)
299-342: Good defense-in-depth security, but note redundancy.The path containment check at lines 335-342 duplicates validation already performed by
isSafePath()at line 307. While defense-in-depth is valuable for security-critical code, consider adding a comment explaining this is intentional redundancy.Also note:
isSafePath()returnsfalsewhenresolvedTarget === resolvedRoot(line 460-462), so theresolvedFullPath === baseExtractPathbranch at line 337 is effectively unreachable. Consider removing it for consistency:🔎 Suggested simplification
- if ( - !resolvedFullPath.startsWith(baseExtractPath + path.sep) && - resolvedFullPath !== baseExtractPath - ) { + // Defense-in-depth: re-verify containment after path.resolve() + if (!resolvedFullPath.startsWith(baseExtractPath + path.sep)) { logger.warn(`Resolved path escapes extract directory, skipping: ${entryPath}`); entry.autodrain(); return; }
309-309: Add "autodrain" to the spell check dictionary.The cspell pipeline is flagging
autodrainas an unknown word, but it's a legitimate method from theunzipperlibrary. Add it to.cspell.jsonto resolve the CI warnings.# In .cspell.json, add to the "words" array: + "autodrain",
549-556: Same race condition exists in streaming extraction.The
entry.pipe(writeStream)at line 555 is not awaited, so the'finish'event at line 571 may fire before all file writes complete. Apply the same tracking pattern suggested for theunzipmethod.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.cspell.jsonpackages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.tspackages/plugins/registry/src/lib/domain/services/plugin-category.service.tspackages/plugins/registry/src/lib/domain/services/plugin-subscription-plan.service.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/[a-z0-9-]*.{ts,html,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Separate words in Angular file names with hyphens and use lowercase (e.g., user-profile.ts, user-profile.html, user-profile.css)
Files:
packages/plugins/registry/src/lib/domain/services/plugin-category.service.tspackages/plugins/registry/src/lib/domain/services/plugin-subscription-plan.service.tspackages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
**/*.ts: Match file names to the primary TypeScript identifier within (class/function) and avoid generic names like helpers.ts or utils.ts
Prefer the inject() function over constructor parameter injection for Angular dependencies
Group Angular-specific properties (injections, inputs, outputs, queries) before class methods in components/directives
Mark class members used only by the component’s template as protected to avoid leaking public API
Mark Angular-initialized members (input/model/output and queries) as readonly; for decorator APIs, apply to outputs and queries (not inputs)
Keep lifecycle hook methods simple; move logic into well‑named private methods and call them from the hook
Implement Angular lifecycle hook interfaces (e.g., OnInit) to ensure correct hook naming
Use a consistent, project-specific selector prefix for components/directives and never use the ng prefix
For directive attribute selectors, use lowercase, dash-case attributes (e.g., button[yt-upload])
Files:
packages/plugins/registry/src/lib/domain/services/plugin-category.service.tspackages/plugins/registry/src/lib/domain/services/plugin-subscription-plan.service.tspackages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.{ts,html,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Use the same base file name for a component’s TypeScript, template, and style files (e.g., user-profile.ts/html/css)
Files:
packages/plugins/registry/src/lib/domain/services/plugin-category.service.tspackages/plugins/registry/src/lib/domain/services/plugin-subscription-plan.service.tspackages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.{html,ts}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Name event handlers for what they do (e.g., saveUserData) instead of the triggering event (e.g., handleClick)
Files:
packages/plugins/registry/src/lib/domain/services/plugin-category.service.tspackages/plugins/registry/src/lib/domain/services/plugin-subscription-plan.service.tspackages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
🧬 Code graph analysis (1)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts (1)
packages/desktop-core/src/lib/logger/logger.ts (1)
logger(34-34)
🪛 GitHub Actions: Check Spelling and Typos with cspell
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
[warning] 19-19: cspell: Unknown word (pluginname)
[warning] 109-109: cspell: Unknown word (backoff)
[warning] 309-309: cspell: Unknown word (autodrain)
[warning] 316-316: cspell: Unknown word (autodrain)
[warning] 340-340: cspell: Unknown word (autodrain)
[warning] 345-345: cspell: Unknown word (autodrain)
[warning] 355-355: cspell: Unknown word (autodrain)
[error] 1-1: 7 spelling issues found in 1 of the 3 files checked.
🪛 GitHub Check: Cspell
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
[warning] 355-355:
Unknown word (autodrain)
[warning] 345-345:
Unknown word (autodrain)
[warning] 340-340:
Unknown word (autodrain)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Greptile Review
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
.cspell.json (1)
810-811: LGTM!The comma addition after "Liskov" fixes the JSON syntax, ensuring the
.cspell.jsonfile remains valid.packages/plugins/registry/src/lib/domain/services/plugin-subscription-plan.service.ts (1)
370-374: LGTM!The parameterized logging format with
%splaceholder is cleaner and allows theerrorobject to be passed separately, which provides better stack trace display in console output.packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts (1)
439-469: Excellent security hardening for path validation.The rewritten
isSafePathfunction comprehensively addresses path traversal attacks:
- Validates non-empty paths
- Normalizes path separators for cross-platform consistency
- Blocks absolute paths and Windows drive letters
- Ensures resolved paths stay within the extraction root
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Show resolved
Hide resolved
packages/plugins/registry/src/lib/domain/services/plugin-category.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @.cspell.json:
- Around line 810-811: Remove the duplicate "Zrdm" entry from the .cspell.json
dictionary: locate the second occurrence of the word "Zrdm" (the duplicate at
the later entry) and delete it so only a single "Zrdm" entry remains in the
list, preserving the original ordering and JSON syntax.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.cspell.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
.cspell.json (1)
812-814: LGTM! New vocabulary entries added correctly.The new spell-check entries "backoff", "autodrain", and "pluginname" are properly formatted and align with the PR's plugin-related and retry mechanism changes. JSON syntax is correct with proper comma placement.
fix(plugin-category): refine slug generation regex for leading/trailing dashes
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In
@packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts:
- Around line 454-484: The isSafePath method should explicitly reject Unix-style
absolute paths to fail fast on platforms where path.isAbsolute(entryPath) may
not detect them; after computing normalizedEntry (the variable in the function),
add a check that returns false if normalizedEntry starts with '/' (or is exactly
'/'), so that any entryPath beginning with a leading slash is rejected before
resolving; keep the existing containment checks (resolvedRoot/resolvedTarget)
intact and continue using entryPath for path.resolve as currently implemented.
- Around line 334-344: The block that re-checks resolvedFullPath against
baseExtractPath (the if that logs via logger.warn, calls entry.autodrain(), and
returns) is redundant because isSafePath was already called earlier (around the
isSafePath check near the isSafePath call at line ~309); remove that entire
containment-check block (the resolvedFullPath.startsWith/... && resolvedFullPath
!== baseExtractPath condition and its body) so the code relies on the existing
isSafePath validation and avoid duplicate draining/logging; ensure no other code
depends on that exact return and that entry.autodrain() is only invoked where
isSafePath indicates unsafe.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/[a-z0-9-]*.{ts,html,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Separate words in Angular file names with hyphens and use lowercase (e.g., user-profile.ts, user-profile.html, user-profile.css)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
**/*.ts: Match file names to the primary TypeScript identifier within (class/function) and avoid generic names like helpers.ts or utils.ts
Prefer the inject() function over constructor parameter injection for Angular dependencies
Group Angular-specific properties (injections, inputs, outputs, queries) before class methods in components/directives
Mark class members used only by the component’s template as protected to avoid leaking public API
Mark Angular-initialized members (input/model/output and queries) as readonly; for decorator APIs, apply to outputs and queries (not inputs)
Keep lifecycle hook methods simple; move logic into well‑named private methods and call them from the hook
Implement Angular lifecycle hook interfaces (e.g., OnInit) to ensure correct hook naming
Use a consistent, project-specific selector prefix for components/directives and never use the ng prefix
For directive attribute selectors, use lowercase, dash-case attributes (e.g., button[yt-upload])
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.{ts,html,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Use the same base file name for a component’s TypeScript, template, and style files (e.g., user-profile.ts/html/css)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.{html,ts}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Name event handlers for what they do (e.g., saveUserData) instead of the triggering event (e.g., handleClick)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
🧠 Learnings (1)
📚 Learning: 2025-06-16T15:15:03.909Z
Learnt from: syns2191
Repo: ever-co/ever-gauzy PR: 8999
File: apps/agent/src/main/window-manager.ts:175-176
Timestamp: 2025-06-16T15:15:03.909Z
Learning: In the apps/agent/src/main/window-manager.ts file, the preload path returned by getPreloadPath() is validated for existence in another function, so additional fs.existsSync checks are not needed before passing it to window constructors like ScreenCaptureNotification.
Applied to files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
🧬 Code graph analysis (1)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts (2)
packages/desktop-core/src/lib/window-manager/concretes/window-config.ts (2)
path(38-40)path(41-43)packages/desktop-core/src/lib/logger/logger.ts (1)
logger(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts (5)
6-6: LGTM! Import added for proper stream completion tracking.The
finishedimport is correctly used later (lines 357, 583) to await write stream completion, which addresses the race condition flagged in previous reviews.
300-302: LGTM! Proper setup for tracking async write operations.The
baseExtractPathresolution andpendingWritesarray initialization correctly prepare for coordinated write completion tracking.
309-313: LGTM! Path traversal check correctly applied.The
isSafePathvalidation properly prevents zip slip attacks by checking paths before extraction.
349-373: Excellent fix for the async write race condition!The refactored write logic correctly:
- Creates parent directories before writing files
- Awaits stream completion using
finished()- Tracks all write operations in
pendingWrites- Waits for
Promise.all(pendingWrites)in the close handlerThis directly addresses the race condition flagged in previous reviews where the 'close' event could fire before async operations completed.
Based on past review comments, this resolves the identified concurrency issue.
527-609: LGTM! Streaming extraction properly secured and coordinated.The streaming path correctly implements:
- Path traversal protection via
isSafePath(lines 535, 549)- Size limits on individual files and total extraction
- Async write coordination with
pendingWritestracking- Proper stream completion awaiting via
finished()- Directory creation before file writes
- Manifest location detection during extraction
The implementation is consistent with the
unzipmethod and avoids the redundant containment check present in that method.
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Outdated
Show resolved
Hide resolved
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Fixed
Show fixed
Hide fixed
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In
@packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts:
- Around line 308-323: The unzip function duplicates path-safety checks already
implemented by isSafePath (used by streamAndExtract); refactor unzip to call the
shared isSafePath helper (or extract the inline logic into that helper) instead
of repeating the inline checks so both code paths use the same validation;
update references in unzip (the entryPath normalization/validation block and the
early-return behavior that calls entry.autodrain()) to delegate to
isSafePath(entryPath) and preserve the same logging and drain behavior when
false.
- Around line 572-583: For directory entries (where type === 'Directory') call
entry.autodrain() synchronously before awaiting fs.mkdir to avoid holding the
entry stream; move the entry.autodrain() out of the async IIFE so it executes
immediately, then perform the async mkdir inside the dirPromise (which is pushed
into pendingWrites) and handle logging on errors from fs.mkdir; reference the
existing dirPromise, pendingWrites, entry.autodrain(), and fs.mkdir symbols when
making the change.
- Around line 604-607: In the catch for the block that pipes entries to disk
(where you call entry.pipe(writeStream) and currently call entry.autodrain()),
don't rely on entry.autodrain() after piping; instead, destroy/close the
writeStream, remove the partial file (fs.unlink or fs.promises.unlink using the
same filePath), and re-throw the error so upstream can handle it; mirror the
approach used in the unzip method by cleaning up the stream and filesystem on
failure rather than calling entry.autodrain().
♻️ Duplicate comments (1)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts (1)
473-479: Add explicit check for Unix-style absolute paths on Windows.As noted in past reviews,
path.isAbsolute('/etc/passwd')returnsfalseon Windows because it only recognizes Windows-style absolute paths. While the containment check at lines 490-492 ultimately prevents escape, explicitly rejecting paths starting with/would fail faster and make the validation clearer.🔎 Proposed enhancement
// Disallow absolute paths and drive letters if (path.isAbsolute(entryPath)) { return false; } + // Explicitly reject Unix-style absolute paths (not caught by path.isAbsolute on Windows) + if (normalizedEntry.startsWith('/')) { + return false; + } if (/^[a-zA-Z]:/.test(normalizedEntry)) { return false; }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/[a-z0-9-]*.{ts,html,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Separate words in Angular file names with hyphens and use lowercase (e.g., user-profile.ts, user-profile.html, user-profile.css)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
**/*.ts: Match file names to the primary TypeScript identifier within (class/function) and avoid generic names like helpers.ts or utils.ts
Prefer the inject() function over constructor parameter injection for Angular dependencies
Group Angular-specific properties (injections, inputs, outputs, queries) before class methods in components/directives
Mark class members used only by the component’s template as protected to avoid leaking public API
Mark Angular-initialized members (input/model/output and queries) as readonly; for decorator APIs, apply to outputs and queries (not inputs)
Keep lifecycle hook methods simple; move logic into well‑named private methods and call them from the hook
Implement Angular lifecycle hook interfaces (e.g., OnInit) to ensure correct hook naming
Use a consistent, project-specific selector prefix for components/directives and never use the ng prefix
For directive attribute selectors, use lowercase, dash-case attributes (e.g., button[yt-upload])
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.{ts,html,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Use the same base file name for a component’s TypeScript, template, and style files (e.g., user-profile.ts/html/css)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.{html,ts}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Name event handlers for what they do (e.g., saveUserData) instead of the triggering event (e.g., handleClick)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
🧠 Learnings (1)
📚 Learning: 2025-06-16T15:15:03.909Z
Learnt from: syns2191
Repo: ever-co/ever-gauzy PR: 8999
File: apps/agent/src/main/window-manager.ts:175-176
Timestamp: 2025-06-16T15:15:03.909Z
Learning: In the apps/agent/src/main/window-manager.ts file, the preload path returned by getPreloadPath() is validated for existence in another function, so additional fs.existsSync checks are not needed before passing it to window constructors like ScreenCaptureNotification.
Applied to files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build
🔇 Additional comments (4)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts (4)
6-6: LGTM!Good use of
finishedfromstream/promisesfor properly awaiting stream completion. This is the idiomatic Node.js approach for converting stream events to promises.
343-355: Appropriate defense-in-depth for Zip Slip prevention.The dual-layer validation (pattern-based at lines 312-319 and resolution-based at lines 348-355) provides robust protection against path traversal attacks. This is a reasonable security pattern for archive extraction.
377-385: LGTM!Good fix for the race condition identified in past reviews. Waiting for
Promise.all(pendingWrites)before resolving ensures all file writes complete before the extraction is considered done.
357-358: LGTM!Skipping explicit directory creation and relying on
fs.mkdir({ recursive: true })when writing files is an efficient approach. Empty directories won't be preserved, but this is acceptable for plugin extraction.
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Show resolved
Hide resolved
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Outdated
Show resolved
Hide resolved
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Outdated
Show resolved
Hide resolved
…paths and removing redundant path validation
…ve redundant baseExtractPath variable
The previous regex ` /\\/g ` for normalizing Windows path separators in `entryPath` was overly broad and could lead to incorrect path normalization in certain edge cases. This change updates the `replaceAll` method to use a literal backslash character `'\'` instead of a regex, ensuring only backslashes are replaced with forward slashes. This provides more precise and reliable path normalization.
Refactors the `CdnDownloadStrategy` to use dedicated helper methods for directory creation and file extraction. This improves readability and maintainability of the archive processing logic. The changes include: - Extracting directory creation into `createDirectory`. - Extracting file writing logic into `extractFile`. - Renaming `filePath` to `entryPath` for clarity. - Using `path.resolve` for safer path handling. - Ensuring `entry.autodrain()` is called in error scenarios.
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Fixed
Show fixed
Hide fixed
Add validation to the `extractEntry` function to prevent path traversal vulnerabilities during the extraction of zip archives. The target path is now resolved against the base directory of the extraction to ensure it remains within the intended scope. This change enhances security by ensuring that malicious zip archives cannot write files outside of their designated extraction directory. The `baseDir` option is now passed to `extractEntry` and used to construct a safe target path.
…dn download This commit addresses several issues in the `CdnDownloadStrategy` related to error handling and stream management during plugin downloads. Key changes: - **Robust Error Logging:** Error messages for directory creation and file extraction now include the full error stack or message for better debugging. They also explicitly convert non-Error objects to strings. - **Stream Pipeline for Extraction:** The `entry.pipe(writeStream)` and `finished(writeStream)` combination has been replaced with `pipeline(entry, writeStream)`. This provides more reliable error propagation and ensures streams are properly cleaned up. - **Resource Cleanup on Error:** In the `extractFile` method, if an error occurs during file extraction, the `entry` and `writeStream` are explicitly destroyed to release resources. - **Incomplete File Removal:** If an error occurs during file extraction, any partially created target file is now removed to prevent leaving incomplete artifacts. This handles potential `ENOENT` errors gracefully. - **Throwing Errors:** Errors are now consistently re-thrown after cleanup, ensuring that the calling code is aware of and can handle extraction failures.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts:
- Around line 634-645: The createDirectory method drains the unzipper Entry too
late (in the finally block) causing backpressure; call entry.autodrain()
immediately for directory entries before awaiting fs.mkdir() so the entry stream
is consumed synchronously, then await fs.mkdir(dirPath, { recursive: true }) and
push dirPath to extractedFiles, and keep the existing error logging/throwing
behavior in the catch block.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/[a-z0-9-]*.{ts,html,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Separate words in Angular file names with hyphens and use lowercase (e.g., user-profile.ts, user-profile.html, user-profile.css)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
**/*.ts: Match file names to the primary TypeScript identifier within (class/function) and avoid generic names like helpers.ts or utils.ts
Prefer the inject() function over constructor parameter injection for Angular dependencies
Group Angular-specific properties (injections, inputs, outputs, queries) before class methods in components/directives
Mark class members used only by the component’s template as protected to avoid leaking public API
Mark Angular-initialized members (input/model/output and queries) as readonly; for decorator APIs, apply to outputs and queries (not inputs)
Keep lifecycle hook methods simple; move logic into well‑named private methods and call them from the hook
Implement Angular lifecycle hook interfaces (e.g., OnInit) to ensure correct hook naming
Use a consistent, project-specific selector prefix for components/directives and never use the ng prefix
For directive attribute selectors, use lowercase, dash-case attributes (e.g., button[yt-upload])
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.{ts,html,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Use the same base file name for a component’s TypeScript, template, and style files (e.g., user-profile.ts/html/css)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.{html,ts}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Name event handlers for what they do (e.g., saveUserData) instead of the triggering event (e.g., handleClick)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
🧠 Learnings (1)
📚 Learning: 2025-06-16T15:15:03.909Z
Learnt from: syns2191
Repo: ever-co/ever-gauzy PR: 8999
File: apps/agent/src/main/window-manager.ts:175-176
Timestamp: 2025-06-16T15:15:03.909Z
Learning: In the apps/agent/src/main/window-manager.ts file, the preload path returned by getPreloadPath() is validated for existence in another function, so additional fs.existsSync checks are not needed before passing it to window constructors like ScreenCaptureNotification.
Applied to files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts (4)
5-5: LGTM! Proper stream utilities imported.The addition of
finishedandpipelinefromnode:stream/promisesprovides robust stream completion tracking and error propagation, addressing previous concerns about race conditions in async extraction operations.
454-489: LGTM! Path validation correctly addresses previous security concerns.The implementation properly:
- Explicitly rejects Unix-style absolute paths (lines 463-465) for fast-fail on all platforms
- Uses the original
entryPathforpath.resolve()(line 477) to ensure correct cross-platform behavior- Validates containment with proper boundary checks (lines 480-486)
Note: Line 480-482 rejects extraction directly to
rootDiritself, which is correct—archive entries should extract within the root, not as the root.Based on past review comments, this resolves the path traversal concerns.
532-592: LGTM! Streaming extraction properly addresses race conditions.The implementation correctly:
- Tracks all async operations in
pendingWrites(line 532)- Uses consistent
isSafePathvalidation (line 540)- Waits for all pending writes before resolving (lines 583-590)
- Delegates to helper methods for modular, testable extraction logic
This addresses the race condition concerns from previous reviews where the
'close'event could fire before async operations completed.
647-716: LGTM! Excellent stream handling and error recovery.The
extractFileimplementation correctly addresses all previous stream handling concerns:
- Proper stream orchestration: Uses
pipeline(line 676) for robust error propagation instead of manual piping- Resource cleanup: Destroys both entry and writeStream on error (lines 689-694)
- Incomplete file removal: Cleans up partial writes with proper ENOENT handling (lines 697-711)
- Error propagation: Re-throws errors (line 714) to ensure failures aren't silently swallowed
- Defense-in-depth validation: Secondary path check using
fs.realpath(lines 662-669) provides additional security beyond the initialisSafePathvalidationThis implementation follows best practices for Node.js stream error handling and addresses the concerns raised in past review comments about
entry.autodrain()after piping.
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Show resolved
Hide resolved
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Show resolved
Hide resolved
The previous implementation of `CdnDownloadStrategy` caught stream write errors and autodrained the entry. This masked the underlying error and prevented `Promise.all` from rejecting, leading to silent failures when downloading plugin files. This change re-throws the caught error. This ensures that any failure during the file writing process is propagated up to `Promise.all`, allowing the overall download operation to fail gracefully and be reported to the user.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/[a-z0-9-]*.{ts,html,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Separate words in Angular file names with hyphens and use lowercase (e.g., user-profile.ts, user-profile.html, user-profile.css)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
**/*.ts: Match file names to the primary TypeScript identifier within (class/function) and avoid generic names like helpers.ts or utils.ts
Prefer the inject() function over constructor parameter injection for Angular dependencies
Group Angular-specific properties (injections, inputs, outputs, queries) before class methods in components/directives
Mark class members used only by the component’s template as protected to avoid leaking public API
Mark Angular-initialized members (input/model/output and queries) as readonly; for decorator APIs, apply to outputs and queries (not inputs)
Keep lifecycle hook methods simple; move logic into well‑named private methods and call them from the hook
Implement Angular lifecycle hook interfaces (e.g., OnInit) to ensure correct hook naming
Use a consistent, project-specific selector prefix for components/directives and never use the ng prefix
For directive attribute selectors, use lowercase, dash-case attributes (e.g., button[yt-upload])
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.{ts,html,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Use the same base file name for a component’s TypeScript, template, and style files (e.g., user-profile.ts/html/css)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.{html,ts}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Name event handlers for what they do (e.g., saveUserData) instead of the triggering event (e.g., handleClick)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
🧠 Learnings (1)
📚 Learning: 2025-06-16T15:15:03.909Z
Learnt from: syns2191
Repo: ever-co/ever-gauzy PR: 8999
File: apps/agent/src/main/window-manager.ts:175-176
Timestamp: 2025-06-16T15:15:03.909Z
Learning: In the apps/agent/src/main/window-manager.ts file, the preload path returned by getPreloadPath() is validated for existence in another function, so additional fs.existsSync checks are not needed before passing it to window constructors like ScreenCaptureNotification.
Applied to files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
🧬 Code graph analysis (1)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts (1)
packages/desktop-core/src/lib/logger/logger.ts (1)
logger(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts (5)
5-5: LGTM: Proper stream utilities imported.The addition of
finishedandpipelinefromnode:stream/promisesprovides better error propagation and stream lifecycle management compared to direct piping.
367-374: LGTM: Race condition resolved.The
closehandler now properly awaits all pending write operations viaPromise.all(pendingWrites)before resolving. This ensures extraction completes fully before the promise resolves, addressing the race condition flagged in previous reviews.
455-490: LGTM: Comprehensive path traversal prevention.The
isSafePathimplementation provides defense-in-depth:
- Explicitly rejects Unix-style absolute paths (line 464) for fast-fail on all platforms
- Rejects Windows absolute paths and drive letters (lines 469-474)
- Validates containment via resolved paths (lines 477-487)
- Correctly uses the original
entryPathwithpath.resolve()(line 478) for cross-platform compatibilityThis addresses path traversal vulnerabilities (Zip Slip) comprehensively.
533-593: LGTM: Consistent validation and proper async handling.The streaming extraction path:
- Delegates to
isSafePathfor consistent validation (line 541)- Enforces per-file and total size limits (lines 547-560)
- Uses helper methods for modularity (lines 566, 571-582)
- Properly awaits all pending writes before resolving (lines 584-592)
This approach is consistent with the refactoring suggested for the
unzipmethod.
684-716: LGTM: Excellent error handling and resource cleanup.The error handling in
extractFile:
- Destroys streams to release resources (lines 690-695)
- Removes incomplete files (lines 698-712) with proper ENOENT handling
- Re-throws errors for propagation (line 715)
- Provides detailed error logging with stack traces (lines 685-687)
This is the correct pattern for stream-based extraction and should be mirrored in the
unzipmethod as suggested in a previous comment.
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts">
<violation number="1" location="packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts:361">
P1: Rethrowing here propagates the error, but the unzipper `entry` stream is no longer drained/destroyed on failure. A write error can leave the entry unread and stall the unzip parse. Drain (or destroy) the entry before rethrowing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Show resolved
Hide resolved
…eam is no longer drained/destroyed on failure. A write error can leave the entry unread and stall the unzip parse. Drain (or destroy) the entry before rethrowing. Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts:
- Around line 348-366: The write branch that creates writePromise is missing the
robust cleanup used by extractFile: replace the current catch block so that on
any error you properly destroy the write stream (the createWriteStream
instance), stop/consume the entry (use entry.autodrain() only if the entry is
not already piped or call entry.resume()/unpipe as appropriate), and remove any
partial file at resolvedFullPath (fs.unlink) before re-throwing; ensure you
await or synchronously handle stream destruction and file unlinking similar to
extractFile, and keep pushing the promise into pendingWrites as before so
Promise.all sees the propagated error.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/[a-z0-9-]*.{ts,html,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Separate words in Angular file names with hyphens and use lowercase (e.g., user-profile.ts, user-profile.html, user-profile.css)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
**/*.ts: Match file names to the primary TypeScript identifier within (class/function) and avoid generic names like helpers.ts or utils.ts
Prefer the inject() function over constructor parameter injection for Angular dependencies
Group Angular-specific properties (injections, inputs, outputs, queries) before class methods in components/directives
Mark class members used only by the component’s template as protected to avoid leaking public API
Mark Angular-initialized members (input/model/output and queries) as readonly; for decorator APIs, apply to outputs and queries (not inputs)
Keep lifecycle hook methods simple; move logic into well‑named private methods and call them from the hook
Implement Angular lifecycle hook interfaces (e.g., OnInit) to ensure correct hook naming
Use a consistent, project-specific selector prefix for components/directives and never use the ng prefix
For directive attribute selectors, use lowercase, dash-case attributes (e.g., button[yt-upload])
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.{ts,html,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Use the same base file name for a component’s TypeScript, template, and style files (e.g., user-profile.ts/html/css)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
**/*.{html,ts}
📄 CodeRabbit inference engine (.cursor/rules/angular.mdc)
Name event handlers for what they do (e.g., saveUserData) instead of the triggering event (e.g., handleClick)
Files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
🧠 Learnings (2)
📚 Learning: 2025-06-16T15:15:03.909Z
Learnt from: syns2191
Repo: ever-co/ever-gauzy PR: 8999
File: apps/agent/src/main/window-manager.ts:175-176
Timestamp: 2025-06-16T15:15:03.909Z
Learning: In the apps/agent/src/main/window-manager.ts file, the preload path returned by getPreloadPath() is validated for existence in another function, so additional fs.existsSync checks are not needed before passing it to window constructors like ScreenCaptureNotification.
Applied to files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
📚 Learning: 2025-10-28T11:15:47.548Z
Learnt from: syns2191
Repo: ever-co/ever-gauzy PR: 9180
File: packages/desktop-ui-lib/src/lib/offline-sync/concretes/sequence-queue.ts:50-52
Timestamp: 2025-10-28T11:15:47.548Z
Learning: In packages/desktop-ui-lib/src/lib/offline-sync/concretes/sequence-queue.ts, error handling for HTTP requests is managed by the UnauthorizedInterceptor, which uses ErrorMapping to provide user feedback. Errors should not be silently caught with `.catch(() => null)` as it bypasses the error handling infrastructure and makes debugging difficult.
Applied to files:
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
🧬 Code graph analysis (1)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts (1)
packages/desktop-core/src/lib/logger/logger.ts (1)
logger(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build
🔇 Additional comments (4)
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts (4)
5-5: Good use ofpipelinefor stream operations.Using
pipelinefromnode:stream/promisesprovides better error handling and automatic cleanup compared to manual piping withfinished. This aligns well with the improvements inextractFile.
456-491: Well-structured path validation with defense-in-depth.The implementation correctly:
- Rejects Unix-style absolute paths explicitly (fast fail on all platforms)
- Uses original
entryPathforpath.resolve()to ensure cross-platform correctness- Applies containment check as final safeguard
649-718: Robust error handling with proper stream cleanup.The implementation correctly:
- Uses
pipeline()for reliable stream handling with error propagation- Destroys both entry and write streams on failure
- Cleans up partial files with proper
ENOENThandling- Re-throws errors to propagate failures upstream
The additional path validation at lines 663-671 (after
isSafePathwas already called) adds defense-in-depth viafs.realpath(), though it introduces extra I/O overhead. Consider adding a comment explaining this is intentional security hardening if the redundancy is by design.
534-594: Good async operation tracking with Promise.all.The pattern of tracking all write operations in
pendingWritesand awaitingPromise.allon the'finish'event ensures complete extraction before proceeding. This addresses the race condition concern from previous reviews.
packages/desktop-lib/src/lib/plugin-system/data-access/strategies/cdn-download.strategy.ts
Show resolved
Hide resolved
Add robust error handling to the CDN download strategy. This includes: - Logging detailed error messages, including stack traces when available. - Destroying pipe streams to release resources on error. - Attempting to remove incomplete files to prevent corrupted downloads. - Handling potential errors during file removal (e.g., if the file doesn't exist). - Ensuring the `entry` stream is also handled correctly on error.
The previous implementation incorrectly called `entry.autodrain()` before re-throwing the error. This prevented the error from being propagated correctly, potentially leading to unhandled promise rejections. This change ensures that the error is thrown immediately, allowing the calling code to catch and handle it as expected.
|
|
@evereq PR is ready |
2 participants
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by cubic
Secures plugin ZIP extraction to prevent path traversal and ensures reliable, safe file writes. Also simplifies slug generation and improves error logging.
Written for commit f9ca126. Summary will update on new commits.