feat(near): report refused token metadata as a diagnostic by shahan-khatchadourian-anchorage · Pull Request #439 · anchorageoss/visualsign-parser

shahan-khatchadourian-anchorage · 2026-08-04T23:10:23Z

Turns on the diagnostics feature for visualsign-near and closes the one gap
that needed new code rather than a flag flip: token-metadata entries the parser
refuses are reported to the signer instead of only to an operator log.

Stacked on #437 (near-e-cli-token-metadata).

What was already in place, and what wasn't

visualsign-near declares diagnostics = ["visualsign/diagnostics"], and
render.rs routes every soft finding (signature, account-binding,
deadline, extraction, unverified-token-metadata) through one
feature-aware diagnostic() helper. But nothing enabled that feature: neither
parser_cli's nor parser_app's diagnostics propagated into the crate, so
the structured half of that helper was dead code.

Refused token-metadata entries were the real gap.
try_extract_from_chain_metadata dropped them with a tracing::warn! and
returned only the survivors. A caller supplying metadata the parser then threw
away learned nothing from the payload: the signer saw an amount in raw base
units against an unresolved asset id, or resolved from a seed instead of the
supplied override, and the reason lived in a log the caller cannot read.

Changes

Extraction reports refusals. It returns
TokenMetadataExtraction { registry, rejected }. A reject! macro binds each
refusal to its continue, so a new rejection path cannot be added without
reporting it — all seven existing paths (oversized value, unsigned under
require-signed, unsigned seed override, unlisted signer, malformed value JSON,
decimals out of range, symbol length) are covered by one table-driven test.

Rendering. rejected_metadata_diagnostics emits a new
rejected-token-metadata rule per refused entry, alongside the intents rather
than replacing them — the refusal protects the render, it doesn't invalidate
it. Both halves of the message pass through charset_safe: the asset id is a
caller-controlled map key and the reason can quote it back (a JSON parse error,
a length), so an embedded newline would otherwise render as extra apparent
fields on the signing screen. That is the same class already fixed for
memo/msg/method_name, reached through a different field, and it has its
own regression test.

One registry per conversion. It was built inside decode_intents, once per
action. Since the metadata is request-scoped, a two-action transaction would
have repeated every rejection. Built once in render_on_chain now, with a test
pinning the count.

Feature propagation and CI. parser_cli, parser_cli_core and
parser_app propagate diagnostics to visualsign-near. make test and
make lint gained -p visualsign-near --features diagnostics --lib and the
parser_cli_core equivalent, matching the convention in
docs/contributor-guides/lint-diagnostics.mdx. A near-json CLI fixture pair
(.display.expected + .diagnostics.expected, generated from real output)
covers the rendered form, and the fixture harness's chain skip-list gained the
near entry it was missing, so a --no-default-features build skips it
correctly.

The CLI human view showed diagnostics as Field: Unknown. format_field
had no Diagnostic arm, so every NEAR finding — the pre-existing deadline
and signature ones included — was already invisible in --output human, and
this one would have been too:

   ├─ Field: Unknown

It now prints level, rule, optional instruction index, and message, which fixes
Solana's diagnostics in that view too:

   ├─ [warn] rejected-token-metadata: token metadata supplied for nep141:my-token.near was rejected and not used: invalid value JSON: missing field `symbol`
   ├─ [warn] deadline: deadline has passed; the intents would be rejected

The arm and its two tests are gated on a new parser_cli_core/diagnostics
feature, because visualsign only defines the Diagnostic variant under its
own.

Payload shape

A refused entry adds one field where there was none. With diagnostics off —
parser_app, the production shape HSMs and wallets derive a metadata digest
from — that is a Warning text field, matching how NEAR's other soft findings
already degrade. Requests that supply no metadata, or whose metadata is
accepted, are unchanged.

Docs

lint-diagnostics.mdx gains NEAR in the feature list and CI-invocation
description, plus a per-chain rule table. The NEAR section explains the two
ways NEAR differs from Solana's convention: bare rule names rather than
domain::rule_name, and a Warning-text fallback instead of dropping the
finding when the feature is off.

