fix: enhance client-side validation for token selector to prevent bypass - #175
Merged
Jaydbrown merged 3 commits intoJul 29, 2026
Conversation
Contributor
|
The validation logic itself is solid — real
Happy to get this in once it's rebased and cleaned up. |
- Added StrKey.isValidContract() validation for token (C-address) fields - Added StrKey.isValidEd25519PublicKey() validation for sender/recipient (G-address) fields - Added amount positivity validation in validatePayload() - Updated StreamBuilder._validateAddress() with address format checks - Added comprehensive test coverage for validation bypass scenarios
…d addresses
Adding real StrKey validation (this PR's whole point) correctly started
rejecting the placeholder-style addresses ('CD...', 'CD1', and a couple of
malformed 55-char strings) that pre-existing tests across this repo used as
stand-ins for real C-/G-addresses. Replaced them with properly checksummed,
valid addresses so the existing suite passes under real validation instead
of silently relying on validation being a no-op.
Also updated pagination-cursor.test.ts's 'returns a nextCursor' test to mock
a full page of results — it was asserting the old hasNextPage semantics
(compared against the global streamCount()) that conduit-protocol#179/conduit-protocol#183 already replaced
with the correct ids.length === limit check.
Removed a duplicate, unused maxBatchSize declaration in builder.ts (a lint
error introduced by this branch's own diff).
Jaydbrown
force-pushed
the
blackboxai/fix-token-selector-validation
branch
from
July 29, 2026 09:00
0cd1305 to
6ae5df2
Compare
Jaydbrown
added a commit
that referenced
this pull request
Jul 29, 2026
- governor.ts: parseGovernorConfig assigned 'undefined' directly to optional GovernorConfig fields, which exactOptionalPropertyTypes (true in tsconfig) rejects — a key must be entirely absent, not present with value undefined. Switched to conditional spreads. Regression from #231. - builder-validation-bypass.test.ts: the fixture-address fix from #175 never actually landed — I'd edited this file locally but forgot to stage it before that PR's final commit, so the squash-merge only included the earlier (broken-placeholder-address) revision. Reapplying the same fix now: replace the invalid 55-character StrKey strings with genuinely valid, checksummed addresses. - walletconnect.test.ts: 'times out a stalled remote disconnect' (#241) left its returned promise unobserved for one microtask tick under fake timers, producing a 'handled asynchronously' unhandled-rejection warning that vitest sometimes misattributes to an unrelated test in the same run. Attach a no-op .catch() immediately to keep it always-handled; behavior/assertions are unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Type of change
Related issue
Changes
Checklist
npm run typecheck— no errorsnpm run lint— no warningsnpm test— all tests passnpm run build— bundle compiles cleanlyanytypes introduceddocs/api.mdbigint— noNumber()conversion in arithmeticsrc/tests/CHANGELOG.mdupdated under[Unreleased]src/index.tsupdated if new exports addedBreaking changes?
BREAKING CHANGE:footer to relevant commitNotes for reviewers
Closes #104