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

Don't encode windows line-breaks with special characters. #211

Closed
wants to merge 4 commits into from

Conversation

thenengah
Copy link

Getting "Invalid Signature". The line breaks are getting encoded as special characters which botch the signature checking.

Sam Gilman added 3 commits May 13, 2020 17:22
Getting "Invalid Signature" without this because the line breaks get converted to special characters which botch the digest.
@thenengah thenengah changed the title Don't encode line-breaks with special characters. Don't encode windows line-breaks with special characters. May 13, 2020
@thenengah
Copy link
Author

Made the update less restrictive to only replace windows line breaks. I also updated the windows line-break test to expect the line-break to change as encoding it is what was producing the invalid signature.

@LoneRifle
Copy link
Collaborator

This is intended, per the issue raised in #195 and addressed in #196. I've cc'd @nicosabena to explain further.

Fix spacing
@nicosabena
Copy link
Contributor

Hi @thenengah !

Basically line end normalization is the job of the XML Processor, before parsing (https://www.w3.org/TR/xml/#sec-line-ends). If there are Windows line breaks after the parser runs, those should be treated as the result of parser-resolved character references. E.g.

Raw input to the parser:

<foo>&#xD;\n</foo>

The parser will return \r\n as the data for foo's text element. This shouldn't be altered by the canonicalization algorithm.

xmldoc does not normalize line endings automatically so, If anything, you can (should) normalize the line endings when reading the source:

function normalizeLineEndings(string) {
  return string.replace(/\r\n?/g, '\n');
}
var xml = await readFileFromSource();
var doc = xmldom.DOMParser().parseFromString(normalizeLineEndings(xml));

Can you give that a try and see if it solves the signature validation?

@thenengah
Copy link
Author

Ok, thanks for the help!

We already implemented this:

  const xml = (Buffer.from(req.body.SAMLResponse, 'base64').toString()).replace(/\r\n?/g, '\n')
  req.body.SAMLResponse = Buffer.from(xml.toString()).toString('base64')

We had to do this to get it working with passport-saml@1.3.3. We have to schedule testing with some affected customers to confirm this actually works. Will confirm here later if so.

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.

None yet

3 participants