From c65cf9a8399c166e1331ec2d453fce4024370cb1 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Fri, 20 Nov 2020 13:06:59 -0500 Subject: [PATCH] Adjust encoding for security management pages (#83629) * Adjust encoding for security management pages * introduce tryDecodeURIComponent --- .../role_mappings_management_app.test.tsx | 6 +-- .../role_mappings_management_app.tsx | 9 ++++- .../roles/roles_management_app.test.tsx | 6 +-- .../management/roles/roles_management_app.tsx | 9 ++++- .../public/management/uri_utils.test.ts | 21 +++++++++++ .../security/public/management/url_utils.ts | 13 +++++++ .../users/users_management_app.test.tsx | 37 +++++++++++-------- .../management/users/users_management_app.tsx | 9 ++++- 8 files changed, 82 insertions(+), 28 deletions(-) create mode 100644 x-pack/plugins/security/public/management/uri_utils.test.ts create mode 100644 x-pack/plugins/security/public/management/url_utils.ts diff --git a/x-pack/plugins/security/public/management/role_mappings/role_mappings_management_app.test.tsx b/x-pack/plugins/security/public/management/role_mappings/role_mappings_management_app.test.tsx index e65310ba399ead..5479bc36d1ed59 100644 --- a/x-pack/plugins/security/public/management/role_mappings/role_mappings_management_app.test.tsx +++ b/x-pack/plugins/security/public/management/role_mappings/role_mappings_management_app.test.tsx @@ -83,18 +83,18 @@ describe('roleMappingsManagementApp', () => { }); it('mount() works for the `edit role mapping` page', async () => { - const roleMappingName = 'someRoleMappingName'; + const roleMappingName = 'role@mapping'; const { setBreadcrumbs, container, unmount } = await mountApp('/', `/edit/${roleMappingName}`); expect(setBreadcrumbs).toHaveBeenCalledTimes(1); expect(setBreadcrumbs).toHaveBeenCalledWith([ { href: `/`, text: 'Role Mappings' }, - { href: `/edit/${roleMappingName}`, text: roleMappingName }, + { href: `/edit/${encodeURIComponent(roleMappingName)}`, text: roleMappingName }, ]); expect(container).toMatchInlineSnapshot(`
- Role Mapping Edit Page: {"name":"someRoleMappingName","roleMappingsAPI":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"rolesAPIClient":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"notifications":{"toasts":{}},"docLinks":{"esDocBasePath":"https://www.elastic.co/guide/en/elasticsearch/reference/mocked-test-branch/"},"history":{"action":"PUSH","length":1,"location":{"pathname":"/edit/someRoleMappingName","search":"","hash":""}}} + Role Mapping Edit Page: {"name":"role@mapping","roleMappingsAPI":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"rolesAPIClient":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"notifications":{"toasts":{}},"docLinks":{"esDocBasePath":"https://www.elastic.co/guide/en/elasticsearch/reference/mocked-test-branch/"},"history":{"action":"PUSH","length":1,"location":{"pathname":"/edit/role@mapping","search":"","hash":""}}}
`); diff --git a/x-pack/plugins/security/public/management/role_mappings/role_mappings_management_app.tsx b/x-pack/plugins/security/public/management/role_mappings/role_mappings_management_app.tsx index bca3a070e64f97..ce4ded5a9acbcf 100644 --- a/x-pack/plugins/security/public/management/role_mappings/role_mappings_management_app.tsx +++ b/x-pack/plugins/security/public/management/role_mappings/role_mappings_management_app.tsx @@ -12,6 +12,7 @@ import { StartServicesAccessor } from 'src/core/public'; import { RegisterManagementAppArgs } from '../../../../../../src/plugins/management/public'; import { PluginStartDependencies } from '../../plugin'; import { DocumentationLinksService } from './documentation_links'; +import { tryDecodeURIComponent } from '../url_utils'; interface CreateParams { getStartServices: StartServicesAccessor; @@ -70,10 +71,14 @@ export const roleMappingsManagementApp = Object.freeze({ const EditRoleMappingsPageWithBreadcrumbs = () => { const { name } = useParams<{ name?: string }>(); + // Additional decoding is a workaround for a bug in react-router's version of the `history` module. + // See https://github.com/elastic/kibana/issues/82440 + const decodedName = name ? tryDecodeURIComponent(name) : undefined; + setBreadcrumbs([ ...roleMappingsBreadcrumbs, name - ? { text: name, href: `/edit/${encodeURIComponent(name)}` } + ? { text: decodedName, href: `/edit/${encodeURIComponent(name)}` } : { text: i18n.translate('xpack.security.roleMappings.createBreadcrumb', { defaultMessage: 'Create', @@ -83,7 +88,7 @@ export const roleMappingsManagementApp = Object.freeze({ return ( { }); it('mount() works for the `edit role` page', async () => { - const roleName = 'someRoleName'; + const roleName = 'role@name'; const { setBreadcrumbs, container, unmount } = await mountApp('/', `/edit/${roleName}`); expect(setBreadcrumbs).toHaveBeenCalledTimes(1); expect(setBreadcrumbs).toHaveBeenCalledWith([ { href: `/`, text: 'Roles' }, - { href: `/edit/${roleName}`, text: roleName }, + { href: `/edit/${encodeURIComponent(roleName)}`, text: roleName }, ]); expect(container).toMatchInlineSnapshot(`
- Role Edit Page: {"action":"edit","roleName":"someRoleName","rolesAPIClient":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"userAPIClient":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"indicesAPIClient":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"privilegesAPIClient":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}},"notifications":{"toasts":{}},"fatalErrors":{},"license":{"features$":{"_isScalar":false}},"docLinks":{"esDocBasePath":"https://www.elastic.co/guide/en/elasticsearch/reference/mocked-test-branch/"},"uiCapabilities":{"catalogue":{},"management":{},"navLinks":{}},"history":{"action":"PUSH","length":1,"location":{"pathname":"/edit/someRoleName","search":"","hash":""}}} + Role Edit Page: {"action":"edit","roleName":"role@name","rolesAPIClient":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"userAPIClient":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"indicesAPIClient":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"privilegesAPIClient":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}},"notifications":{"toasts":{}},"fatalErrors":{},"license":{"features$":{"_isScalar":false}},"docLinks":{"esDocBasePath":"https://www.elastic.co/guide/en/elasticsearch/reference/mocked-test-branch/"},"uiCapabilities":{"catalogue":{},"management":{},"navLinks":{}},"history":{"action":"PUSH","length":1,"location":{"pathname":"/edit/role@name","search":"","hash":""}}}
`); diff --git a/x-pack/plugins/security/public/management/roles/roles_management_app.tsx b/x-pack/plugins/security/public/management/roles/roles_management_app.tsx index 88aeb1d232fc7d..d5b3b4998a09d9 100644 --- a/x-pack/plugins/security/public/management/roles/roles_management_app.tsx +++ b/x-pack/plugins/security/public/management/roles/roles_management_app.tsx @@ -13,6 +13,7 @@ import { RegisterManagementAppArgs } from '../../../../../../src/plugins/managem import { SecurityLicense } from '../../../common/licensing'; import { PluginStartDependencies } from '../../plugin'; import { DocumentationLinksService } from './documentation_links'; +import { tryDecodeURIComponent } from '../url_utils'; interface CreateParams { fatalErrors: FatalErrorsSetup; @@ -68,10 +69,14 @@ export const rolesManagementApp = Object.freeze({ const EditRolePageWithBreadcrumbs = ({ action }: { action: 'edit' | 'clone' }) => { const { roleName } = useParams<{ roleName?: string }>(); + // Additional decoding is a workaround for a bug in react-router's version of the `history` module. + // See https://github.com/elastic/kibana/issues/82440 + const decodedRoleName = roleName ? tryDecodeURIComponent(roleName) : undefined; + setBreadcrumbs([ ...rolesBreadcrumbs, action === 'edit' && roleName - ? { text: roleName, href: `/edit/${encodeURIComponent(roleName)}` } + ? { text: decodedRoleName, href: `/edit/${encodeURIComponent(roleName)}` } : { text: i18n.translate('xpack.security.roles.createBreadcrumb', { defaultMessage: 'Create', @@ -82,7 +87,7 @@ export const rolesManagementApp = Object.freeze({ return ( { + it('properly decodes a URI Component', () => { + expect( + tryDecodeURIComponent('sample%26piece%3Dof%20text%40gmail.com%2520') + ).toMatchInlineSnapshot(`"sample&piece=of text@gmail.com%20"`); + }); + + it('returns the original string undecoded if it is malformed', () => { + expect(tryDecodeURIComponent('sample&piece=of%text@gmail.com%20')).toMatchInlineSnapshot( + `"sample&piece=of%text@gmail.com%20"` + ); + }); +}); diff --git a/x-pack/plugins/security/public/management/url_utils.ts b/x-pack/plugins/security/public/management/url_utils.ts new file mode 100644 index 00000000000000..590863e30d5ec4 --- /dev/null +++ b/x-pack/plugins/security/public/management/url_utils.ts @@ -0,0 +1,13 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +export const tryDecodeURIComponent = (uriComponent: string) => { + try { + return decodeURIComponent(uriComponent); + } catch { + return uriComponent; + } +}; diff --git a/x-pack/plugins/security/public/management/users/users_management_app.test.tsx b/x-pack/plugins/security/public/management/users/users_management_app.test.tsx index 06bd2eff6aa1e5..c9e448d90d925c 100644 --- a/x-pack/plugins/security/public/management/users/users_management_app.test.tsx +++ b/x-pack/plugins/security/public/management/users/users_management_app.test.tsx @@ -86,18 +86,18 @@ describe('usersManagementApp', () => { }); it('mount() works for the `edit user` page', async () => { - const userName = 'someUserName'; + const userName = 'foo@bar.com'; const { setBreadcrumbs, container, unmount } = await mountApp('/', `/edit/${userName}`); expect(setBreadcrumbs).toHaveBeenCalledTimes(1); expect(setBreadcrumbs).toHaveBeenCalledWith([ { href: `/`, text: 'Users' }, - { href: `/edit/${userName}`, text: userName }, + { href: `/edit/${encodeURIComponent(userName)}`, text: userName }, ]); expect(container).toMatchInlineSnapshot(`
- User Edit Page: {"authc":{},"userAPIClient":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"rolesAPIClient":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"notifications":{"toasts":{}},"username":"someUserName","history":{"action":"PUSH","length":1,"location":{"pathname":"/edit/someUserName","search":"","hash":""}}} + User Edit Page: {"authc":{},"userAPIClient":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"rolesAPIClient":{"http":{"basePath":{"basePath":"","serverBasePath":""},"anonymousPaths":{}}},"notifications":{"toasts":{}},"username":"foo@bar.com","history":{"action":"PUSH","length":1,"location":{"pathname":"/edit/foo@bar.com","search":"","hash":""}}}
`); @@ -106,18 +106,23 @@ describe('usersManagementApp', () => { expect(container).toMatchInlineSnapshot(`
`); }); - it('mount() properly encodes user name in `edit user` page link in breadcrumbs', async () => { - const username = 'some 安全性 user'; - - const { setBreadcrumbs } = await mountApp('/', `/edit/${username}`); - - expect(setBreadcrumbs).toHaveBeenCalledTimes(1); - expect(setBreadcrumbs).toHaveBeenCalledWith([ - { href: `/`, text: 'Users' }, - { - href: '/edit/some%20%E5%AE%89%E5%85%A8%E6%80%A7%20user', - text: username, - }, - ]); + const usernames = ['foo@bar.com', 'foo&bar.com', 'some 安全性 user']; + usernames.forEach((username) => { + it( + 'mount() properly encodes user name in `edit user` page link in breadcrumbs for user ' + + username, + async () => { + const { setBreadcrumbs } = await mountApp('/', `/edit/${username}`); + + expect(setBreadcrumbs).toHaveBeenCalledTimes(1); + expect(setBreadcrumbs).toHaveBeenCalledWith([ + { href: `/`, text: 'Users' }, + { + href: `/edit/${encodeURIComponent(username)}`, + text: username, + }, + ]); + } + ); }); }); diff --git a/x-pack/plugins/security/public/management/users/users_management_app.tsx b/x-pack/plugins/security/public/management/users/users_management_app.tsx index 82c55d67b9026e..2f16f85d5fcae8 100644 --- a/x-pack/plugins/security/public/management/users/users_management_app.tsx +++ b/x-pack/plugins/security/public/management/users/users_management_app.tsx @@ -12,6 +12,7 @@ import { StartServicesAccessor } from 'src/core/public'; import { RegisterManagementAppArgs } from '../../../../../../src/plugins/management/public'; import { AuthenticationServiceSetup } from '../../authentication'; import { PluginStartDependencies } from '../../plugin'; +import { tryDecodeURIComponent } from '../url_utils'; interface CreateParams { authc: AuthenticationServiceSetup; @@ -66,10 +67,14 @@ export const usersManagementApp = Object.freeze({ const EditUserPageWithBreadcrumbs = () => { const { username } = useParams<{ username?: string }>(); + // Additional decoding is a workaround for a bug in react-router's version of the `history` module. + // See https://github.com/elastic/kibana/issues/82440 + const decodedUsername = username ? tryDecodeURIComponent(username) : undefined; + setBreadcrumbs([ ...usersBreadcrumbs, username - ? { text: username, href: `/edit/${encodeURIComponent(username)}` } + ? { text: decodedUsername, href: `/edit/${encodeURIComponent(username)}` } : { text: i18n.translate('xpack.security.users.createBreadcrumb', { defaultMessage: 'Create', @@ -83,7 +88,7 @@ export const usersManagementApp = Object.freeze({ userAPIClient={userAPIClient} rolesAPIClient={new RolesAPIClient(http)} notifications={notifications} - username={username} + username={decodedUsername} history={history} /> );