Skip to content

fix: sandbox_file reads a QR-code image instead of uploading its path - #328

Merged
vhmartinezm merged 3 commits into
developfrom
ioc-refang
Sep 30, 2026
Merged

vhmartinezm merged 3 commits into
developfrom
ioc-refang

Conversation

@vhmartinezm

@vhmartinezm vhmartinezm commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

What

Follow-up to #327 (IoC refanging), addressing its review before 4.6.0 is released.

  • sandbox_file now reads a QR-code image instead of uploading its path. With artifact_type=URL and preprocessing={'type': 'qrcode'}, the URL branch handed the path string to LocalArtifact.from_content. That made the upload body the UTF-8 text of the path, so the server created the sandbox task and then failed it as "Image format not recognized", even for a valid image. The branch now reads the file with LocalArtifact.from_path, as submit's QR branch already does, and still never refangs it.
    • The from_content call predates refanging (3.9.0). feat: refang defanged IoC inputs before building requests #327 only added the guard around it, and its tests pinned the path-as-body behaviour. Those tests now spy on from_path and also assert that nothing is uploaded as text.
    • Observable effects: the default artifact name becomes the file's basename, as in submit, and a missing path fails before any task is created.
    • A file handle already took the from_handle branch and is unaffected. refang_iocs makes no difference either way.
  • refang.py's module docstring now points to its spec instead of repeating it. It keeps only the portability paragraph, which describes how the patterns are written, and names specs/05-downstream-contract.md §"IoC refanging" for what the contract is, why clients refang, where it is applied and what is out of scope.
  • The case-table drift guard now says what it actually guards. The sha256 pin catches an edit to this copy made without updating its pin (a formatter rewrite, for example). It cannot see the other clients' copies, so keeping them identical is manual. The test comment, the test name and the spec sentence now say that.

Compatibility

No public surface changes and no version bump. develop already declares the unreleased 4.6.0, and this lands before that release. The CLI is unaffected: it never calls sandbox_file for QR codes (sandbox url --qrcode-file goes through sandbox_url).

Tests

  • New test/sandbox_file_qrcode_respx_test.py: a dual-transport respx test that writes real bytes to a file named qr[.]png, submits it with refanging on, and asserts the S3 PUT body is exactly those bytes and the create body's artifact_name is the unrefanged basename. Against the previous code it fails with the upload body being the text of the path. It uses the respx tier because the e2e stack's sandbox providers can't reasonably process a QR image.
  • The spy-based QR sandbox_file tests now expect from_path. On their own they cannot see the uploaded body; the respx test covers that.
  • test/_client_harness.py gains a public requests accessor (every request, in order) for multi-request flows.
  • The specs no longer describe the path as passed on as content: 03-endpoints says the image is read with from_path (body = image bytes, default name = basename), and 02-resources notes the QR exception on from_content.
  • Full suite: 383 passed.

The QR branch handed the path string to from_content, so the upload body was the text of the path and the server failed the task as an unrecognised image. Read it with from_path, as submit does. Also point refang.py's docstring at its spec section instead of repeating it, and say the case-table pin guards only its own copy.
@claude

claude Bot commented Sep 24, 2026

Copy link
Copy Markdown

The fix is correct. The sandbox_file QR branch now matches submit line for line, the sync mirror is consistent, the base is develop, and there's no version bump (4.6.0 is still unreleased). Two small follow-ups:

  1. Spec drift, specs/03-endpoints.md:267. It says a QR-code argument "is passed on unchanged". That sentence came from the old path-as-body behaviour and now reads as if the string is uploaded as is. Please say it is read from disk with LocalArtifact.from_path (default name = basename) and never refanged, on both submit and sandbox_file. specs/02-resources.md:380 also calls from_content the URL-submission reader; add "except QR-code submissions, which use from_path".
  2. Test gap. Both QR tests replace from_path with a spy that raises _Captured. That means nothing ever checks the body that actually gets uploaded, and this bug was a wrong-body bug. The assert uploaded_as_text == [] can't fail once the from_path spy has raised. Add one respx test (sync and async, or the parametrised ClientTestCase): write a few bytes to a tmp_path file with a [.] in its name, call sandbox_file(..., artifact_type='URL', preprocessing={'type': 'qrcode'}), and assert that the S3 PUT body is those file bytes and the create body's artifact_name is the basename, unrefanged. The respx tier fits here because the e2e stack's sandbox providers can't reasonably process a QR image.

@vhmartinezm
vhmartinezm requested a review from sbneto September 24, 2026 11:02
A respx test on both clients asserts the S3 PUT body is the image and the create body names its unrefanged basename; the spy-based tests could not see the uploaded body. Also fix the two spec sentences that still described the path being passed on as content.
@vhmartinezm

Copy link
Copy Markdown
Contributor Author

Both addressed in 35844b1 (and 6c50e82, import order):

  1. Specs: 03-endpoints.md now says a QR-code argument is read from disk with LocalArtifact.from_path (upload body = the image's bytes, default name = basename) and never refanged, on both submit and sandbox_file. 02-resources.md adds the QR exception to from_content.
  2. Wire test: new test/sandbox_file_qrcode_respx_test.py (ClientTestCase, both transports, refanging on). It writes bytes to a qr[.]png file and asserts the S3 PUT body equals those bytes and the create body's artifact_name is qr[.]png. I ran it against the previous commit's code and it fails there, with the upload body being the text of the path, which is exactly this bug. It is listed in specs/04-testing.md. The harness gains a public requests accessor so the test does not read private router state. 383 passed.

@claude

claude Bot commented Sep 24, 2026

Copy link
Copy Markdown

LGTM. Checked against AGENTS.md and specs/02–05: sandbox_file's QR branch now matches submit (from_path, no refang). The sync mirror matches the async change, the specs were updated in the same PR, the base is develop, and the version is unchanged (4.6.0 is still unreleased). The new respx test checks the S3 PUT body and the unrefanged artifact name on both transports. No issues.

@claude

claude Bot commented Sep 24, 2026

Copy link
Copy Markdown

No issues found against AGENTS.md or specs/. The sandbox_file QR branch now follows submit's from_path path, the sync mirror matches the async source, specs 02/03/04/05 are updated in the same PR, the respx test checks the S3 PUT body, the PR targets develop, and no version bump is needed because 4.6.0 is already declared and unreleased.

@sbneto

sbneto commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Lets an analyst paste an indicator from a threat-intel report (hxxps[:]//evil[.]com,
127[.]0[.]0[.]1) and have every PolySwarm client use its live form. Searches stop silently
missing, and submissions stop creating broken URL artifacts. One gated rewrite ships in three
clients, driven by one byte-identical case table. They are the SDK (opt-in reference), the CLI (on
by default) and the web UI's search surfaces. This PR lands the SDK's review fixes. 4 PRs on two
branch names; 55 files, +1953/−84 across the set; #327 and the CLI merged.

Severity: 0 HIGH · 2 MODERATE · 1 LOW — none of them in this repo. Prior feedback: all 29 points checked and addressed.
Objective: met, with gaps — let analysts paste defanged IoCs into search without re-fanging them by hand.

  • missing: the web UI's header Search page still misses defanged pastes with edge whitespace → F1, F2
  • drift: the web UI's upload card refangs too; not asked for, works

Fixes are proposed, not applied; nothing was run.

Cross-repo coordination

Set: polyswarm-api#327 merged · SDK (reference) — polyswarm-cli#274 merged · CLI — polyswarm-api#328 open · SDK review fixes ← you are here — the web UI's PR (private repo) open · UI

Merge order: polyswarm-api#327 → polyswarm-cli#274 → this PR and the web UI's PR in either order (§14: SDK and CLI before UI). This PR must land before 4.6.0 is released; the web UI consumes neither SDK PR.
No crossing surface carries a finding: both case tables and both pins are unchanged and still byte-identical.

Coherence: the fixes are independent, and none touches this repo.

Findings (round 1)

No defects found in this repo's diff. It resolves everything the SDK's earlier rounds left open on #327: the QR-code sandbox_file upload, the refang.py docstring, and the drift-guard wording. The respx test pins the actual upload body.

elsewhere: F1, F2, F3 → the web UI's PR (private repo)

Standards conformity

Change-level: no violations introduced by this diff.
polyswarm-api — Clean: §2, §14, §15, §16. Not applicable: §3–§13, §17.
Set — Clean: §14. Rule 6 name identity isn't load-bearing: this PR shares ioc-refang with the SDK and CLI, and no CI seam resolves the web UI against them.

@vhmartinezm

Copy link
Copy Markdown
Contributor Author

Thanks, Sam. Nothing is open for this repo, per your round-1 verdict. For the set: F1, F2 and F3, which you routed to the web UI's PR, are fixed there with tests that were red before the fix:

  • F1: the IOC tab Unicode-trims before refanging.
  • F2: the artifact tab trims the term and splits on runs of whitespace, so it leaves no empty token.
  • F3: the type-gate test now uses a value the refang would rewrite, and fails without the gate.

This PR is unchanged at 6c50e82, and CI is green.

@vhmartinezm
vhmartinezm merged commit 6ef9e90 into develop Sep 30, 2026
2 checks passed
@vhmartinezm
vhmartinezm deleted the ioc-refang branch September 30, 2026 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants