Improve short-url redirect validation (#84366) (#84426)

elastic · Nov 26, 2020 · 5dc26e5 · 5dc26e5
1 parent 681991d
commit 5dc26e5
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 25 deletions.
diff --git a/src/plugins/share/server/routes/lib/short_url_assert_valid.test.ts b/src/plugins/share/server/routes/lib/short_url_assert_valid.test.ts
@@ -19,36 +19,39 @@
 
 import { shortUrlAssertValid } from './short_url_assert_valid';
 
+const PROTOCOL_ERROR = /^Short url targets cannot have a protocol/;
+const HOSTNAME_ERROR = /^Short url targets cannot have a hostname/;
+const PATH_ERROR = /^Short url target path must be in the format/;
+
 describe('shortUrlAssertValid()', () => {
   const invalid = [
-    ['protocol', 'http://localhost:5601/app/kibana'],
-    ['protocol', 'https://localhost:5601/app/kibana'],
-    ['protocol', 'mailto:foo@bar.net'],
-    ['protocol', 'javascript:alert("hi")'], // eslint-disable-line no-script-url
-    ['hostname', 'localhost/app/kibana'],
-    ['hostname and port', 'local.host:5601/app/kibana'],
-    ['hostname and auth', 'user:pass@localhost.net/app/kibana'],
-    ['path traversal', '/app/../../not-kibana'],
-    ['deep path', '/app/kibana/foo'],
-    ['deep path', '/app/kibana/foo/bar'],
-    ['base path', '/base/app/kibana'],
+    ['protocol', 'http://localhost:5601/app/kibana', PROTOCOL_ERROR],
+    ['protocol', 'https://localhost:5601/app/kibana', PROTOCOL_ERROR],
+    ['protocol', 'mailto:foo@bar.net', PROTOCOL_ERROR],
+    ['protocol', 'javascript:alert("hi")', PROTOCOL_ERROR], // eslint-disable-line no-script-url
+    ['hostname', 'localhost/app/kibana', PATH_ERROR], // according to spec, this is not a valid URL -- you cannot specify a hostname without a protocol
+    ['hostname and port', 'local.host:5601/app/kibana', PROTOCOL_ERROR], // parser detects 'local.host' as the protocol
+    ['hostname and auth', 'user:pass@localhost.net/app/kibana', PROTOCOL_ERROR], // parser detects 'user' as the protocol
+    ['path traversal', '/app/../../not-kibana', PATH_ERROR], // fails because there are >2 path parts
+    ['path traversal', '/../not-kibana', PATH_ERROR], // fails because first path part is not 'app'
+    ['deep path', '/app/kibana/foo', PATH_ERROR], // fails because there are >2 path parts
+    ['deeper path', '/app/kibana/foo/bar', PATH_ERROR], // fails because there are >2 path parts
+    ['base path', '/base/app/kibana', PATH_ERROR], // fails because there are >2 path parts
+    ['path with an extra leading slash', '//foo/app/kibana', HOSTNAME_ERROR], // parser detects 'foo' as the hostname
+    ['path with an extra leading slash', '///app/kibana', HOSTNAME_ERROR], // parser detects '' as the hostname
+    ['path without app', '/foo/kibana', PATH_ERROR], // fails because first path part is not 'app'
+    ['path without appId', '/app/', PATH_ERROR], // fails because there is only one path part (leading and trailing slashes are trimmed)
   ];
 
-  invalid.forEach(([desc, url]) => {
-    it(`fails when url has ${desc}`, () => {
-      try {
-        shortUrlAssertValid(url);
-        throw new Error(`expected assertion to throw`);
-      } catch (err) {
-        if (!err || !err.isBoom) {
-          throw err;
-        }
-      }
+  invalid.forEach(([desc, url, error]) => {
+    it(`fails when url has ${desc as string}`, () => {
+      expect(() => shortUrlAssertValid(url as string)).toThrowError(error);
     });
   });
 
   const valid = [
     '/app/kibana',
+    '/app/kibana/', // leading and trailing slashes are trimmed
     '/app/monitoring#angular/route',
     '/app/text#document-id',
     '/app/some?with=query',

diff --git a/src/plugins/share/server/routes/lib/short_url_assert_valid.ts b/src/plugins/share/server/routes/lib/short_url_assert_valid.ts
@@ -22,18 +22,22 @@ import { trim } from 'lodash';
 import Boom from '@hapi/boom';
 
 export function shortUrlAssertValid(url: string) {
-  const { protocol, hostname, pathname } = parse(url);
+  const { protocol, hostname, pathname } = parse(
+    url,
+    false /* parseQueryString */,
+    true /* slashesDenoteHost */
+  );
 
-  if (protocol) {
+  if (protocol !== null) {
     throw Boom.notAcceptable(`Short url targets cannot have a protocol, found "${protocol}"`);
   }
 
-  if (hostname) {
+  if (hostname !== null) {
     throw Boom.notAcceptable(`Short url targets cannot have a hostname, found "${hostname}"`);
   }
 
   const pathnameParts = trim(pathname === null ? undefined : pathname, '/').split('/');
-  if (pathnameParts.length !== 2) {
+  if (pathnameParts.length !== 2 || pathnameParts[0] !== 'app' || !pathnameParts[1]) {
     throw Boom.notAcceptable(
       `Short url target path must be in the format "/app/{{appId}}", found "${pathname}"`
     );