Skip to content

fix(js): validate reply prefix in decodeResult#1315

Open
ukint-vs wants to merge 5 commits intomasterfrom
fix/js-decoderesult-header
Open

fix(js): validate reply prefix in decodeResult#1315
ukint-vs wants to merge 5 commits intomasterfrom
fix/js-decoderesult-header

Conversation

@ukint-vs
Copy link
Copy Markdown
Member

Summary

  • v2 SailsProgram decodeResult and v1 Sails decodeResult now validate the reply's Sails header / (service, method) prefix before SCALE-decoding, matching the existing behavior of decodePayload.
  • Previously, passing headerless or mismatched reply bytes silently produced garbage (or a cryptic codec error); now both throw a clear Invalid Sails header for … / Invalid prefix for … result error.
  • Documents the long-existing sails-js/types, sails-js/parser, and sails-js/util subpath exports in both READMEs so downstream tooling can import ISailsTypeDef / ISailsPrimitiveDef instead of falling back to any casts.

Why

decodePayload, event decode(), and ctor decodePayload() already validate the method prefix — see _assertMatchingHeader at js/src/sails-idl-v2.ts and the getServiceNamePrefix / getFnNamePrefix checks at js/src/sails.ts. decodeResult was the only public decode entry point that skipped this validation. The asymmetry is easy to miss because internal builders (e.g. QueryBuilderWithHeader) strip the prefix themselves via result.payload.slice(this._prefixByteLength), so decodeResult is only hit by external consumers — which is exactly where a clear error matters most.

This came up while integrating sails-js into a downstream wallet / tooling stack: passing a reply payload that had already been stripped by an intermediate layer to decodeResult returned plausible-looking but wrong values instead of throwing.

Behavior change

Callers who were (deliberately or accidentally) relying on decodeResult accepting prefix-less bytes will now see a thrown error instead of garbage output. Given the prior behavior produced undefined/garbage results rather than anything semantically useful, this should be considered a bug fix rather than a breaking change — but flagging it explicitly for reviewers.

Test plan

  • New non-live unit tests in js/test/idl-v2-parser-type-resolver.test.ts covering the v2 error paths (bogus 16-byte prefix without magic bytes → Invalid Sails header; prefix whose entry_id belongs to a different method → Header mismatch).
  • New unit test in js/test/encode-decode.test.ts covering the v1 happy path and the mismatched-prefix error path.
  • pnpm jest idl-v2-parser-type-resolver — all 11 tests pass
  • pnpm jest encode-decode — all 3 tests pass
  • pnpm lint — no issues
  • npx prettier --check on edited files — clean

Not covered here (deliberately deferred)

  • Package version coherence (e.g. js/package.json is still at 0.5.1 despite the js/v1.0.0-beta.1 git tag; sails-js-parser standalone vs the sails-js/parser subpath naming overlap). This is a release-cadence / publishing decision rather than a code fix, and should be handled as part of the 1.0 bump rather than dragged in here.

🤖 Generated with Claude Code

`decodePayload` already verifies the Sails header (v2) or the
`(service, method)` prefix (v1) before SCALE-decoding, but `decodeResult`
didn't — it silently interpreted the first 16 bytes (v2) or the first
two compact-length-prefixed strings (v1) without checking they matched
the method being decoded. Passing headerless or mismatched reply bytes
therefore produced garbage or a cryptic SCALE error rather than a clear
"wrong method" signal.

`decodeResult` now calls the same `_assertMatchingHeader` helper as
`decodePayload` in v2, and compares the service/function name prefixes
against the closed-over method identity in v1.

Also documents the `sails-js/types`, `sails-js/parser`, and
`sails-js/util` subpath exports in both READMEs — they were wired in
`package.json` but never mentioned in the docs, so downstream tooling
has been falling back to `any` casts instead of importing the
`ISailsTypeDef` / `ISailsPrimitiveDef` interfaces.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces validation for the reply prefix in decodeResult for both Sails (v1) and SailsProgram (v2) to ensure the data matches the expected service and method. It also documents subpath exports (sails-js/types, sails-js/parser, and sails-js/util) in the README files. Review feedback highlights that the changelog's claim regarding consistency with v1's decodePayload is inaccurate, as decodePayload and event.decode in v1 still lack similar validation. Additionally, it is noted that the sails-js/parser subpath documentation is missing from the v1 README.

