Render matched strings on hunt results

specs/03-formatters.md

@@ -173,3 +173,105 @@ the attributes directly:
 - `source_rule_changed` is tri-state: `None` means UNKNOWN, not "unchanged",
   and prints nothing; the label names its reference point — "changed since
   this hunt froze it" — so it cannot read as "edited recently".
+
+## Matched strings on hunt results
+
+`TextOutput.historical_result` / `TextOutput.live_result` render `result.matched_strings`
+— the yara strings behind a hit — between `Tags:` and `Download Url:`, via the shared
+`TextOutput._matched_strings` helper.
+
+The attribute is **three-state** (the SDK's `05-downstream-contract.md` is authoritative)
+and the three must stay distinguishable in the output — but they do **not** each get a
+line.
+
+| `matched_strings` | Rendered |
+|---|---|
+| `None` | *nothing* — no line is emitted |
+| `[]`, no count | `Matched Strings: none -- the rule matched without byte evidence (a structural or negative match, or private strings)` |
+| `[]`, count > 0 | `Matched Strings: none shown (N withheld, result size limit)` — the count **overrides** the row above, because asserting a structural match while discarding a withheld count would be a confident false statement. Should be unreachable; see below. |
+| `[…]` | `Matched Strings:` followed by one indented `  $ident @ 0xOFFSET (N bytes[, truncated]): DATA` line per entry |
+
+**The silent-`None` branch depends on list routes sending `null`, and that is measured
+rather than assumed.** If a list route ever returned `[]` per row instead, every row of a
+large hunt would carry the loud "matched without byte evidence" line — the permanent
+false alarm this design exists to avoid, arriving through the branch deliberately kept
+loud. The server pins it for **both** hunt pairs in artifact-index's
+`test_list_serializers_render_nulls_not_payloads`, which asserts the key is present-and-null on
+`ScanResultListSerializer` *and* `LiveResultListSerializer`, against fixture rows that do
+carry evidence. This repo cannot verify it; it relies on that test.
+
+Two constraints, both counter-intuitive enough to be worth stating:
+
+- **`None` emits nothing, and `[]` must not follow it into silence.** The instinct is to
+  explain the absence. Resist it: `None` overwhelmingly means "this is a list route",
+  which *can never* carry strings, so a line there is a permanent false alarm on every
+  row rather than information — and `live feed` loops over this same method, with nothing
+  on the resource to tell the routes apart (`live_feed` and `live_result` both yield a
+  `LiveHuntResult`). Route-awareness would mean threading a flag from the command layer
+  into a new parameter on **every** `BaseOutput` implementation (`text`, `json`, all three
+  `hashes` subclasses), which buys too little. `[]` is the opposite case and keeps its
+  line: it only ever reaches a detail route, and "the rule matched with no byte evidence"
+  is a real answer to "why did this hit". Since the analyzer always sends `strings` once
+  this feature ships, `None` on a detail route means a result predating it — nothing to
+  say.
+- **The entry keys are subscripted, deliberately.** `identifier`, `offset`, `length`,
+  `data` and `truncated` are read by subscript, because a partial entry is not version
+  skew but a producer violating its own contract, and rendering half a match as though it
+  were whole is worse than failing. The attribute itself needs no such defence: the floor
+  names an SDK that parses it (§Current floor in [`05-sdk-contract.md`](./05-sdk-contract.md)).
+- **`data` is sanitised before rendering.** It is the only sample-derived field in a hunt
+  result, so it is attacker-controlled end to end. yara escapes non-printables upstream
+  and the analyzer preserves that rendering, so `_safe_data` is a no-op on valid input —
+  it exists because the guarantee lives in another repo, and a raw CSI sequence reaching
+  a terminal would repaint or clear an analyst's screen.
+- **ASCII only, and true of this whole block.** The literals are ASCII; the two
+  server-supplied *string* fields — `data` and `identifier` — go through `_safe_data`;
+  and the three server-supplied *numbers* — `offset`, `length`, `dropped` — carry an
+  integer format spec (`:x` / `:d`), which is what pins them rather than `_safe_data`.
+  Fields *outside* the block (`rule_name`, `tags`) are unfiltered and outside this claim.
+  Stdout under a C/POSIX locale replaces non-ASCII with `?`.
+- **`truncated` is not a byte count.** The stored length is capped server-side, so the
+  marker means "there was more than this" and over-reports at exactly the cap. Never
+  render it as an exact size.
+
+**Read both attributes directly** — `result.matched_strings`, no `getattr`. The floor
+(`polyswarm_api>=4.4.0`, [`05-sdk-contract.md`](./05-sdk-contract.md) §Current floor)
+names an SDK that parses them, so `pip` refuses the install a probe would guard against;
+`specs/05-project-standards.md` §16 has the general rule. `None` then means exactly what
+the table above says — the *server* did not report — which is the reading the silent
+branch depends on.
+
+### The dropped-count line
+
+When `result.matched_strings_dropped` is non-zero, a final line is appended **inside** the
+block, in **yellow** rather than white:
+
+```
+  ... 19 more not shown (result size limit)
+```
+
+It is the one line here reporting something the platform withheld, which is why it is not
+white like the entries above it. Omitted entirely when the count is zero or `None`.
+
+**An empty list with a non-zero count does not claim "no byte evidence".** That
+combination should be unreachable — the analyzer keeps a match's first string, so
+`[] ⇒ dropped == 0` — but the renderer must not *depend* on an invariant owned by another
+repo while making a positive claim about the rule. It reports what is certain instead
+(`none shown (N withheld, result size limit)`), because asserting a structural match and
+discarding the count is the precise wrong inference this line exists to prevent.
+
+This is not cosmetic. Without it a truncated list reads as the whole truth and a user
+concludes their rule hit twice when it hit twenty-one times — the same wrong-inference
+class the three-state contract above exists to prevent, one level down.
+
+`JSONOutput` needs no change — it dumps the resource's `.json`, which already carries the
+raw `matched_strings` and `matched_strings_dropped` keys.
+
+Coverage is two modules. `tests/hunt_matched_strings_test.py` is Style 3 — the formatter
+driven directly with constructed SDK resources — and owns *which line a field value
+produces*. `tests/hunt_matched_strings_cli_test.py` is the Style 1 counterpart
+[`04-testing.md`](./04-testing.md) requires: it drives `live result` through `CliRunner`
+so a broken `output.extend` call is caught, which the Style 3 module cannot see because
+it calls `_matched_strings` itself. The `cli_test.py` cassettes predate the field, so
+every result they render takes the silent `None` branch — they pin that no stray line
+appears, and nothing more.

specs/05-sdk-contract.md

@@ -96,8 +96,10 @@ predates this and is documented where it lives: the known-good rendering attribu
 that is not supported rather than against a version the floor permits.) The hunt-page surfaces —
 `ruleset_favorite` and the `YaraRulesetFavorite` resource, the `ruleset_list` filters,
 `live_feed(livescan_id=, max_results=)`, and the tracking/provenance fields the
