[BUG] Regression between 1.2.0 and 1.3.5 for undefined values

[damien-git]

When the IdP sends the following attribute description:

            <saml2:Attribute Name="attributeName"
                             NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:unspecified"
                             >
                <saml2:AttributeValue xmlns:xs="http://www.w3.org/2001/XMLSchema"
                                      xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                                      xsi:type="xs:string"
                                      />
            </saml2:Attribute>

With passport-saml 1.2.0 I get the value: undefined for profile.attributeName, as I expect. But with passport-saml 1.3.5, I get this object instead:

{
    '$': {
      'xmlns:xs': 'http://www.w3.org/2001/XMLSchema',
      'xmlns:xsi': 'http://www.w3.org/2001/XMLSchema-instance',
      'xsi:type': 'xs:string'
    }
  }

Was that change intended, or is it a bug ? It sure broke my application when I updated passport-saml to avoid a security issue, as I did not expect to get something like that...

[damien-git]

Possibly related: #447

[cjbarth]

This is an interesting one. The metadata is returned, which is potentially valuable, but the metadata says that the returned type is a string, so we would expect non-breaking behavior here. I believe this was an unexpected side-effect and indeed a regression in this case. The problem is that there is in fact a _ object, it just happens to be undefined. The previous fix was too simplistic.

Would you be in a position to at least write a test to account for your case and then perhaps we can get @mans0954 to modify his fix to account for your new test case? We obviously need more tests around this part of the code. Thanks for reporting this with a clear example!

[damien-git]

Something like that ? I didn't test it.

        it( 'An undefined value given with an object should still be undefined', function( done ) {
          const xml = 
          '<Response>' +
            '<saml2:Assertion xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion" Version="2.0">' +
              '<saml2:AttributeStatement>' +
                '<saml2:Attribute Name="attributeName" ' +
                    'NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:unspecified">' +
                  '<saml2:AttributeValue xmlns:xs="http://www.w3.org/2001/XMLSchema" ' +
                    'xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" ' +
                    'xsi:type="xs:string"/>' +
                  '</saml2:AttributeValue>' +
                '</saml2:Attribute>' +
              '</saml2:AttributeStatement>' +
            '</saml2:Assertion>' +
          '</Response>';
          var base64xml = Buffer.from( xml ).toString('base64');
          var container = { SAMLResponse: base64xml };
          var samlObj = new SAML();
          samlObj.validatePostResponse( container, function( err, profile, logout ) {
            should.not.exist( err );
            (profile['attributeName'] === undefined).should.be.true;
            done();
          });
        });
      });

[cjbarth]

@damien-git I think that reasonably captures the problem (without testing it myself). @mans0954 , would you be in a position to modify your previous fix to account for this test?

[markstos]

I followed up with @mans0954 today to nudge him on this.

[mans0954]

Sorry, I missed this before. Will take a look.

[mans0954]

That test passes for me:

npm test

...

  passport-saml /
    saml.js / 
      validatePostResponse checks /
        ✓ An undefined value given with an object should still be undefined


  1 passing (14ms)

[mans0954]

I think the assertion should be:

should(profile['attributeName']).be.undefined();

[mans0954]

Looking at the previous implementation

      var attrValueMapper = function(value) {
        return typeof value === 'string' ? value : value._;
      };

There seems to me to be something flawed about this, as, as far as I can see, the type of value will always be object, in which case it is always value._ which is returned. This is the case for every test that we have. This logic goes back at least as far as the switch to xml2js - prior to that I think value may have had a different structure?

I'm wondering if the condition should have been:

 `typeof value._ === 'string'` ?  value._ : value;

?

[cjbarth]

There seems to me to be something flawed about this, as, as far as I can see, the type of value will always be object, in which case it is always value._ which is returned.

@mans0954 , that is why, when I looked at the patch to this code, it seemed to make sense to me because the existing implementation just didn't. However, we still need to account for the fact that, when possible, we should return a string instead of the object that we parse out. Really, both the old and current logic are too simple to catch the three cases we're working with here: undefined, string and object.

I think the assertion should be:

should(profile['attributeName']).be.undefined();

I would agree. That, or it should have included typeof. Sorry for missing that.

[mans0954]

Started working on this here: https://github.com/node-saml/passport-saml/tree/csh-issue-459-attr-value-regression - will try to pick it up again tomorrow if I can. My next step is to understand xml2js a bit better to see if there's a reliable way to distinguish between the three cases @cjbarth mentions.

[mans0954]

Actually, I think there are 4 cases - one might have both a string and child elements (at least in a general XML document) e.g. the following is valid XML:

<?xml version="1.0" encoding="UTF-8"?>
<root>I am your father<child>Hello</child></root>

Presumably in this case we would want to return the object rather than just a string?

[cjbarth]

@mans0954 , I would expect in that case (Mixed Content) there would be an object returned. It would only be possible to return a string if the <saml2:AttributeValue> had only Character Data content. If it had Empty xml, then we could return undefined.

[mans0954]

Here's one possible solution: https://github.com/node-saml/passport-saml/tree/csh-issue-459-attr-value-regression-ec which passes the tests. It uses xml2js's explicitChildren option to put child nodes in a sub-object called $$. Used in combination with the explicitCharkey option (which was already set) a node may have the following properties:

• _ character data
• $ attributes
• $$ child nodes

The attrValueMapper function then becomes

      var attrValueMapper = function(value) {
        return Object.prototype.hasOwnProperty.call(value,'$$') ? value : value._;
      };

The downside of this solution is that it requires a number of changes at other points of the processValidlySignedAssertion function. It also means that the object returned in the case of a non-sting value is slightly different from the object returned in 1.3.5. For these reasons you may consider this solution unacceptable? @cjbarth @markstos .

[cjbarth]

Since we only started returning objects in 1.3.5, I'm not particularly worried about breaking that. As for the other changes, they seem to add clarity to where those properties are coming from, so I'm not against that either. My cursory glance seems to indicate that this wouldn't result in two different XML parsing patterns in the code; can you confirm that, as that would be a potential objection I would have.

I should note though that we do return this raw object in

passport-saml/lib/passport-saml/saml.js

Line 1042 in 43465d6

profile.getAssertion = () => parsedAssertion;

, see also

passport-saml/lib/passport-saml/saml.js

Line 890 in 43465d6

parsedAssertion = doc;

, so this change would break any consumers of that function. Would that be a semver major change then? If so, could we get that and the xml-crypto fix in too and then do the version bump?

[mans0954]

The low impact option would be to change attrValueMapper to something like this:

      var attrValueMapper = function(value) {
        const hasChildren = Object.keys(value).some((cur)=> { return (cur!=='_' && cur!=='$'); });
        return (hasChildren) ? value : value._;
      };

This then passes all the tests without changing any other code or any of the return values. I've put this on a branch here: https://github.com/node-saml/passport-saml/tree/csh-issue-459-attr-value-regression-some

[cjbarth]

The low impact option would be to change attrValueMapper to something like this:

This seems like the correct solution to resolve the regression, however, I am interested in hearing comments from others about the bigger change for the next semver major release.