Harden Microsoft Defender scan stage to prevent silent gh-aw.exe scan skips#35494
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ailure Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #35494 does not have the 'implementation' label and has 0 new lines of code in default business logic directories (≤100 threshold not exceeded). |
|
🧪 Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
Updates CI workflows to prevent Windows Microsoft Defender scans from silently passing when scanning is skipped, and adds an extra fetch step for the cross-repo smoke workflow to bring in additional refs.
Changes:
- Capture
MpCmdRun.exeoutput/exit code, fail the job if Defender reports the file “was skipped”, and print full scan output for debugging. - Emit Microsoft Defender status/signature/version diagnostics before scanning.
- Fetch additional
main+ PR head refs in the cross-repo smoke update workflow.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/smoke-update-cross-repo-pr.lock.yml | Adds an authenticated git fetch step to pull in additional refs for the side repo workflow execution. |
| .github/workflows/cgo.yml | Hardens the Windows Defender scanning step by logging diagnostics, surfacing full MpCmdRun output, and failing on “scan skipped” despite exit code 0. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 6
| env: | ||
| GH_AW_FETCH_TOKEN: ${{ secrets.GH_AW_SIDE_REPO_PAT }} | ||
| run: | | ||
| header=$(printf "x-access-token:%s" "${GH_AW_FETCH_TOKEN}" | base64 -w 0) | ||
| git -c "http.extraheader=Authorization: Basic ${header}" fetch origin '+refs/heads/main:refs/remotes/origin/main' '+refs/pull/*/head:refs/remotes/origin/pull/*/head' |
| - name: Fetch additional refs for github/gh-aw-side-repo | ||
| if: (!cancelled()) && needs.agent.result != 'skipped' && contains(needs.agent.outputs.output_types, 'push_to_pull_request_branch') | ||
| env: | ||
| GH_AW_FETCH_TOKEN: ${{ secrets.GH_AW_SIDE_REPO_PAT }} | ||
| run: | | ||
| header=$(printf "x-access-token:%s" "${GH_AW_FETCH_TOKEN}" | base64 -w 0) | ||
| git -c "http.extraheader=Authorization: Basic ${header}" fetch origin '+refs/heads/main:refs/remotes/origin/main' '+refs/pull/*/head:refs/remotes/origin/pull/*/head' |
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.2M
| - name: Fetch additional refs for github/gh-aw-side-repo | ||
| if: (!cancelled()) && needs.agent.result != 'skipped' && contains(needs.agent.outputs.output_types, 'push_to_pull_request_branch') | ||
| env: | ||
| GH_AW_FETCH_TOKEN: ${{ secrets.GH_AW_SIDE_REPO_PAT }} |
There was a problem hiding this comment.
Derived credential value won't be masked in Actions logs: the header variable holds a base64-encoded form of the PAT that GitHub Actions never registers as a secret — only the raw GH_AW_FETCH_TOKEN value is masked.
💡 Details and suggested fix
GitHub Actions masks secrets by their literal value. base64 produces a different string, so $header is unmasked. If ACTIONS_STEP_DEBUG=true is set on this runner (e.g. for routine debugging), or if any downstream shell error prints the git config, the PAT is exposed in cleartext-equivalent form.
A safer pattern is to pass the credential directly via git -c credential.helper= or via a GITHUB_TOKEN-style env var that git reads natively, without materialising a base64 derivative in a shell variable:
# Option A: pass via git credential helper (never surfaces in shell)
git -c "credential.helper=" \
-c "(github.com.username/redacted) \
-c "(github.com.password/redacted) \
fetch origin ...
# Option B: add a ::add-mask:: annotation to hide derived value
printf '%s' "$(printf 'x-access-token:%s' "${GH_AW_FETCH_TOKEN}" | base64 -w 0)" >> $GITHUB_ENV
# then ::add-mask:: the env var before using itAlternatively, if the value is known at step start, emit echo "::add-mask::$(printf ... | base64 -w 0)" before using $header.
| # Use -imatch for case-insensitive matching since the exact casing of | ||
| # MpCmdRun output may vary across Defender versions. | ||
| $skipped = $output | Where-Object { $_ -imatch "was skipped" } | ||
| if ($skipped) { |
There was a problem hiding this comment.
Skip detection is purely negative and will silently regress if Defender changes its output: the check only looks for "was skipped" and treats its absence as a successful scan — but absence of a skip string is not the same as presence of a success string.
💡 Details and suggested fix
If a future Defender update changes the skip message wording (e.g. "scan was not performed", "file excluded", or the message disappears entirely), this guard silently stops working and CI goes back to passing on unskipped scans — the exact regression this PR exists to fix.
A more robust approach requires the output to contain a positive confirmation before declaring success:
# Require a positive success indicator as well
$succeeded = $output | Where-Object { $_ -imatch "Threat scan completed" -or $_ -imatch "No threats detected" }
if (-not $succeeded) {
Write-Error "MpCmdRun did not emit a known success message — cannot confirm scan actually ran."
Write-Error "Full output: $($output -join "`n")"
exit 1
}At minimum, document clearly in CI runbooks which exact Defender version was tested so a version upgrade triggers a manual verification.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose — no blocking issues; two suggestions to harden the fix further.
📋 Key Themes & Highlights
Key Themes
- Pattern coverage: only
"was skipped"is checked; Defender can silently bail for other reasons that would still be missed ErrorRecordcoercion:2>&1delivers stderr asErrorRecordobjects; an explicit"$_"cast would make the code unambiguous
Positive Highlights
- ✅ Root cause correctly identified and addressed —
$LASTEXITCODEalone is insufficient for Defender - ✅ Capturing
2>&1and printing all output before checking is excellent for future debuggability - ✅
Get-MpComputerStatusdiagnostics block will make flaky-scan incidents much easier to investigate - ✅
-imatchcase-insensitive matching is the right choice for cross-version output compatibility - ✅ Informative
Write-Errormessages with actionable "Possible causes" guidance
Note on lock file change
The smoke-update-cross-repo-pr.lock.yml fetch-refs addition appears unrelated to the Defender fix — worth confirming it was intentional to bundle here.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.2M
|
@copilot review all comments from copilot and github-actions |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed the actionable Copilot/GitHub Actions feedback in commits Key updates:
|
|
@copilot the binary path looks incorrect. Add a check that the binary exists before running the scanner. https://github.com/github/gh-aw/actions/runs/26587916933/job/78338824734?pr=35494 |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in |
Harden Microsoft Defender Scanning in GitHub ActionsImplement a robust Windows Defender scanning stage for GitHub Actions Windows runners. ProblemCurrent behavior: This is a failure condition.
The workflow must explicitly detect skipped scans and fail the job. RequirementsEnvironment AssumptionsThe workflow runs on:
Potential issues include:
Implementation Requirements1. Defender DiagnosticsCollect and print: Get-MpComputerStatus
Get-MpPreferenceInclude at minimum:
Also print:
2. File StabilizationBefore scanning:
Fail if the file changes during stabilization. 3. Avoid Scanning Workspace PathsDO NOT scan directly from: Instead:
or:
Example: 4. Validate Copy IntegrityAfter copying:
Fail immediately if mismatched. 5. Defender Scan ExecutionUse: MpCmdRun.exe -Scan -ScanType 3 -File <copied file> -DisableRemediationCapture:
Print all output. Failure ConditionsThe workflow MUST fail if: Output contains:or: or:
Critical ConstraintDO NOT trust: as proof that scanning occurred. The implementation MUST explicitly parse Defender output to confirm the executable was actually scanned. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
... Implemented in |
…recompile Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
* Add defender job to release.md; sync_actions now depends on defender Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Recompile workflows: regenerate release.lock.yml with defender job Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Update defender job: reuse hardened script from cgo.yml (PR #35494), recompile Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Co-authored-by: Peli de Halleux <pelikhan@users.noreply.github.com>
MpCmdRun.execan exit0even when the executable was not actually scanned, so relying on$LASTEXITCODEalone can let an unscanned binary pass CI silently.Changes
MpCmdRunoutput and fails on skip-like messages (case-insensitive), includingwas skipped,cannot be scanned,not performed, and exclusion-related outputexit code 0as insufficient unless output confirms scan execution (Scan startingandScan finished)ThreatGet-MpComputerStatus, plus additional context fromGet-MpPreference(exclusions) andWinDefendservice state; all wrapped safely so diagnostics don’t block scan executiongh-aw.exeexists as a file, waits briefly, and confirms size/timestamp stability before scanningMpCmdRun.exe -Scan -ScanType 3 -File <copied file> -DisableRemediation