Set getCertFromKeyInfo to noop

[cjbarth]

getKeyInfoContent was incorrectly set to noop instead of getCertFromKeyInfo

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f8cbbb7) 73.17% compared to head (b12d68e) 73.05%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #445      +/-   ##
==========================================
- Coverage   73.17%   73.05%   -0.12%     
==========================================
  Files           9        9              
  Lines         902      902              
  Branches      239      239              
==========================================
- Hits          660      659       -1     
  Misses        143      143              
- Partials       99      100       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cjbarth]

This PR addresses the concerns correctly pointed out by @srd90 here.

Since it is fixing a mistake, and it seems few, if any are using it, should this be a semver-minor change or a semver-major change @LoneRifle ?

[LoneRifle]

lgtm, thanks for the catch @srd90 !

[cjbarth]

@LoneRifle , do you think I should release this as a semver-minor or semver-major release?

[LoneRifle]

Inclination is towards a major release, given that it's likely a breaking change. At the same time, you could consider deprecating v5 so that users are encouraged to skip it

[srd90]

Shouldn't you consider deprecating also v4 due to the fact that it has same default functionality that was now replaced with noop.

Thing that was about to happen at node-saml/node-saml#341 is a proof that people are quite likely going to disable signature verification accidentally during migration from earlier versions to v4 or v5 (more info from #399 for those who land this PR later) and its going to be undetected if they don't have tests to cover e.g. the case at PR mentioned earlier.

[cjbarth]

Honestly, I think we de-facto deprecate anything other than the current semver-major release. I suppose I can actively deprecate these on NPM though. I have no interest in trying to support anything other the HEAD or the latest release. If someone wants something on an older version, they can make the correct PR, but that is on them.

@LoneRifle what has been your tradition here?

[srd90]

Just one more comment: Maybe CVE for v4 and v5 so that those who have already migrated to those versions would possible become more aware of unsecure default implementation which overrides explicitly set certificate.

[LoneRifle]

@cjbarth - I've usually just left versions alone, but only because I previously saw no significant security issues. I'm happy to defer to you to set the norm going forward