Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression: xmlbuilder2 may have removed some non-essential properties #230

Open
mcab opened this issue Feb 8, 2021 · 3 comments
Open

Comments

@mcab
Copy link
Member

mcab commented Feb 8, 2021

I think the update to xmlbuilder2 also removed some non essential properties, like on the keys nodes. I did not investigate that.

Originally posted by @artonge in #229 (comment)

@mcab
Copy link
Member Author

mcab commented Feb 8, 2021

In order to solve this, we should:

  • snapshot all XML outputs from v2.0.9 that utilize xmlbuilder
    • create_authn_request
    • create_metadata
    • create_logout_request
    • create_logout_response
  • compare against v3.0.0 / v3.0.1

I think this might be the case for create_logout_response() due to the call of headless, but would need to actually verify.

@mcab
Copy link
Member Author

mcab commented Feb 9, 2021

also removed some non essential properties, like on the keys nodes.

This refers to the nodes underneath md:KeyDescriptor missing the xmlns:ds attribute.

From xmlbuilder:

    <md:KeyDescriptor use="signing">
        <ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
            <ds:X509Data>
                <ds:X509Certificate>[removed]</ds:X509Certificate>
            </ds:X509Data>
        </ds:KeyInfo>
    </md:KeyDescriptor>

From xmlbuilder2:

    <md:KeyDescriptor use="signing">
        <ds:KeyInfo>
            <ds:X509Data>
                <ds:X509Certificate>[removed]</ds:X509Certificate>
            </ds:X509Data>
        </ds:KeyInfo>
    </md:KeyDescriptor>

Note, the missing attribute on ds:KeyInfo.

@mcab
Copy link
Member Author

mcab commented Feb 9, 2021

Scratched my head for a little bit.

The structure seems right, but it seems as if the attribute (xmlns:ds) isn't set because it shares the same tuple (attribute name, attribute value) as the one defined higher up in the document.

https://github.com/Clever/saml2/blob/master/lib/saml2.coffee#L72

Opened up oozcitak/xmlbuilder2#72 to try and figure out why that's the case.

It's possible that processing and attribute setting has to be done on the element while building the XML itself, but I'd want to explore via object creation first.

@mcab mcab self-assigned this Feb 9, 2021
@mcab mcab removed their assignment Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant