[Bounty #632] Code sign the Windows releases#1304
Conversation
Greptile SummaryThis PR adds a new
Confidence Score: 2/5Not safe to merge — the script would not sign any files in practice due to multiple correctness bugs and has no connection to the actual build pipeline. Three independent defects each prevent the feature from working: the osslsigncode path writes a signed copy to a new path and returns success without ever replacing the original, signtool is unconditionally skipped on Windows because its help flag exits non-zero, and the script is never invoked from the build workflow. Even if all three were fixed, the certificate password is exposed in the process argument list. The script would ship unsigned binaries while reporting success. aw_windows_codesign.py requires significant rework; .github/workflows/build.yml will also need a Windows signing step before this feature is functional.
|
| Filename | Overview |
|---|---|
| aw_windows_codesign.py | New Windows code-signing helper class with several correctness issues: osslsigncode output file is never swapped back to the original path (signed binary is silently discarded), signtool detection breaks because --help exits non-zero on Windows, the certificate password is exposed in the process argument list, and the script has no integration with the existing CI/CD build workflow. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[WindowsCodeSigner init] --> B[_detect_tool]
B --> C{tool == auto?}
C -- No --> D[Use specified tool]
C -- Yes --> E["Try: signtool --help"]
E -- "CalledProcessError or FileNotFoundError" --> F["Try: osslsigncode --help"]
F -- "CalledProcessError or FileNotFoundError" --> G["tool = none"]
E -- "exit 0" --> H["tool = signtool"]
F -- "exit 0" --> I["tool = osslsigncode"]
J["sign_file(filepath)"] --> K{tool configured and cert present?}
K -- No --> L[return False]
K -- Yes --> M{Which tool?}
M -- signtool --> N["cmd: signtool sign /f cert /t ts filepath"]
M -- osslsigncode --> O["cmd: osslsigncode sign -in filepath -out filepath.signed"]
N --> P["subprocess.run check=True"]
O --> P
P -- success --> Q["log success, return True"]
P -- CalledProcessError --> R["log error, return False"]
Q --> BUG1["BUG: filepath.signed never replaces original"]
E --> BUG2["BUG: signtool --help exits non-zero on Windows"]
Reviews (1): Last reviewed commit: "Add aw_windows_codesign.py for bounty #6..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| elif self.tool == "osslsigncode": | ||
| output = filepath + ".signed" | ||
| cmd = ["osslsigncode", "sign", "-pkcs12", self.certificate_path, "-t", self.timestamp_server, "-in", filepath, "-out", output] | ||
| if self.certificate_password: | ||
| cmd.extend(["-pass", self.certificate_password]) | ||
| else: | ||
| return False | ||
| try: | ||
| subprocess.run(cmd, capture_output=True, text=True, check=True) | ||
| logger.info(f"Successfully signed: {filepath}") | ||
| return True | ||
| except subprocess.CalledProcessError as e: | ||
| logger.error(f"Signing failed: {e.stderr}") | ||
| return False |
There was a problem hiding this comment.
osslsigncode signed file is never swapped back to the original path
After osslsigncode runs, the signed binary is written to filepath + ".signed" but the original unsigned file at filepath is never replaced. The function logs success and returns True, so callers believe the file is signed while the original unsigned binary remains in place. The .signed artifact is also silently leaked on disk. Path(output).replace(filepath) (or os.replace) must be called after the subprocess succeeds to atomically swap the files.
| for t in ["signtool", "osslsigncode"]: | ||
| try: | ||
| subprocess.run([t, "--help"], capture_output=True, check=True) | ||
| return t | ||
| except (subprocess.CalledProcessError, FileNotFoundError): | ||
| continue | ||
| logger.warning("No code signing tool found") | ||
| return "none" |
There was a problem hiding this comment.
signtool tool detection fails when signtool --help exits non-zero
signtool on Windows does not recognise Unix-style --help; it exits with a non-zero code, which raises CalledProcessError and is silently caught. This means signtool is skipped and the signer falls back to osslsigncode (or "none") even on a Windows runner where signtool is installed and should be the preferred tool. The check should not use check=True; instead, catch only FileNotFoundError (tool not installed) and treat any other return code as proof that the binary exists.
| if self.certificate_password: | ||
| cmd.extend(["/p", self.certificate_password]) | ||
| elif self.tool == "osslsigncode": | ||
| output = filepath + ".signed" | ||
| cmd = ["osslsigncode", "sign", "-pkcs12", self.certificate_path, "-t", self.timestamp_server, "-in", filepath, "-out", output] | ||
| if self.certificate_password: | ||
| cmd.extend(["-pass", self.certificate_password]) |
There was a problem hiding this comment.
The certificate password is placed directly in the subprocess argument list (/p password / -pass password). On any multi-user system and in most CI environments, the full process command line is visible to other processes via /proc or the OS process list. GitHub Actions also prints command lines in debug mode. For signtool, consider writing the password to a temporary file and using /p with an env-var indirection, or passing it via stdin. For osslsigncode, the -pass env:VAR syntax reads the secret from an environment variable rather than the command line.
| self, | ||
| certificate_path: Optional[str] = None, | ||
| certificate_password: Optional[str] = None, | ||
| timestamp_server: str = "http://timestamp.digicert.com", |
There was a problem hiding this comment.
| """Windows code signing support for ActivityWatch releases. | ||
|
|
||
| Addresses issue #632: Code sign the Windows releases | ||
| """ | ||
|
|
||
| import os | ||
| import subprocess | ||
| import logging | ||
| from pathlib import Path | ||
| from typing import Optional |
There was a problem hiding this comment.
The new WindowsCodeSigner class has no call site in the existing build workflow. The build.yml Windows job installs Inno Setup and packages the release, but never imports or invokes this module. The macOS equivalent (codesign / xcnotary) is called inline in build.yml under the "Package dmg" step. Without a corresponding "Package Windows" step that calls sign_directory, this script will never actually sign any release artefact.
1 participant
Summary
Addresses #632: Code sign the Windows releases
Changes
Testing
Closes #632