fix(sbom): normalize Windows paths in deployment-URL discovery (#234) - #241
fix(sbom): normalize Windows paths in deployment-URL discovery (#234)#241nikhilpatidar wants to merge 3 commits into
Conversation
When the CLI ran on Windows, scanned file paths used backslashes while the deployment-path hint set used POSIX style. Workflow-path hints like ".github/workflows/", "/k8s/", and "infra/" silently failed to match, so deployment URLs discovered from .github/workflows/*.yml (and Azure App Service URLs reconstructed from AZURE_WEBAPP_NAME) were dropped instead of being harvested into deployment_urls. Normalize path separators to forward slashes before hint matching so Windows path strings match the same set as POSIX path strings.
Lock in the existing sub-folder discovery behaviour so any future walker change that breaks it (e.g. introducing a max depth or accidentally filtering by directory prefix) fails loudly instead of silently dropping packages. Covers requirements.txt, package.json, and pyproject.toml at arbitrary nesting depth, plus the venv / node_modules filtering that must still apply to deeply nested manifest files.
KanishkThamman
left a comment
KanishkThamman
left a comment
There was a problem hiding this comment.
Core fix (backslash normalization) is correct and safe on POSIX CI. Two nits inline on test coverage.
| assert "https://my-backend.azurewebsites.net" in result["deployment_urls"] | ||
|
|
||
|
|
||
| def test_windows_style_kubernetes_path_matches_hint() -> None: |
There was a problem hiding this comment.
Verified by reverting the fix locally: this test still passes without it — it's actually exercised by the content-based apiVersion: check, not the path-hint match it claims to cover. Doesn't prove the fix.
There was a problem hiding this comment.
Added in 8a113c4. Added test_windows_style_kubernetes_path_extracts_url_via_path_hint which uses path myapp\\k8s\\frontend.yaml with content that has no Kubernetes marker (no apiVersion:, no other k8s trigger). The URL only flows into deployment_urls because the /k8s/ path-hint matches after backslash-to-slash normalization. With the fix reverted, the raw backslash path never matches and the test fails.
| assert "Kubernetes" in result["deployment_platforms"] | ||
|
|
||
|
|
||
| def test_windows_style_infra_path_matches_hint() -> None: |
There was a problem hiding this comment.
Same issue as the kubernetes-path test above — passes via the azurerm_app_service content match regardless of the path normalization fix. Consider a case that only matches via a path hint (e.g. an /infra/ folder with content that doesn't independently trigger detection).
There was a problem hiding this comment.
Added in 8a113c4. Added two path-hint tests that use content with no Terraform/Azure markers:
- test_windows_style_infra_path_extracts_url_via_path_hint:
infra\\README.mdwith a plain text URL, noazurerm_resource. - test_windows_style_deployment_path_extracts_url_via_path_hint:
app\\infra\\notes.txtwith a plain text URL, also no Azure marker.
Both rely on the infra/ trailing-slash path hint matching after normalization. With the fix reverted, both fail (the URL is silently dropped). The original vacuous tests on lines 62 and 71 are left in place for backwards compatibility — they still pass and continue to assert the Kubernetes/Azure detection markers on Windows paths, but the new tests are the ones that actually prove the path normalization works.
The two existing Windows-path tests for k8s and infra hints (test_windows_style_kubernetes_path_matches_hint and test_windows_style_infra_path_matches_hint) were vacuous: they passed regardless of the path normalization fix because the test content happened to contain `apiVersion:` (k8s) and `azurerm_app_service` (infra) — content-based triggers that fire before the path hint is even consulted. Reviewer nit on PR NuGuardAI#241: tests should use content that only matches via a path hint, not via content-based detection. Added 3 new path-hint tests: - test_windows_style_kubernetes_path_extracts_url_via_path_hint: uses path `myapp\\k8s\\frontend.yaml` with content that has no Kubernetes marker. After path normalization `/k8s/` substring matches and the URL flows into deployment_urls. Without normalization the raw backslash path never matches the forward-slash hint. - test_windows_style_infra_path_extracts_url_via_path_hint: uses path `infra\\README.md` with content that has no Azure/ Terraform marker. `infra/` trailing-slash hint matches after normalization. - test_windows_style_deployment_path_extracts_url_via_path_hint: uses path `app\\infra\\notes.txt` with a plain text URL. Same `infra/` hint, different parent folder so the k8s test doesn't accidentally also fire. Negative-tested: with the path normalization reverted, all 5 path-hint-dependent tests fail (3 new + 2 existing workflow tests); the 3 content-based tests still pass. The fix is therefore required for the path-hint code path.
f172ff5 to
8a113c4
Compare
2 participants
PR Type
Fixes #234 and adds regression coverage for #226.
Issue #234 — Deployment-URL detection in
application_summary.pydid a case-insensitive substring match on the workflow path, but only against the raw string. On Windows the path uses backslashes, so the deployment-URL hint never matched and the SBOM shipped with no application.summary.deployment_hint. The fix normalizes separators before matching:Issue #226 — Sub-folder manifest discovery (e.g.
apps/api/requirements.txt) was already working via the recursive_iter_dependency_fileswalker. This PR adds regression tests so it stays that way.Tests
tests/sbom/test_application_summary_paths.py— 5 tests for the Windows-path fixnuguard/sbom/tests/test_subfolder_discovery.py— 6 tests for the sub-folder walkerCloses #234, Closes #226