-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Console] Encode pathname #122080
[Console] Encode pathname #122080
Conversation
f0e845d
to
d7cdb31
Compare
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! Code LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for patching this up @mibragimov! changes lgtm, tested locally 🚀
I think would be nice to enhance the current tests in order to include this new behaviour :)
@@ -44,6 +58,7 @@ export const proxyRequest = ({ | |||
}: Args) => { | |||
const { hostname, port, protocol, pathname, search } = uri; | |||
const client = uri.protocol === 'https:' ? https : http; | |||
const encodedPath = encodePathname(pathname); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we add some tests to cover this new behaviour in https://github.com/elastic/kibana/blob/main/src/plugins/console/server/lib/proxy_request.test.ts ?
e80bac8
to
460ffb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test LGTM! Thanks ) 🚀
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: cc @mibragimov |
* Fix encoding pathname * Add a test case Co-authored-by: Muhammad Ibragimov <[email protected]>
Closes #76585