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

* [Console] Handle encoded characters in API requests

* Add a functional test for requests with query params

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

src/plugins/console/public/application/components/network_request_status_bar/network_request_status_bar.tsx

@@ -80,7 +80,10 @@ export const NetworkRequestStatusBar: FunctionComponent<Props> = ({
               }`}</EuiText>
             }
           >
-            <EuiBadge color={mapStatusCodeToBadgeColor(statusCode)}>
+            <EuiBadge
+              data-test-subj="consoleResponseStatusBadge"
+              color={mapStatusCodeToBadgeColor(statusCode)}
+            >
               {/*  Use &nbsp; to ensure that no matter the width we don't allow line breaks */}
               {statusCode}&nbsp;-&nbsp;{statusText}
             </EuiBadge>

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

@@ -6,7 +6,7 @@
  * 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';
@@ -29,6 +29,28 @@ describe(`Console's send request`, () => {
     fakeRequest = null as any;
   });
 
+  const sendProxyRequest = async ({
+    headers = {},
+    uri = new URL('http://noone.nowhere.none'),
+    timeout = 3000,
+    requestPath = '',
+  }: {
+    headers?: OutgoingHttpHeaders;
+    uri?: URL;
+    timeout?: number;
+    requestPath?: string;
+  }) => {
+    return await proxyRequest({
+      agent: null as any,
+      headers,
+      method: 'get',
+      payload: null as any,
+      uri,
+      timeout,
+      requestPath,
+    });
+  };
+
   it('correctly implements timeout and abort mechanism', async () => {
     fakeRequest = {
       destroy: sinon.stub(),
@@ -36,14 +58,7 @@ describe(`Console's send request`, () => {
       once() {},
     } as any;
     try {
-      await proxyRequest({
+      await sendProxyRequest({ timeout: 0 }); // immediately timeout
-        agent: null as any,
-        headers: {},
-        method: 'get',
-        payload: null as any,
-        timeout: 0, // immediately timeout
-        uri: new URL('http://noone.nowhere.none'),
-      });
       fail('Should not reach here!');
     } catch (e) {
       expect(e.message).toEqual('Client request timeout');
@@ -63,16 +78,9 @@ describe(`Console's send request`, () => {
     } as any;
 
     // Don't set a host header this time
-    const result1 = await proxyRequest({
+    const defaultResult = await sendProxyRequest({});
-      agent: null as any,
-      headers: {},
-      method: 'get',
-      payload: null as any,
-      timeout: 30000,
-      uri: new URL('http://noone.nowhere.none'),
-    });
 
-    expect(result1).toEqual('done');
+    expect(defaultResult).toEqual('done');
 
     const [httpRequestOptions1] = stub.firstCall.args;
 
@@ -83,16 +91,9 @@ describe(`Console's send request`, () => {
     });
 
     // Set a host header
-    const result2 = await proxyRequest({
+    const resultWithHostHeader = await sendProxyRequest({ headers: { Host: 'myhost' } });
-      agent: null as any,
-      headers: { Host: 'myhost' },
-      method: 'get',
-      payload: null as any,
-      timeout: 30000,
-      uri: new URL('http://noone.nowhere.none'),
-    });
 
-    expect(result2).toEqual('done');
+    expect(resultWithHostHeader).toEqual('done');
 
     const [httpRequestOptions2] = stub.secondCall.args;
     expect((httpRequestOptions2 as any).headers).toEqual({
@@ -102,7 +103,7 @@ describe(`Console's send request`, () => {
     });
   });
 
-  describe('with percent-encoded uri pathname', () => {
+  describe('with request path', () => {
     beforeEach(() => {
       fakeRequest = {
         abort: sinon.stub(),
@@ -115,39 +116,45 @@ describe(`Console's send request`, () => {
       } as any;
     });
 
-    it('should decode percent-encoded uri pathname and encode it correctly', async () => {
+    const verifyRequestPath = async ({
-      const uri = new URL(
+      initialPath,
-        `http://noone.nowhere.none/%{[@metadata][beat]}-%{[@metadata][version]}-2020.08.23`
+      expectedPath,
-      );
+      uri,
-      const result = await proxyRequest({
+    }: {
-        agent: null as any,
+      initialPath: string;
-        headers: {},
+      expectedPath: string;
-        method: 'get',
+      uri?: URL;
-        payload: null as any,
+    }) => {
-        timeout: 30000,
+      const result = await sendProxyRequest({
+        requestPath: initialPath,
         uri,
       });
-
       expect(result).toEqual('done');
       const [httpRequestOptions] = stub.firstCall.args;
-      expect((httpRequestOptions as any).path).toEqual(
+      expect((httpRequestOptions as any).path).toEqual(expectedPath);
-        '/%25%7B%5B%40metadata%5D%5Bbeat%5D%7D-%25%7B%5B%40metadata%5D%5Bversion%5D%7D-2020.08.23'
+    };
-      );
+
+    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',
+      });
     });
 
-    it('should issue request with date-math format', async () => {
+    it('should not encode the path if it is encoded', async () => {
-      const result = await proxyRequest({
+      await verifyRequestPath({
-        agent: null as any,
+        initialPath: '%3Cmy-index-%7Bnow%2Fd%7D%3E',
-        headers: {},
+        expectedPath: '%3Cmy-index-%7Bnow%2Fd%7D%3E',
-        method: 'get',
-        payload: null as any,
-        timeout: 30000,
-        uri: new URL(`http://noone.nowhere.none/%3Cmy-index-%7Bnow%2Fd%7D%3E`),
       });
+    });
 
-      expect(result).toEqual('done');
+    it('should correctly encode path with query params', async () => {
-      const [httpRequestOptions] = stub.firstCall.args;
+      await verifyRequestPath({
-      expect((httpRequestOptions as any).path).toEqual('/%3Cmy-index-%7Bnow%2Fd%7D%3E');
+        initialPath: '_index/.test',
+        uri: new URL('http://noone.nowhere.none/_index/.test?q=something&v=something'),
+        expectedPath: '_index/.test?q=something&v=something',
+      });
     });
   });
 });

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

@@ -11,8 +11,9 @@ import https from 'https';
 import net from 'net';
 import stream from 'stream';
 import Boom from '@hapi/boom';
-import { URL, URLSearchParams } from 'url';
+import { URL } from 'url';
-import { trimStart } from 'lodash';
+
+import { encodePath } from './utils';
 
 interface Args {
   method: 'get' | 'post' | 'put' | 'delete' | 'patch' | 'head';
@@ -22,6 +23,7 @@ interface Args {
   timeout: number;
   headers: http.OutgoingHttpHeaders;
   rejectUnauthorized?: boolean;
+  requestPath: string;
 }
 
 /**
@@ -31,22 +33,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.
@@ -58,10 +44,11 @@ export const proxyRequest = ({
   timeout,
   payload,
   rejectUnauthorized,
+  requestPath,
 }: Args) => {
-  const { hostname, port, protocol, pathname, search } = uri;
+  const { hostname, port, protocol, search } = uri;
   const client = uri.protocol === 'https:' ? https : http;
-  const encodedPath = encodePathname(pathname);
+  const encodedPath = encodePath(requestPath);
   let resolved = false;
 
   let resolve: (res: http.IncomingMessage) => void;

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,9 @@
+/*
+ * 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';

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

@@ -145,6 +145,10 @@ export const createHandler =
       const host = hosts[idx];
       try {
         const uri = toURL(host, path);
+        // Invalid URL characters included in uri pathname will be percent-encoded by Node URL method, and results in a faulty request in some cases.
+        // To fix this issue, we need to extract the original request path and supply it to proxyRequest function to encode it correctly with encodeURIComponent.
+        // We ignore the search params here, since we are extracting them from the uri constructed by Node URL method.
+        const [requestPath] = path.split('?');
 
         // Because this can technically be provided by a settings-defined proxy config, we need to
         // preserve these property names to maintain BWC.
@@ -174,6 +178,7 @@ export const createHandler =
           payload: body,
           rejectUnauthorized,
           agent,
+          requestPath,
         });
 
         break;

test/functional/apps/console/_console.ts

@@ -124,6 +124,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) => {

test/functional/page_objects/console_page.ts

@@ -221,4 +221,10 @@ export class ConsolePageObject extends FtrService {
       return false;
     }
   }
+
+  public async getResponseStatus() {
+    const statusBadge = await this.testSubjects.find('consoleResponseStatusBadge');
+    const text = await statusBadge.getVisibleText();
+    return text.replace(/[^\d.]+/, '');
+  }
 }