Reject symbolic links in build zip path#538
Conversation
📝 WalkthroughWalkthroughAdds symlink-aware ZIP traversal and a public inclusion predicate: use Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd4beb0f6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (stats.isSymbolicLink()) { | ||
| throw new Error(`Refusing to include symbolic link during zip: ${itemZipPath}`) | ||
| } |
There was a problem hiding this comment.
Throwing on any symbolic link at this point causes native builds to fail for pnpm-based projects, where node_modules/<package> paths are symlinks by design. Because the check runs before the existing include/filter logic, recursing into node_modules now aborts with Refusing to include symbolic link... instead of packaging required native dependencies, which is a regression for a supported package-manager layout.
Useful? React with 👍 / 👎.
![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
🤖 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/build/request.ts`:
- Around line 710-713: When a symlink is detected (using lstatSync and
lstats.isSymbolicLink()) statSync(itemPath) can throw ENOENT for dangling links;
modify the logic around statSync in the block that computes stats so that you
catch errors from statSync, detect ENOENT, and treat that case as a broken
symlink to be skipped (i.e., set stats to null/undefined or continue to skip
adding the entry) before calling shouldIncludeFile(itemZipPath, platform,
nativeDeps, platformDir); ensure you only call shouldIncludeFile when stats is
valid and reference lstatSync, statSync, isSymbolicLink, itemPath, and
shouldIncludeFile to locate the exact spot to add the try/catch and skip
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84503129-d202-4012-beee-a09fba4f0e8d
📒 Files selected for processing (2)
src/build/request.tstest/test-build-zip-filter.mjs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/test-build-zip-filter.mjs (1)
49-50: Use a cross-platform symlink type in tests.Using
'dir'insymlinkSynccan be flaky on Windows CI. Prefer'junction'on Windows and'dir'elsewhere.♻️ Proposed patch
+const dirLinkType = process.platform === 'win32' ? 'junction' : 'dir' ... - symlinkSync(join(testRoot, 'missing-pnpm-store', 'node_modules', '@capacitor'), join(testRoot, 'node_modules', '@capacitor'), 'dir') + symlinkSync(join(testRoot, 'missing-pnpm-store', 'node_modules', '@capacitor'), join(testRoot, 'node_modules', '@capacitor'), dirLinkType) ... - symlinkSync(join(pnpmStore, 'node_modules', '@capacitor'), linkedScopePath, 'dir') + symlinkSync(join(pnpmStore, 'node_modules', '@capacitor'), linkedScopePath, dirLinkType)Applies to: lines 49-50 and 102-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-build-zip-filter.mjs` around lines 49 - 50, Replace the hardcoded symlink type 'dir' in the symlinkSync calls with a cross-platform choice: determine the type using process.platform === 'win32' ? 'junction' : 'dir' and pass that variable into symlinkSync; update both occurrences of symlinkSync (the one that links '@capacitor' and the other at the later occurrence) so Windows CI uses 'junction' while other platforms use 'dir'.
🤖 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/build/request.ts`:
- Around line 714-721: The current bare catch around statSync(itemPath) in the
symlink-handling branch (guarded by isSymbolicLink) swallows all errors; change
it to only suppress dangling-link errors by catching the exception, checking its
code (e.g., error.code === 'ENOENT' or equivalent for a missing target) and only
continue for that case, otherwise rethrow or propagate the error so permission,
I/O, and other real filesystem failures are not silently ignored; update the
block referencing statSync, isSymbolicLink, stats, and itemPath accordingly.
---
Nitpick comments:
In `@test/test-build-zip-filter.mjs`:
- Around line 49-50: Replace the hardcoded symlink type 'dir' in the symlinkSync
calls with a cross-platform choice: determine the type using process.platform
=== 'win32' ? 'junction' : 'dir' and pass that variable into symlinkSync; update
both occurrences of symlinkSync (the one that links '@capacitor' and the other
at the later occurrence) so Windows CI uses 'junction' while other platforms use
'dir'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 472c163e-9980-477d-b282-65ceec7dae28
📒 Files selected for processing (2)
src/build/request.tstest/test-build-zip-filter.mjs
|
Reviewed the remaining symlink feedback against the current branch. The branch already handles the reported case: dangling symlinks are skipped without aborting ZIP creation, and the targeted ZIP filter test file passes locally after |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/build/request.ts (1)
1040-1046:⚠️ Potential issue | 🟠 MajorUse
platformDirin the deep-relative-path rewrite.This only normalizes paths ending in literal
ios/. For configs likeios.path: "apps/native/ios", pnpm-resolved paths look like../../../../../../apps/native/ios/..., so this regex never fires and the broken path survives in the archive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/request.ts` around lines 1040 - 1046, The deep-relative-path rewrite currently hardcodes 'ios' in the regex so paths like ../../../../apps/native/ios/... aren't normalized; update the replace call that runs when platform === 'ios' to use the platformDir variable instead of the literal 'ios' (i.e. build a regex that injects/escapes platformDir) and run rewritten.replace(...) with that dynamic regex so excessive ../ segments before the configured platformDir are collapsed to three levels; refer to the platform, platformDir, rewritten, and the existing replace invocation to locate and change the code.
🤖 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/build/request.ts`:
- Around line 682-684: Move the inline RegExp literals used in request parsing
into shared module-level constants and reuse them instead of constructing them
inside the matchAll/capture calls (e.g., the regex used where
spmContent.matchAll(...) is called); create descriptive constant names (for
example SPM_PACKAGE_REGEX and any other regexes used at the other occurrences)
at the top of src/build/request.ts and replace the inline /.package\s*\(.../g
and the similar literals found in the other match/capture sites with references
to those constants so the same compiled regex is reused across the functions
(also update any helper parsing functions to accept or import these constants
rather than re-declaring patterns).
- Around line 866-878: When handling symbolic links in the isSymbolicLink branch
(around statSync/itemPath), resolve the link target to its real path (using
realpathSync or async equivalent), ensure the resolved path is inside projectDir
(reject and skip if it is outside), and maintain a Set of visited real paths to
dedupe and prevent recursive loops (skip if already visited); then recurse using
the resolved real path instead of the original symlink path. Apply the same
realpath/inside-project/visited-set checks to the other symlink-handling section
referenced (lines ~905-961) so external targets are never included and symlink
loops cannot cause infinite recursion.
- Around line 1525-1534: The three log.error messages in src/build/request.ts
currently hardcode an iOS example and the unsupported flag "--apple-id"; update
these error messages to be platform-aware and to use the actual flags the
command parses: change the static messages inside the log.error calls to select
examples based on the current platform variable (e.g., if platform === 'ios'
emit iOS CLI/env/credentials examples using the real iOS flags the parser
expects; if platform === 'android' emit Android-specific CLI/env/credentials
examples such as keystore-related flags/env vars), or construct the message
dynamically from the parsed option names instead of hardcoding "--apple-id";
ensure the messages are updated where log.error(...) is called so users see the
correct recovery instructions for the requested platform.
---
Outside diff comments:
In `@src/build/request.ts`:
- Around line 1040-1046: The deep-relative-path rewrite currently hardcodes
'ios' in the regex so paths like ../../../../apps/native/ios/... aren't
normalized; update the replace call that runs when platform === 'ios' to use the
platformDir variable instead of the literal 'ios' (i.e. build a regex that
injects/escapes platformDir) and run rewritten.replace(...) with that dynamic
regex so excessive ../ segments before the configured platformDir are collapsed
to three levels; refer to the platform, platformDir, rewritten, and the existing
replace invocation to locate and change the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a63725f1-d588-4811-b092-c476c6133c04
📒 Files selected for processing (2)
src/build/request.tstest/test-build-zip-filter.mjs
| const spmMatches = spmContent.matchAll( | ||
| /\.package\s*\([^)]*path:\s*["'](?:\.\.\/)*node_modules\/([^"']+)["']\s*\)/g, | ||
| ) |
There was a problem hiding this comment.
Hoist the new static regexes to module scope.
These literals are all flagged by e18e/prefer-static-regex, so this file will keep failing lint until they become shared module-level constants/helpers.
As per coding guidelines Run \bun run lint` after making CLI changesandPrefer small reusable helpers for parsing and normalization logic instead of repeating ad hoc parsing inside command bodies`.
Also applies to: 717-719, 741-743, 1010-1011
🧰 Tools
🪛 ESLint
[error] 683-683: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/build/request.ts` around lines 682 - 684, Move the inline RegExp literals
used in request parsing into shared module-level constants and reuse them
instead of constructing them inside the matchAll/capture calls (e.g., the regex
used where spmContent.matchAll(...) is called); create descriptive constant
names (for example SPM_PACKAGE_REGEX and any other regexes used at the other
occurrences) at the top of src/build/request.ts and replace the inline
/.package\s*\(.../g and the similar literals found in the other match/capture
sites with references to those constants so the same compiled regex is reused
across the functions (also update any helper parsing functions to accept or
import these constants rather than re-declaring patterns).
| if (isSymbolicLink) { | ||
| try { | ||
| stats = statSync(itemPath) | ||
| } | ||
| catch (error) { | ||
| const code = (error as NodeJS.ErrnoException).code | ||
| if (code === 'ENOENT' || code === 'ENOTDIR') { | ||
| // Broken symlink: skip gracefully to avoid failing the whole zip. | ||
| continue | ||
| } | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
Keep followed symlinks inside projectDir.
Once the symlink branch resolves successfully, the later recursion walks whatever target it points to. A repo can hang ios, resources, or a matched node_modules package off a path outside the project and the CLI will zip/upload arbitrary local files. Please resolve the real path before recursing, reject targets outside the project root, and dedupe visited real paths so symlink loops cannot recurse forever.
Also applies to: 905-961
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/build/request.ts` around lines 866 - 878, When handling symbolic links in
the isSymbolicLink branch (around statSync/itemPath), resolve the link target to
its real path (using realpathSync or async equivalent), ensure the resolved path
is inside projectDir (reject and skip if it is outside), and maintain a Set of
visited real paths to dedupe and prevent recursive loops (skip if already
visited); then recurse using the resolved real path instead of the original
symlink path. Apply the same realpath/inside-project/visited-set checks to the
other symlink-handling section referenced (lines ~905-961) so external targets
are never included and symlink loops cannot cause infinite recursion.
| log.error('Provide credentials via:') | ||
| log.error(' 1. CLI arguments: npx @capgo/cli build request --platform ios --apple-id "..." --p12-password "..."') | ||
| log.error(' 2. Environment variables: export APPLE_ID="..." P12_PASSWORD="..."') | ||
| log.error(' 3. Saved credentials: npx @capgo/cli build credentials save --platform ios ...') | ||
| log.error( | ||
| ' 1. CLI arguments: npx @capgo/cli build request --platform ios --apple-id "..." --p12-password "..."', | ||
| ) | ||
| log.error( | ||
| ' 2. Environment variables: export APPLE_ID="..." P12_PASSWORD="..."', | ||
| ) | ||
| log.error( | ||
| ' 3. Saved credentials: npx @capgo/cli build credentials save --platform ios ...', | ||
| ) |
There was a problem hiding this comment.
Fix the recovery example for missing credentials.
This still points users at --apple-id, which is not parsed anywhere in this command, and it prints the same iOS example even when the missing credentials are for Android. Copy-pasting the message keeps the user blocked.
💡 Possible fix
- log.error(
- ' 1. CLI arguments: npx `@capgo/cli` build request --platform ios --apple-id "..." --p12-password "..."',
- )
+ log.error(
+ options.platform === 'ios'
+ ? ' 1. CLI arguments: npx `@capgo/cli` build request --platform ios --apple-key-id "..." --apple-issuer-id "..." --apple-key-content "..." --p12-password "..."'
+ : ' 1. CLI arguments: npx `@capgo/cli` build request --platform android --android-keystore-file "..." --keystore-key-alias "..." --keystore-store-password "..."',
+ )📝 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.
| log.error('Provide credentials via:') | |
| log.error(' 1. CLI arguments: npx @capgo/cli build request --platform ios --apple-id "..." --p12-password "..."') | |
| log.error(' 2. Environment variables: export APPLE_ID="..." P12_PASSWORD="..."') | |
| log.error(' 3. Saved credentials: npx @capgo/cli build credentials save --platform ios ...') | |
| log.error( | |
| ' 1. CLI arguments: npx @capgo/cli build request --platform ios --apple-id "..." --p12-password "..."', | |
| ) | |
| log.error( | |
| ' 2. Environment variables: export APPLE_ID="..." P12_PASSWORD="..."', | |
| ) | |
| log.error( | |
| ' 3. Saved credentials: npx @capgo/cli build credentials save --platform ios ...', | |
| ) | |
| log.error('Provide credentials via:') | |
| log.error( | |
| options.platform === 'ios' | |
| ? ' 1. CLI arguments: npx `@capgo/cli` build request --platform ios --apple-key-id "..." --apple-issuer-id "..." --apple-key-content "..." --p12-password "..."' | |
| : ' 1. CLI arguments: npx `@capgo/cli` build request --platform android --android-keystore-file "..." --keystore-key-alias "..." --keystore-store-password "..."', | |
| ) | |
| log.error( | |
| ' 2. Environment variables: export APPLE_ID="..." P12_PASSWORD="..."', | |
| ) | |
| log.error( | |
| ' 3. Saved credentials: npx `@capgo/cli` build credentials save --platform ios ...', | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/build/request.ts` around lines 1525 - 1534, The three log.error messages
in src/build/request.ts currently hardcode an iOS example and the unsupported
flag "--apple-id"; update these error messages to be platform-aware and to use
the actual flags the command parses: change the static messages inside the
log.error calls to select examples based on the current platform variable (e.g.,
if platform === 'ios' emit iOS CLI/env/credentials examples using the real iOS
flags the parser expects; if platform === 'android' emit Android-specific
CLI/env/credentials examples such as keystore-related flags/env vars), or
construct the message dynamically from the parsed option names instead of
hardcoding "--apple-id"; ensure the messages are updated where log.error(...) is
called so users see the correct recovery instructions for the requested
platform.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21f4e2a552
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isSymbolicLink) | ||
| continue |
There was a problem hiding this comment.
This unconditional skip regresses projects that share build-critical files via symlink. statSync() has already resolved the target, but a symlinked package.json, capacitor.config.json, or shared Podfile is still omitted from the archive. The capacitor.config.json case is especially problematic: zipDirectory() only adds the JSON fallback when existsSync(configJsonPath) is false, so a symlinked config file can disappear from the zip entirely and the remote builder loses custom ios.path/android.path settings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/build/request.ts (1)
714-775:⚠️ Potential issue | 🔴 CriticalSymlinked directory recursion can still escape
projectDirand loop.At Line 774, recursion still uses
itemPathfor symlinked directories. If a symlink that passesshouldRecursepoints outside the repo (or back to an ancestor), traversal can include arbitrary local files and recurse indefinitely via cycles.🔒 Proposed hardening patch
-import { existsSync, lstatSync, readdirSync, readFileSync, statSync } from 'node:fs' +import { existsSync, lstatSync, readdirSync, readFileSync, realpathSync, statSync } from 'node:fs' -import { basename, join, resolve } from 'node:path' +import { basename, isAbsolute, join, relative, resolve } from 'node:path'function addDirectoryToZip( zip: AdmZip, dirPath: string, zipPath: string, platform: 'ios' | 'android', nativeDeps: NativeDependencies, platformDir: string, + projectRootReal: string, + visitedRealDirs: Set<string>, ) { + const currentReal = realpathSync(dirPath) + if (visitedRealDirs.has(currentReal)) + return + visitedRealDirs.add(currentReal) + const items = readdirSync(dirPath) for (const item of items) { const itemPath = join(dirPath, item) const itemZipPath = zipPath ? `${zipPath}/${item}` : item @@ - if (stats.isDirectory()) { + let recursePath = itemPath + if (isSymbolicLink && stats.isDirectory()) { + const resolved = realpathSync(itemPath) + const rel = relative(projectRootReal, resolved) + if (isAbsolute(rel) || rel.startsWith('..')) + continue + if (visitedRealDirs.has(resolved)) + continue + recursePath = resolved + } + + if (stats.isDirectory()) { @@ if (item === 'node_modules') { - addDirectoryToZip(zip, itemPath, itemZipPath, platform, nativeDeps, platformDir) + addDirectoryToZip(zip, recursePath, itemZipPath, platform, nativeDeps, platformDir, projectRootReal, visitedRealDirs) continue } @@ if (item === 'resources') { - addDirectoryToZip(zip, itemPath, itemZipPath, platform, nativeDeps, platformDir) + addDirectoryToZip(zip, recursePath, itemZipPath, platform, nativeDeps, platformDir, projectRootReal, visitedRealDirs) continue } @@ if (shouldRecurse) { - addDirectoryToZip(zip, itemPath, itemZipPath, platform, nativeDeps, platformDir) + addDirectoryToZip(zip, recursePath, itemZipPath, platform, nativeDeps, platformDir, projectRootReal, visitedRealDirs) } }export async function zipDirectory(projectDir: string, outputPath: string, platform: 'ios' | 'android', capConfig: any): Promise<void> { @@ - addDirectoryToZip(zip, projectDir, '', platform, nativeDeps, platformDir) + const projectRootReal = realpathSync(projectDir) + addDirectoryToZip(zip, projectDir, '', platform, nativeDeps, platformDir, projectRootReal, new Set<string>())#!/bin/bash # Verify whether symlink traversal is bounded to project root and loop-protected. # Expected (for a fix): usage of realpathSync + relative/isAbsolute checks + visited set in addDirectoryToZip. rg -n -C3 'addDirectoryToZip\(|isSymbolicLink|realpathSync|relative\(|visitedReal' src/build/request.ts # Verify test coverage includes outside-project symlink and cycle cases. # Expected (for a fix): explicit tests for escaping projectDir and symlink loops. rg -n -C2 'symlink|dangling|outside|projectDir|loop|cycle|ELOOP' test/test-build-zip-filter.mjs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/request.ts` around lines 714 - 775, Symlinked directories can escape projectDir and cause infinite recursion because addDirectoryToZip still recurses on the symlink's itemPath; fix by resolving symlinks with realpathSync (or fs.realpathSync.native), check that the resolved path is inside the project root (projectDir) and skip if it is outside, and maintain a visited set of resolved directory paths (e.g., visitedRealPaths) in addDirectoryToZip to detect and skip cycles; update logic around isSymbolicLink/itemPath/itemZipPath to use the resolved path for stat/recursion and ensure shouldRecurse is evaluated against normalized real paths so outside-project and loop cases are prevented.
🤖 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/build/request.ts`:
- Around line 714-775: Symlinked directories can escape projectDir and cause
infinite recursion because addDirectoryToZip still recurses on the symlink's
itemPath; fix by resolving symlinks with realpathSync (or
fs.realpathSync.native), check that the resolved path is inside the project root
(projectDir) and skip if it is outside, and maintain a visited set of resolved
directory paths (e.g., visitedRealPaths) in addDirectoryToZip to detect and skip
cycles; update logic around isSymbolicLink/itemPath/itemZipPath to use the
resolved path for stat/recursion and ensure shouldRecurse is evaluated against
normalized real paths so outside-project and loop cases are prevented.
|
|
See PR #538. I checked the current state: CI is green on the GitHub Actions side, but SonarCloud Code Analysis is still failing, and the remaining review threads are about symlink traversal / required symlinked files / platform credential text. I do not see a safe branch-local fix that would avoid changing the intended build behavior, so this needs a broader follow-up outside this PR. |
|
Thanks for the contribution. This PR looks like a low-signal or template-style submission, which is hard to review and maintain. Please submit a focused PR with clear problem context, concrete diffs, and a short validation plan. |
1 participant
This PR updates build zipping to handle symbolic links safely.
It now avoids hard-failing by skipping dangling symlinks during directory traversal instead of crashing the zip.
Relevant symlinked node_modules package paths are still included when required by native dependency filtering.
Test coverage was updated in test/test-build-zip-filter.mjs with a pnpm-style symlinked package fixture and a dangling-symlink regression case.
Files changed: src/build/request.ts, test/test-build-zip-filter.mjs.
Summary by CodeRabbit
Bug Fixes
Tests