cxp-846 return XML as a generic map: map targets and non-map roots - #1061
cxp-846 return XML as a generic map: map targets and non-map roots#1061agustin-conductor wants to merge 3 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if resp.StatusCode >= 200 && resp.StatusCode < 300 && len(resp.Body) == 0 { | ||
| return nil | ||
| } | ||
| return unmarshalXMLToMap(genericResponse, resp) |
There was a problem hiding this comment.
🟡 Suggestion: A typed-nil (*map[string]any)(nil) passes the response == nil check above (interface holds a type), then this assertion succeeds with a nil genericResponse, and unmarshalXMLToMap does *response = vMap → nil-pointer panic on a non-empty body. WithGenericResponse guards this with an explicit nil check; consider mirroring it here. Low confidence — an unusual call pattern, but the map branch is new. (confidence: low)
There was a problem hiding this comment.
Good catch — confirmed, and it's a regression rather than a latent edge case, so fixed in 0cf06e7.
Verified the premise and the prior behavior:
iface == nil? false // typed nil carries a type, so it passes the guard
xml.Unmarshal(typedNil): nil pointer passed to Unmarshal // old behavior: clean error
So routing map targets through xmlMap turned that clean error into a panic. Reverting just the guard and running the new test reproduces it:
panic: runtime error: invalid memory address or nil pointer dereference
Guarded inside unmarshalXMLToMap rather than in WithAlwaysXMLResponse, so WithGenericResponse and any future caller are covered by the same check and it can't be reintroduced at a new call site. Returns InvalidArgument to match WithGenericResponse's existing nil handling. Test added: should error rather than panic on a typed-nil map target.
General PR Review: cxp-846 fix XML list decoding in the generic XML decoderBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. This change reshapes Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
A typed-nil target such as (*map[string]any)(nil) gets past the `response == nil` check in WithAlwaysXMLResponse, because the interface still carries a type. The map branch was then reached with a nil pointer and assigning through it panicked with a nil-pointer dereference. encoding/xml rejected that input with "nil pointer passed to Unmarshal", so routing map targets through xmlMap had turned a clean error into a panic. Guard inside unmarshalXMLToMap rather than at each call site, so neither this option nor WithGenericResponse nor any future caller can assign through a nil pointer. Reported by the PR review bot on #1061. CXP-846 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
General PR Review: cxp-846 return XML as a generic map: map targets and non-map rootsBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commit ( Risk triage (per Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
| // zero value of the assertion is a nil []any, which append handles, | ||
| // so the first occurrence creates the slice. | ||
| list, _ := result[e.key].([]any) | ||
| result[e.key] = append(list, e.value) |
There was a problem hiding this comment.
Since this changes the structure of decoded XML, any connector that uses WithXMLResponse/WithGenericResponse will need to be updated, right? It looks like only a few connectors call WithXMLResponse directly: https://github.com/search?q=org%3AConductorOne+WithXMLResponse&type=code and only baton-http calls WithGenericResponse(), so that's acceptable.
Will an existing baton-http config break because of this change?
There was a problem hiding this comment.
Based on what I've research with claude the baton connectors would not be affected "No updates needed for panorama, litmos, sage-intacct, or sap-grc. xmlMap is
unreachable from WithXMLResponse, all their targets are typed structs, and
they build and test identically against patched vs unpatched v0.22.0."
But baton-http is trickier and I'm not sure how to evaluate the impact, which would depend on how the config.yaml is set.
2 paths
mechanism: jsonpath
used by: items_path, item_path, entitlements_path, resources_path,
details/secondary EvaluateJSONPath
today: broken — error, or silently 0 items
after my change: fixed
mechanism: CEL / templates
used by: cel: and tmpl: expressions
today: works correctly
after my change: breaks — loud on indexing, silent N → 1 on size/len
the second one is a problem, silently losing pages.
There was a problem hiding this comment.
It looks like we're safe to make this change. There are no active http connectors in prod that use this part of the config.
0cf06e7 to
25669be
Compare
agustin-conductor
changed the title
encoding/xml cannot unmarshal into a map, so WithAlwaysXMLResponse failed
for every response with a body when handed a *map[string]any, returning
"unknown type map[string]interface {}". Callers wanting an arbitrary XML
document as a map had no working option, which is why baton-http's
`parse_as: xml` has never functioned.
Route that one target type through the xmlMap decoder the generic path
already uses, and share the code as unmarshalXMLToMap. Any other target
still goes straight to xml.Unmarshal, so callers passing a typed struct
are untouched, and WithXMLResponse is not modified at all.
This changes no shapes: the map target now produces exactly what
WithGenericResponse already produces for the same document.
The behavior change is confined to a branch that previously always
failed:
XML body, map target error "unknown type map…" -> decoded map
204 / empty body error -> nil, map empty
typed-nil map target error "nil pointer passed…" -> InvalidArgument
root holds only text error -> Internal
Nothing that returns successfully today returns anything different. The
typed-nil guard lives inside unmarshalXMLToMap so assigning through the
pointer cannot panic; a typed nil survives an `any == nil` check because
the interface still carries a type.
Part of CXP-846.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
25669be to
b3884f0
Compare
| vMap, ok := xm.data.(map[string]any) | ||
| if !ok { | ||
| // A document whose root holds only text decodes to a string, which has no | ||
| // sensible map representation. | ||
| return status.Errorf(codes.Internal, "unsupported XML structure: %T", xm.data) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the comment says the non-map case is "a document whose root holds only text", but unmarshalXMLElement also returns []map[string]any whenever the root's direct children repeat (see xml_test.go:24). So a very common list shape — <Users><User>…</User><User>…</User></Users> — still hard-fails here with Internal: unsupported XML structure: []map[string]interface {}, which is arguably the main case parse_as: xml needs. Pre-existing in WithGenericResponse and not a regression, but worth either handling the slice case (e.g. wrap it under the root element name) or at least correcting the comment and adding a test so the limitation is explicit. (medium confidence)
There was a problem hiding this comment.
Good catch on both halves — the comment was wrong, and the slice case is the more important one. Fixed in 681b067.
The comment. Corrected to name both ways the root's content can be a non-map, since I'd only documented the string case.
The slice case. Handled as you suggested, keyed by the root element name — xmlMap now records start.Name.Local, which it was discarding:
<users><user><login>a</login></user><user><login>b</login></user></users>
before: Internal: unsupported XML structure: []map[string]interface {}
after: {"users": [{"user":{"login":"a"}}, {"user":{"login":"b"}}]}
I did the text-only root the same way (<Code>OK</Code> → {"Code": "OK"}), so unmarshalXMLToMap can no longer fail on structure at all. Both were errors before, here and on WithGenericResponse, so it stays error → success.
One caveat worth recording, since wrapping is reactive rather than a real fix. It inherits the decoder's arity asymmetry:
| document | decoded | path |
|---|---|---|
<users><user/><user/></users> |
{"users": [{"user":…},{"user":…}]} |
users |
<users><user/></users> |
{"user": {…}} |
user |
A single child means nothing repeats, so the content is a map, so the root name is discarded as usual — which means one config can't serve both arities for a root-level list. Measured, not assumed, and pinned by should keep stripping the root when its content is a map so it can't drift silently.
Still a clear win: the ≥2 case is the one every real tenant hits, and it went from an opaque SDK-internal error to something reachable by a path.
What actually closes the seam is grouping repeated children under their shared name — which would also make this slice branch unreachable. That was in an earlier revision of this PR and is deferred (see the PR description) because it changes what existing CEL and template expressions read on paths that never touch items extraction. Your finding is a second argument for it, so it's parked for its own audit rather than dropped.
Verified end-to-end against ConductorOne/baton-http#144 through a Go workspace: root-level lists now sync at both arities (with the path difference above), nested lists sync at both arities from a single path.
| if resp.StatusCode >= 200 && resp.StatusCode < 300 && len(resp.Body) == 0 { | ||
| return nil | ||
| } | ||
| return unmarshalXMLToMap(genericResponse, resp) |
There was a problem hiding this comment.
🟡 Suggestion: this newly routes bodies that previously always errored into unmarshalXMLElement, which recurses once per nesting level with no depth cap, on a body read with an unbounded io.ReadAll (wrapper.go:503) and with the content-type check bypassed by design. A few MB of nested open tags from a hostile or broken endpoint is a fatal (unrecoverable) stack overflow rather than a returned error. Pre-existing in the WithGenericResponse path, so not introduced here, but a depth limit in unmarshalXMLElement would cheaply close it. (low confidence on real-world reachability)
There was a problem hiding this comment.
^ is there any easy way to address this?
There was a problem hiding this comment.
Addressed in cf7410f8: added a depth cap of 10000 to unmarshalXMLElement.
Real XML nests tens of levels, so 10000 covers production cases with plenty of room while staying well under the stack limit — a pathological body now returns an error instead of a fatal stack overflow that recover() can't catch. (Measured: ~1M levels / 7 MB body is fatal, ~500k isn't.) encoding/json uses the same 10000 for the same reason.
Applies to WithGenericResponse too, which already reached this decoder.
unmarshalXMLToMap required the root element's content to be a map and
returned Internal "unsupported XML structure" otherwise. That is
reachable two ways, and the review flagged that the comment named only
the second:
- The root's own children repeat: <Users><User/><User/></Users> decodes
to a []map[string]any. A root-level list is a common API shape, so
this hard-failed for a whole class of responses -- arguably the main
case parse_as: xml is wanted for.
- The root holds only text: <Code>OK</Code> decodes to a string.
Key both by the root element name, which the decoder recorded nowhere and
otherwise discards, so the document is reachable by a path instead of
being an error. unmarshalXMLToMap can no longer fail on structure at all.
Both cases were errors before, on this path and on WithGenericResponse,
so this stays error -> success with no working caller affected. xml.go
gains a root field but no decoding change, so shapes are unchanged.
Leaves an arity seam, now pinned by a test: a root holding a single
<User> decodes to a map and keeps the root stripped, so its path is
"User" while the repeated case is "Users", and one config cannot serve
both. Grouping repeated children under their shared name is what closes
that, and would make the slice case here unreachable.
Part of CXP-846.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| return status.Errorf(codes.Internal, "unsupported XML structure: %T", xm.data) | ||
| } | ||
| return nil | ||
| return unmarshalXMLToMap(response, resp) |
There was a problem hiding this comment.
🟡 Suggestion: This is no longer a pure extraction — the shared helper's new root-keying changes WithGenericResponse's observable behavior. <users><user/><user/></users> used to return Internal: unsupported XML structure: []map[string]interface {} and now succeeds as {"users": [...]}; <Code>OK</Code> used to error and now returns {"Code": "OK"}. That direction is error→success so it can't break a working caller, but it means the arity seam documented at lines 278-282 now also applies to this already-shipping API: the same endpoint keys on the root name at 2+ items and on the child name at 1 item, and the 2-item case used to be a loud error rather than a silently different key. The new tests all go through WithAlwaysXMLResponse; TestWrapper_WithGenericResponse has no case pinning either new shape. Worth adding the 1-item/N-item pair there directly, and correcting the PR description, which still says this branch is "same decoder, same error wrapping" and still lists root-text as producing Internal: unsupported XML structure: string. (medium confidence)
| // repeated case is "Users". One config cannot serve both. Closing that | ||
| // needs the decoder to group repeated children under their shared name, | ||
| // which would also make the slice case here unreachable. | ||
| *response = map[string]any{xm.root: xm.data} |
There was a problem hiding this comment.
🟡 Suggestion: Because WithGenericResponse now shares this helper, this line also changes that function's documented contract. Its doc comment (line 389) says "if the response is a list, its values will be put into the items field" — the JSON branch still honors that, but an XML root-level list now lands under the root element's own name instead. Worth updating that comment so the public contract matches both branches. (medium confidence)
agustin-conductor
changed the title
unmarshalXMLElement recurses once per nesting level with no bound, on a body read with an unbounded io.ReadAll. Deeply nested open tags therefore exhaust the goroutine stack, and a Go stack overflow is a fatal runtime error rather than a panic: recover() cannot turn it back into a failed request, so the process dies mid-sync. Measured against the 1 GB default stack limit: a 3.5 MB body of 500k nested tags decodes, and a 7 MB body of 1M nested tags is fatal. That is a small payload for an endpoint to return. Cap the recursion at 10000 levels and return an error instead. Real documents nest tens of levels at most, so the limit is ~300x clear of anything legitimate, and encoding/json caps nesting at 10000 for the same reason. Tested in both directions so the limit cannot later be tightened into one that rejects real responses. This closes the recursion path for every caller of the generic decoder, so it also applies to WithGenericResponse, which reached it already for any XML content-type response. Raised in review of the map-target change, which adds a second way in: WithAlwaysXMLResponse skips the content-type check by design, so a parse_as: xml config parses whatever a server returns. Note this is not the error -> success shape of the rest of this branch: a body nested past the limit now returns an error where it previously took the process down. That is the improvement, but it is a behavior change on input that did not previously error. Part of CXP-846. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // pathologically nested body fails as a request error instead of taking the | ||
| // process down with a stack overflow. | ||
| func unmarshalXMLElement(d *xml.Decoder, depth int) (any, error) { | ||
| if depth > maxXMLDepth { |
There was a problem hiding this comment.
🟡 Suggestion: The depth cap closes the fatal case (stack overflow), but the other half of the same unbounded-io.ReadAll exposure is breadth, not depth, and it stays open. For <r><a/><a/>…</r>, each 4-byte sibling costs an entry (32B) plus an empty map[string]any, and because hasDuplicates trips, the result is one single-key map per sibling (hmap + a bucket, ~300B). That's roughly a 100x heap amplification, so the same ~7 MB body the commit message cites as fatal-by-depth becomes several hundred MB by breadth — OOM rather than stack overflow, but still process death mid-sync from one response.
This is pre-existing and reachable before this PR via WithGenericResponse, so not a blocker on this diff. But the PR is explicitly the resource-exhaustion hardening for this decoder, and the depth constant was chosen from a measured ceiling with a two-sided boundary test — the breadth dimension has neither a bound nor a benchmark pinning the cost curve. Worth either capping total element/entry count in the same change, or noting explicitly that breadth is knowingly deferred so the next reader doesn't take the depth cap as full coverage. (medium confidence)
7 participants
Summary
Two fixes that make
uhttpable to hand an arbitrary XML document back as a generic map.1.
WithAlwaysXMLResponserejected map targets. It hands its target straight toencoding/xml, which cannot unmarshal into a map, so a*map[string]anytarget failed for every response with a body:Route that one target type through the
xmlMapdecoderWithGenericResponsealready uses, sharing the code asunmarshalXMLToMap.2. A non-map XML root hard-failed.
unmarshalXMLToMaprequired the root element's content to be a map and returnedInternal: unsupported XML structureotherwise — which a root-level list hits, a common API shape. Key those documents by the root element name instead. Added in response to review feedback;unmarshalXMLToMapcan no longer fail on structure at all.3. The generic decoder had no recursion bound.
unmarshalXMLElementrecursed once per nesting level on a body read with an unboundedio.ReadAll, so deeply nested open tags exhausted the goroutine stack — and a Go stack overflow is fatal, not a panic, sorecover()could not turn it back into a failed request. Measured: a 3.5 MB body of 500k nested tags decodes; a 7 MB body of 1M nested tags kills the process. Capped at 10000 levels. Also raised in review.Scope note. An earlier revision reshaped the decoder so repeated siblings grouped under their shared key. That commit is dropped — see Deferred. No existing shape changes here:
xml.gogains arootfield and a depth bound, but the mapping from document to map is untouched.Why
Callers wanting an arbitrary XML document as a map have no working option today. Concretely, baton-http maps
parse_as: xmlontoWithAlwaysXMLResponse(&map[string]any{}), so that config key has never functioned since it was added in65f49692— it hard-fails on every response with a body.And fix 1 alone would not have been enough for the shape that matters most.
<Users><User/><User/></Users>decodes to a[]map[string]any, so it still died inunmarshalXMLToMap— arguably the main caseparse_as: xmlis wanted for.Compatibility
Scoped by target type. The new branch in
WithAlwaysXMLResponsefires only for*map[string]any; every other target falls through to the unchangedxml.Unmarshalcall. All existing call sites in the connector fleet pass typed structs ornil.WithXMLResponse— which panorama, litmos, and sage-intacct use — is not modified, and a test pins that it still rejects map targets.With one exception noted below, every divergence is
error → something else, neversuccess → something else:unknown type map[string]interface {}nil, map left empty(*map[string]any)(nil)nil pointer passed to UnmarshalInvalidArgument: response is nil<Users><User/><User/></Users>)unsupported XML structure: []map[string]interface {}{"Users": [{"User":…},{"User":…}]}<Code>OK</Code>)unsupported XML structure: string{"Code": "OK"}The one exception is the depth cap. A body nested past 10000 levels previously did not error — it took the process down with a fatal stack overflow. It now returns an error. That is the improvement, but unlike everything else here it is a behavior change on input that did not previously fail, and it applies to
WithGenericResponseas well, which already reached the decoder for any XML content-type response. Real documents nest tens of levels at most, so the limit is ~300x clear of anything legitimate, andencoding/jsoncaps nesting at 10000 for the same reason.The last two rows also apply to
WithGenericResponse, since both paths shareunmarshalXMLToMap. Both were hard errors there too, so the same argument covers it — but note that this PR does change generic-path behavior for those two document shapes, not only the map target.WithAlwaysXMLResponseandWithGenericResponsenow produce byte-identical output for the same document, asserted by test. That matters downstream: a baton-http config gets the same tree whether or not it setsparse_as: xml.The
WithGenericResponserefactor is a pure extraction: its XML branch previously routed throughWithXMLResponse(&xm), whose content-type and nil checks are both dead inside that branch (already guarded byIsXMLContentType, and&xmis never nil). Same decoder, same error wrapping, one code path.The typed-nil guard addresses the earlier review finding: the map branch would have turned
encoding/xml's clean "nil pointer passed to Unmarshal" into a nil-pointer panic. It lives insideunmarshalXMLToMaprather than at each call site, so assigning through the pointer cannot panic. A typed nil survives anany == nilcheck because the interface still carries a type.Known limitation: the arity seam
Keying by the root name is reactive, so it inherits the decoder's arity asymmetry:
<users><user/><user/></users>{"users": [{"user":…},{"user":…}]}users<users><user/></users>{"user": {…}}userOne child means nothing repeats, so the content is a map, so the root name is discarded as always. One config cannot serve both arities for a root-level list. This is pinned by a test rather than left to be rediscovered.
It is still a clear improvement: previously the ≥2 case — the one essentially every real tenant hits — failed outright, and the error it produced was an opaque
Internalfrom inside the SDK rather than something a config author could act on.Grouping repeated children under their shared name is what closes the seam, and it would make the slice case here unreachable. Nested lists already have no seam, thanks to ConductorOne/baton-http#144.
Testing
go build ./...,go test ./pkg/uhttp/..., andgolangci-lint run ./pkg/uhttp/...(0 issues) all pass.Depth cap tested in both directions — a body at 2× the limit errors, and a body at the limit still decodes, so the cap cannot later be tightened into one that rejects real responses.
Cases on
WithAlwaysXMLResponse: map target decoding despite a non-XML content type, the typed-struct path unchanged, a root-level list keyed by the root name, a single-child root still stripping it (the seam), a text-only root keyed by the root name, 204 and empty-200 leaving the map untouched, a typed-nil target erroring rather than panicking, andWithXMLResponsestill rejecting map targets.Verified end-to-end against baton-http#144 through a Go workspace — raw XML →
WithGenericResponse→ExtractItems:Deferred: the decoder shape change
The dropped commit made a container with 2+ same-named children decode to
{"USER_LIST": {"USER": [...]}}instead of{"USER_LIST": [{"USER":…},{"USER":…}]}, so thatjsonpathcould walk it.It is not needed for the consumer problem it targeted — baton-http's
items_pathfailing on XML list responses — which is fixed entirely by ConductorOne/baton-http#144, at the extraction sites, with no SDK release.And it carries a risk this PR does not.
[]map[string]anyis only untraversable for jsonpath; CEL and Go templates walk it fine. In baton-http, responses on the provisioning, action, and pre-request paths never reach items extraction and are read solely by CEL — socel:size(response.body.USER_LIST)returns N today and would return 1 after the reshape: a silent wrong answer in a config that works.Worth noting it has gained a second motivation, though — it is also what would close the arity seam above and retire the slice branch entirely. So it is deferred for its own audit, not dismissed.
Part of CXP-846
🤖 Generated with Claude Code