Skip to content

feat: refang defanged IoC inputs before building requests - #327

Merged
vhmartinezm merged 9 commits into
developfrom
ioc-refang
Sep 23, 2026
Merged

vhmartinezm merged 9 commits into
developfrom
ioc-refang

Conversation

@vhmartinezm

@vhmartinezm vhmartinezm commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

What

The client can now refang defanged indicators of compromise before it builds a request, so hxxps[:]//evil[.]com is sent as https://evil.com and 127[.]0[.]0[.]1 as 127.0.0.1. The helpers are public in the new module polyswarm_api.refang: refang_text, is_network_ioc and refang_ioc(value, accept=None). A new constructor keyword, refang_iocs, switches it on for both clients. It is opt-in (default False): without it every input is sent verbatim, exactly as in 4.5.0.

Why

Threat-intel reports print indicators defanged so that nobody clicks one by accident. Analysts paste them straight from the report, and a defanged value never works: the server looks URLs up by an exact hash of the string, so a search silently finds nothing, and it stores a submitted URL verbatim, so a submission creates a new, broken URL artifact. The server deliberately does not guess what a string was meant to be, so each client refangs at its own edge, before the request exists.

Exactly which inputs are touched

With refang_iocs=True:

  • search_url(url)
  • search_by_metadata(ips=, urls=, domains=). The free-form query is never touched.
  • search_by_ioc(ip=, domain=)
  • check_known_hosts(ips=, domains=)
  • The host of add_known_good_host, add_known_bad_host and update_known_good_host. A defanged host written to the known-host catalogue would never match a real lookup, so it gets the same treatment as a search.
  • The URL of every URL submission from a string: submit(..., artifact_type=URL), sandbox_file(..., artifact_type=URL) and sandbox_url(url). Both the uploaded content and the default artifact name are refanged; an explicit artifact_name is kept as given.

Never touched: hashes, ids, and the argument of a QR-code submission (preprocessing={'type': 'qrcode'} on submit or sandbox_file). That argument names an image file, not a URL, so qr[.]png stays qr[.]png.

The rewrite is gated so that it can only ever fix an indicator, never damage one. It applies only when:

  • something was actually defanged, and
  • the result has no whitespace or double quote (so it is not a query), and
  • the rewrite does not keep the input's scheme and host intact (if it does, only a path would change, so a legitimate [.] in example.com/a[.]b or https://example.com/a[.]b survives), and
  • the result is a URL, domain or IP.

Otherwise the value is sent byte-for-byte as before.

Opt in with PolyswarmAPI(..., refang_iocs=True). The setting is also readable as the public attribute api.refang_iocs, so a consumer that handles a value outside the endpoint methods can apply refang.refang_ioc under the same switch.

Two side notes

  • submit now reads the QR preprocessing type with preprocessing.get("type"), as sandbox_file already did. This is a small behaviour change: a preprocessing dict with no type key used to raise KeyError and now takes the regular URL-content branch.
  • A bare string passed as ips= / urls= / domains= is refanged, but only check_known_hosts accepts that form. search_by_metadata has always iterated such a string one character per param, and this PR does not change that.

A contract shared with other clients

Other PolySwarm clients implement the same refang contract: the same rules in the same order, the same gate, and the same case table. test/fixtures/refang_cases.json is kept byte-identical across them, so changing a rule means changing it everywhere. Portability is part of the contract:

  • no case-insensitive flag, because Python's folds Unicode (U+212A KELVIN SIGN would match k);
  • no \b, \d, \w, \s or \S; whitespace is the explicit ASCII set;
  • full matches use fullmatch, because $ also matches before a trailing newline.

Out of scope: email [at], a bare-word dot, http__host / http:\\host, stripping bare brackets (IPv6 literal syntax), and non-ASCII hosts.

Version: 4.6.0, and why refanging is opt-in

Refanging on by default would not be additive. It changes what the write paths store: a known-host row written verbatim as evil[.]com by an earlier client would stop matching a lookup that now sends evil.com, and a URL submission would create a different artifact than the same call did on 4.5.0. The versioning table in specs/05-downstream-contract.md only allows a new keyword in a minor release when its default preserves current behaviour, so the default is False and 4.6.0 is a minor bump. The CLI opts in through its own --refang/--no-refang option. The bump lands in this feature PR under the standing exception in AGENTS.md: the CLI resolves this SDK from source by branch name, and its paired change raises its floor to polyswarm_api>=4.6.0, a version this repo must already declare. The emitted string is a clean 4.6.0, with no dev suffix.

Tests

  • Pure-unit tests for polyswarm_api.refang, driven by the shared 42-case table.
  • Request-shape tests for every method listed above, on both transports, with the flag on and off, plus the default (off) on both clients. They capture the built request at the _paginate / _single boundary and assert the outgoing params or JSON body. They also cover the uploaded URL content on both transports (on and off), the QR-code paths, and the bare-string form of the list arguments.
  • No VCR cassettes: this is client-side input normalization and the wire contract is unchanged, so there is no new endpoint behaviour for a recording to pin.
  • A drift guard pins the case table's sha256; the other clients pin the same digest over their copy, so editing the table in one repo fails that repo's suite until every copy and pin change together.
  • Full suite: 381 passed.

Threat-intel reports print indicators defanged (hxxps[:]//evil[.]com,
127[.]0[.]0[.]1). The server looks URLs up by an exact hash and stores
a submitted URL verbatim, so a defanged value silently missed a search
or became a broken URL artifact.

Add polyswarm_api.refang (refang_text, is_network_ioc, refang_ioc) and
a refang_iocs=True constructor flag. When on, the client refangs the
URL/domain/IP inputs of search_url, search_by_metadata (ips/urls/domains,
never the free-form query), search_by_ioc, check_known_hosts, the
known-host writes, and every URL submission path. Live values are sent
unchanged; refang_iocs=False sends inputs verbatim.

The rules, gate and case table are shared with other PolySwarm clients;
test/fixtures/refang_cases.json is kept byte-identical across them.
Python's case-insensitive flag folds Unicode (U+212A KELVIN SIGN matched
"k", so "evil[.]Kom" was refanged), \s/\S cover Unicode whitespace, and
"$" matches before a trailing newline - all unlike the other clients
that implement the same contract. Spell letters as explicit [aA]
classes, use the ASCII whitespace set, trim only that set, and
fullmatch the network-IoC pattern. The shared case table grows to 39
cases pinning these.
submit already skipped the refang for preprocessing type qrcode, whose
argument names an image file rather than a URL; sandbox_file did not, so
a file called qr[.]png was sent as qr.png. Pin both paths on both
transports, cover the async opt-out of the known-host writes and the
bare-string form of the list arguments, and put the httpx_kwargs
paragraph back under the constructor block in the contract spec.
@claude

claude Bot commented Sep 22, 2026

Copy link
Copy Markdown

Reviewed against AGENTS.md and specs/00–05. Verified: sync mirror matches the canonical async at every refang site, the pinned digest (78359d1a…) matches test/fixtures/refang_cases.json, every case in the table holds against the rules as written (including the fullmatch/trailing-\n, KELVIN-SIGN and IPv6-literal guards), the refang sites are exactly the set spec'd in 05-downstream-contract.md §"Where the client applies it", gitflow base is develop, and the bump emits a clean 4.6.0 in both pyproject.toml and __init__.py — correct under the standing downstream-floor exception.

1. Spec drift — specs/05-downstream-contract.md:66, async constructor signature not updated

The ### polyswarm_api.api block was updated (:52):

def __init__(self, key=None, ..., *, session=None, refang_iocs=True, **httpx_kwargs): ...

but the ### polyswarm_api.aio block (:66) still reads:

def __init__(self, key=None, ..., *, session=None, **httpx_kwargs): ...

PolySwarmAsyncAPI.__init__ does accept refang_iocs (src/polyswarm_api/aio/api.py:56), and the same spec asserts two lines below the sync block that "Public surface is identical to the canonical async version". The published async signature is now under-documented in the file this PR edited. Add refang_iocs=True to the aio block.

2. Low — the live-URL gate only covers scheme-bearing inputs

