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

added request validation for urls with and without ports #491

Merged
merged 4 commits into from
Oct 21, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 71 additions & 14 deletions lib/webhooks/webhooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,51 @@
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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should add a case for the gopher protocol (:
https://nodejs.org/api/url.html#url_url_port


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

@param {URL} parsedUrl - The parsed url object that Twilio requested on your server
@returns {string} - URL with port
*/
function addPort(parsedUrl) {
eshanholtz marked this conversation as resolved.
Show resolved Hide resolved
if (!parsedUrl.port) {
return buildUrlWithStandardPort(parsedUrl);
}
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 = '';
return parsedUrl.toString();
}

/**
Utility function to get the expected signature for a given request
Expand All @@ -18,7 +62,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'))
Expand Down Expand Up @@ -48,8 +92,25 @@ function getExpectedBodyHash(body) {
*/
function validateRequest(authToken, twilioHeader, url, params) {
twilioHeader = twilioHeader || '';
var expectedSignature = getExpectedTwilioSignature(authToken, url, params);
return scmp(Buffer.from(twilioHeader), Buffer.from(expectedSignature));
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));
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));
}

/**
Expand All @@ -58,18 +119,14 @@ function validateRequest(authToken, twilioHeader, url, params) {

@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) {
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));
function validateRequestWithBody(authToken, twilioHeader, url, body) {
const urlObject = new urllib.URL(url);
return validateRequest(authToken, twilioHeader, url, {}) &&
validateBody(body, urlObject.searchParams.get('bodySHA256'));
}

/**
Expand All @@ -95,7 +152,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
Expand Down
55 changes: 55 additions & 0 deletions spec/validation.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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:[email protected]/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://[email protected]/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', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be the same as the should validate urls with credentials test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one ensures standard ports are added to the URL for validation. Note the expected signatures of these two tests are different, but they both pass.

const urlWithCreds = 'https://user:[email protected]/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', () => {
Expand Down