[Console] Handle encoded characters in API requests (#136788)

* Revert "Revert "[Console] Handle encoded characters in API requests (#135441)" (#136750)"

This reverts commit 44d4b2a8d7.

* Debug

* Fix requests failing on cloud

Co-authored-by: Muhammad Ibragimov <muhammad.ibragimov@elastic.co>

src/plugins/console/server/lib/proxy_request.test.ts

@@ -6,11 +6,12 @@
  * Side Public License, v 1.
  */
 
-import http, { ClientRequest } from 'http';
+import http, { ClientRequest, OutgoingHttpHeaders } from 'http';
 import * as sinon from 'sinon';
 import { proxyRequest } from './proxy_request';
 import { URL } from 'url';
 import { fail } from 'assert';
+import { toURL } from './utils';
 
 describe(`Console's send request`, () => {
   let sandbox: sinon.SinonSandbox;
@@ -29,6 +30,25 @@ describe(`Console's send request`, () => {
     fakeRequest = null as any;
   });
 
+  const sendProxyRequest = async ({
+    headers = {},
+    uri = new URL('http://noone.nowhere.none'),
+    timeout = 3000,
+  }: {
+    headers?: OutgoingHttpHeaders;
+    uri?: URL;
+    timeout?: number;
+  }) => {
+    return await proxyRequest({
+      agent: null as any,
+      headers,
+      method: 'get',
+      payload: null as any,
+      uri,
+      timeout,
+    });
+  };
+
   it('correctly implements timeout and abort mechanism', async () => {
     fakeRequest = {
       destroy: sinon.stub(),
@@ -36,14 +56,7 @@ describe(`Console's send request`, () => {
       once() {},
     } as any;
     try {
-      await proxyRequest({
-        agent: null as any,
-        headers: {},
-        method: 'get',
-        payload: null as any,
-        timeout: 0, // immediately timeout
-        uri: new URL('http://noone.nowhere.none'),
-      });
+      await sendProxyRequest({ timeout: 0 }); // immediately timeout
       fail('Should not reach here!');
     } catch (e) {
       expect(e.message).toEqual('Client request timeout');
@@ -63,16 +76,9 @@ describe(`Console's send request`, () => {
     } as any;
 
     // Don't set a host header this time
-    const result1 = await proxyRequest({
-      agent: null as any,
-      headers: {},
-      method: 'get',
-      payload: null as any,
-      timeout: 30000,
-      uri: new URL('http://noone.nowhere.none'),
-    });
+    const defaultResult = await sendProxyRequest({});
 
-    expect(result1).toEqual('done');
+    expect(defaultResult).toEqual('done');
 
     const [httpRequestOptions1] = stub.firstCall.args;
 
@@ -83,16 +89,9 @@ describe(`Console's send request`, () => {
     });
 
     // Set a host header
-    const result2 = await proxyRequest({
-      agent: null as any,
-      headers: { Host: 'myhost' },
-      method: 'get',
-      payload: null as any,
-      timeout: 30000,
-      uri: new URL('http://noone.nowhere.none'),
-    });
+    const resultWithHostHeader = await sendProxyRequest({ headers: { Host: 'myhost' } });
 
-    expect(result2).toEqual('done');
+    expect(resultWithHostHeader).toEqual('done');
 
     const [httpRequestOptions2] = stub.secondCall.args;
     expect((httpRequestOptions2 as any).headers).toEqual({
@@ -102,7 +101,7 @@ describe(`Console's send request`, () => {
     });
   });
 
-  describe('with percent-encoded uri pathname', () => {
+  describe('with request path', () => {
     beforeEach(() => {
       fakeRequest = {
         abort: sinon.stub(),
@@ -115,39 +114,42 @@ describe(`Console's send request`, () => {
       } as any;
     });
 
-    it('should decode percent-encoded uri pathname and encode it correctly', async () => {
-      const uri = new URL(
-        `http://noone.nowhere.none/%{[@metadata][beat]}-%{[@metadata][version]}-2020.08.23`
-      );
-      const result = await proxyRequest({
-        agent: null as any,
-        headers: {},
-        method: 'get',
-        payload: null as any,
-        timeout: 30000,
-        uri,
+    const verifyRequestPath = async ({
+      initialPath,
+      expectedPath,
+    }: {
+      initialPath: string;
+      expectedPath: string;
+      uri?: URL;
+    }) => {
+      const result = await sendProxyRequest({
+        uri: toURL('http://noone.nowhere.none', initialPath),
       });
-
       expect(result).toEqual('done');
       const [httpRequestOptions] = stub.firstCall.args;
-      expect((httpRequestOptions as any).path).toEqual(
-        '/%25%7B%5B%40metadata%5D%5Bbeat%5D%7D-%25%7B%5B%40metadata%5D%5Bversion%5D%7D-2020.08.23'
-      );
+      expect((httpRequestOptions as any).path).toEqual(expectedPath);
+    };
+
+    it('should correctly encode invalid URL characters included in path', async () => {
+      await verifyRequestPath({
+        initialPath: '%{[@metadata][beat]}-%{[@metadata][version]}-2020.08.23',
+        expectedPath:
+          '/%25%7B%5B%40metadata%5D%5Bbeat%5D%7D-%25%7B%5B%40metadata%5D%5Bversion%5D%7D-2020.08.23?pretty=true',
+      });
     });
 
-    it('should issue request with date-math format', async () => {
-      const result = await proxyRequest({
-        agent: null as any,
-        headers: {},
-        method: 'get',
-        payload: null as any,
-        timeout: 30000,
-        uri: new URL(`http://noone.nowhere.none/%3Cmy-index-%7Bnow%2Fd%7D%3E`),
+    it('should not encode the path if it is encoded', async () => {
+      await verifyRequestPath({
+        initialPath: '%3Cmy-index-%7Bnow%2Fd%7D%3E',
+        expectedPath: '/%3Cmy-index-%7Bnow%2Fd%7D%3E?pretty=true',
       });
+    });
 
-      expect(result).toEqual('done');
-      const [httpRequestOptions] = stub.firstCall.args;
-      expect((httpRequestOptions as any).path).toEqual('/%3Cmy-index-%7Bnow%2Fd%7D%3E');
+    it('should correctly encode path with query params', async () => {
+      await verifyRequestPath({
+        initialPath: '_index/.test?q=something&v=something',
+        expectedPath: '/_index/.test?q=something&v=something&pretty=true',
+      });
     });
   });
 });

src/plugins/console/server/lib/proxy_request.ts

@@ -11,8 +11,7 @@ import https from 'https';
 import net from 'net';
 import stream from 'stream';
 import Boom from '@hapi/boom';
-import { URL, URLSearchParams } from 'url';
-import { trimStart } from 'lodash';
+import { URL } from 'url';
 
 interface Args {
   method: 'get' | 'post' | 'put' | 'delete' | 'patch' | 'head';
@@ -31,22 +30,6 @@ interface Args {
 const sanitizeHostname = (hostName: string): string =>
   hostName.trim().replace(/^\[/, '').replace(/\]$/, '');
 
-/**
- * Node URL percent-encodes any invalid characters in the pathname which results a 400 bad request error.
- * We need to decode the percent-encoded pathname, and encode it correctly with encodeURIComponent
- */
-
-const encodePathname = (pathname: string) => {
-  const decodedPath = new URLSearchParams(`path=${pathname}`).get('path') ?? '';
-
-  // Skip if it is valid
-  if (pathname === decodedPath) {
-    return pathname;
-  }
-
-  return `/${encodeURIComponent(trimStart(decodedPath, '/'))}`;
-};
-
 // We use a modified version of Hapi's Wreck because Hapi, Axios, and Superagent don't support GET requests
 // with bodies, but ES APIs do. Similarly with DELETE requests with bodies. Another library, `request`
 // diverged too much from current behaviour.
@@ -59,9 +42,9 @@ export const proxyRequest = ({
   payload,
   rejectUnauthorized,
 }: Args) => {
-  const { hostname, port, protocol, pathname, search } = uri;
+  const { hostname, port, protocol, search, pathname } = uri;
   const client = uri.protocol === 'https:' ? https : http;
-  const encodedPath = encodePathname(pathname);
+
   let resolved = false;
 
   let resolve: (res: http.IncomingMessage) => void;
@@ -84,7 +67,7 @@ export const proxyRequest = ({
     host: sanitizeHostname(hostname),
     port: port === '' ? undefined : parseInt(port, 10),
     protocol,
-    path: `${encodedPath}${search || ''}`,
+    path: `${pathname}${search || ''}`,
     headers: {
       ...finalUserHeaders,
       'content-type': 'application/json',

src/plugins/console/server/lib/utils/encode_path.test.ts

@@ -0,0 +1,37 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import { encodePath } from './encode_path';
+
+describe('encodePath', () => {
+  const tests = [
+    {
+      description: 'encodes invalid URL characters',
+      source: '/%{[@metadata][beat]}-%{[@metadata][version]}-2020.08.23',
+      assert:
+        '/%25%7B%5B%40metadata%5D%5Bbeat%5D%7D-%25%7B%5B%40metadata%5D%5Bversion%5D%7D-2020.08.23',
+    },
+    {
+      description: 'ignores encoded characters',
+      source: '/my-index/_doc/this%2Fis%2Fa%2Fdoc',
+      assert: '/my-index/_doc/this%2Fis%2Fa%2Fdoc',
+    },
+    {
+      description: 'ignores slashes between',
+      source: '_index/test/.test',
+      assert: '_index/test/.test',
+    },
+  ];
+
+  tests.forEach(({ description, source, assert }) => {
+    test(description, () => {
+      const result = encodePath(source);
+      expect(result).toEqual(assert);
+    });
+  });
+});

src/plugins/console/server/lib/utils/encode_path.ts

@@ -0,0 +1,28 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import { URLSearchParams } from 'url';
+import { trimStart } from 'lodash';
+
+export const encodePath = (path: string) => {
+  const decodedPath = new URLSearchParams(`path=${path}`).get('path') ?? '';
+  // Take the initial path and compare it with the decoded path.
+  // If the result is not the same, the path is encoded.
+  const isEncoded = trimStart(path, '/') !== trimStart(decodedPath, '/');
+
+  // Return the initial path if it is already encoded
+  if (isEncoded) {
+    return path;
+  }
+
+  // Encode every component except slashes
+  return path
+    .split('/')
+    .map((component) => encodeURIComponent(component))
+    .join('/');
+};

src/plugins/console/server/lib/utils/index.ts

@@ -0,0 +1,10 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+export { encodePath } from './encode_path';
+export { toURL } from './to_url';

src/plugins/console/server/lib/utils/to_url.test.ts

@@ -0,0 +1,38 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import { toURL } from './to_url';
+
+describe('toURL', () => {
+  it('should replace + sign in query params with %2b', () => {
+    const urlResult = toURL(
+      'http://nowhere.none',
+      'test/?q=create_date:[2020-05-10T08:00:00.000+08:00 TO *]'
+    );
+    expect(urlResult.search).toEqual(
+      '?q=create_date%3A%5B2020-05-10T08%3A00%3A00.000%2B08%3A00+TO+*%5D&pretty=true'
+    );
+  });
+
+  describe('with a path without the "pretty" search param', () => {
+    it('should append the "pretty" search param', () => {
+      const urlResult = toURL('http://nowhere.none', 'test');
+      expect(urlResult.href).toEqual('http://nowhere.none/test?pretty=true');
+    });
+  });
+
+  it('should handle encoding pathname', () => {
+    const urlResult = toURL(
+      'http://nowhere.none',
+      '/%{[@metadata][beat]}-%{[@metadata][version]}-2020.08.23'
+    );
+    expect(urlResult.pathname).toEqual(
+      '/%25%7B%5B%40metadata%5D%5Bbeat%5D%7D-%25%7B%5B%40metadata%5D%5Bversion%5D%7D-2020.08.23'
+    );
+  });
+});

src/plugins/console/server/lib/utils/to_url.ts

@@ -0,0 +1,34 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+import { URL } from 'url';
+import { trimEnd, trimStart } from 'lodash';
+import { encodePath } from './encode_path';
+
+export function toURL(base: string, path: string) {
+  const [pathname, query = ''] = path.split('?');
+
+  // if there is a '+' sign in query e.g. ?q=create_date:[2020-05-10T08:00:00.000+08:00 TO *]
+  // node url encodes it as a whitespace which results in a faulty request
+  // we need to replace '+' with '%2b' to encode it correctly
+  if (/\+/g.test(query)) {
+    path = `${pathname}?${query.replace(/\+/g, '%2b')}`;
+  }
+  const urlResult = new URL(`${trimEnd(base, '/')}/${trimStart(path, '/')}`);
+  // Appending pretty here to have Elasticsearch do the JSON formatting, as doing
+  // in JS can lead to data loss (7.0 will get munged into 7, thus losing indication of
+  // measurement precision)
+  if (!urlResult.searchParams.get('pretty')) {
+    urlResult.searchParams.append('pretty', 'true');
+  }
+
+  // Node URL percent-encodes any invalid characters in url, which results in a bad request in some cases. E.g. urls with % character
+  // To fix this, we set the pathname to the correctly encoded path here
+  urlResult.pathname = encodePath(pathname);
+  return urlResult;
+}

src/plugins/console/server/routes/api/console/proxy/create_handler.ts

@@ -7,8 +7,7 @@
  */
 
 import { Agent, IncomingMessage } from 'http';
-import * as url from 'url';
-import { pick, trimStart, trimEnd } from 'lodash';
+import { pick } from 'lodash';
 import { SemVer } from 'semver';
 
 import { KibanaRequest, RequestHandler } from '@kbn/core/server';
@@ -27,25 +26,7 @@ import {
 import { RouteDependencies } from '../../..';
 
 import { Body, Query } from './validation_config';
-
-function toURL(base: string, path: string) {
-  const [p, query = ''] = path.split('?');
-
-  // if there is a '+' sign in query e.g. ?q=create_date:[2020-05-10T08:00:00.000+08:00 TO *]
-  // node url encodes it as a whitespace which results in a faulty request
-  // we need to replace '+' with '%2b' to encode it correctly
-  if (/\+/g.test(query)) {
-    path = `${p}?${query.replace(/\+/g, '%2b')}`;
-  }
-  const urlResult = new url.URL(`${trimEnd(base, '/')}/${trimStart(path, '/')}`);
-  // Appending pretty here to have Elasticsearch do the JSON formatting, as doing
-  // in JS can lead to data loss (7.0 will get munged into 7, thus losing indication of
-  // measurement precision)
-  if (!urlResult.searchParams.get('pretty')) {
-    urlResult.searchParams.append('pretty', 'true');
-  }
-  return urlResult;
-}
+import { toURL } from '../../../../lib/utils';
 
 function filterHeaders(originalHeaders: object, headersToKeep: string[]): object {
   const normalizeHeader = function (header: string) {

src/plugins/console/server/routes/api/console/proxy/query_string.test.ts

@@ -43,7 +43,7 @@ describe('Console Proxy Route', () => {
           await request('GET', 'http://evil.com/test');
           expect(proxyRequestMock.mock.calls.length).toBe(1);
           const [[args]] = (requestModule.proxyRequest as jest.Mock).mock.calls;
-          expect(args.uri.href).toBe('http://localhost:9200/http://evil.com/test?pretty=true');
+          expect(args.uri.href).toBe('http://localhost:9200/http%3A//evil.com/test?pretty=true');
         });
       });
       describe('starts with a slash', () => {

test/functional/apps/console/_console.ts

@@ -126,6 +126,22 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
       });
     });
 
+    describe('with query params', () => {
+      it('should issue a successful request', async () => {
+        await PageObjects.console.clearTextArea();
+        await PageObjects.console.enterRequest(
+          '\n GET _cat/aliases?format=json&v=true&pretty=true'
+        );
+        await PageObjects.console.clickPlay();
+        await PageObjects.header.waitUntilLoadingHasFinished();
+
+        await retry.try(async () => {
+          const status = await PageObjects.console.getResponseStatus();
+          expect(status).to.eql(200);
+        });
+      });
+    });
+
     describe('multiple requests output', () => {
       const sendMultipleRequests = async (requests: string[]) => {
         await asyncForEach(requests, async (request) => {