make lint and make test are green across the workspace in both feature
states.

🤖 Generated with Claude Code

pepe-anchor

Peer Review Summary — NEAR stack #439 (COMMENT)\n\n8 findings: 1 MEDIUM, 2 LOW, 5 INFO. Detailed inline comments below.\n\nKey concern: Diagnostic creation failure aborts entire transaction conversion. All 7 refusal paths correctly use reject! macro.

AI Review on behalf of @pepe-anchor. Please flag any inaccuracies.

pepe-anchor · 2026-08-05T15:49:06Z

                .signable_payload_field,
        );
+        // Built once for the whole transaction: the metadata is request-scoped,
+        // so a rejection is a property of the request, not of each action that


[MEDIUM] Diagnostic creation failure aborts entire transaction conversion

token_registry_for changed from infallible (LayeredRegistry) to fallible (Result), propagating errors from rejected_metadata_diagnostics via ? at both call sites (render_on_chain and render_intent_envelope). If create_text_field or create_diagnostic_field fails inside rejected_metadata_diagnostics (e.g. charset_safe output is empty), the entire transaction conversion aborts with Err -- the signer gets no payload at all rather than a payload with a diagnostic. Before this change, the same refusal was silently logged and the transaction parsed fine. In practice charset_safe is extremely unlikely to produce empty output given the static message prefix ("token metadata supplied for ... was rejected and not used: "), but a new refusal reason added later could tip this. Consider collecting diagnostic results and logging individual failures rather than propagating rejection-reporting errors to abort conversion.

Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.

Good find, and agreed -- this is the one substantive issue in the batch. Fixed in 14f33cc3.

rejected_metadata_diagnostics is now infallible and token_registry_for no longer returns Result, so a diagnostic that fails to build is logged and dropped instead of aborting the conversion. Reporting a refusal must never be able to withhold the transaction it's a caveat about, and before this it could: the signer got no payload at all where the pre-PR behaviour rendered fine and logged.

You're right that charset_safe is unlikely to empty the string today given the static prefix -- the reason to fix it anyway is that the fragility isn't visible at the ?. A new refusal reason, or a future field builder with a stricter validator, would reintroduce it silently.

Both call sites (render_on_chain, render_intent_envelope) drop the ? accordingly.

pepe-anchor · 2026-08-05T15:49:07Z

