Merge pull request #6 from change/chris/fix-domain-vulnerability

Fix domain vulnerability

api/store.js

@@ -37,13 +37,18 @@ class HttpError extends Error {
 }
 
 async function validate(longUrl) {
-  let host;
+  let parsed;
   debugLog(`validating longUrl ${longUrl}`);
   try {
-    ({ host } = url.parse(longUrl));
+    parsed = url.parse(longUrl);
   } catch (ex) {
     throw new HttpError(400, 'Not a valid URL');
   }
+  const { host, path } = parsed;
+  if (path.charAt(0) !== '/') {
+    // Disallow anything with a doctored path
+    throw new HttpError(400, 'Not a valid URL for shortening');
+  }
   const suffixMatcher = suffix => suffix === host || endsWith(host, `.${suffix}`);
   if (!find(domainSafeList, suffixMatcher)) {
     throw new HttpError(400, `Not an allowed domain for shortening: ${host}`);

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@change-org/longlinks",
-  "version": "0.1.2",
+  "version": "0.1.3",
   "description": "A serverless URL shortener that leverages Amazon Lambda for short link creation, and S3 for link storage and redirection.",
   "author": "Change Engineering",
   "license": "MIT",

test/api.test.js

@@ -126,6 +126,11 @@ describe('When the `url` property', () => {
     () => expectStatusForUrl('https://evildomain1.com', 400)
   );
 
+  test(
+    'even more sneakily prepends the allowed domain to a malicious domain with an encoded character',
+    () => expectStatusForUrl('https://www.domain1.com%2Eevil.com?foo=bar', 400)
+  );
+
   test(
     'contains an allowed domain string, but in the path. Nice try but still 400',
     () => expectStatusForUrl('https://www.evil.com/https://www.domain1.com', 400)