_LIVE_URL_HOST (src/polyswarm_api/refang.py:66) is anchored on _SCHEME://, so the "a legitimate [.] in a path survives" guarantee holds only for URLs that carry a scheme. search_url('example.com/a[.]b') → refang_text → example.com/a.b, live is None, is_network_ioc accepts (host + path) → the search goes out rewritten, silently missing an artifact whose stored URL really does contain [.]. search_url('https://example.com/a[.]b') is correctly left alone (covered by test_live_values_are_sent_unchanged). Non-blocking — scheme-less URL artifacts are the uncommon case and refang_iocs=False is the escape hatch — but the case table has no scheme-less-path entry either way, so the behaviour is currently unpinned in the shared contract.

Nothing else: the sandbox_file QR branch handing the image path to from_content rather than from_path predates this PR (submit uses from_path); the new code only gates the refang around it, which is right.

@vhmartinezm

Copy link
Copy Markdown
Contributor Author

Addressed in 95682a7:

  1. specs/05-downstream-contract.md: the polyswarm_api.aio constructor block now carries refang_iocs=True.
  2. Scheme-less paths: fixed at the contract level rather than special-cased. The gate now drops any rewrite that keeps the input's scheme and host intact, with or without a scheme, so example.com/a[.]b survives exactly like https://example.com/a[.]b. A defanged scheme (hxxp://example.com/a[.]b) still refangs the whole value. The shared case table gains three rows pinning this (42 cases), and the digest pin moved with it. Every client that shares the table was updated in the same change.

@claude

claude Bot commented Sep 22, 2026

Copy link
Copy Markdown

Review — refang defanged IoC inputs

The refang.py implementation is sound: I walked the rule order, the _SCHEME_AND_HOST gate and every row of refang_cases.json by hand and they agree (including the [/]-after-live-host, xn--, KELVIN / LONG-S, NBSP-trim and IPv6-bracket rows). Specs 00/01/03/04/05 are updated in-PR, base is develop, and the version string is a clean 4.6.0 in both pyproject.toml locations and __init__.py. Four comments.


1. refang_iocs=True default contradicts the minor-bump row in specs/05-downstream-contract.md

From the Versioning table:

| New optional keyword argument on an existing method, with a default that preserves the current behaviour | minor … |
| Behaviour change on a documented contract | major |

refang_iocs=True is the additive-keyword row without the qualifier that row requires — the default does not preserve current behaviour. The PR body argues the change only affects inputs that were defanged network IoCs, which previously could never match or produced a broken URL artifact. That holds for search_url / search_by_ioc / search_by_metadata, but not for the two paths that write:

  • add_known_good_host / add_known_bad_host / update_known_good_host store whatever host string they are handed. A catalogue populated by 4.5.0 straight from report text — rows stored literally as evil[.]com — becomes silently unreachable after a pip install -U: check_known_hosts(domains=[...]) given that same defanged string now sends domain=evil.com and returns nothing, and a re-add writes a second, different row. Nothing errors; the consumer just sees empty results.
  • submit(..., artifact_type=URL) and sandbox_url now create a different artifact — different uploaded content, different name, different sha — than the same call did on 4.5.0. A consumer that keyed a stored id or sha to the defanged string it submitted no longer round-trips.

This wants an explicit maintainer decision rather than being folded into a minor: either default refang_iocs=False (then the minor row genuinely applies and the CLI opts in), or take the major bump. Flagging rather than prescribing — but 4.6.0 plus default-on is the one combination the versioning table does not support.

2. The QR guard in submit still uses preprocessing["type"]

src/polyswarm_api/aio/api.py:1409 — if preprocessing and preprocessing["type"] == "qrcode": — while the guard added next door in sandbox_file (aio/api.py:1517) uses preprocessing.get("type"). Pre-existing, but this PR now asserts parity between the two in specs/03-endpoints.md ("A QR-code submission … is the exception on both submit and sandbox_file"), and on submit any preprocessing dict without a type key raises KeyError before the refang branch is reached. One-character fix; worth taking here so the spec sentence is true.

3. Test coverage: the uploaded-content assertion is sync-only