Comment thread js/CHANGELOG.md Outdated
Comment thread js/README.md
Comment thread js/src/sails.ts Outdated
ukint-vs and others added 3 commits April 23, 2026 00:35
Previously the only decodeResult coverage in sails-idl-v2 was the two
error-path tests. Add a happy-path test that extracts a method's real
16-byte header from `encodePayload(...)`, uses it as the reply prefix,
and verifies the decoded return value — this protects against the
header-validation change silently breaking the valid path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When reply bytes are too short for the service/function prefix,
getServiceNamePrefix / getFnNamePrefix throw from the compact SCALE
codec with an opaque message. Catch that and re-throw with the same
`Invalid prefix for <service>.<method> result` shape the caller already
sees for mismatched prefixes, so downstream consumers only have to
handle one error surface.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses review comments on #1315:

- v1 decodePayload (both function and constructor) and event.decode
  previously skipped prefix validation, so the gap was wider than the
  CHANGELOG originally claimed. Extract `_assertMatchingServicePrefix`
  and `_assertMatchingCtorPrefix` helpers and apply them to every v1
  decode site for parity with v2's `_assertMatchingHeader` coverage.
- Correct the CHANGELOG wording to accurately describe the fix as
  covering every v1/v2 decode entry point rather than claiming parity
  with a `decodePayload` that was also missing validation.
- Document the `sails-js/parser` subpath in the v1 README with a note
  clarifying it's the IDL v2 parser; v1 users still import from the
  standalone `sails-js-parser` package.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ukint-vs
Copy link
Copy Markdown
Member Author

Addressed all three review comments in c70556a (plus a3fcd7d for the earlier truncated-bytes polish):

Comment Fix
"decodePayload in v1 does not currently perform this validation — inconsistency that should be addressed" v1 decodePayload (function + constructor) and event.decode now go through shared _assertMatchingServicePrefix / _assertMatchingCtorPrefix helpers, matching v2's _assertMatchingHeader coverage.
"Changelog's claim regarding consistency with v1's decodePayload is inaccurate" CHANGELOG rewritten to describe the fix as covering every v1/v2 decode entry point rather than claiming parity with an already-validated decodePayload.
"sails-js/parser subpath missing from v1 README" Added, with a note clarifying it's the IDL v2 parser; v1 users still import from the standalone sails-js-parser package.

Coverage bumped to 7 tests for v1 (3 existing + 4 new validation tests across decodePayload/decodeResult/event.decode/ctor decodePayload) and 12 for v2. Lint + Prettier clean.

Two follow-ups from the adversarial review on #1315:

1. Accept Uint8Array in _assertMatchingServicePrefix. Callers previously
   doing `fn.decodeResult(message.payload as any)` or
   `event.decode(message.payload as any)` with Uint8Array worked at runtime
   because registry.createType accepts it. The new guard went through
   getServiceNamePrefix/getFnNamePrefix, which expect hex strings, so those
   `as any` callers regressed. Normalize via u8aToHex up front, matching
   the ctor helper.

2. Sanitize echoed prefix strings in error messages. The `got <service>.<fn>`
   text was interpolated verbatim from untrusted reply bytes, so a malformed
   or malicious payload could inject control characters / ANSI escapes into
   logs and error sinks. Truncate to 64 chars and replace non-printable
   bytes with "?" before echoing.

Tests cover both: a Uint8Array round-trip and a control-character prefix
that verifies ESC/BEL are stripped from the thrown message.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ukint-vs
Copy link
Copy Markdown
Member Author

Ran /review with Codex adversarial pass. Codex flagged three things; two applied, one deferred:

Applied (commit 4441368):

  1. HIGH: v1 Uint8Array regression. The new _assertMatchingServicePrefix went through getServiceNamePrefix/getFnNamePrefix, which only accept hex strings — but registry.createType accepts Uint8Array too, so callers doing fn.decodeResult(message.payload as any) with bytes worked before and broke after this PR. Fixed by normalizing via u8aToHex up front, matching the existing ctor helper pattern. Added a Uint8Array round-trip test.

  2. MEDIUM: error-message log injection surface. The got ${actualService}.${actualFn} text echoed attacker-controlled bytes verbatim — a malformed reply could smuggle ANSI escapes / control chars into log sinks. Added a _safeEchoedName helper that truncates to 64 chars and replaces non-printable ASCII with ?. Test verifies ESC/BEL are stripped.

Deferred:

  1. MEDIUM: _assertMatchingHeader doesn't check routeIdx. Pre-existing design — all v2 decode sites share this behavior, not something this PR introduced. Worth a separate issue if cross-route reply confusion is a real risk for multi-mounted services, but out of scope here.

Updated coverage: v1 9/9 tests pass (2 new), v2 12/12 pass. Lint + Prettier clean.

@ukint-vs
Copy link
Copy Markdown
Member Author

Filed the deferred route_idx follow-up as #1316.

@vobradovich vobradovich requested a review from osipov-mit April 24, 2026 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants