feat(install): enhance binary URL resolution with environment variabl…#690
Open
sang-neo03 wants to merge 4 commits intomainfrom
Open
feat(install): enhance binary URL resolution with environment variabl…#690sang-neo03 wants to merge 4 commits intomainfrom
sang-neo03 wants to merge 4 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe installer now computes an ordered mirror chain at runtime via Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as Installer
participant Env as Environment (LARK_CLI_DOWNLOAD_HOST, npm_config_registry)
participant Resolver as resolveMirrorUrls
participant Network as Network / curl
Installer->>Env: read env vars
Installer->>Resolver: resolveMirrorUrls(env)
Resolver-->>Installer: ordered mirror list (GitHub first, then mirrors)
alt GitHub available
Installer->>Network: download from GitHub
Network-->>Installer: binary or error
else iterate mirrors
Installer->>Network: attempt download from mirror[0]
alt success
Network-->>Installer: binary
else
Installer->>Network: attempt mirror[1] ... mirror[N]
Network-->>Installer: binary or final error
end
end
alt final failure
Installer-->>Installer: emit install guidance (--registry, LARK_CLI_DOWNLOAD_HOST, proxy tips)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
the
size/M
![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 `@scripts/install.js`:
- Around line 42-49: The module currently computes MIRROR_URL at module scope
using resolveMirrorUrl(process.env, archiveName, VERSION), which can throw
before install() runs; move that computation into the guarded path inside the
install() function (or a helper invoked from install()), and inside the existing
try/catch add the derived host to ALLOWED_HOSTS using new
URL(mirrorUrl).hostname; remove the module-scope call to resolveMirrorUrl so no
URL parsing happens during module load and ensure archiveName and VERSION are
referenced from the same scope used by install().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a666a0c2-e820-42cb-a047-38cbbe2aee28
📒 Files selected for processing (2)
scripts/install.jsscripts/install.test.js
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
==========================================
+ Coverage 62.92% 63.55% +0.63%
==========================================
Files 484 497 +13
Lines 41522 42455 +933
==========================================
+ Hits 26127 26984 +857
- Misses 13117 13129 +12
- Partials 2278 2342 +64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@ebbcef8e7172b3c3c1478fc87e797e47a651853a🧩 Skill updatenpx skills add larksuite/cli#fix/install-respect-registry -y -g |
…dly errors resolveMirrorUrl was called at module scope, so an invalid LARK_CLI_DOWNLOAD_HOST (e.g. file://) threw before the try/catch in the postinstall entrypoint, dumping a raw stack trace instead of the recovery guidance with proxy/registry/host-override options. Move resolution into install() via getMirrorUrl() so the throw is caught and the user sees the actionable help text.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install.js (1)
127-130: Prefer scoped allowlist admission over mutating globalALLOWED_HOSTS.Adding the resolved host to a module-global
Setmakes host permissions sticky for the rest of the process. It’s safer and easier to test if mirror host admission is per-install invocation.♻️ Suggested refactor (scoped host allowlist)
-function assertAllowedHost(url) { +function assertAllowedHost(url, allowedHosts = ALLOWED_HOSTS) { const { hostname } = new URL(url); - if (!ALLOWED_HOSTS.has(hostname)) { + if (!allowedHosts.has(hostname)) { throw new Error(`Download host not allowed: ${hostname}`); } } function getMirrorUrl(env) { const mirrorUrl = resolveMirrorUrl(env, archiveName, VERSION); - ALLOWED_HOSTS.add(new URL(mirrorUrl).hostname); - return mirrorUrl; + return { + mirrorUrl, + mirrorHost: new URL(mirrorUrl).hostname, + }; } -function download(url, destPath) { - assertAllowedHost(url); +function download(url, destPath, allowedHosts = ALLOWED_HOSTS) { + assertAllowedHost(url, allowedHosts); const args = [ @@ function install() { - const mirrorUrl = getMirrorUrl(process.env); + const { mirrorUrl, mirrorHost } = getMirrorUrl(process.env); + const allowedHosts = new Set(ALLOWED_HOSTS); + allowedHosts.add(mirrorHost); @@ - download(GITHUB_URL, archivePath); + download(GITHUB_URL, archivePath, allowedHosts); } catch (err) { - download(mirrorUrl, archivePath); + download(mirrorUrl, archivePath, allowedHosts); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.js` around lines 127 - 130, The function getMirrorUrl currently mutates the module-global ALLOWED_HOSTS by calling ALLOWED_HOSTS.add(new URL(mirrorUrl).hostname); instead make host admission scoped to the installation call: remove the global mutation from getMirrorUrl, and either (a) change getMirrorUrl(resolveMirrorUrl, archiveName, VERSION) to just return mirrorUrl and have the caller create a local Set and add new URL(mirrorUrl).hostname to that local allowlist, or (b) change getMirrorUrl signature to accept an allowlist Set param (e.g., allowlist) and add the hostname to that Set instead of ALLOWED_HOSTS; update all callers to pass a per-invocation Set so host permission is not sticky across the process. Ensure references to resolveMirrorUrl, archiveName, and VERSION remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/install.js`:
- Around line 127-130: The function getMirrorUrl currently mutates the
module-global ALLOWED_HOSTS by calling ALLOWED_HOSTS.add(new
URL(mirrorUrl).hostname); instead make host admission scoped to the installation
call: remove the global mutation from getMirrorUrl, and either (a) change
getMirrorUrl(resolveMirrorUrl, archiveName, VERSION) to just return mirrorUrl
and have the caller create a local Set and add new URL(mirrorUrl).hostname to
that local allowlist, or (b) change getMirrorUrl signature to accept an
allowlist Set param (e.g., allowlist) and add the hostname to that Set instead
of ALLOWED_HOSTS; update all callers to pass a per-invocation Set so host
permission is not sticky across the process. Ensure references to
resolveMirrorUrl, archiveName, and VERSION remain unchanged.
sang-neo03
force-pushed
the
fix/install-respect-registry
branch
from
a16506f to
8fbaace
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install.js (1)
60-60: Avoid hardcoding package name in binary path.
resolveMirrorUrluses a literallark-clieven thoughNAMEalready defines it. ReusingNAMEavoids drift if package naming changes.Suggested refactor
- const binaryPath = `/-/binary/lark-cli/v${version}/${archive}`; + const binaryPath = `/-/binary/${NAME}/v${version}/${archive}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.js` at line 60, The binaryPath construction hardcodes "lark-cli" instead of reusing the package name constant; update the template string in the binaryPath assignment to interpolate the existing NAME constant (e.g., use `${NAME}`) so it reads the mirror path using NAME along with the existing version and archive variables, and scan for any other occurrences of the literal "lark-cli" in this module to replace with NAME for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/install.js`:
- Line 60: The binaryPath construction hardcodes "lark-cli" instead of reusing
the package name constant; update the template string in the binaryPath
assignment to interpolate the existing NAME constant (e.g., use `${NAME}`) so it
reads the mirror path using NAME along with the existing version and archive
variables, and scan for any other occurrences of the literal "lark-cli" in this
module to replace with NAME for consistency.
resolveMirrorUrl returned a single URL, so any non-default
npm_config_registry replaced the npmmirror fallback entirely. Corporate
npm proxies (Verdaccio, Artifactory, Nexus) often only serve npm package
metadata and don't host /-/binary/<pkg>/..., turning previously-working
installs into 404s when GitHub is unreachable.
Switch to resolveMirrorUrls returning an ordered chain:
- LARK_CLI_DOWNLOAD_HOST set → [override] only (explicit user choice;
no silent leak to npmmirror).
- Otherwise → [derived_from_registry?, npmmirror_default]; npmmirror
is always the final entry, restoring the pre-PR safety net.
install() now walks [GITHUB_URL, ...mirrorUrls] and stops at the first
success.
github-actions
Bot
added
size/L
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 `@scripts/install.js`:
- Around line 47-49: The loop that builds download candidate URLs still prepends
GITHUB_URL even when LARK_CLI_DOWNLOAD_HOST is explicitly set, which causes
GitHub to be tried first; update the logic around where the candidate host list
is constructed (the code using LARK_CLI_DOWNLOAD_HOST and GITHUB_URL) so that
when process.env.LARK_CLI_DOWNLOAD_HOST (or the LARK_CLI_DOWNLOAD_HOST variable)
is present you do not prepend or include GITHUB_URL — only use the explicit host
(and any allowed fallbacks) so the GitHub attempt is skipped; locate the
host-list construction and loop that references GITHUB_URL and
LARK_CLI_DOWNLOAD_HOST and gate inclusion of GITHUB_URL behind the absence of
LARK_CLI_DOWNLOAD_HOST.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb864672-ca7b-48df-a2be-cb9274fc63f4
📒 Files selected for processing (2)
scripts/install.jsscripts/install.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/install.test.js
The download loop unconditionally tried GITHUB_URL first, even when the
user explicitly named a download host. In locked-down networks, probing
github.com can trigger DLP / firewall alerts and contradicts the
explicit-override semantics ("use only this host, nothing else").
When LARK_CLI_DOWNLOAD_HOST is set, the chain is now just [override].
When it isn't, behavior is unchanged: [GITHUB_URL, derived?, npmmirror].
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install.js (1)
69-69: PreferNAMEconstant inbinaryPathto avoid future drift.This is currently correct, but using the existing constant makes renames safer.
♻️ Proposed small refactor
- const binaryPath = `/-/binary/lark-cli/v${version}/${archive}`; + const binaryPath = `/-/binary/${NAME}/v${version}/${archive}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.js` at line 69, The path string for binaryPath currently embeds a literal name; replace the literal with the existing NAME constant to avoid drift: update the template used to build binaryPath (the variable binaryPath that currently uses archive and version) so it references NAME instead of the hard-coded segment (e.g., use `${NAME}` in the template alongside version and archive), ensuring the final path format remains `/-/binary/<NAME>/v${version}/${archive}`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/install.js`:
- Line 69: The path string for binaryPath currently embeds a literal name;
replace the literal with the existing NAME constant to avoid drift: update the
template used to build binaryPath (the variable binaryPath that currently uses
archive and version) so it references NAME instead of the hard-coded segment
(e.g., use `${NAME}` in the template alongside version and archive), ensuring
the final path format remains `/-/binary/<NAME>/v${version}/${archive}`.
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.
Summary
Fix issue #640: enterprise intranet
npm install -g @larksuite/cli --registry=<corp-mirror>fails because the postinstall script is hardcoded toregistry.npmmirror.comfor thebinary download. Add two user-controllable override channels (
LARK_CLI_DOWNLOAD_HOSTenv var and the--registryvalue), with HTTPS-only protocol guard to preventfile:///http:///ftp://from bypassing the host allowlist.Changes
scripts/install.js:MIRROR_URLwithresolveMirrorUrl(). Resolution order:LARK_CLI_DOWNLOAD_HOST→ non-defaultnpm_config_registry→registry.npmmirror.com./-/binary/...).https://+ non-empty hostname forLARK_CLI_DOWNLOAD_HOST(throw on bad input). Apply the same check to the implicitnpm_config_registrypath, but silently fallthrough on bad input to avoid breaking users with http-only internal registries that npm itself can still use.
ALLOWED_HOSTSto aSetand add the resolved mirror host at runtime; checksum verification remains the primary integrity control.scripts/install.test.js: add 13 unit tests covering the new resolver and HTTPS-only guard (file://,http://,ftp://, malformed URL, empty hostname, default-npmjs skip,http-registry fall-through, etc.).
Test Plan
node --test scripts/install.test.js→ 29/29LARK_CLI_DOWNLOAD_HOST=https://corp.example.com→ resolved tohttps://corp.example.com/-/binary/lark-cli/v<ver>/<archive>and admitted byassertAllowedHostLARK_CLI_DOWNLOAD_HOST=file:///tmp/http://.../ftp://...→ rejected withLARK_CLI_DOWNLOAD_HOST must be an https:// URL with a hostnamenpm_config_registry=https://registry.npmjs.org/→ does not derive, still uses npmmirror defaultRelated Issues
Summary by CodeRabbit
New Features
Bug Fixes