Repository navigation
fix(netexec): route stdout to action_output instead of a dead text output (#399) - #400
Conversation
…tput (#399) Every netexec contract declared a `text` output with isFindingCompatible=True, but the parser's _DISPATCHERS never populated a "text" field, so it never produced a Finding despite being advertised on nearly every contract (e.g. the "NetExec SMB - Disk Listing" inject: execution succeeds and produces a message, but the Findings tab shows nothing). Rename the dead TEXT output type to ACTION_OUTPUT and always populate outputs["action_output"] with the raw stdout when non-blank, independently of whether any typed dispatcher (credentials, shares, etc.) matched anything. isFindingCompatible=False keeps it out of the visible Findings tab while making it usable as a chaining/event filter. Depends on pyoaev exposing ContractOutputType.ActionOutput (OpenAEV-Platform/client-python#332).
There was a problem hiding this comment.
🟡 Not ready to approve
action_output is currently stored as stdout.strip() (contradicting “raw/unfiltered” intent) and the new ContractOutputType.ActionOutput usage needs a clearer compatibility guard given the pinned non-dev pyoaev version.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes NetExec’s “dead” generic output path by renaming the unused text output type to action_output and ensuring the parser always emits raw stdout under that key so executions have a consistent structured output (without surfacing it as a visible Finding).
Changes:
- Renames the legacy
TEXToutput type toACTION_OUTPUTacross the NetExec output registry and contract output elements. - Updates
NetExecOutputParser.parse()to always includeoutputs["action_output"]when stdout is non-blank, independent of typed extractors. - Updates/extends tests to validate
action_outputpresence (including partial-failure scenarios) and expectation-signature ordering.
File summaries
| File | Description |
|---|---|
| netexec/test/helpers/test_netexec_output_parser.py | Updates parsing expectations and adds tests ensuring action_output is preserved even when typed extraction fails. |
| netexec/test/contracts/test_expectation_signature_output.py | Renames test coverage from text to action_output and updates expected base output types. |
| netexec/netexec/helpers/netexec_output_parser.py | Adds routing of stdout into outputs["action_output"] and updates parser docstring/comments accordingly. |
| netexec/netexec/contracts/output_registry.py | Replaces TEXT with ACTION_OUTPUT throughout the output-type registry and defaults. |
| netexec/netexec/contracts/contract_outputs.py | Replaces the Text contract output element with an ActionOutput element (non-finding-compatible, single). |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| if stdout.strip(): | ||
| outputs["action_output"] = stdout.strip() |
There was a problem hiding this comment.
Good catch, fixed in 26eb253 — reworded the comment/docstring to describe the actual behavior (whitespace-trimmed, not fully unfiltered) instead of changing the strip() call. Trimming surrounding blank lines is harmless for chaining/event filters and consistent with how the rest of the parser handles stdout.
| _ACTION_OUTPUT_OUTPUT = ContractOutputElement( | ||
| type=ContractOutputType.ActionOutput, | ||
| field="action_output", | ||
| isMultiple=False, | ||
| isFindingCompatible=False, | ||
| labels=["netexec"], | ||
| ) |
There was a problem hiding this comment.
Not adding a runtime guard here — the failure mode is already fully explained and self-resolving: this PR's non-dev pyoaev pin (3.260731.1) intentionally stays untouched until client-python#332 (adding ContractOutputType.ActionOutput) merges and gets released, which is documented in the PR description's "Dependency" section. The current CI failure (ci/circleci: test-netexec, AttributeError: ContractOutputType has no attribute ActionOutput) is exactly that expected state — installing pyoaev@main today doesn't have ActionOutput yet. Adding a try/except around a one-line enum reference just to produce a "nicer" error for a state that's already unambiguous (full traceback pointing at the exact line) and temporary (goes away once the dependency chain lands) felt like defensive code for a scenario that's already self-explanatory rather than a real risk. Happy to reconsider if you think this needs to merge before client-python#332 for some other reason.
The comment claimed the stdout routed to action_output was "unfiltered", but the code trims leading/trailing whitespace via strip(). Reword to describe the actual behavior instead of changing it: dropping the surrounding whitespace-only noise is harmless for chaining/event filters and matches the rest of the codebase's stdout handling.
Summary
textoutput (isFindingCompatible=True), butNetExecOutputParser._DISPATCHERShas no extractor writing a"text"key — so it never produced a Finding, on any contract, ever. Confirmed reproduction: a "NetExec SMB - Disk Listing" inject executes and produces a message in Execution details, but the Findings tab shows "No finding found".TEXToutput type toACTION_OUTPUTthroughoutoutput_registry.pyandcontract_outputs.py(isFindingCompatible=False,isMultiple=False).NetExecOutputParser.parse()now always populatesoutputs["action_output"]with the stdout when non-blank (trimmed of leading/trailing whitespace) — independently of whether any typed dispatcher (credentials, shares, etc.) matched anything.isFindingCompatible=Falsekeeps it out of the visible Findings tab, while still storing it as a finding usable in chaining/event filters (e.g. matching a substring of the command output).Dependency
Requires
pyoaevto exposeContractOutputType.ActionOutput— see client-python#331 / client-python#332 (open, not yet released).pyproject.toml's pinnedpyoaevversion is intentionally left untouched — it needs bumping to the release that includes #332 before this can actually run against the published package (the dev extra already points at the local../../client-pythoncheckout, which is how the test suite below was validated).Closes #399. Related to #401 (tracking issue for the client-python + netexec migration to action_output).
Test plan
pytest test/— 125 passed, 3 subtests passed (including new/updated coverage intest_netexec_output_parser.pyandtest_expectation_signature_output.py)black --check/isort --check-only/ruff checkon changed files — cleanmypy— no new errors introduced (pre-existingimport-untypednoise from the local editablepyoaevinstall, unrelated to this change)test_action_output_preserved_on_partial_failure, reproducing the exact reported scenario (SMB session dropped mid-run) to confirm the raw stdout still reachesaction_output