fix(msh): SuspiciousScripts check 5 was failing for every npm project (StrictMode + empty collection)#5
Open
mbfromit wants to merge 1 commit into
Open
Conversation
…ions
Symptom: every Node.js project scan emitted a WARN line:
"SuspiciousScripts check failed for <project>:
The property 'Name' cannot be found on this object."
Users mistook these for findings and were generating noise calls.
Root cause: under StrictMode Latest, accessing `.Name` (or `.Count`) on
an EMPTY PSMemberInfoCollection throws. The `$obj.PSObject.Properties.Name
-contains 'foo'` idiom relies on auto-member-expansion which strict-mode
refuses when the collection has zero items. Many published npm packages
ship `"scripts": {}` in their package.json (declared, no hooks) — the
common case, not an edge case — which made $scripts.PSObject.Properties
empty and threw at line 94 of Find-MshSuspiciousScripts. The whole
project's Check 5 was killed for every engineer running the scanner.
Fix: switch the three `.PSObject.Properties.Name -contains 'X'` accesses
in Find-MshSuspiciousScripts to the indexer pattern
`$obj.PSObject.Properties['X']` which returns either the property or
$null, regardless of whether the collection is empty.
Regression coverage: added two Pester cases — `{"scripts": {}}` (the
real-world trigger) and `{}` (sibling edge case). All 15 tests in
the suite pass with the fix.
Latent same-pattern bugs in Find-MshBadPackages.ps1 (lines 286, 334,
335) operate on package.json `dependencies` / `devDependencies`
sections that can also be `{}` in the wild. Not changed in this commit
per smallest-safe-change discipline — flagged for follow-up.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the noisy WARN-spam everyone running the scanner has been seeing:
One line per Node.js project. Users were mistaking these for findings and generating "did I break it?" calls.
Root cause
Under
Set-StrictMode -Version Latest, accessing.Name(or.Count) on an emptyPSMemberInfoCollectionthrows. The repo-wide idiom$obj.PSObject.Properties.Name -contains 'foo'relies on auto-member-expansion which strict-mode refuses when the collection is empty.Many published npm packages ship
"scripts": {}in theirpackage.json(declared, no hooks) — common, not edge. This made$scripts.PSObject.Propertiesempty and threw at line 94 ofFind-MshSuspiciousScripts. The catch in the caller suppressed the crash but the WARN line reached the console for every affected project.Confirmed via local repro:
Fix
Switched the three
.PSObject.Properties.Name -contains 'X'accesses inFind-MshSuspiciousScripts.ps1(lines 85, 94, 108) to the indexer pattern$obj.PSObject.Properties['X']. The indexer returns either the property or\$null, regardless of whether the collection is empty.Tests
Added two Pester regression cases:
{"scripts": {}}— the actual real-world trigger{}— sibling empty-PSCustomObject edge caseAll 15 tests in
Find-MshSuspiciousScripts.Tests.ps1pass (12 existing + 2 new + 1 mixed). Verified locally.Latent same-pattern bugs (NOT fixed in this PR)
Private/MiniShaiHulud/Find-MshBadPackages.ps1uses the same broken idiom at lines 286, 334, 335 againstpackage.jsondependencies/devDependenciessections — which can also legally be{}. Same root cause; not changed here per smallest-safe-change discipline. Owe a follow-up PR to give it the same indexer treatment.Other usages across the codebase (
Invoke-MshNpmAudit,Invoke-MshNpmCacheScan,Get-MshIocs,New-MshScanReport,New-MshExecBriefing,Invoke-MiniShaiHulud.ps1finding-array filters) operate on objects that are guaranteed to have properties by their schemas — low risk, but worth sweeping defensively in the same follow-up.Test plan
.\Invoke-MiniShaiHulud.ps1and confirms zero "SuspiciousScripts check failed" WARN lines for clean projects🤖 Generated with Claude Code