Skip to content

fix(strkey): bound decodeCheck input length before base32 decode - #1583

Merged
Ryang-21 merged 4 commits into
v17-feature-branchfrom
fix/strkey-decode-bounds
Aug 4, 2026
Merged

fix(strkey): bound decodeCheck input length before base32 decode#1583
Ryang-21 merged 4 commits into
v17-feature-branchfrom
fix/strkey-decode-bounds

Conversation

@Ryang-21

@Ryang-21 Ryang-21 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What

decodeCheck now validates the encoded string's length against the requested strkey type before it decodes anything. Each type has a legal encoded length — 56 characters for G/S/T/X/C/L, 58 for B, 69 for M, and a 56–165 range for the variable-length P — and an input outside that range throws invalid encoded string length immediately. The unknown-version-byte-name check moved up alongside it, so a bad type name is also rejected before any decoding work. The per-type length table lives in a new encodedLengthRange helper; isValid no longer duplicates it and relies on decodeCheck throwing instead.

Why

The length check used to live only in isValid, so the two ways of reaching decodeCheck directly — calling the exported function, and SignerKey.decodeAddress — had no bound on their input at all. That had two consequences.

First, an attacker-supplied string of arbitrary length drove a full base32 decode plus a canonical re-encode comparison before anything rejected it. A megabyte-long G... string did a megabyte of work to produce an error that its second character already implied.

Second, and worse, a strkey with a valid version byte and a correct CRC16 but a wrong-sized payload decoded successfully and returned a mis-sized buffer. A 37-byte payload encoded as an ed25519PublicKey strkey came back as 37 bytes, and the caller only failed later, downstream, if it happened to check — isValid would have caught it, but nothing on the direct path did. That case now throws.

One consumer-visible consequence: an input that was already invalid may now report a length error where it previously reported a checksum, version-byte, or base32-character error. The input is rejected either way, but anyone matching on the message text will see the new one. Two existing tests asserted the old base32 message for 58-character inputs and were updated.

encodedLengthRange has no default case, and the repo builds with strict and noImplicitReturns, so adding a strkey type will fail the build until the table covers it.

@github-project-automation github-project-automation Bot moved this to Backlog (Not Ready) in DevX Aug 3, 2026
@Ryang-21
Ryang-21 requested a review from Copilot August 3, 2026 20:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Bounds StrKey inputs before Base32 decoding to prevent unnecessary work and malformed payloads.

Changes:

  • Adds per-type encoded-length validation.
  • Adds regression tests and updates error expectations.
  • Updates changelog and generated API references.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/base/strkey.ts Adds encoded-length guards.
test/unit/base/strkey.test.ts Tests guards and valid types.
docs/reference/core-keys.md Updates generated source links.
CHANGELOG.md Documents behavior change.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/base/strkey.ts
@Ryang-21
Ryang-21 requested a review from quietbits August 3, 2026 21:25
@Ryang-21
Ryang-21 merged commit a4bc598 into v17-feature-branch Aug 4, 2026
13 checks passed
@Ryang-21
Ryang-21 deleted the fix/strkey-decode-bounds branch August 4, 2026 17:16
@github-project-automation github-project-automation Bot moved this from Backlog (Not Ready) to Done in DevX Aug 4, 2026
Ryang-21 added a commit that referenced this pull request Aug 10, 2026
* Class XDR Implementation (#1422)

* feat(xdr): codegen tool + schema source

* feat(xdr): add class-based XDR runtime and sep51 JSON walker

* refactor(numbers): drop LargeInt classes, delegate to new XDR layer

* refactor(base): migrate src/base to new XDR layer; drop legacy xdr.ts + generated

* refactor: migrate downstream consumers (bindings/contract/horizon/rpc/webauth)

* feat(base/scval): add bool to ScValType

* allow opaque xdr types to be initalized via string

* refactor xdr strings to be represented soley via bytes with a dx friendly XdrString wrapper class

* generate a value getter function for void union cases

* add a is() function to the generated XDR union classes for instanceOf checks

* feat(xdr): regenerate schemas against @stellar/js-xdr and add CAP-71 credentials

* feat(xdr): wire the toJSON hook so JSON.stringify emits SEP-0051

* fix(bindings): emit Uint8Array for bytes/bytesN to match scValToNative

* refactor(contract): rename fromJSON to fromJson with deprecated aliases

* feat(xdr): accept ASCII asset codes with zero padding in constructors

* Migrate public API from Buffer to Uint8Array (#1564)

* feat(base): migrate crypto and strkey APIs to Uint8Array

* feat(base)!: migrate value types to Uint8Array

* feat(base)!: migrate transactions, operations, and auth to Uint8Array

* feat!: migrate contract, rpc, and webauth layers to Uint8Array

* build!: drop buffer polyfill and dependency

* fix(horizon): type manage_data value as string to match runtime API

* fix(xdr): emit SEP-51 key `type` instead of Rust-escaped `type_` (#1571)

* build(xdr): regenerate xdr.json via docker from pinned stellar-xdr commit (#1575)

* build(xdr): regenerate schema from stellar-xdr with CAP-83 and CAP-85 ungated (#1576)

* build(xdr): regenerate schema from stellar-xdr with CAP-83 and CAP-85 ungated

* fix(xdr): keep consumers compiling against the regenerated union arms

* fix(vitest): isolate browser dep cache per transport

* feat(xdr): support CAP-83 empty tx set values and CAP-85 external executables (#1577)

* build(xdr): fail the schema download instead of masking it in a pipe

* feat(xdr): support CAP-83 and CAP-85 protocol values

* fix(xdr): bound decimal string length before BigInt parse in JSON decode (#1581)

* fix(xdr): bound decimal string length before BigInt parse in json decode

* refactor(xdr): name the digit-budget constants in bigint-parts

* fix(xdr): restrict fromJson to SEP-0051 keys and reject unknown fields (#1582)

* fix(xdr): restrict fromJson to SEP-51 keys and reject unknown fields

* fix(test): correct horizon corpus fixture path so corpus tests run

* fix(strkey): bound decodeCheck input length before base32 decode (#1583)

* fix(xdr): reject AssetCode12 JSON codes shorter than 5 bytes (#1585)

* fix(horizon)!: make TransactionFailedExtras result_codes.operations optional (#1586)

* Fix: signed payload strkey framing (#1588)

* fix(strkey): validate signed payload framing in decodeCheck

* Fix: xdr json decode validation (#1592)

* fix(xdr): throw on unknown union discriminant in fromXdrObject

* fix(strkey): validate the claimable balance discriminant byte

* fix(xdr): reject non-decimal integer strings in JSON decoding

* V17.0.0 rc.1 (#1593)

* chore(release): cut v17.0.0-rc.1

* fix(spec): restore instanceof Map check lost in the v17 merge

* feat(xdr): add validateXdr static to every generated type (#1597)

* feat(xdr): add validateXdr static to every generated type

* fix(contract): declare error classes, make types self-contained (#1627)

* perf(strkey): reject by length and prefix before decodeCheck throws (#1629)

* fix: restore wide-int bounds statics, document Memo.text break (#1628)

* fix(xdr): restore wide-int MIN_VALUE/MAX_VALUE statics

---------

Co-authored-by: Iveta <quietbits@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants