From f4f6cb687cfe9d55b370503fca556d0bfbb59eab Mon Sep 17 00:00:00 2001 From: Constance Date: Wed, 20 Jan 2021 11:07:32 -0800 Subject: [PATCH] [App Search] Add new encodePathParams helper (fixes unencoded document 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 <42973632+kibanamachine@users.noreply.github.com> --- .../enterprise_search_request_handler.test.ts | 30 +++++++++++++++-- .../lib/enterprise_search_request_handler.ts | 33 ++++++++++++++++++- .../routes/app_search/analytics.test.ts | 12 ++----- .../server/routes/app_search/analytics.ts | 20 ++++------- .../routes/app_search/credentials.test.ts | 21 ++---------- .../server/routes/app_search/credentials.ts | 16 ++++----- .../routes/app_search/documents.test.ts | 15 ++------- .../server/routes/app_search/documents.ts | 24 +++++--------- .../server/routes/app_search/engines.test.ts | 10 +++--- .../server/routes/app_search/engines.ts | 16 ++++----- .../server/routes/app_search/settings.test.ts | 3 -- .../server/routes/app_search/settings.ts | 8 ++--- 12 files changed, 100 insertions(+), 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 index e55f997a6b51..a27932e84417 100644 --- 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 index 8e4a817a8255..a626198ad9c4 100644 --- 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); 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) { + 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 index 9ede6989052b..f93b205059b2 100644 --- 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 index f7d0786b27fd..9807ca9ad791 100644 --- 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 index af498e346529..f6bcd4adda1f 100644 --- 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 index a5611af9bba7..29425eedef69 100644 --- 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 index 5f57db40cd7e..c12a2e69057d 100644 --- 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 index 60cd64b32479..665691c3a9ea 100644 --- 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 index ed6847a02910..9755fff02f73 100644 --- 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 index f9169d8795f4..c0bbc40ff8d2 100644 --- 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 index be3b2632eb67..613ecee90d98 100644 --- 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', }); diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/settings.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/settings.ts index f9684cdbc060..bec56c9e3de0 100644 --- a/x-pack/plugins/enterprise_search/server/routes/app_search/settings.ts +++ b/x-pack/plugins/enterprise_search/server/routes/app_search/settings.ts @@ -40,10 +40,8 @@ export function registerSettingsRoutes({ }), }, }, - async (context, request, response) => { - return enterpriseSearchRequestHandler.createRequest({ - path: '/as/log_settings', - })(context, request, response); - } + enterpriseSearchRequestHandler.createRequest({ + path: '/as/log_settings', + }) ); }