From 7bf1a69cd122c1a8da736b32d2899347ec7f6552 Mon Sep 17 00:00:00 2001 From: Elise Shanholtz Date: Thu, 17 Oct 2019 12:07:04 -0700 Subject: [PATCH 1/4] first wave of changes --- lib/webhooks/webhooks.js | 57 +++++++++++++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 12 deletions(-) diff --git a/lib/webhooks/webhooks.js b/lib/webhooks/webhooks.js index 6ed25878c9..3970b55688 100644 --- a/lib/webhooks/webhooks.js +++ b/lib/webhooks/webhooks.js @@ -5,6 +5,30 @@ var _ = require('lodash'); var scmp = require('scmp'); var url = require('url'); +/** + Utility function to add a port number to a URL + + @param {URL} parsedUrl - The parsed url object that Twilio requested on your server + @returns {string} - URL with port + */ +function addPort(parsedUrl) { + if (!parsedUrl.port) { + parsedUrl.port = parsedUrl.protocol === 'https' ? '443' : '80'; + } + return parsedUrl.toString(); +} + +/** + Utility function to remove a port number from a URL + + @param {URL} parsedUrl - The parsed url object that Twilio requested on your server + @returns {string} - URL without port + */ +function removePort(parsedUrl) { + parsedUrl.port = null; + return parsedUrl.toString(); +} + /** Utility function to get the expected signature for a given request @@ -18,7 +42,7 @@ function getExpectedTwilioSignature(authToken, url, params) { var data = Object.keys(params) .sort() .reduce((acc, key) => acc + key + params[key], url); - + return crypto .createHmac('sha1', authToken) .update(Buffer.from(data, 'utf-8')) @@ -42,14 +66,27 @@ function getExpectedBodyHash(body) { @param {string} authToken - The auth token, as seen in the Twilio portal @param {string} twilioHeader - The value of the X-Twilio-Signature header from the request - @param {string} url - The full URL (with query string) you configured to handle this request + @param {string} requestUrl - The full URL (with query string) you configured to handle this request @param {object} params - the parameters sent with this request @returns {boolean} - valid */ -function validateRequest(authToken, twilioHeader, url, params) { +function validateRequest(authToken, twilioHeader, requestUrl, params) { twilioHeader = twilioHeader || ''; - var expectedSignature = getExpectedTwilioSignature(authToken, url, params); - return scmp(Buffer.from(twilioHeader), Buffer.from(expectedSignature)); + const urlObject = new url.URL(requestUrl); + const urlWithPort = addPort(urlObject); + const urlWithoutPort = removePort(urlObject); + + const signatureWithPort = getExpectedTwilioSignature(authToken, urlWithPort, params); + const signatureWithoutPort = getExpectedTwilioSignature(authToken, urlWithoutPort, params); + const validSignatureWithPort = scmp(Buffer.from(twilioHeader), Buffer.from(signatureWithPort)); + const validSignatureWithoutPort = scmp(Buffer.from(twilioHeader), Buffer.from(signatureWithoutPort)); + + return validSignatureWithoutPort || validSignatureWithPort; +} + +function validateBody(body, bodyHash) { + var expectedHash = getExpectedBodyHash(body); + return scmp(Buffer.from(bodyHash), Buffer.from(expectedHash)); } /** @@ -63,13 +100,9 @@ function validateRequest(authToken, twilioHeader, url, params) { @returns {boolean} - valid */ function validateRequestWithBody(authToken, twilioHeader, requestUrl, body) { - var urlObject = new url.URL(requestUrl); - return validateRequest(authToken, twilioHeader, requestUrl, {}) && validateBody(body, urlObject.searchParams.get('bodySHA256')); -} - -function validateBody(body, bodyHash) { - var expectedHash = getExpectedBodyHash(body); - return scmp(Buffer.from(bodyHash), Buffer.from(expectedHash)); + const urlObject = new url.URL(requestUrl); + return validateRequest(authToken, twilioHeader, requestUrl, {}) && + validateBody(body, urlObject.searchParams.get('bodySHA256')); } /** From b4be75e9a1b858a0968a199ef007f61d08ac91a7 Mon Sep 17 00:00:00 2001 From: Elise Shanholtz Date: Fri, 18 Oct 2019 16:15:30 -0700 Subject: [PATCH 2/4] added url builder utility function and validation unit tests --- lib/webhooks/webhooks.js | 38 ++++++++++++++++++++------- spec/validation.spec.js | 55 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 9 deletions(-) diff --git a/lib/webhooks/webhooks.js b/lib/webhooks/webhooks.js index 3970b55688..bb4a8ff80f 100644 --- a/lib/webhooks/webhooks.js +++ b/lib/webhooks/webhooks.js @@ -3,7 +3,27 @@ var crypto = require('crypto'); var _ = require('lodash'); var scmp = require('scmp'); -var url = require('url'); +var urllib = require('url'); + +/** + * Utility function to construct the URL string, since Node.js url library won't include standard port numbers + * + * @param {URL} parsedURL - The parsed url object that Twilio requested on your server + * @returns {string} - URL with standard port number included + */ +function buildUrlWithStandardPort(parsedUrl) { + let url = ''; + const port = parsedUrl.protocol === 'https:' ? ':443' : ':80'; + + url += parsedUrl.protocol ? parsedUrl.protocol + '//' : ''; + url += parsedUrl.username; + url += parsedUrl.password ? ':' + parsedUrl.password : ''; + url += (parsedUrl.username || parsedUrl.password) ? '@' : ''; + url += parsedUrl.host ? parsedUrl.host + port : ''; + url += parsedUrl.pathname + parsedUrl.search + parsedUrl.hash; + + return url; +} /** Utility function to add a port number to a URL @@ -13,7 +33,7 @@ var url = require('url'); */ function addPort(parsedUrl) { if (!parsedUrl.port) { - parsedUrl.port = parsedUrl.protocol === 'https' ? '443' : '80'; + return buildUrlWithStandardPort(parsedUrl); } return parsedUrl.toString(); } @@ -25,7 +45,7 @@ function addPort(parsedUrl) { @returns {string} - URL without port */ function removePort(parsedUrl) { - parsedUrl.port = null; + parsedUrl.port = ''; return parsedUrl.toString(); } @@ -72,7 +92,7 @@ function getExpectedBodyHash(body) { */ function validateRequest(authToken, twilioHeader, requestUrl, params) { twilioHeader = twilioHeader || ''; - const urlObject = new url.URL(requestUrl); + const urlObject = new urllib.URL(requestUrl); const urlWithPort = addPort(urlObject); const urlWithoutPort = removePort(urlObject); @@ -95,13 +115,13 @@ function validateBody(body, bodyHash) { @param {string} authToken - The auth token, as seen in the Twilio portal @param {string} twilioHeader - The value of the X-Twilio-Signature header from the request - @param {string} requestUrl - The full URL (with query string) you configured to handle this request + @param {string} url - The full URL (with query string) you configured to handle this request @param {string} body - The body of the request @returns {boolean} - valid */ -function validateRequestWithBody(authToken, twilioHeader, requestUrl, body) { - const urlObject = new url.URL(requestUrl); - return validateRequest(authToken, twilioHeader, requestUrl, {}) && +function validateRequestWithBody(authToken, twilioHeader, url, body) { + const urlObject = new urllib.URL(url); + return validateRequest(authToken, twilioHeader, url, {}) && validateBody(body, urlObject.searchParams.get('bodySHA256')); } @@ -128,7 +148,7 @@ function validateExpressRequest(request, authToken, opts) { var protocol = options.protocol || request.protocol; var host = options.host || request.headers.host; - webhookUrl = url.format({ + webhookUrl = urllib.format({ protocol: protocol, host: host, pathname: request.originalUrl diff --git a/spec/validation.spec.js b/spec/validation.spec.js index b1a63be2a6..f2d710e5de 100644 --- a/spec/validation.spec.js +++ b/spec/validation.spec.js @@ -75,6 +75,61 @@ describe('Request validation', () => { expect(isValid).toBeFalsy(); }); + + it('should validate https urls with ports by stripping them', () => { + const requestUrlWithPort = requestUrl.replace('.com', '.com:1234'); + const isValid = validateRequest(token, defaultSignature, requestUrlWithPort, defaultParams); + + expect(isValid).toBeTruthy(); + }); + + it('should validate http urls with ports', () => { + let requestUrlWithPort = requestUrl.replace('.com', '.com:1234'); + requestUrlWithPort = requestUrlWithPort.replace('https', 'http'); + const signature = 'Zmvh+3yNM1Phv2jhDCwEM3q5ebU='; // hash of http url with port 1234 + const isValid = validateRequest(token, signature, requestUrlWithPort, defaultParams); + + expect(isValid).toBeTruthy(); + }); + + it('should validate https urls without ports by adding standard port 443', () => { + const signature = 'kvajT1Ptam85bY51eRf/AJRuM3w='; // hash of https url with port 443 + const isValid = validateRequest(token, signature, requestUrl, defaultParams); + + expect(isValid).toBeTruthy(); + }); + + it('should validate http urls without ports by adding standard port 80', () => { + const requestUrlHttp = requestUrl.replace('https', 'http'); + const signature = '0ZXoZLH/DfblKGATFgpif+LLRf4='; // hash of http url with port 80 + const isValid = validateRequest(token, signature, requestUrlHttp, defaultParams); + + expect(isValid).toBeTruthy(); + }); + + it('should validate urls with credentials', () => { + const urlWithCreds = 'https://user:pass@mycompany.com/myapp.php?foo=1&bar=2'; + const signature = 'CukzLTc1tT5dXEDIHm/tKBanW10='; // hash of this url + const isValid = validateRequest(token, signature, urlWithCreds, defaultParams); + + expect(isValid).toBeTruthy(); + }); + + it('should validate urls with just username', () => { + const urlWithCreds = 'https://user@mycompany.com/myapp.php?foo=1&bar=2'; + const signature = '2YRLlVAflCqxaNicjMpJcSTgzSs='; // hash of this url + const isValid = validateRequest(token, signature, urlWithCreds, defaultParams); + + expect(isValid).toBeTruthy(); + }); + + it('should validate urls with credentials by adding port', () => { + const urlWithCreds = 'https://user:pass@mycompany.com/myapp.php?foo=1&bar=2'; + const signature = 'ZQFR1PTIZXF2MXB8ZnKCvnnA+rI='; // hash of this url with port 443 + const isValid = validateRequest(token, signature, urlWithCreds, defaultParams); + + expect(isValid).toBeTruthy(); + }); }); describe('Request validation middleware', () => { From 75c86e936ac9e49f23130a2334cccfaac7834b68 Mon Sep 17 00:00:00 2001 From: Elise Shanholtz Date: Fri, 18 Oct 2019 16:19:47 -0700 Subject: [PATCH 3/4] fixed incorrect casing --- lib/webhooks/webhooks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/webhooks/webhooks.js b/lib/webhooks/webhooks.js index bb4a8ff80f..ba69747b14 100644 --- a/lib/webhooks/webhooks.js +++ b/lib/webhooks/webhooks.js @@ -8,7 +8,7 @@ var urllib = require('url'); /** * Utility function to construct the URL string, since Node.js url library won't include standard port numbers * - * @param {URL} parsedURL - The parsed url object that Twilio requested on your server + * @param {URL} parsedUrl - The parsed url object that Twilio requested on your server * @returns {string} - URL with standard port number included */ function buildUrlWithStandardPort(parsedUrl) { From d074d60406e7f2402d651ce121d2a6edb92dc724 Mon Sep 17 00:00:00 2001 From: Elise Shanholtz Date: Mon, 21 Oct 2019 09:32:54 -0700 Subject: [PATCH 4/4] fixed parameter name change and added comments --- lib/webhooks/webhooks.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/webhooks/webhooks.js b/lib/webhooks/webhooks.js index ba69747b14..86bc7c2fe0 100644 --- a/lib/webhooks/webhooks.js +++ b/lib/webhooks/webhooks.js @@ -86,16 +86,20 @@ function getExpectedBodyHash(body) { @param {string} authToken - The auth token, as seen in the Twilio portal @param {string} twilioHeader - The value of the X-Twilio-Signature header from the request - @param {string} requestUrl - The full URL (with query string) you configured to handle this request + @param {string} url - The full URL (with query string) you configured to handle this request @param {object} params - the parameters sent with this request @returns {boolean} - valid */ -function validateRequest(authToken, twilioHeader, requestUrl, params) { +function validateRequest(authToken, twilioHeader, url, params) { twilioHeader = twilioHeader || ''; - const urlObject = new urllib.URL(requestUrl); + const urlObject = new urllib.URL(url); const urlWithPort = addPort(urlObject); const urlWithoutPort = removePort(urlObject); + /* + * Check signature of the url with and without the port number + * since signature generation on the back end is inconsistent + */ const signatureWithPort = getExpectedTwilioSignature(authToken, urlWithPort, params); const signatureWithoutPort = getExpectedTwilioSignature(authToken, urlWithoutPort, params); const validSignatureWithPort = scmp(Buffer.from(twilioHeader), Buffer.from(signatureWithPort));