Skip to content

Commit ccbdfa3

Browse files
fix(scripts): eliminate false positives in dependency pinning npm pattern (#273)
## Description Replace regex-based pattern matching with JSON parsing for package.json validation, targeting only actual dependency sections. The previous regex approach incorrectly flagged metadata fields like `name`, `version`, `description`, and `repository` URLs as unpinned dependencies. - Add `Get-NpmDependencyViolations` function that parses package.json as structured JSON and checks only `dependencies`, `devDependencies`, `peerDependencies`, and `optionalDependencies` sections - Update npm pattern config to use `ValidationFunc` dispatcher instead of `VersionPatterns` regex approach - Add ValidationFunc dispatch logic to `Get-DependencyViolation` for type-specific validation functions - Update `Test-ShellDownloadSecurity` to use consistent `FileInfo` hashtable parameter - Create test fixtures for metadata-only and with-dependencies npm scenarios - Add Pester tests covering JSON parsing, dependency extraction, and version pattern matching ## Related Issue(s) Fixes #267 ## Type of Change Select all that apply: **Code & Documentation:** - [x] Bug fix (non-breaking change fixing an issue) - [ ] New feature (non-breaking change adding functionality) - [ ] Breaking change (fix or feature causing existing functionality to change) - [ ] Documentation update **Infrastructure & Configuration:** - [ ] GitHub Actions workflow - [ ] Linting configuration (markdown, PowerShell, etc.) - [ ] Security configuration - [ ] DevContainer configuration - [ ] Dependency update **AI Artifacts:** - [ ] Reviewed contribution with `prompt-builder` agent and addressed all feedback - [ ] Copilot instructions (`.github/instructions/*.instructions.md`) - [ ] Copilot prompt (`.github/prompts/*.prompt.md`) - [ ] Copilot agent (`.github/agents/*.agent.md`) > **Note for AI Artifact Contributors**: > > - **Agents**: Research, indexing/referencing other project (using standard VS Code GitHub Copilot/MCP tools), planning, and general implementation agents likely already exist. Review `.github/agents/` before creating new ones. > - **Model Versions**: Only contributions targeting the **latest Anthropic and OpenAI models** will be accepted. Older model versions (e.g., GPT-3.5, Claude 3) will be rejected. > - See [Agents Not Accepted](../docs/contributing/custom-agents.md#agents-not-accepted) and [Model Version Requirements](../docs/contributing/ai-artifacts-common.md#model-version-requirements). **Other:** - [x] Script/automation (`.ps1`, `.sh`, `.py`) - [ ] Other (please describe): ## Sample Prompts (for AI Artifact Contributions) <!-- Not applicable - no AI artifacts in this PR --> ## Testing - PSScriptAnalyzer: All files passed with 0 issues - Pester: 33 tests passed (including 4 new tests for `Get-NpmDependencyViolations`) - Full dependency scan: 100% compliance, 0 violations ## Checklist ### Required Checks - [x] Documentation is updated (if applicable) - [x] Files follow existing naming conventions - [x] Changes are backwards compatible (if applicable) - [x] Tests added for new functionality (if applicable) ### AI Artifact Contributions <!-- Not applicable - no AI artifacts in this PR --> ### Required Automated Checks The following validation commands must pass before merging: - [ ] Markdown linting: `npm run lint:md` - [ ] Spell checking: `npm run spell-check` - [ ] Frontmatter validation: `npm run lint:frontmatter` - [ ] Link validation: `npm run lint:md-links` - [x] PowerShell analysis: `npm run lint:ps` ## Security Considerations - [x] This PR does not contain any sensitive or NDA information - [x] Any new dependencies have been reviewed for security issues - [x] Security-related scripts follow the principle of least privilege ## Additional Notes The `ValidationFunc` dispatcher pattern enables type-specific validation functions without relying on regex patterns. This approach provides better accuracy for structured file formats like JSON and can be extended to other dependency types in the future. 🔧 - Generated by Copilot
1 parent c52d6e2 commit ccbdfa3

File tree

6 files changed

+352
-23
lines changed

6 files changed

+352
-23
lines changed

scripts/security/Test-DependencyPinning.ps1

Lines changed: 127 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -131,16 +131,10 @@ $DependencyPatterns = @{
131131
}
132132

133133
'npm' = @{
134-
FilePatterns = @('**/package.json', '**/package-lock.json')
135-
VersionPatterns = @(
136-
@{
137-
Pattern = '"([^"]+)":\s*"([^@][^"]*)"'
138-
Groups = @{ Package = 1; Version = 2 }
139-
Description = 'NPM dependencies in package.json'
140-
}
141-
)
142-
SHAPattern = '^[a-fA-F0-9]{40}$'
143-
RemediationUrl = 'https://registry.npmjs.org/{0}/{1}'
134+
FilePatterns = @('**/package.json')
135+
ValidationFunc = 'Get-NpmDependencyViolations'
136+
SHAPattern = '^[a-fA-F0-9]{40}$'
137+
RemediationUrl = 'https://registry.npmjs.org/{0}/{1}'
144138
}
145139

146140
'pip' = @{
@@ -211,15 +205,17 @@ function Test-ShellDownloadSecurity {
211205
have corresponding checksum verification (sha256sum/shasum) within the
212206
following lines.
213207
214-
.PARAMETER FilePath
215-
Path to the shell script file to scan.
208+
.PARAMETER FileInfo
209+
Hashtable with Path, Type, and RelativePath keys from Get-FilesToScan.
216210
#>
217211
[CmdletBinding()]
218212
param(
219213
[Parameter(Mandatory)]
220-
[string]$FilePath
214+
[hashtable]$FileInfo
221215
)
222216

217+
$FilePath = $FileInfo.Path
218+
223219
if (-not (Test-Path $FilePath)) {
224220
return @()
225221
}
@@ -246,14 +242,89 @@ function Test-ShellDownloadSecurity {
246242
}
247243

248244
if (-not $hasChecksum) {
249-
$violations += [PSCustomObject]@{
250-
File = $FilePath
251-
Line = $i + 1
252-
Pattern = $line.Trim()
253-
Issue = 'Download without checksum verification'
254-
Severity = 'warning'
255-
Ecosystem = 'shell-downloads'
256-
}
245+
$violation = [DependencyViolation]::new()
246+
$violation.File = $FileInfo.RelativePath
247+
$violation.Line = $i + 1
248+
$violation.Type = $FileInfo.Type
249+
$violation.Name = $line.Trim()
250+
$violation.Severity = 'warning'
251+
$violation.Description = 'Download without checksum verification'
252+
$violation.Metadata = @{ Pattern = $line.Trim() }
253+
$violations += $violation
254+
}
255+
}
256+
}
257+
258+
return $violations
259+
}
260+
261+
function Get-NpmDependencyViolations {
262+
<#
263+
.SYNOPSIS
264+
Analyzes package.json files for unpinned npm dependencies.
265+
.DESCRIPTION
266+
Parses package.json as JSON and checks only actual dependency sections
267+
(dependencies, devDependencies, peerDependencies, optionalDependencies)
268+
for SHA-pinned versions. Ignores metadata fields like name, version,
269+
description, contributes, scripts, repository, etc.
270+
.PARAMETER FileInfo
271+
Hashtable with Path, Type, and RelativePath keys from Get-FilesToScan.
272+
.OUTPUTS
273+
Array of PSCustomObjects representing dependency violations.
274+
#>
275+
[CmdletBinding()]
276+
param(
277+
[Parameter(Mandatory)]
278+
[hashtable]$FileInfo
279+
)
280+
281+
$filePath = $FileInfo.Path
282+
$relativePath = $FileInfo.RelativePath
283+
$type = $FileInfo.Type
284+
$violations = @()
285+
286+
if (-not (Test-Path -Path $filePath -PathType Leaf)) {
287+
return $violations
288+
}
289+
290+
try {
291+
$content = Get-Content -Path $filePath -Raw -ErrorAction Stop
292+
$packageJson = $content | ConvertFrom-Json -ErrorAction Stop
293+
}
294+
catch {
295+
Write-Warning "Failed to parse $relativePath as JSON: $_"
296+
return $violations
297+
}
298+
299+
$dependencySections = @('dependencies', 'devDependencies', 'peerDependencies', 'optionalDependencies')
300+
301+
foreach ($section in $dependencySections) {
302+
$deps = $packageJson.$section
303+
if ($null -eq $deps) {
304+
continue
305+
}
306+
307+
foreach ($prop in $deps.PSObject.Properties) {
308+
$packageName = $prop.Name
309+
$version = $prop.Value
310+
311+
if ([string]::IsNullOrWhiteSpace($version)) {
312+
continue
313+
}
314+
315+
$isPinned = Test-SHAPinning -Version $version -Type $type
316+
317+
if (-not $isPinned) {
318+
$violation = [DependencyViolation]::new()
319+
$violation.File = $relativePath
320+
$violation.Line = 0
321+
$violation.Type = $type
322+
$violation.Name = $packageName
323+
$violation.Version = $version
324+
$violation.Severity = 'warning'
325+
$violation.Description = "Unpinned npm dependency in $section"
326+
$violation.Metadata = @{ Section = $section }
327+
$violations += $violation
257328
}
258329
}
259330
}
@@ -365,6 +436,41 @@ function Get-DependencyViolation {
365436
return $violations
366437
}
367438

439+
# Check if this type uses a validation function instead of regex patterns
440+
if ($null -ne $DependencyPatterns[$fileType].ValidationFunc) {
441+
$funcName = $DependencyPatterns[$fileType].ValidationFunc
442+
$rawViolations = & $funcName -FileInfo $FileInfo
443+
444+
if ($null -eq $rawViolations) {
445+
return @()
446+
}
447+
448+
foreach ($v in $rawViolations) {
449+
if ($null -eq $v) {
450+
continue
451+
}
452+
453+
if (-not ($v -is [DependencyViolation])) {
454+
$actualType = $v.GetType().FullName
455+
throw "Validation function '$funcName' must return [DependencyViolation] objects, got '$actualType'."
456+
}
457+
458+
if (-not $v.File) {
459+
$v.File = $FileInfo.RelativePath
460+
}
461+
462+
if ($v.Line -lt 1) {
463+
$v.Line = 0
464+
}
465+
466+
if (-not $v.Type) {
467+
$v.Type = $fileType
468+
}
469+
}
470+
471+
return $rawViolations
472+
}
473+
368474
try {
369475
$content = Get-Content -Path $filePath -Raw
370476
$lines = Get-Content -Path $filePath
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
"name": "empty-version-test",
3+
"version": "1.0.0",
4+
"dependencies": {
5+
"valid-package": "^1.0.0",
6+
"empty-version": "",
7+
"whitespace-version": " "
8+
}
9+
}
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"name": "invalid-json-test"
3+
"version": "1.0.0"
4+
}
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
{
2+
"name": "test-package",
3+
"version": "1.0.0",
4+
"description": "Test package with only metadata fields",
5+
"author": "Test Author",
6+
"license": "MIT",
7+
"repository": {
8+
"type": "git",
9+
"url": "https://github.com/test/test-package.git"
10+
},
11+
"bugs": {
12+
"url": "https://github.com/test/test-package/issues"
13+
},
14+
"homepage": "https://github.com/test/test-package#readme",
15+
"keywords": [
16+
"test",
17+
"fixture"
18+
],
19+
"scripts": {
20+
"build": "echo 'build'",
21+
"test": "echo 'test'"
22+
},
23+
"main": "index.js",
24+
"engines": {
25+
"node": ">=18.0.0"
26+
},
27+
"contributes": {
28+
"commands": [
29+
{
30+
"command": "test.command",
31+
"title": "Test Command"
32+
}
33+
]
34+
}
35+
}
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
{
2+
"name": "test-package-with-deps",
3+
"version": "2.0.0",
4+
"description": "Test package with actual dependencies",
5+
"dependencies": {
6+
"lodash": "^4.17.21",
7+
"express": "~4.18.2"
8+
},
9+
"devDependencies": {
10+
"jest": "29.7.0",
11+
"typescript": "*"
12+
},
13+
"peerDependencies": {
14+
"react": ">=17.0.0"
15+
},
16+
"optionalDependencies": {
17+
"fsevents": "^2.3.3"
18+
},
19+
"scripts": {
20+
"build": "tsc",
21+
"test": "jest"
22+
}
23+
}

0 commit comments

Comments
 (0)