fix: Windows executable signature invalid #3175
Conversation
…e validity Fixes #3174 The Windows executable was showing 'signed but invalid' because fuses were being applied after signing, which modifies the binary and invalidates the signature. Changes: - Move fuse application from afterSign to afterPack (before signing) - Add signature verification after each signing step - Build fails if any signature is invalid Per Electron docs: fuses must be flipped 'at package time before you code sign your app' so the OS can verify signature integrity.
WalkthroughElectron fuses are now applied unconditionally in afterPack for all platforms; the Windows-specific fuse application was removed from notarize/afterSign. The Windows release action adds pre- and post-signing signature verification via a new verification module; minor timer cleanup in notarize replaced clearTimeout with clearInterval. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI / Build
participant AfterPack as afterPack
participant PreVerify as Pre-sign Verify
participant Signer as Code Signer (winSignKms)
participant AfterSign as afterSign / notarize
participant PostVerify as Post-sign Verify
CI->>AfterPack: run afterPack
AfterPack->>AfterPack: apply electron fuses (all platforms)
AfterPack-->>CI: afterPack complete
CI->>PreVerify: verify executables in dist (Rocket.Chat.exe)
PreVerify-->>CI: verification result
CI->>Signer: sign binaries & installers
Signer-->>CI: signing complete
CI->>AfterSign: run afterSign/notarize
AfterSign->>AfterSign: log "fuses already applied"
AfterSign-->>CI: afterSign complete
CI->>PostVerify: verify installer signatures (*.exe, *.msi)
PostVerify-->>CI: final verification result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
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-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
build/notarize.js (1)
21-45: Minor issue: Timer cleanup uses wrong method.The timer uses
setIntervalbut cleanup callsclearTimeout. While this works in Node.js (both methods can clear either type), it's semantically incorrect and could cause confusion.Suggested fix
- clearTimeout(timer); + clearInterval(timer); console.log(); resolve(); }) .catch((error) => { - clearTimeout(timer); + clearInterval(timer); console.log(); reject(error); });build/afterPack.js (1)
32-42: This is new code being added in this PR; verifyEnableCookieEncryption: falseis intentional.
EnableCookieEncryptionis explicitly set tofalsein this new fuse configuration. Note that enabling cookie encryption is a one-way transition—once enabled, existing plaintext cookies are encrypted on-write, and the setting cannot be safely disabled afterwards without leaving the cookie store unusable. Confirm that keeping itfalseis the intended choice for your deployment model.
🤖 Fix all issues with AI agents
In @workspaces/desktop-release-action/src/windows/verify-signature.ts:
- Around line 19-31: The PowerShell command construction embeds filePath without
escaping backslashes, so update the code that builds the script to first escape
backslashes and then single quotes (e.g., create a safePath via
filePath.replace(/\\/g, '\\\\').replace(/'/g, "''")), use safePath when
interpolating into the script string passed to runAndBuffer, and keep the
existing escaping for double quotes/newlines when building the final powershell
-Command invocation; adjust references to filePath in the script construction to
use this new safePath variable (affecting the script constant and the
runAndBuffer call that uses script).
🧹 Nitpick comments (1)
workspaces/desktop-release-action/src/windows/verify-signature.ts (1)
72-79: Consider usingfs.existsSyncinstead ofglob.syncfor exact paths.Using
glob.sync(exePath)on an exact file path (no wildcards) works but is less efficient than a simple file existence check. This is a minor optimization opportunity.Suggested improvement
+import * as fs from 'fs'; // ... for (const dir of unpackedDirs) { const exePath = path.join(distPath, dir, 'Rocket.Chat.exe'); - const files = glob.sync(exePath); - - if (files.length === 0) { + if (!fs.existsSync(exePath)) { core.debug(`No executable found in ${dir}`); continue; } - for (const file of files) { - core.info(`Verifying: ${path.relative(distPath, file)}`); - const result = await verifySignature(file); + core.info(`Verifying: ${path.relative(distPath, exePath)}`); + const result = await verifySignature(exePath);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
workspaces/desktop-release-action/dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (4)
build/afterPack.jsbuild/notarize.jsworkspaces/desktop-release-action/src/windows/index.tsworkspaces/desktop-release-action/src/windows/verify-signature.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-09T14:37:09.225Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:37:09.225Z
Learning: Applies to {rollup.config.mjs,electron-builder.json,package.json,**/*.build.{ts,js}} : When modifying Windows build commands, ensure all architectures are included: x64, ia32, and arm64 with electron-builder
Applied to files:
build/notarize.jsbuild/afterPack.jsworkspaces/desktop-release-action/src/windows/index.ts
📚 Learning: 2026-01-09T14:37:09.225Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:37:09.225Z
Learning: Windows code signing uses Google Cloud KMS in two phases: build packages without signing first, then sign with jsign using Google Cloud KMS to prevent MSI build failures from KMS CNG provider installation conflicts
Applied to files:
workspaces/desktop-release-action/src/windows/index.ts
🧬 Code graph analysis (2)
workspaces/desktop-release-action/src/windows/index.ts (3)
workspaces/desktop-release-action/src/shell.ts (1)
runElectronBuilder(66-69)workspaces/desktop-release-action/src/windows/verify-signature.ts (2)
verifyExecutableSignature(58-111)verifyInstallerSignatures(113-163)workspaces/desktop-release-action/src/windows/update-yaml-checksums.ts (1)
updateYamlChecksums(122-124)
workspaces/desktop-release-action/src/windows/verify-signature.ts (1)
workspaces/desktop-release-action/src/shell.ts (1)
runAndBuffer(41-64)
🪛 GitHub Check: CodeQL
workspaces/desktop-release-action/src/windows/verify-signature.ts
[failure] 29-29: Incomplete string escaping or encoding
This does not escape backslash characters in the input.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (macos-latest)
- GitHub Check: build (ubuntu-latest, linux)
- GitHub Check: build (windows-latest, windows)
- GitHub Check: build (macos-latest, mac)
🔇 Additional comments (7)
build/notarize.js (1)
6-9: LGTM!The Windows platform handling now correctly logs that fuses are applied in afterPack and returns early. This aligns with the PR objective of applying fuses before signing to preserve signature validity.
build/afterPack.js (1)
25-44: LGTM - Core fix for the signing issue.This correctly moves fuse application to afterPack (before signing), which is the proper timing according to Electron documentation. The comment accurately explains why this ordering matters for signature validity.
workspaces/desktop-release-action/src/windows/index.ts (2)
79-92: LGTM - Verification flow is well-integrated.The signature verification steps are correctly positioned:
verifyExecutableSignatureruns after electron-builder signs the executablesverifyInstallerSignaturesruns after the installers are signedThis ensures builds fail early if any signature is invalid, matching the PR objectives.
48-55: Good documentation of the build flow.The comment on line 49 accurately describes the new ordering: fuses applied in afterPack, then signing via winSignKms.js. This aligns with the PR's fix for preserving signature validity.
workspaces/desktop-release-action/src/windows/verify-signature.ts (3)
6-12: LGTM - Clean interface definition.The
SignatureResultinterface is well-structured with appropriate optional fields for error handling.
113-163: LGTM - Installer verification logic is correct.The function properly:
- Searches for both
.exeand.msiinstallers- Excludes unpacked directories to avoid re-verifying the already-checked executables
- Uses consistent error handling and reporting patterns
52-56: Good abstraction for future extensibility.The
verifySignaturewrapper allows for potential alternative verification methods in the future while keeping the public API stable.
Linux installer download |
Windows installer download |
macOS installer download |
…testing - Fix backslash escaping in PowerShell command (CodeQL) - Fix clearTimeout -> clearInterval for setInterval timer - Use fs.existsSync instead of glob.sync for exact paths - Temporarily enable build-release workflow for PR testing
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @workspaces/desktop-release-action/src/windows/verify-signature.ts:
- Around line 20-32: The current two-stage escaping of filePath into the
PowerShell script (see safePath and script) and then into the shell command
passed to runAndBuffer is fragile and misses characters like $ and backtick;
replace this with PowerShell's -EncodedCommand approach: build the script string
(including the Get-AuthenticodeSignature block and use of filePath), UTF-16LE
encode it and Base64-encode that payload, then invoke powershell -NoProfile
-EncodedCommand "<base64>" so you avoid manual escaping entirely and pass the
encoded command into runAndBuffer instead of embedding script text or doing
multiple replace calls.
🧹 Nitpick comments (1)
workspaces/desktop-release-action/src/windows/verify-signature.ts (1)
54-58: Consider removing this thin wrapper.
verifySignatureis a one-liner that delegates toverifyWithPowerShell. If no other verification backends are planned, consider inlining or documenting why the abstraction exists.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
workspaces/desktop-release-action/dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (3)
.github/workflows/build-release.ymlbuild/notarize.jsworkspaces/desktop-release-action/src/windows/verify-signature.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:37:09.225Z
Learning: Windows code signing uses Google Cloud KMS in two phases: build packages without signing first, then sign with jsign using Google Cloud KMS to prevent MSI build failures from KMS CNG provider installation conflicts
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:37:09.225Z
Learning: Applies to {rollup.config.mjs,electron-builder.json,package.json,**/*.build.{ts,js}} : When modifying Windows build commands, ensure all architectures are included: x64, ia32, and arm64 with electron-builder
📚 Learning: 2026-01-09T14:37:09.225Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:37:09.225Z
Learning: Applies to {rollup.config.mjs,electron-builder.json,package.json,**/*.build.{ts,js}} : When modifying Windows build commands, ensure all architectures are included: x64, ia32, and arm64 with electron-builder
Applied to files:
build/notarize.js
🧬 Code graph analysis (1)
workspaces/desktop-release-action/src/windows/verify-signature.ts (1)
workspaces/desktop-release-action/src/shell.ts (1)
runAndBuffer(41-64)
🪛 GitHub Check: CodeQL
workspaces/desktop-release-action/src/windows/verify-signature.ts
[failure] 31-31: Incomplete string escaping or encoding
This does not escape backslash characters in the input.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build (windows-latest, windows)
- GitHub Check: build (ubuntu-latest, linux)
- GitHub Check: build (macos-latest, mac)
- GitHub Check: check (macos-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (6)
.github/workflows/build-release.yml (1)
12-15: Reminder: Remove this temporary trigger after PR merge.The comment is clear, but consider adding a GitHub issue or task to track removal, as inline comments can be overlooked. This trigger will run full builds (including signing) on all PRs to master, which is resource-intensive if left in place.
build/notarize.js (2)
6-9: LGTM!The Windows branch now correctly documents that fuses are applied in afterPack, maintaining a clear audit trail of the build flow changes.
35-44: Good fix:clearIntervalcorrectly matchessetInterval.The timer on line 22 is created with
setInterval, so usingclearIntervalis the correct fix.workspaces/desktop-release-action/src/windows/verify-signature.ts (3)
7-13: LGTM!Clean interface definition with appropriate optional fields for error cases.
60-108: LGTM!The executable verification logic correctly:
- Covers all three Windows architectures (x64, ia32, arm64) per project conventions
- Uses
fs.existsSyncfor exact path checks (per commit message)- Provides clear logging with pass/fail indicators
- Throws on any signature failure while allowing missing architectures
110-160: LGTM!The installer verification correctly excludes unpacked directories and handles both
.exeand.msiinstallers. The glob patterns and ignore list are well-structured.
workspaces/desktop-release-action/src/windows/verify-signature.ts
Outdated
Show resolved
Hide resolved
…approach Based on electron-builder's windowsExecutableCodeSignatureVerifier.ts: - Use -LiteralPath instead of -FilePath (handles paths literally) - Only escape single quotes (double them) - Use chcp 65001 for UTF-8 encoding - Use -NonInteractive -InputFormat None flags - Check Status === 0 (JSON serialized enum) or 'Valid' for compatibility Reference: electron-builder/packages/electron-updater/src/windowsExecutableCodeSignatureVerifier.ts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
workspaces/desktop-release-action/src/windows/verify-signature.ts (1)
20-23: Consider escaping additional special characters in file paths.The current escaping only handles single quotes. If a file path contains double quotes or backticks, it could break the outer PowerShell command string. While build output filenames are controlled and this is unlikely to occur, a more defensive approach would escape these characters as well.
♻️ Optional: More defensive path escaping
- const escapedPath = filePath.replace(/'/g, "''"); - const psCommand = `"Get-AuthenticodeSignature -LiteralPath '${escapedPath}' | ConvertTo-Json -Compress"`; + const escapedPath = filePath + .replace(/'/g, "''") + .replace(/`/g, '``') + .replace(/"/g, '`"'); + const psCommand = `"Get-AuthenticodeSignature -LiteralPath '${escapedPath}' | ConvertTo-Json -Compress"`;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
workspaces/desktop-release-action/dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (1)
workspaces/desktop-release-action/src/windows/verify-signature.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:37:09.225Z
Learning: Windows code signing uses Google Cloud KMS in two phases: build packages without signing first, then sign with jsign using Google Cloud KMS to prevent MSI build failures from KMS CNG provider installation conflicts
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:37:09.225Z
Learning: Applies to {rollup.config.mjs,electron-builder.json,package.json,**/*.build.{ts,js}} : When modifying Windows build commands, ensure all architectures are included: x64, ia32, and arm64 with electron-builder
📚 Learning: 2026-01-09T14:37:09.225Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T14:37:09.225Z
Learning: Applies to src/main/**/*.{ts,js} : Use defensive coding with optional chaining for Linux-only APIs (e.g., `process.getuid?.() ?? 1000` for process.getuid(), `process.getgid?.() ?? 1000` for process.getgid()) to maintain cross-platform test compatibility instead of relying on mocks
Applied to files:
workspaces/desktop-release-action/src/windows/verify-signature.ts
🧬 Code graph analysis (1)
workspaces/desktop-release-action/src/windows/verify-signature.ts (1)
workspaces/desktop-release-action/src/shell.ts (1)
runAndBuffer(41-64)
🔇 Additional comments (7)
workspaces/desktop-release-action/src/windows/verify-signature.ts (7)
1-5: LGTM!Imports are appropriate for the module's functionality. The namespace import for
globis compatible with v11.
7-13: LGTM!Clean interface design with appropriate required and optional fields for representing signature verification results.
50-54: LGTM!Clean abstraction layer that enables future extensibility for different verification methods.
92-95: Verify intended behavior when no executables are found.The AI summary states this function "throws on any invalid signature or on no executables found," but the implementation only logs a warning and returns. This could be intentional (e.g., when building for a subset of architectures), but please confirm this is the expected behavior.
If missing executables should fail the build, consider throwing an error instead:
💡 Alternative: Throw on no executables
if (results.length === 0) { - core.warning('No executables found to verify'); - return; + throw new Error('No executables found to verify - check build output'); }
61-65: Good architecture coverage.The unpacked directory list correctly includes all three Windows architectures (x64, ia32, arm64), which aligns with the project's build requirements.
144-147: Same observation: verify intended behavior when no installers found.Similar to
verifyExecutableSignature, this warns and returns when no installers are found, though the AI summary indicates it should throw. Confirm this is intentional for consistency.
115-124: LGTM!Good use of glob with
ignorepatterns to exclude unpacked directories, preventing duplicate verification of executables that are verified separately byverifyExecutableSignature.
jeanfbrito
changed the title
Summary
Fixes #3174 - Windows executable showing "signed but invalid"
https://rocketchat.atlassian.net/browse/CORE-1669
Problem
The Windows executable (
Rocket.Chat.exe) was being signed, but then Electron fuses were applied after signing in theafterSignhook. This modified the binary and invalidated the signature.Root Cause
The previous code had incorrect comments stating fuses should be applied after signing. Per Electron's official documentation:
Solution
afterSigntoafterPack- Fuses are now applied before electron-builder signs the executableChanges
build/afterPack.js- Remove Windows skip logic, apply fuses for all platformsbuild/notarize.js- Remove Windows fuse code, keep only macOS notarizationverify-signature.ts- New file to verify signatures using PowerShellGet-AuthenticodeSignatureindex.ts- Integrate verification into build pipelineNew Build Flow
Testing
The CI will run the full Windows build with signing to verify the fix works correctly.
Summary by CodeRabbit
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.