Skip to content

harden(winrm): reject malformed XML in the FromXml layer#37

Merged
irvingouj@Devolutions (irvingoujAtDevolution) merged 2 commits into
masterfrom
stack/xml-fromxml-harden
Jun 25, 2026
Merged

harden(winrm): reject malformed XML in the FromXml layer#37
irvingouj@Devolutions (irvingoujAtDevolution) merged 2 commits into
masterfrom
stack/xml-fromxml-harden

Conversation

@irvingoujAtDevolution

Copy link
Copy Markdown
Collaborator

Part 5/6. Base: stack/xml-fromxml-tag-macro (#36).

Restores strict rejection of malformed/untrusted input that the visitor→FromXml migration had silently relaxed: a leaf_text helper rejecting mixed content, Empty strictness, exactly-one-child descend, derive duplicate-singleton rejection (syn::Error), and hardening of the hand-written impls. Output of the review round.

FromXml stack (review/merge bottom-up):

  1. core + winrm migration (feat(xml): namespace-correct FromXml core + winrm migration #33)
  2. nested-tag descend fix (fix(winrm): descend into child for a tag whose value is a tag #34)
  3. delete dead AnyTag/TagList + markers (refactor(winrm): delete dead AnyTag/TagList and stale tag markers #35)
  4. tag! macro + aliases (refactor(winrm): alias-based tags via tag! macro #36)
  5. harden / reject malformed XML
  6. tests

…rom APIs

Keeps the compile-time tag safety (Tag<'a, V, N> with an N: TagName marker is
still the enforcer) but hides the verbosity behind a `tag!` macro that, per tag,
generates the marker (`<Alias>Tag`, internal) plus an ergonomic type alias
`Alias<'a> = Tag<'a, V, AliasTag>`. Fields and construction now read `Get<'a>` /
`Get::new(..)` instead of `Tag<'a, Text<'a>, Get>` / `Tag::from_name(Get)...`.

- `#[derive(FromXml)]` reads identity via `NamedTag` on the field type, so it
  works through the aliases (added in the prior commit).
- Leaf-valued tags (Text/WsUuid/Time/Empty/…) are a readable table in
  cores/tag_name.rs; composite-valued tags (Shell=ShellValue, Body=SoapBody, …)
  are defined with `tag!` next to their value struct — no cores→domain dep
  inversion.
- Multi-value names get distinct aliases sharing a wire name
  (LocaleEmpty/LocaleText, DataLocaleEmpty/DataLocaleText). Only genuinely
  value-varying construction (CommandLine's <Command>) keeps an explicit marker.

Verified: fmt + clippy (-D warnings) clean, workspace tests pass, real-server
transport×auth×sealing matrix 10/10.
Review-loop findings (Claude subagents + Codex, converged). The visitor→FromXml
migration had relaxed several validations the old visitors enforced; this
restores strict rejection of malformed/untrusted input across the parse layer:

- `leaf_text()` helper: text-valued leaves (Text/WsUuid/Time/numerics) reject any
  non-text child (mixed content, comments, PIs) instead of silently truncating
  via `node.text()`. `Empty` rejects any non-whitespace content.
- `Tag::from_xml` wrapper-descend (`Tag<Tag<..>,_>`): requires exactly one child
  element which must be `N` — rejects extra siblings, wrong child, duplicates.
- `#[derive(FromXml)]`: `if … else if …` matcher chain; rejects a duplicate
  element for a singleton field; reports shape errors via `syn::Error` not
  `panic!`; doc no longer references the deleted `SimpleXmlDeserialize`.
- Hand-written impls made consistent: commandline (leaf_text; empty `<Command/>`
  → None via a seen flag; duplicate `<Command>` rejected), receive (duplicate
  `<CommandState>` rejected), SelectorSet/OptionSet (leaf_text; duplicate Name
  rejected).
- Comments: flagged the deliberate `Send` alias / `std::marker::Send` shadow and
  the `Locale`/`DataLocale` empty-vs-text deserialize-collapse invariant.

Trimming in `leaf_text` is intentional and kept (original leaf behavior; strips
pretty-printed-XML indentation; WinRM values carry no significant edge
whitespace) — a Codex suggestion to drop it was declined as a regression.

Verified: fmt + clippy (-D warnings) clean, workspace tests pass, real-server
matrix 10/10.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant