Add check for game capture signatures on the Build machine - #6156
sandboxcoder wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new CI script should handle its top-level async Promise deterministically, and the added Windows job likely needs the same gating as other expensive jobs to preserve the workflow’s existing skip-when-non-testable behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a Windows CI verification step to ensure the obs-studio-node Game Capture binaries shipped/consumed by the repo are Authenticode-signed and published by the expected issuer (“OBS Project, LLC”), helping detect unsigned/tampered/untrusted binaries early in the pipeline.
Changes:
- Introduces a CI script that checks Authenticode signature validity and publisher for the win-capture dependency binaries via PowerShell.
- Adds a new GitHub Actions job on
windows-2022to run the signature verification and includes it in the required results collation.
File summaries
| File | Description |
|---|---|
| scripts/ci/verify_game_signatures.ts | New TypeScript CI script that validates Authenticode signatures and expected publisher for required win-capture binaries. |
| .github/workflows/tests.yml | Adds a Windows job to execute the new signature verification script and makes it part of the workflow’s required job collation. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
BundleMonUnchanged files (4)
No change in files bundle size Final result: ✅ View report in BundleMon website ➡️ |
f3d869d to
1e771ef
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new CI script’s PowerShell stderr/CLIXML parsing can produce unhelpful raw XML “details,” reducing diagnosability when the check fails.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
d38b895 to
225dde3
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new CI job’s gating condition can skip signature verification on dependency-only PRs (e.g., yarn.lock/package.json updates), which undermines the intended protection for obs-studio-node binary changes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
8d0c9a6 to
9c847a4
Compare
There was a problem hiding this comment.
🟡 Changes recommended
CI now executes a new ts-node-based script path that diverges from existing repo scripting conventions and should be adjusted to reduce fragility.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
| - name: 'Install Dependencies' | ||
| run: yarn install --immutable 2>&1 | ||
| - name: 'Verify Game Capture Binary Signatures' | ||
| run: yarn ts-node scripts/ci/verify_game_signatures.ts | ||
| shell: bash |
There was a problem hiding this comment.
🟡 Changes recommended
The new CI job is currently misconfigured (artifact/checkout/dependency ordering), which will cause the workflow to fail or run without the required files.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new CI job is currently wired such that it can be skipped or fail due to missing build/artifact preparation and execution context (extraction/working directory/prepare gating).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Critical signature and CI execution issues, plus a dependency-detection workflow defect, remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/tests.yml:46
- Because this workflow sets
predicate-quantifier: 'every', each changed path must match every pattern in a filter. No path can match bothpackage.jsonandyarn.lock, sodepsis always false; dependency-only PRs still skipprepare-frontend-testsand this verification job. Use one alternation pattern (or separate outputs) for the two dependency files.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
* Verify game capture libraries have "OBS Project, LLC" signature * Add test to verify HEVC encoder is present
9e93ffd to
179d5c2
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Fix the workflow YAML indentation and handle grep status before assignment.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
.github/workflows/tests.yml:121
- With GitHub Actions'
bashshell,-e/pipefailmakes the assignment fail whengrepreturns 1 for no match, so the shell exits at this line before the[ -z "$output" ]branch can print the intended diagnostic. Put the assignment in anif ! ...; thencondition (or otherwise handle grep's status explicitly).
output=$(./ffmpeg.exe -hide_banner -encoders 2>/dev/null | grep hevc)
if [ -z "$output" ]; then
echo "HEVC encoder not found in ffmpeg encoders list. This may indicate that the HEVC encoder is missing or not properly configured."
exit 1
fi
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The workflow has two unresolved moderate issues and one nit affecting execution and coverage.
Review details
Suppressed comments (3)
.github/workflows/tests.yml:113
ts-nodewill load the archived roottsconfig.json, whosemoduleisesnext(tsconfig.json:10), but this invokes the CommonJSts-nodeCLI under Node. The script therefore fails on itsimportstatements before checking any binaries; forcing CommonJS also needs interop for the defaultexecaimport. Run with a CommonJS/interop compiler override or a dedicated CJS tsconfig.
run: yarn ts-node scripts/ci/verify_game_signatures.ts
.github/workflows/tests.yml:46
- Because
predicate-quantifier: everyapplies to this filter too,depsis true only when every changed path ispackage.jsonoryarn.lock. A dependency PR that also changes a README, workflow, or other excluded path makes bothdepsandsrcfalse, so the build and signature check are skipped despite a dependency change. Detect dependency paths independently (for example with a separatesome-quantifier filter) while retaining the current all-files behavior forsrc.
deps:
- 'package.json'
- 'yarn.lock'
.github/workflows/tests.yml:121
- The workflow's Bash shell runs with
-e/pipefail; whengrepfinds no HEVC encoder, this assignment returns status 1 and the shell exits before the[ -z "$output" ]branch, so the intended diagnostic is never printed. Put the assignment in anif ! ...condition (or otherwise neutralize the grep status) before checking/reporting the empty result.
output=$(./ffmpeg.exe -hide_banner -encoders 2>/dev/null | grep hevc)
if [ -z "$output" ]; then
echo "HEVC encoder not found in ffmpeg encoders list. This may indicate that the HEVC encoder is missing or not properly configured."
exit 1
fi
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
4990644 to
323aa96
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The workflow’s dependency filter and TypeScript execution configuration must be corrected before approval.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/tests.yml:46
- Because this action sets
predicate-quantifier: 'every'globally, adepspath must match bothpackage.jsonandyarn.lockpredicates at the same time. No changed file can satisfy both, sohas-dependency-changesis always false and dependency-only updates still skip preparation and signature verification. Use one brace-expanded predicate (for example'{package.json,yarn.lock}') or separate filters and OR their outputs.
.github/workflows/tests.yml:113
- This invokes the regular
ts-nodeentry point, which reads the archived repository'stsconfig.json(module: "esnext", with noesModuleInterop) even though the package is run as CommonJS. The step will fail before verification with ESM/CJS orexeca.defaulterrors. Pass CommonJS compiler options including interop (or use a dedicated CJS tsconfig), for example--compiler-options '{"module":"commonjs","esModuleInterop":true}'.
run: yarn ts-node scripts/ci/verify_game_signatures.ts
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate workflow issues and one documentation nit remain unresolved.
Review details
Suppressed comments (3)
.github/workflows/tests.yml:46
predicate-quantifier: 'every'makes each changed file satisfy every rule in a filter. A change to onlypackage.jsonmatches the first rule but notyarn.lock(and vice versa), sodepsremains false and the new verification job is skipped for the dependency-only updates this filter is intended to cover. Use a single alternation/brace rule (or otherwise preserve the existingsrcexclusions while making these two paths an OR).
deps:
- 'package.json'
- 'yarn.lock'
.github/workflows/tests.yml:121
- GitHub Actions runs this Bash step with
-e -o pipefail; whengrepfinds no HEVC encoder it exits 1, so the assignment itself terminates the step before the[ -z "$output" ]branch and its diagnostic can run. Make the pipeline an explicit conditional (or otherwise tolerate grep's no-match status) so the intended failure message is emitted.
output=$(./ffmpeg.exe -hide_banner -encoders 2>/dev/null | grep hevc)
if [ -z "$output" ]; then
echo "HEVC encoder not found in ffmpeg encoders list. This may indicate that the HEVC encoder is missing or not properly configured."
exit 1
fi
scripts/ci/verify_game_signatures.ts:13
- These values are not hashes of the binaries: line 55 computes SHA-256 over
$sig.SignerCertificate.RawData. The current wording and update instruction can lead maintainers to regenerate the allowlist for every binary update even when the signer certificate is unchanged. Describe these as SHA-256 digests of the signer certificates and say to update them on certificate rotation.
// Hashes have been computed using SHA256 for the binaries shipped with the current release of obs-studio-node.
// If you update the binaries, you must update this list with the new thumbprints.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Uh oh!
There was an error while loading. Please reload this page.