[App Search] Add new encodePathParams helper (fixes unencoded documen…

…t IDs) (#88648) * Add encodePathParams helper to EnterpriseSearchRequestHandler This helper accomplishes two things: - Fixes 404s from the Enterprise Search server for user-generated IDs with special characters (e.g. ? char) - Allows us to simplify some of our createRequest calls - no longer will we have to grab request.params to create paths, this helper will do so for us * Update AS document route to use new helper - This was the primary view/API I was testing to fix this bug * Update remaining AS routes to use new helper - shorter, saves us a few lines + remove unnecessary payload: params that doesn't actually validate params Co-authored-by: Kibana Machine <[email protected]>
elastic · Jan 20, 2021 · f4f6cb6 · f4f6cb6
1 parent 8b1a228
commit f4f6cb6
Show file tree

Hide file tree

Showing 12 changed files with 100 additions and 108 deletions.
diff --git a/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts b/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts
@@ -116,16 +116,40 @@ describe('EnterpriseSearchRequestHandler', () => {
         );
       });
 
-      it('correctly encodes paths and query string parameters', async () => {
+      it('correctly encodes query string parameters', async () => {
         const requestHandler = enterpriseSearchRequestHandler.createRequest({
-          path: '/api/some example',
+          path: '/api/example',
         });
         await makeAPICall(requestHandler, { query: { 'page[current]': 1 } });
 
         EnterpriseSearchAPI.shouldHaveBeenCalledWith(
-          'http://localhost:3002/api/some%20example?page%5Bcurrent%5D=1'
+          'http://localhost:3002/api/example?page%5Bcurrent%5D=1'
         );
       });
+
+      describe('encodePathParams', () => {
+        it('correctly replaces :pathVariables with request.params', async () => {
+          const requestHandler = enterpriseSearchRequestHandler.createRequest({
+            path: '/api/examples/:example/some/:id',
+          });
+          await makeAPICall(requestHandler, { params: { example: 'hello', id: 'world' } });
+
+          EnterpriseSearchAPI.shouldHaveBeenCalledWith(
+            'http://localhost:3002/api/examples/hello/some/world'
+          );
+        });
+
+        it('correctly encodes path params as URI components', async () => {
+          const requestHandler = enterpriseSearchRequestHandler.createRequest({
+            path: '/api/examples/:example',
+          });
+          await makeAPICall(requestHandler, { params: { example: 'hello#@/$%^/&[]{}/";world' } });
+
+          EnterpriseSearchAPI.shouldHaveBeenCalledWith(
+            'http://localhost:3002/api/examples/hello%23%40%2F%24%25%5E%2F%26%5B%5D%7B%7D%2F%22%3Bworld'
+          );
+        });
+      });
     });
 
     describe('response passing', () => {

diff --git a/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts b/x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts
@@ -65,11 +65,12 @@ export class EnterpriseSearchRequestHandler {
     ) => {
       try {
         // Set up API URL
+        const encodedPath = this.encodePathParams(path, request.params as Record<string, string>);
         const queryParams = { ...(request.query as object), ...params };
         const queryString = !this.isEmptyObj(queryParams)
           ? `?${querystring.stringify(queryParams)}`
           : '';
-        const url = encodeURI(this.enterpriseSearchUrl + path) + queryString;
+        const url = encodeURI(this.enterpriseSearchUrl) + encodedPath + queryString;
 
         // Set up API options
         const { method } = request.route;
@@ -126,6 +127,36 @@ export class EnterpriseSearchRequestHandler {
     };
   }
 
+  /**
+   * This path helper is similar to React Router's generatePath, but much simpler &
+   * does not use regexes. It enables us to pass a static '/foo/:bar/baz' string to
+   * createRequest({ path }) and have :bar be automatically replaced by the value of
+   * request.params.bar.
+   * It also (very importantly) wraps all URL request params with encodeURIComponent(),
+   * which is an extra layer of encoding required by the Enterprise Search server in
+   * order to correctly & safely parse user-generated IDs with special characters in
+   * their names - just encodeURI alone won't work.
+   */
+  encodePathParams(path: string, params: Record<string, string>) {
+    const hasParams = path.includes(':');
+    if (!hasParams) {
+      return path;
+    } else {
+      return path
+        .split('/')
+        .map((pathPart) => {
+          const isParam = pathPart.startsWith(':');
+          if (!isParam) {
+            return pathPart;
+          } else {
+            const pathParam = pathPart.replace(':', '');
+            return encodeURIComponent(params[pathParam]);
+          }
+        })
+        .join('/');
+    }
+  }
+
   /**
    * Attempt to grab a usable error body from Enterprise Search - this isn't
    * always possible because some of our internal endpoints send back blank

diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/analytics.test.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/analytics.test.ts
@@ -27,12 +27,8 @@ describe('analytics routes', () => {
     });
 
     it('creates a request handler', () => {
-      mockRouter.callRoute({
-        params: { engineName: 'some-engine' },
-      });
-
       expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
-        path: '/as/engines/some-engine/analytics/queries',
+        path: '/as/engines/:engineName/analytics/queries',
       });
     });
 
@@ -84,12 +80,8 @@ describe('analytics routes', () => {
     });
 
     it('creates a request handler', () => {
-      mockRouter.callRoute({
-        params: { engineName: 'some-engine', query: 'some-query' },
-      });
-
       expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
-        path: '/as/engines/some-engine/analytics/query/some-query',
+        path: '/as/engines/:engineName/analytics/query/:query',
       });
     });
 

diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/analytics.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/analytics.ts
@@ -32,13 +32,9 @@ export function registerAnalyticsRoutes({
         query: schema.object(queriesSchema),
       },
     },
-    async (context, request, response) => {
-      const { engineName } = request.params;
-
-      return enterpriseSearchRequestHandler.createRequest({
-        path: `/as/engines/${engineName}/analytics/queries`,
-      })(context, request, response);
-    }
+    enterpriseSearchRequestHandler.createRequest({
+      path: '/as/engines/:engineName/analytics/queries',
+    })
   );
 
   router.get(
@@ -52,12 +48,8 @@ export function registerAnalyticsRoutes({
         query: schema.object(querySchema),
       },
     },
-    async (context, request, response) => {
-      const { engineName, query } = request.params;
-
-      return enterpriseSearchRequestHandler.createRequest({
-        path: `/as/engines/${engineName}/analytics/query/${query}`,
-      })(context, request, response);
-    }
+    enterpriseSearchRequestHandler.createRequest({
+      path: '/as/engines/:engineName/analytics/query/:query',
+    })
   );
 }
diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/credentials.test.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/credentials.test.ts
@@ -200,16 +200,8 @@ describe('credentials routes', () => {
     });
 
     it('creates a request to enterprise search', () => {
-      const mockRequest = {
-        params: {
-          name: 'abc123',
-        },
-      };
-
-      mockRouter.callRoute(mockRequest);
-
       expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
-        path: '/as/credentials/abc123',
+        path: '/as/credentials/:name',
       });
     });
 
@@ -311,7 +303,6 @@ describe('credentials routes', () => {
       mockRouter = new MockRouter({
         method: 'delete',
         path: '/api/app_search/credentials/{name}',
-        payload: 'params',
       });
 
       registerCredentialsRoutes({
@@ -321,16 +312,8 @@ describe('credentials routes', () => {
     });
 
     it('creates a request to enterprise search', () => {
-      const mockRequest = {
-        params: {
-          name: 'abc123',
-        },
-      };
-
-      mockRouter.callRoute(mockRequest);
-
       expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
-        path: '/as/credentials/abc123',
+        path: '/as/credentials/:name',
       });
     });
   });

diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/credentials.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/credentials.ts
@@ -81,11 +81,9 @@ export function registerCredentialsRoutes({
         body: tokenSchema,
       },
     },
-    async (context, request, response) => {
-      return enterpriseSearchRequestHandler.createRequest({
-        path: `/as/credentials/${request.params.name}`,
-      })(context, request, response);
-    }
+    enterpriseSearchRequestHandler.createRequest({
+      path: '/as/credentials/:name',
+    })
   );
   router.delete(
     {
@@ -96,10 +94,8 @@ export function registerCredentialsRoutes({
         }),
       },
     },
-    async (context, request, response) => {
-      return enterpriseSearchRequestHandler.createRequest({
-        path: `/as/credentials/${request.params.name}`,
-      })(context, request, response);
-    }
+    enterpriseSearchRequestHandler.createRequest({
+      path: '/as/credentials/:name',
+    })
   );
 }
diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/documents.test.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/documents.test.ts
@@ -27,13 +27,8 @@ describe('documents routes', () => {
     });
 
     it('creates a request to enterprise search', () => {
-      mockRouter.callRoute({
-        params: { engineName: 'some-engine' },
-        body: { documents: [{ foo: 'bar' }] },
-      });
-
       expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
-        path: '/as/engines/some-engine/documents/new',
+        path: '/as/engines/:engineName/documents/new',
       });
     });
 
@@ -79,10 +74,8 @@ describe('document routes', () => {
     });
 
     it('creates a request to enterprise search', () => {
-      mockRouter.callRoute({ params: { engineName: 'some-engine', documentId: '1' } });
-
       expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
-        path: '/as/engines/some-engine/documents/1',
+        path: '/as/engines/:engineName/documents/:documentId',
       });
     });
   });
@@ -104,10 +97,8 @@ describe('document routes', () => {
     });
 
     it('creates a request to enterprise search', () => {
-      mockRouter.callRoute({ params: { engineName: 'some-engine', documentId: '1' } });
-
       expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
-        path: '/as/engines/some-engine/documents/1',
+        path: '/as/engines/:engineName/documents/:documentId',
       });
     });
   });

diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/documents.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/documents.ts
@@ -24,11 +24,9 @@ export function registerDocumentsRoutes({
         }),
       },
     },
-    async (context, request, response) => {
-      return enterpriseSearchRequestHandler.createRequest({
-        path: `/as/engines/${request.params.engineName}/documents/new`,
-      })(context, request, response);
-    }
+    enterpriseSearchRequestHandler.createRequest({
+      path: `/as/engines/:engineName/documents/new`,
+    })
   );
 }
 
@@ -46,11 +44,9 @@ export function registerDocumentRoutes({
         }),
       },
     },
-    async (context, request, response) => {
-      return enterpriseSearchRequestHandler.createRequest({
-        path: `/as/engines/${request.params.engineName}/documents/${request.params.documentId}`,
-      })(context, request, response);
-    }
+    enterpriseSearchRequestHandler.createRequest({
+      path: `/as/engines/:engineName/documents/:documentId`,
+    })
   );
   router.delete(
     {
@@ -62,10 +58,8 @@ export function registerDocumentRoutes({
         }),
       },
     },
-    async (context, request, response) => {
-      return enterpriseSearchRequestHandler.createRequest({
-        path: `/as/engines/${request.params.engineName}/documents/${request.params.documentId}`,
-      })(context, request, response);
-    }
+    enterpriseSearchRequestHandler.createRequest({
+      path: `/as/engines/:engineName/documents/:documentId`,
+    })
   );
 }
diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts
@@ -59,6 +59,7 @@ describe('engine routes', () => {
 
     describe('hasValidData', () => {
       it('should correctly validate that the response has data', () => {
+        mockRequestHandler.createRequest.mockClear();
         const response = {
           meta: {
             page: {
@@ -73,6 +74,7 @@ describe('engine routes', () => {
       });
 
       it('should correctly validate that a response does not have data', () => {
+        mockRequestHandler.createRequest.mockClear();
         const response = {};
 
         mockRouter.callRoute(mockRequest);
@@ -125,10 +127,8 @@ describe('engine routes', () => {
     });
 
     it('creates a request to enterprise search', () => {
-      mockRouter.callRoute({ params: { name: 'some-engine' } });
-
       expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
-        path: '/as/engines/some-engine/details',
+        path: '/as/engines/:name/details',
       });
     });
   });
@@ -150,10 +150,8 @@ describe('engine routes', () => {
     });
 
     it('creates a request to enterprise search', () => {
-      mockRouter.callRoute({ params: { name: 'some-engine' } });
-
       expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
-        path: '/as/engines/some-engine/overview_metrics',
+        path: '/as/engines/:name/overview_metrics',
       });
     });
   });

diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/engines.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/engines.ts
@@ -54,11 +54,9 @@ export function registerEnginesRoutes({
         }),
       },
     },
-    async (context, request, response) => {
-      return enterpriseSearchRequestHandler.createRequest({
-        path: `/as/engines/${request.params.name}/details`,
-      })(context, request, response);
-    }
+    enterpriseSearchRequestHandler.createRequest({
+      path: `/as/engines/:name/details`,
+    })
   );
   router.get(
     {
@@ -69,10 +67,8 @@ export function registerEnginesRoutes({
         }),
       },
     },
-    async (context, request, response) => {
-      return enterpriseSearchRequestHandler.createRequest({
-        path: `/as/engines/${request.params.name}/overview_metrics`,
-      })(context, request, response);
-    }
+    enterpriseSearchRequestHandler.createRequest({
+      path: `/as/engines/:name/overview_metrics`,
+    })
   );
 }
diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/settings.test.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/settings.test.ts
@@ -26,8 +26,6 @@ describe('log settings routes', () => {
     });
 
     it('creates a request to enterprise search', () => {
-      mockRouter.callRoute({});
-
       expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
         path: '/as/log_settings',
       });
@@ -52,7 +50,6 @@ describe('log settings routes', () => {
     });
 
     it('creates a request to enterprise search', () => {
-      mockRouter.callRoute({});
       expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({
         path: '/as/log_settings',
       });