test/refang_test.py:257 — test_uploaded_url_content_is_refanged is parametrised over all three submission methods but only builds a _sync_client(). The artifact_name assertions cover both transports; the content, which is what the server stores as the URL artifact (per the comment on line 259), is pinned only on the generated mirror, never on aio/api.py, the canonical source. There is also no opt-out twin asserting the uploaded content is raw under refang_iocs=False — the opt-out is checked via artifact_name only, so a refang applied to the content but not the name (or the reverse) passes today for that flag state.

4. Minor: the request-shape harness bypasses the transport

_sync_client / _async_client monkeypatch the private _paginate / _single. specs/04-testing.md invariant 7 does bless the pure-unit tier for request-shape assertions, so this is not a violation — but test/_client_harness.py already gives dual-transport coverage with mock.last_request_url for exactly the query-composition assertions in SEARCH_CALLS, and it exercises the real path down to httpx. A sentence in the module docstring on why the private-method capture was chosen instead would stop the next reader treating it as a third pattern to copy.

A default-on refang changes what the write paths store (known-host rows,
URL submissions), which the versioning table only allows in a major.
With refang_iocs=False as the default the new keyword is purely additive;
the CLI opts in through its own option. Also read the QR preprocessing
type with .get() in submit, as sandbox_file already does, and cover the
uploaded URL content on both transports with its opt-out twin.
@vhmartinezm

Copy link
Copy Markdown
Contributor Author

Addressed in 1647bdd:

  1. Versioning: agreed that default-on is not additive, since it changes what the write paths store. refang_iocs now defaults to False, so 4.6.0 stays in the minor row of the versioning table and 4.5.0 behaviour is unchanged unless a consumer opts in. The CLI opts in through its own --refang/--no-refang. The specs (03, 05) and the PR body now say why, including the known-host and submission consequences a consumer accepts when it opts in. New tests pin the default on both clients.
  2. submit now reads the QR preprocessing type with .get("type"), matching sandbox_file.
  3. The uploaded URL content is now asserted on both transports, each with a refang_iocs=False counterpart that asserts the raw content.
  4. The module docstring now explains why capture happens at _paginate / _single rather than through test/_client_harness.py.

Full suite: 381 passed. The previous run's e2e failure was test_search_by_ioc getting HTTP 500 from the server because its search cluster returned 503 search_phase_execution_exception (the request carried a plain, un-refanged IP). That is infrastructure, not this change; the commit before it passed the same e2e.

@claude

claude Bot commented Sep 22, 2026

Copy link
Copy Markdown

Reviewed against AGENTS.md + specs/00,01,03,04,05. Clean against the documented conventions — base is develop, the 4.6.0 bump is correctly classified (new keyword arg with a behaviour-preserving default → minor row of the versioning table) and lands here under the standing downstream-floor exception, the emitted string is a clean 4.6.0 in both pyproject.toml and __init__.py, refang.py is pure/no-I/O and correctly excluded from unasync, and every touched spec is updated in the same PR.

I hand-traced all 42 rows of refang_cases.json through _RULES and the four gates in refang_ioc (rule ordering [://] → [:] → [/] → [.]/[dot] → anchored schemes; the _SCHEME_AND_HOST prefix gate; fullmatch); they are self-consistent, including the ones that are easy to get wrong (evil.com[/]payload rewritten vs example.com/a[.]b preserved, the NBSP row, the U+212A/U+017F rows, the trailing-newline row). The call sites all refang before the value is read twice (artifact_name=artifact_name or artifact picks up the refanged string in submit / sandbox_file / sandbox_url), and _refang(None) passes through, so the ip=/domain= defaults are safe.

Two non-blocking notes, neither a bug:

  1. src/polyswarm_api/aio/api.py:1410 (and its mirror src/polyswarm_api/api.py:1741) changes preprocessing["type"] to preprocessing.get("type"). That is a real behaviour change outside the PR's stated scope: submit(..., artifact_type=URL, preprocessing={'foo': 1}) used to raise KeyError and now falls into the URL-content branch (and gets refanged). It is the right behaviour and matches the new sandbox_file guard, but it is not mentioned in the PR body or in specs/03-endpoints.md — worth a line so it is not read as pure refactor noise.

  2. _refang_all's isinstance(values, str) branch only does something useful for check_known_hosts (params={'ip': ips} passes the string straight through). For search_by_metadata, Metadata._get_params iterates the value (params.extend(('ips', v) for v in ips)), so a bare string was already exploded one character per param before this PR and still is — the branch refangs it and then it is shredded anyway. Pre-existing, and test/refang_test.py correctly only pins the string form on check_known_hosts; just flagging that the helper implies a tolerance search_by_metadata does not actually have.

@vhmartinezm

Copy link
Copy Markdown
Contributor Author

Thanks. Both notes are now in the PR body under "Two side notes", with no code change: the .get("type") behaviour change (a preprocessing dict without type now takes the URL-content branch instead of raising KeyError), and the fact that the bare-string form only helps check_known_hosts, because search_by_metadata's per-character iteration of a string predates this PR.

@vhmartinezm
vhmartinezm requested review from mjbradford89 and sbneto and removed request for mjbradford89 September 22, 2026 22:13
@mjbradford89

Copy link
Copy Markdown
Contributor

Summary

Lets an analyst paste an indicator straight out of a threat-intel report. A defanged URL, domain or IP (hxxps[:]//evil[.]com, 127[.]0[.]0[.]1) is rewritten to its live form before the request is built, so a search finds the artifact instead of silently missing it and a submission creates the real URL rather than a broken one. A new pure module does the rewrite behind a deliberately narrow gate; the SDK keeps it opt-in so 4.6.0 stays a minor bump, and the CLI opts in by default behind --refang/--no-refang. 2 PRs on ioc-refang; 26 files, +1032/−31 across the set.

Severity: 0 HIGH · 1 MODERATE · 1 LOW.
Prior feedback: all 21 points checked and addressed (2 answered with a disposition that stands).
Objective: met — refang defanged URL/domain/IP inputs before the request is built, measured against the PR bodies.

  • drift: submit now reads the QR type with .get("type"), so a preprocessing dict without type takes the URL branch instead of raising KeyError — beyond the stated scope, documented, works

Fixes are proposed, not applied; nothing was run.

Cross-repo coordination

Member PR State Role
polyswarm-api #327 OPEN SDK ← you are here
polyswarm-cli polyswarm/polyswarm-cli#274 OPEN CLI

Merge order: polyswarm-api#327 → polyswarm-cli#274 — the CLI's floor names 4.6.0, and only this branch declares it.

Contracts crossing the set: none carries a finding. refang_iocs=, polyswarm_api.refang.refang_ioc and the public api.refang_iocs attribute are all consumed by the CLI, and all three match.

Coherence: fixes are independent; no cross-repo adjustment needed.

Worth knowing: a third client — the web UI — implements this same contract on its own branch, with a byte-identical refang_cases.json and the same pinned digest. So a refang rule change is a three-repo change, and no branch-name scan starting from these two PRs will surface it.

Findings (round 1)

Every finding below is work for this change set; each entry's Lands in: names the repo whose PR carries the fix, on the branch name every member already shares.

[MODERATE] F1. QR-code sandbox submissions upload the file path, not the image

What happens: An SDK caller submitting a QR-code image through sandbox_file uploads the six characters qr.png as the artifact body instead of the PNG. The server's QR preprocessing opens that body as an image, rejects it as an unrecognised image format, and the task ends FAILED. The same call through submit works, so the parity this PR's specs now claim does not hold.

When: Any sandbox_file(..., artifact_type=URL) call with a string artifact and preprocessing={'type': 'qrcode'} — the flow the new inline comment and two new spec sentences describe as supported. refang_iocs is irrelevant; the new guard only suppresses refanging.

Why:

  • The URL branch calls from_content on the raw string on every path including the new qrcode one — the guard skips only the refang (api.py:1522)
  • from_content encodes the string into a BytesIO, so the path text becomes the uploaded body (resources.py:701)
  • submit's qrcode branch — the method the new comment says shares the rule — opens the file with from_path (api.py:1411)
  • The PR newly asserts the exception holds "on both submit and sandbox_file: its argument names an image file" (03-endpoints.md:267)

Lands in: polyswarm-api
Pre-existing, exposed here — the from_content call predates this PR; the guard, the comment, the two spec sentences and two tests pinning from_content are new. The CLI never reaches it: sandbox url --qrcode-file goes through sandbox_url, which uses from_path correctly.

Proposed fix (untested): Mirror submit's qrcode branch — from_path when the type is qrcode, _refang + from_content otherwise. Edit only the canonical async source and run scripts/regenerate_sync.py, since the mirror carries # DO NOT EDIT and CI rejects a stale one. In the same PR, flip the two sandbox_file spies in test/refang_test.py from from_content to from_path, drop the "(its URL branch has no path reader)" clause, and make both spec sentences say the QR argument is read from disk on both methods.

            elif artifact_type == resources.ArtifactType.URL:
                if preprocessing and preprocessing.get("type") == "qrcode":
                    # A QR-code submission's argument is an image path, not a URL:
                    # read from disk, never refanged (same rule as ``submit``).
                    artifact = resources.LocalArtifact.from_path(
                        self, artifact, artifact_type=artifact_type, artifact_name=artifact_name
                    )
                else:
                    artifact = self._refang(artifact)
                    artifact = resources.LocalArtifact.from_content(
                        self, artifact, artifact_name=artifact_name or artifact,
                        artifact_type=artifact_type,
                    )
  • [LOW] F2. refang.py's 27-line module docstring restates what specs/05-downstream-contract.md §"IoC refanging" already says — the why-refang paragraph, the cross-client invariant and the out-of-scope list — and carries no reference back to it, so the two drift independently (the org rule is that the same rationale appears in one spec, referenced, not at two sites) (refang.py:1). Fix (untested): keep the portability constraints — "why an unusual construct is deliberate" is exactly what an inline comment is for, and they are what stops a future editor reaching for re.IGNORECASE — cut the three duplicated blocks, and close with one line pointing at the spec section.

Standards conformity

Change-level: §15 (comment the fact, specify the design) → F2.

Project-level: no non-clean rows for this repo.

polyswarm-api — Clean: §2 (shared CI template, build/e2e/release all extend it), §14 (delivery order), §16 (cross-repo version pin: the floor covers the surface, no runtime probes, clean 4.6.0 with no dev suffix, release order stated). Not applicable: §3–§13, §17.

Set-level: §14 clean — one externally-facing capability rather than a layer, and no UI change merging ahead of it. Branch-name identity clean: ioc-refang is byte-identical and tag-safe across both members whose CI resolves companions by name. ## Requires clean — the CLI links this PR; this PR depends on nothing in the set.

@mjbradford89 mjbradford89 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed. Open: 1 MODERATE (F1 — sandbox_file's QR-code branch uploads the image path as the artifact body instead of the file; pre-existing behaviour that this PR newly documents as working) and 1 LOW (F2). Details in the review comment above.

@vhmartinezm
vhmartinezm merged commit d97314d into develop Sep 23, 2026
2 checks passed
@vhmartinezm
vhmartinezm deleted the ioc-refang branch September 23, 2026 17:23
@sbneto

sbneto commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Lets an analyst paste an indicator straight 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, the reference), the CLI
(on by default) and the web UI's search bars and upload card. 3 PRs on two branch names; 40 files,
+1733/−34 across the set; SDK and CLI already merged.

Severity: 0 HIGH · 2 MODERATE · 4 LOW. Prior feedback: 23 checked · 2 open.
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 searches defanged values verbatim → F1
  • 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) ← you are here — polyswarm-cli#274 merged · CLI — the web UI's PR (private repo) open · UI

Merge order: polyswarm-api#327 → polyswarm-cli#274 → the web UI's PR, already honoured (§14: SDK and CLI before UI).
F2, F3 and this repo's half of F4 need a new PR from a re-created ioc-refang against develop, merged before 4.6.0 is released (master is still 4.5.0). The web UI doesn't wait for it.

Surface Producer Consumer
refang_cases.json + rule set (standalone run: both implementations agree on all 250,042 generated inputs) polyswarm-api the web UI → F4, F6

Coherence: the fixes are independent across repos. The web UI's three LOW fixes share two passages there and are applied together.

Findings (round 2)

Every finding below is work for this change set; each entry's Lands in: names the repo whose PR carries the fix.

[MODERATE] F2. QR-code sandbox_file submissions upload the file path, not the image

What happens: An SDK caller who sandboxes a QR-code image through sandbox_file uploads the text of the file's path instead of the image. The server creates the sandbox task, then fails it as "Image format not recognized", though the image is valid. The same call through submit works.
When:

  • sandbox_file gets a path string with artifact_type=URL and preprocessing={'type': 'qrcode'}.
  • A file handle takes another branch and works; refang_iocs makes no difference.

Why:

  • In the QR case the URL branch still hands the raw string to from_content; the new guard only skips the refang (api.py:1522).
  • from_content UTF-8-encodes that string into the upload body (resources.py:703).
  • submit's QR branch reads the file with from_path instead (api.py:1410).
  • The new tests spy on from_content, pinning the path-as-body behaviour (refang_test.py:394).
  • Unlike round 1's proposal, no spec edit is needed: both specs already call the argument an image file (round 1).

Lands in: polyswarm-api
Pre-existing, exposed here: the from_content call dates from 3.9.0. This PR added the guard, the spec sentences and the tests that now pin it. The CLI never reaches it, because sandbox url --qrcode-file goes through sandbox_url.
Proposed fix (untested): Mirror submit's QR branch in aio/api.py (lines 1517–1525) and regenerate api.py with scripts/regenerate_sync.py. Point the sandbox_file QR spies in test/refang_test.py (395, 403) and their comment at from_path. Observable effects: the default artifact name becomes the file's basename, as in submit, and a missing path fails before any task is created.

            elif artifact_type == resources.ArtifactType.URL:
                # A QR-code submission's argument is an image path, not a URL
                # (same rule as ``submit``): read the image, never refang it.
                if preprocessing and preprocessing.get("type") == "qrcode":
                    artifact = resources.LocalArtifact.from_path(
                        self, artifact, artifact_type=artifact_type, artifact_name=artifact_name
                    )
                else:
                    artifact = self._refang(artifact)
                    artifact = resources.LocalArtifact.from_content(
                        self, artifact, artifact_name=artifact_name or artifact,
                        artifact_type=artifact_type,
                    )
  • [LOW] F3. refang.py's module docstring repeats three blocks of its spec section with no pointer back, so the two copies drift apart (§15) (refang.py:1). Fix (untested), in polyswarm-api: keep the portability paragraph, cut the why-refang, cross-client and out-of-scope blocks, and end with a line naming specs/05-downstream-contract.md §"IoC refanging".
  • [LOW] F4. The drift-guard comment says a one-sided edit of refang_cases.json fails CI until both copies change, but each suite pins only its own copy (refang_test.py:32). Fix (untested), here and in the web UI's copy of the same comment: say the pin catches an edit to this copy made without updating its pin, such as a formatter rewrite. Say too that keeping the other copy identical is manual.

elsewhere: F1, F5, F6 → the web UI's PR (private repo), where the other half of F4 also lands

Outstanding review feedback

Status Raised The ask Disposition
not addressed #327, round 1 QR sandbox_file uploads the path, not the image → F2
not addressed #327, round 1 trim refang.py's docstring to point at the spec → F3

Standards conformity

The round 1 audit stands. There is no new change-level violation: F3 is the §15 item it already recorded.
Set — Clean: §14 (one capability, three clients, SDK and CLI merged first). Rule 6 name identity isn't load-bearing here, because no CI seam resolves the web UI against the SDK or CLI.

@vhmartinezm

Copy link
Copy Markdown
Contributor Author

F2, F3 and this repo's half of F4 are addressed in #328 (re-created ioc-refang → develop), ahead of the 4.6.0 release: sandbox_file now reads the QR image with from_path, the refang.py docstring points at its spec section, and the drift-guard comment, the test name and the spec sentence say the pin guards only its own copy.

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