Skip to content

feat(remediation): validate upgrade suggestions against transitive dependency graph (#708)#790

Open
dinesh9997 wants to merge 19 commits into
utksh1:mainfrom
dinesh9997:bug/remediation-safety
Open

feat(remediation): validate upgrade suggestions against transitive dependency graph (#708)#790
dinesh9997 wants to merge 19 commits into
utksh1:mainfrom
dinesh9997:bug/remediation-safety

Conversation

@dinesh9997

Copy link
Copy Markdown

Resolves #708

Problem Statement

Remediation suggestions (such as version upgrades) generated by scanners could conflict with other existing project dependencies, resulting in dependency conflicts and broken builds when applied.

Solution Overview

Integrated a transitive dependency graph resolver that parses project manifests and validates recommended library upgrades against package version constraints.

  1. Dependency Parsing & Graph Building: Scans local project manifests (Python requirements.txt / requirements-dev.txt and Node.js package.json / package-lock.json).
  2. NPM to PEP-440 Version Mapping: Maps NPM/Node semver range specifiers (carets ^, tildes ~, wildcards, partial versions) to PEP-440 specifiers using packaging.specifiers to enable standard Python version compliance checking.
  3. Remediation Conflict Checking: Checks proposed upgrade versions against constraints in the dependency graph.
  4. Enriched Safety Indicators: Stores a "safe_to_apply" indicator, "compatible_range", and actionable "alternatives" (e.g., upgrading the parent package) inside finding metadata and exposes them as top-level fields in API payloads.

Key Changes

  • backend/secuscan/remediation.py [NEW]: Encapsulates all package normalization, version cleaning, semver-to-pep440 conversion, lockfile/manifest parsing, and validation logic.
  • backend/secuscan/models.py: Added optional safe_to_apply, compatible_range, and alternatives fields to the Pydantic Finding schema.
  • backend/secuscan/executor.py: Integrated validation logic into the executor's _build_result_contract normalization phase.
  • backend/secuscan/routes.py: Enriched findings deserialization helper and /finding/{finding_id} routes to expose safety indicators at the top level of responses.
  • testing/backend/unit/test_remediation_safety.py [NEW]: Added 10 unit tests covering range translations, manifest parsing, safe/unsafe validations, and serialization.

Verification and Testing Done

Automated Tests

  • Ran unit tests specifically for remediation validation logic:
    .\venv\Scripts\pytest testing/backend/unit/test_remediation_safety.py -v

@dinesh9997 dinesh9997 closed this Jun 12, 2026
@dinesh9997 dinesh9997 reopened this Jun 12, 2026
@dinesh9997 dinesh9997 closed this Jun 12, 2026
@dinesh9997 dinesh9997 reopened this Jun 12, 2026
@utksh1 utksh1 added level:advanced 55 pts difficulty label for advanced contributor PRs type:feature Feature work category bonus label area:backend Backend API, database, or service work area:security Security-sensitive implementation or tests labels Jun 12, 2026

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs changes before it can merge.

Blocking issues:

  1. build_dependency_graph(target) falls back to this SecuScan repository’s backend/frontend manifests whenever the supplied target does not contain requirements/package files. For normal URL/IP scan targets, that means remediation safety is computed against SecuScan’s own dependencies, not the scanned project. That can mark unrelated remediation advice safe/unsafe based on the wrong dependency graph.

  2. _build_result_contract wraps the whole remediation validation block in except Exception: pass, so parser bugs, malformed manifests, and version parsing errors disappear silently. At minimum, log the failure with task/plugin context and avoid adding misleading safety metadata when validation did not actually run.

  3. The feature adds top-level API fields but only sources them from metadata during deserialization. Please add route/serialization coverage that proves list/detail finding responses expose these fields only when they came from validated remediation metadata.

  4. The resolver includes deterministic mock transitive dependencies in production code. Test fixtures should live in tests, not in runtime remediation logic.

@dinesh9997

Copy link
Copy Markdown
Author

Hi @utksh1 ,
I have addressed all the blocking review items for the transitive dependency safety engine:

  1. Target Manifest Fallback Fixed: build_dependency_graph(target_dir) now checks for local target directory existence. If the target is a URL, IP address, or invalid path, it immediately returns {} instead of falling back to local SecuScan manifests (preventing validation against the host repository's dependencies).

2.Relocated Test Mock Fixtures: Removed the hardcoded get_mock_dependencies registry from remediation.py. Test fixtures now live entirely in test_remediation_safety.py, where get_python_transitive_dependencies is mocked dynamically.

3.Atomic Safety Enrichment: Validation is now performed atomically inside executor.py (_build_result_contract). If an exception or parser crash occurs, we print warning logs containing task and plugin contexts, and avoid adding any partial or misleading safety metadata to the findings.

4.Integration Route Coverage: Created test_routes_remediation_safety.py to test both /findings (list) and /finding/{finding_id} (detail) responses, ensuring safe_to_apply, compatible_range, and alternatives are exposed correctly only when sourced from validated remediation metadata.

5.Formatting Hygiene: Cleaned up all trailing whitespaces in the test files to pass the formatting checks.

so let me know any changes needed to be done.

@dinesh9997 dinesh9997 requested a review from utksh1 June 13, 2026 12:48

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. This still needs changes before merge.

The PR remains broader than the remediation graph fix and now includes .audit-config.yaml plus scan-cache/main/routes churn. The remediation change is security-sensitive and should be reviewable on its own.

Please remove the unrelated audit/config/cache/API churn and keep this focused on validating upgrade suggestions against the dependency graph with direct unit/integration coverage. Once the patch is narrow, the remaining remediation behavior can be reviewed properly.

@dinesh9997 dinesh9997 force-pushed the bug/remediation-safety branch from 73796d2 to 8aa1e83 Compare June 14, 2026 12:44
@dinesh9997

Copy link
Copy Markdown
Author

Hi @utksh1,

I have cleaned up this branch as requested:

Reverted all unrelated .audit-config.yaml, cache.py, main.py, plugin metadata, and config/manifest changes back to the upstream base.
Removed the test_scan_cache.py test suite from this branch.
Cleaned up the executor.py and routes.py changes to keep only the remediation safety validation loop inside _build_result_contract and the serialization exposure helpers.
This PR is now strictly focused on the Transitive Dependency Safety Engine implementation and its related unit/integration tests.so let me know any changes needed to be done.

@dinesh9997 dinesh9997 requested a review from utksh1 June 14, 2026 12:53

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecking after the latest merge from main: this is still blocked.

The frontend-checks job is failing on the current head. This remediation graph change also needs to stay focused on remediation dependency validation, without unrelated audit/config/cache/API churn, before it can be reviewed safely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:backend Backend API, database, or service work area:security Security-sensitive implementation or tests level:advanced 55 pts difficulty label for advanced contributor PRs type:feature Feature work category bonus label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Remediation suggestions conflict with existing dependencies, causing breakage

2 participants