Skip to content

An unknown context tag makes a V2 factory payload parse as AcceptV2 #44

Description

@gyorgybalazsi

ChoiceArgumentsVariations is #[serde(untagged)], so variant order
decides which variant a payload deserializes into. The comment above
the enum explains the order by which fields each variant requires. That
reasoning is incomplete, and one gap lets a payload parse as the wrong
variant with no error.

TransferFactoryV2 and AcceptV2 carry different ExtraArgs
types, with different strictness:

  • transfer_factory::ExtraArgs has context.values: HashMap<String, ContextValue>. ContextValue is a tagged enum listing
    eleven AV_* tags.
  • accept::ExtraArgs has context.values: serde_json::Value, which
    accepts anything.

So a V2 factory payload whose context carries a tag ContextValue does
not list fails TransferFactoryV2 and then satisfies AcceptV2, which
needs only actors and extraArgs. The transfer field is dropped
without an error.

Measured. This payload parses as AcceptV2:

{
  "transfer": { "sender": {"owner":"alice::1220ab","provider":null,"id":""}, "...": "..." },
  "actors": ["alice::1220ab"],
  "extraArgs": {
    "context": { "values": { "k": { "tag": "AV_BrandNew", "value": "x" } } },
    "meta": { "values": {} }
  }
}

Nothing in this repository deserializes the enum at runtime. Every
use constructs it for serialization, so this is latent rather than
live. It becomes live if a consumer parses a stored command, or if the
registry ever adds an AV_* tag and something round-trips a payload.

The existing unit tests do not catch it, because they all use an empty
context, which both variants accept.

Two ways to close it, and both change a shared public type, so this
needs a decision rather than a quick patch.

  1. Put #[serde(deny_unknown_fields)] on accept::v2::ChoiceArguments
    and accept::ChoiceArguments. A payload carrying transfer then
    falls through to Generic, which is visible rather than wrong.
  2. Give the enum a real tag and stop relying on order.

Either way, add a test with a populated context and an unrepresentable
tag, so the ordering rule has cover.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions