Skip to content

fix(install): detect curl version before using --ssl-revoke-best-effort#1116

Closed
Ellien-Tang wants to merge 1 commit into
larksuite:mainfrom
Ellien-Tang:main
Closed

fix(install): detect curl version before using --ssl-revoke-best-effort#1116
Ellien-Tang wants to merge 1 commit into
larksuite:mainfrom
Ellien-Tang:main

Conversation

@Ellien-Tang

@Ellien-Tang Ellien-Tang commented May 26, 2026

Copy link
Copy Markdown
Contributor

Problem

On older Windows systems (e.g., Windows 10 build 19044 with curl 7.55.1), the npm installation fails because the --ssl-revoke-best-effort flag is unconditionally added on Windows, but this flag was only introduced in curl 7.70.0 (2020-04-29).

Error message:

curl: option --ssl-revoke-best-effort: is unknown

Related Issue: Fixes #1099

Solution

Add a curlSupportsSslRevokeBestEffort() helper that:

  1. Runs curl --version to get the installed version
  2. Parses the semantic version from the first line
  3. Returns true only when curl >= 7.70.0

The download() function now checks both isWindows AND curlSupportsSslRevokeBestEffort() before adding the flag. Older curl versions gracefully fall back to standard SSL revocation checks.

Changes

  • Added curlSupportsSslRevokeBestEffort() function with version parsing logic
  • Modified the condition in download() from if (isWindows) to if (isWindows && curlSupportsSslRevokeBestEffort())
  • Exported the new function for testability
  • Added JSDoc comments explaining the version requirement

Backward Compatibility

  • Newer Windows (curl >= 7.70.0): Behavior unchanged, --ssl-revoke-best-effort is still used to avoid CRYPT_E_REVOCATION_OFFLINE errors
  • Older Windows (curl < 7.70.0): Installation now succeeds without the unsupported flag

Test Coverage

The version parsing correctly handles:

curl version Result
7.55.1 (old Windows 10) false - flag not used
7.69.0 (just below threshold) false - flag not used
7.70.0 (minimum supported) true - flag used
8.0.0+ (future versions) true - flag used

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced installation process compatibility with various curl versions on Windows by making flag application conditional based on system support.

Review Change Stack

@CLAassistant

CLAassistant commented May 26, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7e945f5d-42ef-4ec2-afb9-b84b516ea3e1

📥 Commits

Reviewing files that changed from the base of the PR and between 367cfc9 and da14737.

📒 Files selected for processing (1)
  • scripts/install.js

📝 Walkthrough

Walkthrough

The PR fixes an issue where curl installations on older Windows systems fail because they don't support the --ssl-revoke-best-effort flag. The solution adds version detection for curl, conditionally applies the flag only when supported, and exports the detection helper as part of the public module API.

Changes

Windows curl flag compatibility

Layer / File(s) Summary
Curl flag support detection and conditional application
scripts/install.js
Adds curlSupportsSslRevokeBestEffort() helper that runs curl --version, parses the semantic version, and safely defaults to false on errors; integrates the check into download() to conditionally prepend --ssl-revoke-best-effort only on Windows when supported; exports the helper alongside existing utility functions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • larksuite/cli#226: Both PRs modify scripts/install.js's curl download behavior on Windows—main PR adds conditional detection for --ssl-revoke-best-effort, while the retrieved PR introduces it unconditionally as part of switching downloads to curl.

Suggested labels

size/M

Suggested reviewers

  • liangshuo-1

Poem

A curl version dance on Windows so old, 🐰
Where flags were unknown, or so we are told,
Now checking what's there before we apply,
The fix brings sweet peace—no more installation cry! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: detecting curl version before conditionally using the --ssl-revoke-best-effort flag on Windows.
Description check ✅ Passed The description provides comprehensive context including the problem, solution, backward compatibility implications, and test coverage details.
Linked Issues check ✅ Passed The PR directly addresses issue #1099 by implementing curl version detection to prevent failures on older Windows systems with curl < 7.70.0.
Out of Scope Changes check ✅ Passed All changes in scripts/install.js are focused on the curl version detection feature and directly support the stated objective of fixing the installer failure on older Windows systems.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added the size/M Single-domain feat or fix with limited business impact label May 26, 2026
@liangshuo-1

Copy link
Copy Markdown
Collaborator

Thanks for the contribution, @Ellien-Tang! The change looks good. The only thing blocking the merge is the CLA — please sign it here: https://cla-assistant.io/larksuite/cli?pullRequest=1116 and the check will turn green automatically. 🙏

@liangshuo-1

Copy link
Copy Markdown
Collaborator

Thanks a lot for catching and fixing this, @Ellien-Tang! Your fix has been merged via #1124 with your original commit and authorship preserved. On top of it I added a small testability refactor (extracted the version comparison into a pure isCurlVersionSupported() so it can be unit-tested without spawning curl), a curl --version memoization so we probe at most once per install, and tests covering the 7.69.0 / 7.70.0 boundary plus the libcurl/X.Y.Z regex edge case.

Closing this one as superseded by #1124. Really appreciate the contribution — looking forward to more!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

windows低版本 使用curl安装二进制程序 --ssl-revoke-best-effort 不支持 导致安装失败

3 participants