+                tracing::warn!("Skipping token metadata for '{asset_id}': {reason}");
+                rejected.push(RejectedTokenMetadata {
+                    asset_id: asset_id.clone(),
+                    reason,


[INFO] All 7 refusal paths now use reject! -- no missed paths

Audit confirms every refusal path in the extraction loop uses the reject! macro, which pushes to rejected AND continues: (1) oversized value at line 443, (2) unsigned-under-require-signed at line 462, (3) unsigned-seed-override at line 473, (4) failed signature validation at line 489, (5) invalid JSON at line 494, (6) decimals-out-of-range at line 497, (7) symbol-length-out-of-range at line 500. No tracing::warn! + continue paths remain that omit rejected.push. The comment "Bind each refusal to its continue so a new rejection path can't be added without also reporting it" is upheld. Closing as informational.

Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.

Thanks for auditing all seven paths -- matches what the reject! macro is there to guarantee, and the enumeration is more useful than the comment on its own.

One thing your audit makes true that wasn't before: because every refusal now pushes to rejected, the aggregate unsigned_count log and the per-entry diagnostics can't disagree about what happened. That's the property worth keeping, so if a future path skips reject! the symptom will be a missing signer-visible caveat rather than a missing log line.

pepe-anchor · 2026-08-05T15:49:08Z

+) -> Result<Fields, VisualSignError> {
+    rejected
+        .iter()
+        .map(|r| {


[INFO] Rejected metadata diagnostics apply charset_safe defense-in-depth

rejected_metadata_diagnostics applies charset_safe to the fully-formatted message string containing both the asset_id (caller-controlled map key) and the reason (which can quote asset_id back via serde JSON parse errors, or include signer error messages). charset_safe filters control characters, double quotes, and backslashes -- correctly blocking newline injection that would render as extra apparent fields on the signing screen. The end-to-end test rejected_metadata_diagnostic_strips_newlines_from_the_asset_id (convert.rs:576) verifies that a newline in the asset_id does not survive to the JSON output. The reason half also goes through charset_safe since it's part of the same format! string. Closing as informational.

Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.

Correct, and thanks for tracing both halves of the interpolation -- the asset id is a caller-controlled map key and the reason can quote it back through a serde error, so both needed to be inside the filter rather than just one.

Worth noting the filter's exact shape changed during #426's review: quotes are no longer stripped (they serialize as \", which the core validator permits, and ft_transfer_call's msg legitimately carries embedded JSON), while backslash still is. So the newline-spoofing hole you're describing is closed by the control-character half, not by quote stripping.

pepe-anchor · 2026-08-05T15:49:09Z

                allowlists,
            ) {
-                tracing::warn!("Skipping token metadata for '{asset_id}': {e}");
-                continue;


[INFO] Rejection diagnostics do not expose allowlist identities

The reject! macro constructs RejectedTokenMetadata from only asset_id (a caller-supplied key) and reason (a human-readable explanation). When validate_token_metadata_signature fails, the error message is "signer not in allowlist" or "Signature verification failed" -- neither includes the signer's actual public key, allowlist contents, or internal path/config data. The tracing::warn! emitted in the same macro expansion uses the same reason string, so operator logs and signer-facing diagnostics contain the same information. No env var names, file paths, or allowlist membership details leak.

Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.

Agreed -- and this one is a deliberate constraint rather than an accident, so it's good to have it checked.

RejectedTokenMetadata carries only asset_id and reason precisely so a signer-visible field can never become an oracle for allowlist membership or local configuration. The reason strings are fixed phrasings (signer not in allowlist, Signature verification failed: ...) that say a check failed without saying which key would have passed.

That the operator log and the signer-facing diagnostic share the same string is intentional too: it keeps the two from drifting into different accounts of the same refusal.

pepe-anchor · 2026-08-05T15:49:10Z

+    /// entry. Without the diagnostic, the signer sees an amount in raw base
+    /// units against an `unresolved` asset id and has no way to tell that
+    /// metadata was supplied at all.
+    #[test]


[INFO] Multi-action deduplication works correctly and is tested

token_registry_for is called once in render_on_chain before the action loop, and rejection diagnostics are fields.extend-ed once. The registry is then passed by reference to decode_intents for each action. The test rejected_metadata_reports_once_for_a_multi_action_transaction (line 545) confirms that a transaction with two execute_intents calls produces exactly one rejected-token-metadata field. The pre-PR code rebuilt the registry per action (line 212 of old convert.rs: let registry = token_registry_for(options, trust_policy);), which would have duplicated rejection diagnostics for multi-action transactions had this not been refactored. The refactoring is correct.

Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.

Right, and that per-action rebuild is exactly what this PR had to restructure -- worth stating plainly since the diff makes it look like a mechanical move.

The registry is request-scoped, so a refusal is a property of the request rather than of each action that consults it. Building it inside the action loop meant a two-execute_intents transaction would show the same rejection twice, which reads as two separate problems. rejected_metadata_reports_once_for_a_multi_action_transaction pins the single-field outcome.

One addition since your audit: token_registry_for no longer returns Result (see the diagnostic-abort thread), so the single call site is now infallible as well as once-per-payload.

pepe-anchor · 2026-08-05T15:49:11Z

@@ -153,14 +166,21 @@ impl NearVisualSignConverter {
            create_address_field("To", tx.receiver_id().as_str(), None, None, None, None)?


[INFO] All callers of token_registry_for and try_extract_from_chain_metadata are updated

token_registry_for changed from -> LayeredRegistry<NearTokenRegistry> to -> Result<(LayeredRegistry, Vec<SignablePayloadField>), VisualSignError>. Both callers in convert.rs (render_on_chain at line 172, render_intent_envelope at line 242) properly destructure the tuple with ?. try_extract_from_chain_metadata changed from -> Option<NearTokenRegistry> to -> TokenMetadataExtraction. The sole production caller (token_registry_for) reads .registry and .rejected. All tests in token_signature.rs either use the extract_registry test helper or call try_extract_from_chain_metadata directly and access .registry/.rejected fields. No caller is missed.

Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.

Confirmed -- and the caller sweep is now slightly larger than your audit describes, so worth re-checking against the current head.

token_registry_for's signature changed again in 14f33cc3: it returns the tuple directly rather than Result<(...), VisualSignError>, since propagating a diagnostic-rendering failure could withhold the payload. Both convert.rs call sites destructure without ?.

Separately, two tests added upstack for symbol charset and origin_chain fallback called try_extract_from_chain_metadata(..).is_none()/.expect(..) against the old Option return; they now go through the extract_registry shim like the rest of the module (f1411773). So the answer to "is any caller missed" is still no, but the set moved.

pepe-anchor · 2026-08-05T15:49:12Z

+            // a diagnostic printed as "Field: Unknown" tells the reader nothing,
+            // which defeats the point of emitting it. `--output json` carries the
+            // full structured form; this is the human view of the same finding.
+            #[cfg(feature = "diagnostics")]


[LOW] Diagnostic rendering in CLI output properly feature-gated

The SignablePayloadField::Diagnostic match arm in HumanReadableFormatter is gated with #[cfg(feature = "diagnostics")]. When diagnostics is OFF, NEAR diagnostics are emitted as TextV2 "Warning" fields (see render.rs:38), which fall through to the existing _ catch-all and render as "Warning: rule: message". When diagnostics is ON, the structured Diagnostic variant is handled explicitly with rule/level/message display. The parser_cli/Cargo.toml propagates diagnostics to parser_cli_core/diagnostics so the feature gate resolves correctly in the CLI binary. Tests in output.rs verify both the rule-level-message rendering and the instruction-index display.

Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.

Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.

Confirmed, and the both-modes behaviour you describe is the intended contract: diagnostics on gives the structured Diagnostic variant, off carries the same rule and message as a Warning-labelled TextV2 so the production payload shape is unchanged.

That dual shape is why the NEAR tests assert on message text rather than on a Diagnostic field -- an assertion keyed to the structured variant passes vacuously in the default build, since the field simply isn't there to match.

One related change landing below this PR: parser_cli and parser_app now propagate diagnostics to visualsign-near (#430), which they previously did only for visualsign-solana. So this PR's manifest diff on those lines reduces to the parser_cli_core/diagnostics entry.

visualsign-near declares a `diagnostics` feature and render.rs routes every soft finding through a feature-aware `diagnostic()` helper, but nothing enabled that feature: neither parser_cli's nor parser_app's `diagnostics` propagated into the crate, so the structured half was dead code. Refused token-metadata entries were a real gap rather than a flag flip. `try_extract_from_chain_metadata` dropped them with a `tracing::warn!` and returned only what survived, so a caller supplying metadata the parser then threw away learned nothing: the signer saw an amount in raw base units against an `unresolved` asset id, or resolved from a seed instead of the supplied override, with the reason only in an operator log the caller has no access to. - Extraction returns `TokenMetadataExtraction { registry, rejected }`. A `reject!` macro binds each refusal to its `continue`, so a new rejection path can't be added without reporting it. All seven paths are covered by a table-driven test. - `rejected_metadata_diagnostics` renders each one under a new `rejected-token-metadata` rule, alongside the intents rather than replacing them: the refusal protects the render, it doesn't invalidate it. - Both halves of the message go through `charset_safe`. The asset id is a caller-controlled map key and the reason can quote it back, so an embedded newline would otherwise render as extra apparent fields on the signing screen -- the same class already fixed for `memo`/`msg`/`method_name`, reached through a different field. - The registry is now built once per conversion rather than once per action, so a multi-action transaction reports the request's rejections once. - `parser_cli`, `parser_cli_core` and `parser_app` propagate `diagnostics` to `visualsign-near`; `make test`/`make lint` gained the matching diagnostics-ON invocations, and a `near-json` CLI fixture pair covers the rendered output in both feature states. The CLI's human view rendered every diagnostic as `Field: Unknown`, since `format_field` had no `Diagnostic` arm -- so NEAR's existing `deadline` and `signature` findings were already invisible there, and this one would have been too. It now prints level, rule, optional instruction index, and message, which fixes Solana's diagnostics in that view as well. The arm and its tests are gated on a new `parser_cli_core/diagnostics` feature, because the core crate only defines the `Diagnostic` variant under its own. Payload-shape note: a refused entry adds one field where there was none. With `diagnostics` off (parser_app, the production shape) that is a `Warning` text field, matching how NEAR's other soft findings already degrade. Requests that supply no metadata, or whose metadata is accepted, are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

rejected_metadata_diagnostics was fallible and token_registry_for propagated it, so a diagnostic that failed to build aborted the whole conversion: the signer got no payload at all, where previously the same refusal was logged and the transaction rendered fine. Reporting a refusal must never be able to withhold the transaction it is a caveat about. Both are now infallible. A field that cannot be built is logged and dropped, leaving the signer a payload minus one caveat rather than no payload, and the registry it reports on is unaffected either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

This PR changes try_extract_from_chain_metadata to return TokenMetadataExtraction, so the two tests added upstack for symbol charset and origin_chain fallback now go through the extract_registry shim like the rest of the module's tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ey cover `output.rs` carries no general test module on this branch, so `tests` was the obvious name. It is the wrong one: the module covers the `Diagnostic` arm specifically and is gated on the `diagnostics` feature, while the file's other formatter behavior is untested and wants a module of its own. Two modules named `tests` in one file is E0428, so the general one cannot be added until this is renamed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions Bot added the chain:near label Aug 4, 2026

shahan-khatchadourian-anchorage force-pushed the shahankhatchadourian/near-e-cli-token-metadata branch from b9b8c2d to 40c3f20 Compare August 5, 2026 00:46

shahan-khatchadourian-anchorage force-pushed the shahankhatchadourian/near-f-diagnostics branch from 8d1eabe to 92521a5 Compare August 5, 2026 00:46

shahan-khatchadourian-anchorage force-pushed the shahankhatchadourian/near-e-cli-token-metadata branch from 40c3f20 to 485a961 Compare August 5, 2026 02:26

shahan-khatchadourian-anchorage force-pushed the shahankhatchadourian/near-f-diagnostics branch from 92521a5 to 48265fe Compare August 5, 2026 02:26

pepe-anchor reviewed Aug 5, 2026

View reviewed changes

shahan-khatchadourian-anchorage and others added 3 commits August 6, 2026 09:54

shahan-khatchadourian-anchorage force-pushed the shahankhatchadourian/near-e-cli-token-metadata branch from 485a961 to 2834460 Compare August 6, 2026 14:12

shahan-khatchadourian-anchorage force-pushed the shahankhatchadourian/near-f-diagnostics branch from 48265fe to 7858b56 Compare August 6, 2026 14:12

shahan-khatchadourian-anchorage added the CI label Aug 6, 2026

This was referenced Aug 6, 2026

fix(near): sanitize untrusted strings on the intents render path #459

Open

fix(cli): render every field variant instead of "Field: Unknown" #462

Open

		@@ -153,14 +166,21 @@ impl NearVisualSignConverter {
		create_address_field("To", tx.receiver_id().as_str(), None, None, None, None)?

Uh oh!

Conversation

shahan-khatchadourian-anchorage commented Aug 4, 2026

What was already in place, and what wasn't

Changes

Payload shape

Docs

Uh oh!

pepe-anchor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pepe-anchor left a comment •

edited

Loading