Skip to content

Commit 42f41d9

Browse files
security: Tier 2 review — pagination SSRF fix + threat model + supply-chain audit (#404)
* security: validate pagination 'next' URL origin + Tier 2 review doc Addresses a HIGH finding from the 2026-04-18 Tier 2 security review (STRIDE + OWASP API Top 10 + dependency/supply-chain audit): InvokeNetboxRequest's -All pagination loop passed the server-returned 'next' URL straight into [System.UriBuilder]::new($nextUrl) and then used the same Authorization headers to fetch it. A compromised NetBox server or a successful HTTPS MITM could return: {"next": "https://attacker.example.com/steal/?cursor=xxx"} and the client would hand over the live Bearer token in plaintext. OWASP API10:2023 — Unsafe Consumption of APIs, applied to a client. Fix adds scheme + host + port comparison against the original request URI. Mismatched origin throws a descriptive error instead of silently following. This matches the default behaviour of modern browsers and most HTTP client libraries regarding Authorization headers across cross-origin redirects. 4 new unit tests: - Different host → throws - Scheme downgrade https→http → throws - Port change → throws - Matching scheme/host/port → follows normally (regression) Review doc (docs/superpowers/reviews/2026-04-18-tier2-security-review.md) captures the HIGH finding and 5 lower-severity items documented for follow-up PRs (PSGallery runner version pinning, module code-signing, Docker digest pinning, optional throttling, SECURITY.md). Overall posture is strong: zero runtime dependencies, correct URL / HTML encoding, no dynamic code execution, hardened file upload path, and all Tier 1 auth/TLS hardening intact. * security: use Uri.GetLeftPart(Authority) for origin comparison (Gemini round 1) Gemini flagged the previous scheme/host/port triple-comparison as unreliable for a real-world edge case: [UriBuilder]::new('https', 'netbox.domain.com', 443, '/api/') → Port=443 [UriBuilder]::new('https://netbox.domain.com/api/?cursor=x') → Port=443 (default substituted) ...which is the same. But: [UriBuilder]::new('https', 'host', -1, '/api/') → Port=-1 ...would false-positive against any parsed URL that has a resolved default port. While PowerNetbox's Connect-NBAPI always yields an explicit port, the Uri.GetLeftPart(Authority) approach is strictly more robust because it: 1. Normalises both URIs to canonical 'scheme://host[:port]' form 2. Omits the default port when it matches the scheme's default 3. Reduces the check to a single string equality Verified empirically against the full truth table: - same origin, with/without explicit default port: eq=True - different host: eq=False - different port (non-default): eq=False (e.g. ':8443') - different scheme (https→http): eq=False Added a 5th regression test covering the specific edge case Gemini raised: URI constructed with explicit port 443, 'next' URL omits the port. Both normalise to 'https://netbox.domain.com' — should follow. 82/82 Helpers tests pass. * docs: sync tier 2 review doc with actual GetLeftPart(Authority) fix Gemini round 2 caught that the review doc still showed the original scheme/host/port triple-comparison snippet, while the merged code uses Uri.GetLeftPart(Authority). Updated the snippet and explanation to match the actual implementation, and bumped the test-count count from 4 to 5 (the round-1 reply added an explicit-default-port edge-case regression test). Gemini's other round-2 comment is a repeat of the round-1 finding against the pre-fix code — acknowledged but no action needed; the fix from 0d65d7a is correct. * fix: replace em-dashes with ASCII hyphens in Helpers tests PS 5.1 on Windows parses .ps1 files as Windows-1252 when no UTF-8 BOM is present, which causes non-ASCII characters (em-dash U+2014, etc.) to produce confusing "Missing closing '}' in statement block" errors at parse time. PR #404 introduced two em-dashes in test names and a comment: - Context "Pagination next-URL origin validation (... review — TM-1/IV-1)" - Inline comment "# No explicit port — UriBuilder fills ..." Ubuntu / macOS PS 7 parsed them fine, but Windows PS 5.1 CI failed the whole file's discovery. Same class of bug as v4.5.8.0 PR #398. Fixed: replaced both em-dashes with ASCII hyphens. 82/82 Helpers tests still pass locally.
1 parent ee93fc8 commit 42f41d9

3 files changed

Lines changed: 447 additions & 2 deletions

File tree

Functions/Helpers/InvokeNetboxRequest.ps1

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,24 @@ function InvokeNetboxRequest {
110110
do {
111111
$pageNum++
112112
$currentUri = if ($nextUrl) {
113-
# Use the next URL from API response
114-
[System.UriBuilder]::new($nextUrl)
113+
# Validate that the server-returned pagination URL matches the
114+
# original request's origin (scheme + host + port) before
115+
# following it. Without this check, a compromised or malicious
116+
# NetBox server could redirect pagination to an attacker-
117+
# controlled host and receive the Authorization header (Bearer
118+
# token) in plaintext.
119+
# Reference: 2026-04-18 Tier 2 security review, finding TM-1/IV-1.
120+
# Using Uri.GetLeftPart(Authority) gives a canonical
121+
# "scheme://host[:port]" that handles default-port substitution
122+
# consistently across how the URI was constructed (explicit
123+
# port vs parsed from a string).
124+
$nextBuilder = [System.UriBuilder]::new($nextUrl)
125+
$originalOrigin = $URI.Uri.GetLeftPart([System.UriPartial]::Authority)
126+
$nextOrigin = $nextBuilder.Uri.GetLeftPart([System.UriPartial]::Authority)
127+
if ($nextOrigin -ne $originalOrigin) {
128+
throw "Refusing to follow pagination 'next' URL to a different origin. Expected $originalOrigin, got $nextOrigin. This may indicate a compromised server or man-in-the-middle attack."
129+
}
130+
$nextBuilder
115131
}
116132
else {
117133
$URI

Tests/Helpers.Tests.ps1

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -569,6 +569,118 @@ Describe "Helpers tests" -Tag 'Core', 'Helpers' {
569569
$result.results | Should -Not -BeNullOrEmpty
570570
}
571571
}
572+
573+
Context "Pagination next-URL origin validation (Tier 2 security review - TM-1/IV-1)" {
574+
It "Should throw when 'next' points to a different host" {
575+
InModuleScope -ModuleName 'PowerNetbox' {
576+
Mock -CommandName 'Invoke-RestMethod' -MockWith {
577+
return @{
578+
count = 2
579+
next = 'https://attacker.example.com/api/dcim/devices/?limit=1&offset=1'
580+
previous = $null
581+
results = @(@{id=1; name='device1'})
582+
}
583+
}
584+
585+
$URI = BuildNewURI -Segments 'dcim', 'devices' -SkipConnectedCheck
586+
{ InvokeNetboxRequest -URI $URI -All -PageSize 1 } |
587+
Should -Throw -ExpectedMessage '*Refusing to follow pagination*different origin*attacker.example.com*'
588+
}
589+
}
590+
591+
It "Should throw when 'next' downgrades scheme from https to http" {
592+
InModuleScope -ModuleName 'PowerNetbox' {
593+
Mock -CommandName 'Invoke-RestMethod' -MockWith {
594+
return @{
595+
count = 2
596+
next = 'http://netbox.domain.com/api/dcim/devices/?limit=1&offset=1'
597+
previous = $null
598+
results = @(@{id=1; name='device1'})
599+
}
600+
}
601+
602+
$URI = BuildNewURI -Segments 'dcim', 'devices' -SkipConnectedCheck
603+
{ InvokeNetboxRequest -URI $URI -All -PageSize 1 } |
604+
Should -Throw -ExpectedMessage '*Refusing to follow pagination*'
605+
}
606+
}
607+
608+
It "Should throw when 'next' changes the port" {
609+
InModuleScope -ModuleName 'PowerNetbox' {
610+
Mock -CommandName 'Invoke-RestMethod' -MockWith {
611+
return @{
612+
count = 2
613+
next = 'https://netbox.domain.com:8443/api/dcim/devices/?limit=1&offset=1'
614+
previous = $null
615+
results = @(@{id=1; name='device1'})
616+
}
617+
}
618+
619+
$URI = BuildNewURI -Segments 'dcim', 'devices' -SkipConnectedCheck
620+
{ InvokeNetboxRequest -URI $URI -All -PageSize 1 } |
621+
Should -Throw -ExpectedMessage '*Refusing to follow pagination*'
622+
}
623+
}
624+
625+
It "Should allow 'next' with matching scheme, host, and port" {
626+
InModuleScope -ModuleName 'PowerNetbox' {
627+
$script:sameOriginPages = 0
628+
Mock -CommandName 'Invoke-RestMethod' -MockWith {
629+
$script:sameOriginPages++
630+
if ($script:sameOriginPages -eq 1) {
631+
return @{
632+
count = 2
633+
next = 'https://netbox.domain.com/api/dcim/devices/?limit=1&offset=1'
634+
previous = $null
635+
results = @(@{id=1; name='device1'})
636+
}
637+
}
638+
return @{
639+
count = 2
640+
next = $null
641+
previous = $null
642+
results = @(@{id=2; name='device2'})
643+
}
644+
}
645+
646+
$URI = BuildNewURI -Segments 'dcim', 'devices' -SkipConnectedCheck
647+
$results = InvokeNetboxRequest -URI $URI -All -PageSize 1
648+
$results.Count | Should -Be 2
649+
}
650+
}
651+
652+
It "Should allow 'next' when original URI has explicit default port but 'next' omits it" {
653+
# Edge case (Gemini review round 1): URI constructed with
654+
# explicit port 443, server returns 'next' without the port.
655+
# Both should normalise to the same origin via GetLeftPart.
656+
InModuleScope -ModuleName 'PowerNetbox' {
657+
$script:edgeCasePages = 0
658+
Mock -CommandName 'Invoke-RestMethod' -MockWith {
659+
$script:edgeCasePages++
660+
if ($script:edgeCasePages -eq 1) {
661+
return @{
662+
count = 2
663+
# No explicit port - UriBuilder fills 443 but GetLeftPart omits it
664+
next = 'https://netbox.domain.com/api/dcim/devices/?limit=1&offset=1'
665+
previous = $null
666+
results = @(@{id=1; name='device1'})
667+
}
668+
}
669+
return @{
670+
count = 2
671+
next = $null
672+
previous = $null
673+
results = @(@{id=2; name='device2'})
674+
}
675+
}
676+
677+
# Construct URI with explicit port 443 to exercise the edge case
678+
$URI = [System.UriBuilder]::new('https', 'netbox.domain.com', 443, '/api/dcim/devices/')
679+
$results = InvokeNetboxRequest -URI $URI -All -PageSize 1
680+
$results.Count | Should -Be 2
681+
}
682+
}
683+
}
572684
}
573685

574686
Context "InvokeNetboxRequest Error Handling" {

0 commit comments

Comments
 (0)