Adjust encoding for security management pages #83629
Conversation
legrego
added
release_note:fix
Team:Security
|
Pinging @elastic/kibana-security (Team:Security) |
|
ACK: reviewing... |
| @@ -66,10 +66,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 ? decodeURIComponent(username) : undefined; | |||
There was a problem hiding this comment.
issue: hmm, it seems while we fixed issue for usernames with @, we introduced it for the ones with %. For example for user%not, decodeURIComponent will throw malformed URI sequence :/ Should we temporarily create tryDecodeURIComponent function with a try/catch block inside?
There was a problem hiding this comment.
Ah good catch. I had tested with valid encoded entities (e.g. %20), but didn't think about the malformed sequences. Yeah I think a tryDecodeURIComponent makes the most sense here, at least for the time being. I'll make this change!
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
LGTM, thanks! The fix is good enough for the time being and covers the most probable cases. To cover the rest we really need a proper fix in react-router-dom.
Do we also need a Release Note:-like comment in the issue description for our release note automation?
* Adjust encoding for security management pages * introduce tryDecodeURIComponent
* Adjust encoding for security management pages * introduce tryDecodeURIComponent
Summary
This PR takes a different set of tradeoffs for encoding links to the User, Role, and Role Mapping management screens.
We perform an extra call to
decodeURIComponentwhen resolving the username, role name, and role mapping name, so that we can correctly fetch these assets. This is to workaround a bug in thehistorymodule, which is used byreact-router-dom(#82440). Thehistorymodule has been fixed, butreact-router-domhas not updated their dependencies. The fix resulted in a breaking change, and so forcefully resolvinghistoryto the fixed version is a non-starter for us.This will not fix all use cases, and it will result in some UI inconsistencies.
For example, a username of
foo%20userwill render in the UI asfoo userbecause of this additional decoding step.Resolves #83541