fix(parser_app): invoke validate_charset unconditionally on signing path#331
Open
pepe-anchor wants to merge 4 commits into
Open
Conversation
The signing path in `parse()` relied on per-converter
`to_visual_sign_payload_from_string` impls to invoke
`SignablePayload::validate_charset`. Chain-specific overrides (e.g.
the Ethereum converter) skip that wrapper and call
`to_visual_sign_payload` directly, leaving the charset unchecked.
Caller-supplied metadata (Ethereum `abi_mappings`, Solana
`idl_mappings`) flows into displayed `label`/`title`/`text` strings.
Without an unconditional check, a function name like
`transfer\u{202E}approve` (bidi override) reaches the signed payload
verbatim and renders backwards in the wallet UI, masking the
attacker's intent.
Enforce the check in `parse()` after `convert_transaction` and before
serialization. Extract a `parse_with_registry` test seam so unit tests
can register stub converters that emulate the bypassing override and
the default path; both must reject U+202E. Belt-and-suspenders: the
default `to_validated_visual_sign_payload` path also keeps its own
internal call, the new check is defense in depth.
Closes PRS-230
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the parser_app signing boundary by ensuring SignablePayload::validate_charset() is always enforced after transaction conversion, preventing chain-specific converters from accidentally bypassing charset validation and allowing spoofing characters (e.g., bidi controls) to reach wallet-displayed fields.
Changes:
- Refactored
parse()to delegate to a newparse_with_registry(...)helper to allow injecting a customTransactionConverterRegistry(primarily for unit tests). - Added an unconditional
signable_payload.validate_charset()call afterconvert_transaction(...)and before serialization, mapping failures toCode::InvalidArgument. - Added regression tests covering both a validation-bypassing converter and benign ASCII payloads.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two follow-ups from Copilot on PR #331. 1. Don't misclassify server-side bugs as InvalidArgument. validate_charset can return SerializationError if its internal to_json call fails. That's a server bug, not bad client input. Match on the error and route ValidationError to InvalidArgument, everything else to Internal. 2. Make the default-converter-path regression test discriminating. StubConverter previously used the default to_visual_sign_payload_from_string which still calls validate_charset internally, so the test would pass even if the new unconditional check in parse_with_registry were removed. Override to_validated_visual_sign_payload on StubConverter to skip inner validation, so the test now fails closed without the fix. Covers a second bypass surface (validator-method override) distinct from BypassingConverter (from-string override).
Drop redundant `VisualSignError` import in the test module (already in scope from the parent module's `visualsign::errors::VisualSignError` re-export, also reachable via the `visualsign::vsptrait` path). Tighten the failure message on the StubConverter regression test: the converter actually skips its inner `validate_charset` call, so say so rather than the misleading "even when the converter validates internally". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Heads up: this repo is public, so internal Linear refs ( This PR's leaks:
Suggested convention: describe the bug in the name (e.g., |
Removes PRS-NNN references from comments so external readers of this public repo don't see internal triage IDs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Scrubbed PRS-230 refs from body and code/comments. Branch left as-is since it'll disappear on merge. |
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
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.
Summary
Strings drawn from caller-supplied metadata (Ethereum
abi_mappings, Solanaidl_mappings) flow into displayedlabel/title/textfields of theSignablePayload.SignablePayload::validate_charsetexisted but was only invoked insideto_validated_visual_sign_payload, which chain-specific converters can (and do) bypass. The production Ethereum converter overridesto_visual_sign_payload_from_stringto callto_visual_sign_payloaddirectly, leaving the charset unchecked on that path.Result: a function name like
transfer\u{202E}approve(right-to-left override) reaches the signed payload verbatim and renders backwards in the wallet, masking attacker intent.What changed
src/parser/app/src/routes/parse.rs:parse()now callssignable_payload.validate_charset()unconditionally afterconvert_transaction(...)and before JSON serialization. Failures map toCode::InvalidArgument.parse_with_registry(...)test seam so unit tests can inject a stub converter without depending on the production registry.parse_rejects_non_ascii_payload_when_converter_skips_validation— the load-bearing one. Uses a stub converter that mirrors Ethereum's override (skips the default'sto_validated_visual_sign_payloadwrapper). Proves the unconditional check inparse()catches U+202E even when the converter doesn't validate.parse_rejects_non_ascii_payload_via_default_converter_path— same poisoned input through the default-validating converter. Belt-and-suspenders: guards against a future refactor removing the default's own validation.parse_accepts_ascii_payload_when_converter_skips_validation— benign input still parses. Confirms the new check doesn't reject legitimate payloads.This is defense in depth: the existing per-converter validation stays, the new check guarantees the property holds at the signing boundary regardless of converter choice.
Rollback
Revert this commit. No persisted state, no API surface changes (the
parse_with_registryhelper ispub(crate)), no protobuf changes. Safe rollback.Backwards compatibility
No breaking changes. The public
parse(...)signature is unchanged. The validation only adds rejection of payloads that contain non-ASCII characters in displayed strings, which was already the intent ofvalidate_charset; this PR ensures it's actually enforced on every code path.Test plan
cd src && cargo fmt -- --checkcleancargo clippy -p parser_app --all-targets -- -D warningscleancargo clippy -p visualsign --all-targets -- -D warningscleancargo test -p parser_app— 5/5 passing (3 new regression tests + 2 existing)cargo test -p visualsign— 48/48 passing, doc-tests 3/3 passing