fix: add manual download option for macOS users with old code signing#13378
fix: add manual download option for macOS users with old code signing#13378kangfenmao wants to merge 8 commits intomainfrom
Conversation
Due to developer signing certificate change, macOS users with the old Team ID (Q24M7JR2C4) cannot auto-update. This adds a manual download button that directs them to the official website. Also refactors AppUpdater headers into a shared getCommonHeaders() function and adds App-Name, App-Version, and OS headers for better update server analytics. Signed-off-by: kangfenmao <kangfenmao@qq.com>
GeorgeDong32
left a comment
There was a problem hiding this comment.
Code Review Summary
Thanks for this PR addressing the macOS code signing certificate change issue. The overall approach is solid and the refactoring to getCommonHeaders() follows DRY principles well.
⚠️ Required Changes
Security Issue in src/main/ipc.ts:180: The use of execSync with string interpolation poses a potential command injection risk. Please address this by using execFileSync with an argument array instead (see inline comment).
💡 Recommendations (Optional)
- Performance: Consider adding a timeout to the
codesigncommand execution to prevent potential hanging - Header Compatibility: Confirm that the server-side analytics has been updated to recognize the renamed header
Client-Id(previouslyX-Client-Id)
✅ What's Good
- Clean extraction of
getCommonHeaders()function - Comprehensive i18n coverage for all supported languages
- Proper error handling with fallback null values
- Good TypeScript type definitions
- Appropriate platform checks (
isMac)
Reviewed by kimi-k2.5
GeorgeDong32
left a comment
There was a problem hiding this comment.
This PR provides a well-designed solution for macOS users with old code signing certificates who cannot auto-update. Code quality is high, the fix addresses a real user pain point, and the refactoring improves maintainability. Approved for merge with optional minor suggestions addressed in inline comments. Reviewed by kimi-k2.5
Signed-off-by: kangfenmao <kangfenmao@qq.com>
|
Thanks for the review! Added timeout (5s) to the codesign command in commit 9387866 to prevent potential main process blocking. |
Critical Release NoteThis PR is essential and must be merged before the next release. Background: Starting from the next version, we will switch to a new code signing certificate. Due to macOS security restrictions, apps signed with the old certificate cannot auto-update to apps signed with a new certificate. Solution: This version serves as a transition release - the last version signed with the old certificate. It includes a manual download option that allows users to download and install the new version manually when auto-update fails due to certificate mismatch. Server-side coordination: The update server has been configured to route all older version users to this transition version first, and only expose newer versions (with new certificate) to users already on this version. Impact: All macOS users must update to this version before they can receive future updates. This PR ensures a smooth migration path for our entire macOS user base. |
There was a problem hiding this comment.
Note
This issue/comment/review was translated by Claude.
Code Review
The overall approach is sound. Extracting getCommonHeaders() and the manual download guidance are well-conceived strategies. There are several points to note:
- Regex Matching Bug: The
Identifier=regex incorrectly matchesTeamIdentifier=lines and requires a line-start anchor spawnSyncError Handling:spawnSyncwon't throw exceptions; currenttry/catchcan't cover timeout scenarios- Header Name Change:
X-Client-Id→Client-Idneeds server-side compatibility confirmation isMacReuse:isMacin@renderer/config/constantexists in the project, no need to redeclare
See inline comments for details.
| // Constants | ||
| const ZIP_PATTERN = /^Cherry-Studio-\d+\.\d+\.\d+(-arm64)?\.zip$/ | ||
| const APP_NAME = 'Cherry Studio.app' | ||
| const TARGET_PATH = `/Applications/${APP_NAME}` |
There was a problem hiding this comment.
Note
This issue/comment/review was translated by Claude.
Don't hardcode this. Many users don't have admin privileges, so they won't install in this directory, but rather in the Applications directory under their user directory, or in other directories under their user directory.
Original Content
不要写死,很多用户没有admin权限,就不会安装在这个目录,而是安装用户目录的application目录下面,或者用户目录下的其它目录。
There was a problem hiding this comment.
Note
This review was translated by Claude.
Re-review: New code in manualInstallUpdate()
The previous 3 issues (regex anchors, spawnSync error handling, isMac reuse) have been fixed ✅
The newly added manualInstallUpdate() method has complete logic, but here are some suggestions:
- Security:
execSyncuses string concatenation to executeunzip→ recommended to usespawnSyncwith array arguments - Code Style:
const execAsyncis interleaved between imports;spawnusesrequireinstead of top-level import - Robustness:
kill -9force-kills without triggering Electron lifecycle events, recommended to useapp.quit()for graceful shutdown
See inline comments for details.
Original Content
Re-review: manualInstallUpdate() 新增代码
之前的 3 个问题(正则锚点、spawnSync 错误处理、isMac 复用)都已修复 ✅
新增的 manualInstallUpdate() 方法逻辑完整,但有几个建议:
- Security:
execSync字符串拼接执行unzip→ 建议用spawnSync数组参数 - Code Style:
const execAsync夹在 import 之间;spawn用require而非顶部 import - Robustness:
kill -9强杀不触发 Electron 生命周期事件,建议用app.quit()优雅退出
详见 inline comments。
| const extractZip = (zipPath: string): void => { | ||
| fs.rmSync(extractDir, { recursive: true, force: true }) | ||
| fs.mkdirSync(extractDir, { recursive: true }) | ||
| execSync(`unzip -o "${zipPath}" -d "${extractDir}"`) |
There was a problem hiding this comment.
Note
This comment was translated by Claude.
Security: execSync uses string concatenation to execute shell commands
Although zipPath and extractDir currently come from controlled paths, using shell string concatenation is an unsafe pattern. If filenames contain special characters (such as spaces, quotes), it may lead to unexpected behavior.
It is recommended to use spawnSync to avoid shell interpretation:
const extractZip = (zipPath: string): void => {
fs.rmSync(extractDir, { recursive: true, force: true })
fs.mkdirSync(extractDir, { recursive: true })
const result = spawnSync('unzip', ['-o', zipPath, '-d', extractDir], { timeout: 30000 })
if (result.error || result.status !== 0) {
throw new Error(`unzip failed: ${result.stderr?.toString() || result.error?.message}`)
}
}Similarly, execAsync(\osascript "${scriptPath}"`)inreplaceAppWithAdminPrivilegesshould also be considered to be replaced withexecFile`.
Original Content
Security: execSync 使用字符串拼接执行 shell 命令
虽然 zipPath 和 extractDir 目前来自受控路径,但使用 shell 字符串拼接是不安全的模式。如果文件名包含特殊字符(如空格、引号),可能导致意外行为。
建议使用 spawnSync 避免 shell 解释:
const extractZip = (zipPath: string): void => {
fs.rmSync(extractDir, { recursive: true, force: true })
fs.mkdirSync(extractDir, { recursive: true })
const result = spawnSync('unzip', ['-o', zipPath, '-d', extractDir], { timeout: 30000 })
if (result.error || result.status !== 0) {
throw new Error(`unzip failed: ${result.stderr?.toString() || result.error?.message}`)
}
}同理,replaceAppWithAdminPrivileges 中的 execAsync(\osascript "${scriptPath}"`)也应考虑用execFile` 替代。
| import { promisify } from 'util' | ||
|
|
||
| const execAsync = promisify(exec) |
There was a problem hiding this comment.
Note
This comment was translated by Claude.
Code Style: Import and statement order mixed up
const execAsync = promisify(exec) is placed between two groups of import statements, which will cause lint errors (simple-import-sort). It is recommended to put all imports together, and place const execAsync after the import block.
Additionally, spawn is dynamically imported at line 401 via require('child_process'), but child_process is already imported at the top. It is recommended to add spawn directly to the top import:
import { exec, execSync, spawn } from 'child_process'Original Content
Code Style: import 和语句顺序混乱
const execAsync = promisify(exec) 被放在了两组 import 语句之间,这会导致 lint 报错(simple-import-sort)。建议把所有 import 放在一起,const execAsync 放在 import 块之后。
另外,spawn 在第 401 行通过 require('child_process') 动态引入,但顶部已经 import 了 child_process。建议直接在顶部 import 中添加 spawn:
import { exec, execSync, spawn } from 'child_process'| const scheduleRelaunch = (): void => { | ||
| const pid = process.pid | ||
| const scriptPath = path.join(app.getPath('temp'), `cherry-relaunch-${Date.now()}.sh`) | ||
| const script = `#!/bin/sh | ||
| sleep 1 | ||
| kill -9 ${pid} 2>/dev/null | ||
| # Wait for process exit (max 30s) | ||
| for i in $(seq 1 60); do kill -0 ${pid} 2>/dev/null || break; sleep 0.5; done | ||
| open "${TARGET_PATH}" | ||
| rm -f "${scriptPath}" | ||
| ` | ||
| fs.writeFileSync(scriptPath, script, { mode: 0o755 }) | ||
|
|
||
| const { spawn } = require('child_process') | ||
| spawn('/bin/sh', [scriptPath], { detached: true, stdio: 'ignore' }).unref() |
There was a problem hiding this comment.
Note
This comment was translated by Claude.
Robustness: scheduleRelaunch uses kill -9 to forcefully kill its own process
kill -9 doesn't give the process any chance to clean up (doesn't trigger Electron lifecycle events like before-quit, will-quit, etc.), which could lead to data loss or leftover file locks.
Suggestions:
- Prefer using
app.quit()orapp.exit()to let Electron exit normally, withkillonly as a fallback in the shell script - Or call
app.quit()before invokingscheduleRelaunch, so the shell script only waits for exit and relaunches
const scheduleRelaunch = (): void => {
const pid = process.pid
// ...write relaunch script...
spawn('/bin/sh', [scriptPath], { detached: true, stdio: 'ignore' }).unref()
// Let Electron shut down gracefully
app.quit()
}Original Content
Robustness: scheduleRelaunch 使用 kill -9 强杀自身进程
kill -9 不给进程任何清理机会(不触发 before-quit、will-quit 等 Electron 生命周期事件),可能导致数据丢失或文件锁残留。
建议:
- 优先使用
app.quit()或app.exit()让 Electron 正常退出,在 shell 脚本中只做 fallbackkill - 或者在调用
scheduleRelaunch之前先调用app.quit(),shell 脚本只负责等待退出后重启
const scheduleRelaunch = (): void => {
const pid = process.pid
// ...write relaunch script...
spawn('/bin/sh', [scriptPath], { detached: true, stdio: 'ignore' }).unref()
// Let Electron shut down gracefully
app.quit()
}
What this PR does
Before this PR:
After this PR:
App-Name,App-Version, andOSheaders to update requests for better server analyticsgetCommonHeaders()functionWhy we need it and why it was done in this way
The following tradeoffs were made:
codesign -dvto detect the Team ID at runtime is simple and reliable on macOSThe following alternatives were considered:
Breaking changes
None
Special notes for your reviewer
The old Team ID
Q24M7JR2C4is hardcoded as a constant. This is intentional since it represents the previous signing certificate that will no longer be used.Checklist
Release note