Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@larksuite/cli",
"version": "1.0.62",
"version": "1.0.63",
"description": "The official CLI for Lark/Feishu open platform",
"bin": {
"lark-cli": "scripts/run.js"
Expand Down
11 changes: 4 additions & 7 deletions scripts/install.js
Original file line number Diff line number Diff line change
Expand Up @@ -265,9 +265,10 @@ function getExpectedChecksum(archiveName, checksumsDir) {
const checksumsPath = path.join(dir, "checksums.txt");

if (!fs.existsSync(checksumsPath)) {
throw new Error(
"[SECURITY] checksums.txt not found; refusing to install an unverified binary."
console.error(
"[WARN] checksums.txt not found, skipping checksum verification"
);
return null;
Comment on lines +268 to +271

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Gate unverified installs behind an explicit opt-in.

This path now installs a downloaded binary with no checksum verification whenever checksums.txt is missing. A warning is easy to miss during npm install; consider failing closed by default and allowing the old behavior only via an explicit escape hatch.

Suggested direction
   if (!fs.existsSync(checksumsPath)) {
-    console.error(
-      "[WARN] checksums.txt not found, skipping checksum verification"
-    );
-    return null;
+    const message = "checksums.txt not found";
+    if (process.env.LARK_CLI_ALLOW_UNVERIFIED_INSTALL === "1") {
+      console.error(
+        `[WARN] ${message}, skipping checksum verification`
+      );
+      return null;
+    }
+    throw new Error(
+      `[SECURITY] ${message}; refusing to install an unverified binary. Set LARK_CLI_ALLOW_UNVERIFIED_INSTALL=1 to proceed.`
+    );
   }

Also applies to: 289-289

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/install.js` around lines 268 - 271, The checksum fallback in the
install flow currently allows a downloaded binary to be installed unverified
when checksums.txt is missing. Update the install logic in the checksum
verification path and the binary install flow to fail closed by default, and
require an explicit opt-in escape hatch to continue without verification. Use
the existing checksum handling in the install script (the checksums.txt branch
and the later install call site) to gate the unverified path, keeping the old
behavior only behind a clearly named opt-in flag or environment variable.

}

const content = fs.readFileSync(checksumsPath, "utf8");
Expand All @@ -285,11 +286,7 @@ function getExpectedChecksum(archiveName, checksumsDir) {
}

function verifyChecksum(archivePath, expectedHash) {
if (typeof expectedHash !== "string" || expectedHash.length === 0) {
throw new Error(
"[SECURITY] missing expected checksum; refusing to install an unverified binary."
);
}
if (expectedHash === null) return;

// Stream the file to avoid loading the entire archive into memory.
// Archives can be 10-100MB; streaming keeps RSS constant.
Expand Down
25 changes: 3 additions & 22 deletions scripts/install.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,17 +52,11 @@ describe("getExpectedChecksum", () => {
);
});

it("throws [SECURITY] when checksums.txt does not exist (fail-closed)", () => {
it("returns null when checksums.txt does not exist", () => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "checksum-test-"));
// No checksums.txt in dir
assert.throws(
() => getExpectedChecksum("anything.tar.gz", dir),
(err) => {
assert.match(err.message, /^\[SECURITY\]/);
assert.match(err.message, /checksums\.txt not found/);
return true;
}
);
const result = getExpectedChecksum("anything.tar.gz", dir);
assert.equal(result, null);
Comment on lines +55 to +59

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Assert the warning before accepting the fail-open result.

This test now verifies the null sentinel but not the user-visible security warning, so a future change could silently skip checksum verification and still pass.

Suggested test hardening
-  it("returns null when checksums.txt does not exist", () => {
+  it("returns null and warns when checksums.txt does not exist", () => {
     const dir = fs.mkdtempSync(path.join(os.tmpdir(), "checksum-test-"));
     // No checksums.txt in dir
-    const result = getExpectedChecksum("anything.tar.gz", dir);
-    assert.equal(result, null);
+    const originalError = console.error;
+    const messages = [];
+    console.error = (...args) => messages.push(args.join(" "));
+    try {
+      const result = getExpectedChecksum("anything.tar.gz", dir);
+      assert.equal(result, null);
+      assert.match(
+        messages.join("\n"),
+        /\[WARN\] checksums\.txt not found, skipping checksum verification/
+      );
+    } finally {
+      console.error = originalError;
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it("returns null when checksums.txt does not exist", () => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "checksum-test-"));
// No checksums.txt in dir
assert.throws(
() => getExpectedChecksum("anything.tar.gz", dir),
(err) => {
assert.match(err.message, /^\[SECURITY\]/);
assert.match(err.message, /checksums\.txt not found/);
return true;
}
);
const result = getExpectedChecksum("anything.tar.gz", dir);
assert.equal(result, null);
it("returns null and warns when checksums.txt does not exist", () => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "checksum-test-"));
// No checksums.txt in dir
const originalError = console.error;
const messages = [];
console.error = (...args) => messages.push(args.join(" "));
try {
const result = getExpectedChecksum("anything.tar.gz", dir);
assert.equal(result, null);
assert.match(
messages.join("\n"),
/\[WARN\] checksums\.txt not found, skipping checksum verification/
);
} finally {
console.error = originalError;
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/install.test.js` around lines 55 - 59, The checksum fallback test in
getExpectedChecksum should also assert the user-visible warning before accepting
the null fail-open result, so the behavior cannot silently regress. Update the
test around getExpectedChecksum in scripts/install.test.js to capture the
warning output and verify it is emitted when checksums.txt is missing, while
still asserting the null sentinel.

});

it("skips malformed lines and still finds valid entry", () => {
Expand Down Expand Up @@ -131,19 +125,6 @@ describe("verifyChecksum", () => {
}
);
});

it("verifyChecksum throws [SECURITY] on null/empty expectedHash (fail-closed)", () => {
const filePath = makeTmpFile("content");
for (const expectedHash of [null, ""]) {
assert.throws(
() => verifyChecksum(filePath, expectedHash),
(err) => {
assert.match(err.message, /^\[SECURITY\]/);
return true;
}
);
}
});
});

describe("assertAllowedHost", () => {
Expand Down
Loading