Skip to content

fix: wire validate_command_network_egress into execute_task#631

Open
ionfwsrijan wants to merge 7 commits into
utksh1:mainfrom
ionfwsrijan:fix/614-validate-command-network-egress
Open

fix: wire validate_command_network_egress into execute_task#631
ionfwsrijan wants to merge 7 commits into
utksh1:mainfrom
ionfwsrijan:fix/614-validate-command-network-egress

Conversation

@ionfwsrijan
Copy link
Copy Markdown
Contributor

Description

The function validate_command_network_egress() in validation.py inspects every command argument for network destinations against the safe-mode + network policy configuration. However, it is never imported or invoked anywhere in the execution pipeline (executor.py or routes.py). While the primary target field is validated, secondary plugin fields like proxy, virtual_host, config_file, and user_agent can contain arbitrary network destinations that bypass target validation entirely.

Fix: wire validate_command_network_egress() into executor.py's execute_task() right after the command is built and before Docker sandboxing, ensuring every command argument is validated.

Related Issues

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

The validate_command_network_egress function is now called after plugin_manager.build_command() completes and before subprocess execution. If any argument fails validation, a ValueError is raised, preventing task execution.

Checklist

  • My code follows the code style of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.

The function validate_command_network_egress() was defined in
validation.py but never imported or invoked anywhere in the execution
pipeline. This meant secondary plugin fields like proxy, virtual_host,
config_file, and user_agent could contain arbitrary network destinations
that bypassed target validation entirely.

Now validate_command_network_egress() is called right after the command
is built and before Docker sandboxing, ensuring every command argument
is inspected for network destinations against the safe-mode + network
policy configuration.

Fixes utksh1#614
@ionfwsrijan ionfwsrijan force-pushed the fix/614-validate-command-network-egress branch 2 times, most recently from 8cb3b8d to 06d0663 Compare June 6, 2026 09:02
@ionfwsrijan ionfwsrijan force-pushed the fix/614-validate-command-network-egress branch from 06d0663 to c610ba5 Compare June 6, 2026 09:26
@ionfwsrijan
Copy link
Copy Markdown
Contributor Author

@utksh1 You may review and merge this

@utksh1 utksh1 added level:advanced 55 pts difficulty label for advanced contributor PRs type:bug Bug fix work category bonus label type:security Security work category bonus label area:backend Backend API, database, or service work area:security Security-sensitive implementation or tests labels Jun 7, 2026
Copy link
Copy Markdown
Owner

@utksh1 utksh1 left a comment

Choose a reason for hiding this comment

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

Focused security fix wiring validate_command_network_egress into task execution before sandbox execution. Checks are green; approving.

@utksh1 utksh1 added gssoc:approved Admin validation: approved for GSSoC scoring and removed gssoc:approved Admin validation: approved for GSSoC scoring labels Jun 7, 2026
Copy link
Copy Markdown
Owner

@utksh1 utksh1 left a comment

Choose a reason for hiding this comment

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

After #629 merged, this branch now conflicts with main because it carries the same CI baseline files. The security fix is still useful, but this PR is not mergeable now. Please rebase on latest main and keep only the validate_command_network_egress execution-path change plus its directly related validation change/tests.

Copy link
Copy Markdown
Owner

@utksh1 utksh1 left a comment

Choose a reason for hiding this comment

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

Re-reviewed after the latest update. The branch is no longer conflicted, but it still carries unrelated CI-baseline/frontend test changes in addition to the executor/validation security fix. Please rebase on latest main and narrow the PR to backend/secuscan/executor.py, backend/secuscan/validation.py, and directly related tests only.

@ionfwsrijan
Copy link
Copy Markdown
Contributor Author

@utksh1 You may merge this now

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:bug Bug fix work category bonus label type:security Security work category bonus label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants