Skip to content

Conversation

@RivalAUT
Copy link
Contributor

When using SAAJ versions from 1.4 onwards, the signature element of the request was not removed due to type mismatch. This led to failed validations because the digest was wrongly calculated.
With this fix the excludeNode will be excluded even if the currentNode and excludeNode differ in type (e.g. com.sun.xml.messaging.saaj.soap.impl.ElementImpl vs. com.sun.org.apache.xerces.internal.dom.ElementNSImpl).

Signature element was not removed due to type mismatch
Copy link
Contributor

@coheigea coheigea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work with just currentNode.isSameNode(excludeNode) ?

@RivalAUT
Copy link
Contributor Author

RivalAUT commented Mar 5, 2025

Sadly not, it does not work in my specific case with just currentNode.isSameNode(excludeNode)
It is needed to check both ways.

@coheigea
Copy link
Contributor

coheigea commented Mar 5, 2025

It's a bit strange - do you have a test case where isSameNode only works one way?

@RivalAUT
Copy link
Contributor Author

RivalAUT commented Mar 5, 2025

I sent a test case per mail on 13th January.
It happens because currentNode is of type com.sun.org.apache.xerces.internal.dom.ElementNSImpl and excludeNode is of type com.sun.xml.messaging.saaj.soap.impl.ElementImpl, where only checking from com.sun.xml.messaging.saaj.soap.impl.ElementImpl returns the correct result. As both currentNode and excludeNode can be from this class (in my case it is excludeNode, but in the original Jira Ticket it was the other way around), both have to be checked.

@coheigea coheigea requested a review from seanjmullan March 5, 2025 11:42
Copy link
Member

@seanjmullan seanjmullan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look ok, although it may be more optimal to first check if the references are equal and if not then fallback to calling isSameNode. WDYT?

@RivalAUT
Copy link
Contributor Author

I changed the code to check the references first - should be better like this, i agree.

@coheigea coheigea merged commit 0791fc5 into apache:main Mar 21, 2025
3 checks passed
coheigea pushed a commit that referenced this pull request Mar 21, 2025
* [SANTUARIO-576] Fix saml validation with saaj 1.4

Signature element was not removed due to type mismatch

* [SANTUARIO-576] Check for reference equality first

---------

Co-authored-by: Lukas Fabian <lukas.fabian@svc.co.at>
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

Successfully merging this pull request may close these issues.

3 participants