-formatters render — are what moved the floor to 4.4.0. Code and tests use them
-directly.
+formatters render — are what moved the floor to 4.4.0, together with
+`matched_strings` / `matched_strings_dropped` on the four hunt-result classes
+(the yara evidence behind a hit; see [`03-formatters.md`](./03-formatters.md)
+§Matched strings on hunt results). Code and tests use them directly.
 
 **Raising the floor is the whole procedure** when this repo needs something new from
 the SDK:

src/polyswarm/formatters/text.py

@@ -1,5 +1,6 @@
 import sys
 import functools
+import re
 import json
 from datetime import datetime
 
@@ -17,6 +18,18 @@ def pretty_print_datetime(value):
     return datetime.strftime(value, '%Y-%m-%d %H:%M:%S UTC')
 
 
+# `data` and `identifier` are sample-derived, so attacker-controlled; the escaping that
+# makes them safe is upstream's contract, not ours. Whitelist, not blacklist -- an
+# earlier blacklist stopped at \x7f and let U+009B (8-bit CSI) through.
+# Rationale: specs/03-formatters.md, Matched strings.
+_UNPRINTABLE = re.compile(r'[^\x20-\x7e]')
+
+
+def _safe_data(value):
+    """Matched bytes as yara rendered them, with anything unprintable neutralised."""
+    return _UNPRINTABLE.sub('.', value)
+
+
 def is_grouped(fn):
     @functools.wraps(fn)
     def wrapper(self, text):
@@ -41,6 +54,33 @@ def _get_score_format(self, score):
         else:
             return self._red
 
+    # Three-state, and NOT interchangeable: None renders nothing (it is dominated by list
+    # routes, which can never carry strings), [] keeps its line, a list renders entries.
+    # Do not collapse them or add a line for None. Why: specs/03-formatters.md.
+    def _matched_strings(self, strings, dropped=None):
+        if strings is None:
+            return []
+        if not strings:
+            if dropped:
+                # Contradictory input (the analyzer keeps a match's first string).
+                # Report only what is certain rather than assert a rule property.
+                return [self._yellow(
+                    f'Matched Strings: none shown ({dropped:d} withheld, result size limit)')]
+            return [self._white('Matched Strings: none -- the rule matched without byte '
+                                'evidence (a structural or negative match, or private strings)')]
+        lines = [self._white('Matched Strings:')]
+        for string in strings:
+            size = f'{string["length"]:d} bytes'
+            if string['truncated']:
+                size += ', truncated'
+            lines.append(self._white(
+                f'  {_safe_data(string["identifier"])} @ 0x{string["offset"]:x} ({size}): {_safe_data(string["data"])}'))
+        if dropped:
+            # Yellow: the one line here reporting something the platform withheld.
+            lines.append(self._yellow(
+                f'  ... {dropped:d} more not shown (result size limit)'))
+        return lines
+
     def _output(self, output, write):
         if write:
             click.echo('\n'.join(output) + '\n', file=self.out)
@@ -252,6 +292,8 @@ def historical_result(self, result, write=True):
                     output.append(self._white(malicious))
         if result.tags:
             output.append(self._white(f'Tags: {result.tags}'))
+        output.extend(self._matched_strings(result.matched_strings,
+                                            result.matched_strings_dropped))
         if result.download_url:
             output.append(self._white(f'Download Url: {result.download_url}'))
         return self._output(output, write)
@@ -283,6 +325,8 @@ def live_result(self, result, write=True):
                     output.append(self._white(malicious))
         if result.tags:
             output.append(self._white(f'Tags: {result.tags}'))
+        output.extend(self._matched_strings(result.matched_strings,
+                                            result.matched_strings_dropped))
         if result.download_url:
             output.append(self._white(f'Download Url: {result.download_url}'))
         return self._output(output, write)

tests/hunt_matched_strings_cli_test.py

@@ -0,0 +1,83 @@
+"""Command-tree coverage for the matched-strings block (Style 1 — SDK-boundary mocks).
+
+`hunt_matched_strings_test.py` drives the formatter directly and covers *which line a
+given field value produces*, including the `output.extend` call inside `live_result` --
+it invokes that method, so a deletion there does fail it. What it cannot observe is
+everything ABOVE the formatter: that `live result` parses its argument, calls the SDK,
+and hands the result to `output.live_result` rather than some other renderer.
+`specs/04-testing.md` §Style 3 requires exactly this counterpart -- a command whose
+rendering is covered that way "still needs at least one `CliRunner` test proving the
+command reaches the formatter".
+
+Measured, not assumed: rewiring the command to `output.live_feed([...])` leaves all 36
+assertions in the sibling module passing and fails this module. The cassette tests cannot
+catch it either -- they predate the field, so every result they render takes the silent
+`None` branch.
+"""
+from unittest import TestCase, mock
+
+from click.testing import CliRunner
+from polyswarm_api import resources
+
+from polyswarm.client import polyswarm as client
+
+_API_KEY = '1' * 32
+_API_URL = 'http://artifact-index-e2e:9696/v3'
+_COMMUNITY = 'gamma'
+
+_CONTENT = {
+    'id': 123,
+    'instance_id': 2,
+    'livescan_id': 3,
+    'created': '2022-05-26T19:41:33.797898',
+    'sha256': 'f' * 64,
+    'rule_name': 'dos_stub_message',
+    'tags': '{pe,stub}',
+    'polyscore': 0.5,
+    'malware_family': None,
+    'detections': {'malicious': 1, 'total': 1},
+}
+
+_STRINGS = [
+    {'offset': 78, 'identifier': '$stub', 'length': 14,
+     'data': '54 68 69 73 20 70 72 6F 67 72 61 6D 20 63', 'truncated': False},
+]
+
+
+def _live_result(**overrides):
+    """A real `LiveHuntResult`, not a mock — the formatter reads polyscore, detections and
+    the rest of the row, so a bare stub dies before it reaches the block under test. The
+    floor (`polyswarm_api>=4.4.0`) guarantees this parses both matched-strings keys."""
+    return resources.LiveHuntResult(dict(_CONTENT, **overrides))
+
+
+class HuntMatchedStringsCliTest(TestCase):
+    def setUp(self):
+        self.cli = CliRunner()
+
+    def _run(self, *cmd):
+        return self.cli.invoke(
+            client.polyswarm_cli,
+            ['-a', _API_KEY, '-u', _API_URL, '-c', _COMMUNITY] + list(cmd),
+            catch_exceptions=False,
+        )
+
+    def _live_result_output(self, **overrides):
+        with mock.patch('polyswarm_api.api.PolyswarmAPI.live_result',
+                        return_value=_live_result(**overrides)):
+            return self._run('--output-format', 'text', 'live', 'result', '123').output
+
+    def test_live_result_renders_the_block_through_the_command_tree(self):
+        output = self._live_result_output(matched_strings=_STRINGS)
+        assert 'Matched Strings:' in output
+        assert '$stub @ 0x4e' in output
+
+    def test_live_result_reports_a_withheld_count_through_the_command_tree(self):
+        output = self._live_result_output(matched_strings=_STRINGS,
+                                          matched_strings_dropped=19)
+        assert '19 more not shown' in output
+
+    def test_live_result_stays_silent_when_nothing_was_reported(self):
+        # No matched_strings key at all -- what a detail route serves for a result that
+        # predates the feature, and what every list route serves always.
+        assert 'Matched Strings' not in self._live_result_output()