cxp-846 decode XML into a map target in WithAlwaysXMLResponse - #1061
cxp-846 decode XML into a map target in WithAlwaysXMLResponse#1061agustin-conductor wants to merge 1 commit 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 fix XML list decoding in the generic XML decoderBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryScanned the full PR diff (4 files, all in 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.
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>
0cf06e7 to
25669be
Compare
agustin-conductor
changed the title
5 participants
Summary
WithAlwaysXMLResponsehands its target straight toencoding/xml, which cannot unmarshal into a map. A*map[string]anytarget therefore failed for every response with a body:Route that one target type through the
xmlMapdecoderWithGenericResponsealready uses, sharing the code asunmarshalXMLToMap.This scope was narrowed. It previously also reshaped the decoder so repeated siblings grouped under their shared key. That commit is dropped — see Deferred below. What remains changes no shapes at all.
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. This is the change that makes it work.Compatibility
Two independent reasons this cannot affect a working caller.
1. Scoped by target type. The new branch fires only for
*map[string]any; every other target falls through to the unchangedxml.Unmarshalcall. All existingWithAlwaysXMLResponsecall sites in the connector fleet pass typed structs ornil.WithXMLResponse— which is what panorama, litmos, and sage-intacct use — is not modified.2. The branch it does reach always failed. For a map target the old code returned an error for every input, so there is no successful behavior to preserve.
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<foo>bar</foo>)Internal: unsupported XML structure: stringThe map target now produces exactly what
WithGenericResponseproduces for the same document — a container with repeated children stays a[]map[string]anyof single-key maps, as today. A test asserts that shape explicitly so it is visible in review rather than implied.The
WithGenericResponsechange is a pure extraction: its XML branch previously routed throughWithXMLResponse(&xm), whose content-type and nil checks are both dead inside that branch (it is already guarded byIsXMLContentType, and&xmis never nil). Same decoder, same error wrapping, one code path.The typed-nil guard addresses the review finding on the earlier revision: my 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.Testing
go build ./...,go test ./pkg/uhttp/, andgolangci-lint run ./pkg/uhttp/...(0 issues) all pass.New cases on
WithAlwaysXMLResponse: map target decoding despite a non-XML content type, the typed-struct path unchanged, root-text-only erroring, 204 and empty-200 leaving the map untouched, a typed-nil target erroring rather than panicking, andWithXMLResponsestill rejecting map targets.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 no longer needed. The consumer problem it targeted — baton-http's
items_pathfailing on XML list responses — is fixed entirely in baton-http by ConductorOne/baton-http#144, which normalizes the existing shape at the extraction sites and needs no SDK release.And it carried real risk that 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 working config. Reshaping is worth revisiting on its own merits, with that exposure audited first, rather than riding along with a fix that has none.Part of CXP-846
🤖 Generated with Claude Code