fix(contract): reject control characters in the scan target remote

[rohanpoudel2]

Fixes #231

Problem

validateCanonicalContract validated scan.target.remote against a string that was not the string it stored.

The authority is extracted with:

const authority = /^[A-Za-z][A-Za-z0-9+.-]*:\/\/([^/?#]+)/.exec(remote)?.[1];

[^/?#] matches ASCII tab, LF and CR. The follow-up new URL(remote) check does not catch them either, because the WHATWG URL parser strips exactly those three characters from its input before parsing. So a manifest carrying:

"remote": "https://example.com\nhttps://evil.example.net"

loaded successfully — new URL() saw host example.comhttps, while loadContract handed the original two-line string straight back to callers.

Neither surrounding guard closes it: the schema pattern ^(?![^:/?#]+://[^/?#]*@)[^?#]+$ also matches newlines, and validateParsedJson checks only for well-formed Unicode.

The producer had the same hole from the other side. _validate_remote in finalize_scan_contract.py inspected only what urlsplit returned, and urlsplit strips ASCII tab, LF and CR before parsing exactly like the WHATWG parser does, so that same value split to netloc example.comhttps: and sealed cleanly.

Change

One rule, applied on both halves of the contract.

Loader — sdk/typescript/src/contract.ts rejects C0/C1 control characters and Unicode line separators in remote before the authority regex and the new URL() check run, mirroring the class requireModelSafeOutputDir already applies in runtime.ts for the same reason. That constant is not exported, so it is mirrored locally with a comment naming the source of truth rather than widening runtime.ts's public surface for one caller.

Producer — _bundled_plugin/scripts/finalize_scan_contract.py applies the identical class in _validate_remote, ahead of the urlsplit checks, in the same order the loader uses. Without this half the loader-side fix would turn a smuggled remote into a scan that completes, seals, and then cannot be loaded at all.

The broad class earns its width. Measured against the current code, position matters:

• authority position — new URL("https://exa<c>mple.com") already threw on 64 of the 67 code points in the class. Only TAB, LF and CR slipped through.
• path position — new URL() percent-encodes rather than throwing, so all 67 slipped through before this change, and the raw code point stayed in the stored string. The class is load-bearing there, not merely defence in depth.

Why not require href === remote

I tested this and rejected it. The WHATWG serializer legitimately rewrites valid URLs — it strips a default :443, lowercases mixed-case hostnames, and punycode-encodes internationalized ones — so exact canonical equality would reject real remotes. The bundled producer validates remote only via urlsplit scheme and netloc checks and imposes no canonical-equality requirement, so it can legitimately emit any of those forms.

Why not widen the class further to cover the remaining smuggling characters

Because a character class is the wrong instrument for what is left, and widening it would reject valid remotes.

This change narrows the smuggling alphabet without emptying it. Driving the real loadContract and the real _validate_remote over the Problem section's own payload shape, seven non-control characters still seal and load:

character	new URL().host the validator saw
U+200B ZERO WIDTH SPACE	github.laiyagushi.comhttps
U+FEFF ZERO WIDTH NO-BREAK SPACE	github.laiyagushi.comhttps
U+3002 IDEOGRAPHIC FULL STOP	github.laiyagushi.com.https
U+FF0E FULLWIDTH FULL STOP	github.laiyagushi.com.https
U+FF61 HALFWIDTH IDEOGRAPHIC FULL STOP	github.laiyagushi.com.https
U+2044 FRACTION SLASH	github.xn--comhttps-g03d
U+2215 DIVISION SLASH	github.xn--comhttps-0f7d

Adding them to the class is not available: U+3002 is a legitimate IDN label separator. https://例え。テスト/repo.git and https://例え.テスト/repo.git both resolve to host xn--r8jz45g.xn--zckzah, and the producer accepts both today, so rejecting the ideographic form would reject a valid remote.

Closing the remainder needs a structural rule instead — require the authority's port segment, outside any [...] IPv6 literal, to be digits — which changes the published validation contract on both halves and deserves its own issue and its own argument. Keeping this change to the control-character class keeps it revert-proof and free of false positives.

Impact, stated plainly

remote is not inert once it leaves the contract layer. finalize_scan_contract.py emits it as SARIF versionControlProvenance[].repositoryUri for git_revision targets, and _bundled_plugin/skills/track-findings/SKILL.md instructs the agent to read scan.target from the manifest and "prefer its canonical remote" when resolving a tracking repository. A two-line value that passed validation therefore reached GitHub code scanning and the agent's repository resolution.

Nothing under src/ reads the field, so this closes a broken guarantee rather than a live CLI exploit. Credential smuggling remains correctly blocked — any @ in the authority is still rejected before URL parsing, and that check is untouched.

Upgrade cost is real but narrow. At load, the only verdicts that flip from accept to reject are remotes carrying a C0/C1 control character, U+2028 or U+2029; across a 311-input probe corpus, 73 inputs change verdict and nothing else does. An already-sealed scan carrying such a remote also stops projecting to SARIF, because _load_sealed_contract re-runs _validate_manifest. No form git remote get-url produces is affected.

Verification

Both halves are revert-proof. Run from sdk/typescript:

state	bun test --timeout 30000 ./tests-ts/contract.test.ts
this branch	31 pass / 0 fail, 132 expect() calls
loader guard deleted from contract.ts	29 pass / 2 fail
producer guard deleted from finalize_scan_contract.py	30 pass / 1 fail

Deleting the loader guard fails seals and loads the same set of remote URLs on expect(await loaderAcceptsRemote(remote)).toBe(false) — Expected: false, Received: true — and also fails the table-driven rejects schema-valid but canonically invalid contract data. Deleting the producer guard fails the parity test on expect(producer[remote]).toContain("canonical absolute URL") with Received: "". The parity test drives the bundled _validate_remote in a subprocess and loadContract in-process over the same 19 remotes, so neither side can drift without failing it, and the 11 accepted remotes (plain HTTPS, trailing .git, punycode and Unicode IDN hosts, explicit :443, a non-default port, a mixed-case host, git+ssh://, ssh://, an IPv6 literal, a percent-encoded path) prove the rule cannot pass by rejecting everything.

Full suite: 740 pass / 5 skip / 0 fail, 5204 expect() calls, 745 tests across 34 files.

node scripts/generate-models.cjs --check and tsc --noEmit are clean. prettier --check reports "All matched files use Prettier code style!". python3 -m py_compile is clean on the changed script; node-ci.yml runs no Python linter, so that is the available check.

Every figure above was measured by running the command, and the residual table in "Why not widen the class further" was measured by driving the real loadContract and the real _validate_remote over the payloads rather than by reading the code.

[mldangelo-oai]

@codex review exact head dcbad0e

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: dcbad0efe3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Security review completed. No security issues were found in this pull request.

Reviewed commit: dcbad0efe3

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

• You comment "@codex security review"
• A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.