Skip to content

Commit 4af720c

Browse files
gnareaziluvatar
authored andcommitted
Fix remaining cases where an invalid identityProviderUrl isn't handled (#77)
We used to parse `options.identityProviderUrl` assuming it was a string, which failed when it wasn't. We'd fixed this in Samlp.getSamlRequestUrl(), but its siblings getSamlRequestParams() and getSamlRequestForm() were still susceptible.
1 parent 2125e71 commit 4af720c

File tree

3 files changed

+49
-25
lines changed

3 files changed

+49
-25
lines changed

lib/passport-wsfed-saml2/samlp.js

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ var querystring = require('querystring');
99
var SignedXml = require('xml-crypto').SignedXml;
1010
var templates = require('./templates');
1111
var EventEmitter = require('events');
12+
var validUrl = require('valid-url');
1213

1314
var utils = require('./utils');
1415
var AuthenticationFailedError = require('./errors/AuthenticationFailedError');
@@ -109,11 +110,16 @@ Samlp.prototype = {
109110
getSamlRequestParams: function (opts, callback) {
110111
var options = xtend(opts || {}, this.options);
111112

113+
var idpUrl = options.identityProviderUrl;
114+
if (typeof idpUrl !== 'string' || !validUrl.isWebUri(idpUrl)) {
115+
return callback(new Error(`Invalid identity provider URL: ${JSON.stringify(idpUrl)}`));
116+
}
117+
112118
var signatureAlgorithm = options.signatureAlgorithm || 'rsa-sha256';
113119
var digestAlgorithm = options.digestAlgorithm || 'sha256';
114120

115121
var assert_and_destination = templates.assert_and_destination({
116-
Destination: options.identityProviderUrl,
122+
Destination: idpUrl,
117123
AssertionConsumerServiceURL: options.callback
118124
});
119125

@@ -133,7 +139,7 @@ Samlp.prototype = {
133139
}
134140

135141
var SAMLRequest = trimXml(!options.requestTemplate ? templates.samlrequest(model) : supplant(options.requestTemplate, model));
136-
var parsedUrl = url.parse(options.identityProviderUrl, true);
142+
var parsedUrl = url.parse(idpUrl, true);
137143
var params = {
138144
SAMLRequest: null,
139145
RelayState: options.RelayState || (parsedUrl.query && parsedUrl.query.RelayState) || ''
@@ -196,15 +202,10 @@ Samlp.prototype = {
196202
getSamlRequestUrl: function (opts, callback) {
197203
var options = xtend(opts || {}, this.options);
198204

199-
if (!options.identityProviderUrl) {
200-
return callback(new Error('Missing value for the identity provider login URL'));
201-
}
202-
203-
var parsedUrl = url.parse(options.identityProviderUrl, true);
204-
205205
this.getSamlRequestParams(options, function (err, params) {
206206
if (err) return callback(err);
207207

208+
var parsedUrl = url.parse(options.identityProviderUrl, true);
208209
var samlRequestUrl = options.identityProviderUrl.split('?')[0] + '?' + qs.encode(xtend(parsedUrl.query, params));
209210
return callback(null, samlRequestUrl);
210211
});

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
"jsonwebtoken": "~5.0.4",
2525
"passport-strategy": "^1.0.0",
2626
"uid2": "0.0.x",
27+
"valid-url": "^1.0.9",
2728
"xml-crypto": "auth0/xml-crypto#fix-digest",
2829
"xml-encryption": "0.11.0",
2930
"xml2js": "0.1.x",

test/samlp.tests.js

Lines changed: 39 additions & 17 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)