Handle encoding and decoding of angular route url components

[eliperelman]

Summary

Handle encoding and decoding of Angular route URL components. This used to be handled automatically by Angular, but was reverted. This patch re-introduces this change and handles it in KbnUrlProvider.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

• This was checked for cross-browser compatibility, including a check against IE11
• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• This was checked for keyboard-only and screenreader accessibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately
• This includes a feature addition or change that requires a release note and was labeled appropriately

Dev Docs

Handle encoding and decoding of Angular route URL components. This addition has since been reverted.

[elasticmachine]

Pinging @elastic/kibana-platform

[sorenlouv]

This is awesome! Good job!
The reason Angular didn't fix this (they actually did but had to revert) was because too much of the ecosystem was relying on the broken behaviour. We should make sure that's not the case in Kibana.

[eliperelman]

Thank you! Just a note: I walked through every usage of parameter encoding in Kibana I could find to see if there was any codec work around the URL and these were all I could find.

[sorenlouv]

I walked through every usage of parameter encoding in Kibana I could find to see if there was any codec work around the URL and these were all I could find.
Wow, that sounds painful 😁

I'll spin this up tomorrow, and let you know how APM fares.

[sorenlouv]

Hmm.. just tried it and it seems to break the url.
This url:

http://localhost:5601/app/apm#/opbeans-node/transactions/request/GET%20%2Fapi%2Fcustomers

Redirects to:

http://localhost:5601/app/apm#/opbeans-node/transactions/request/GET%20/api/customers

(Notice how the encoded forward slashes gets decoded)

[sorenlouv]

I added a breakpoint here, and it seems that url has already been decoded at this point.

Going to http://localhost:5601/app/apm#/opbeans-node/transactions/request/GET%20%2Fapi%2Fcustomers, I get url="/opbeans-node/transactions/request/GET%20/api/customers"

[eliperelman]

Thanks for testing! I'll take a look and get this fixed.

[sorenlouv]

Found it:

kibana/src/legacy/ui/public/chrome/directives/kbn_chrome.js

Lines 77 to 88 in 7198a1f

	$rootScope.$on('$locationChangeStart', (e, newUrl) => {
	// This handler fixes issue #31238 where browser back navigation
	// fails due to angular 1.6 parsing url encoded params wrong.
	const absUrlHash = url.parse($location.absUrl()).hash.slice(1);
	const decodedAbsUrlHash = decodeURIComponent(absUrlHash);
	const hash = url.parse(newUrl).hash.slice(1);
	const decodedHash = decodeURIComponent(hash);
	if (absUrlHash !== hash && decodedHash === decodedAbsUrlHash) {
	// replace the urlencoded hash with the version that angular sees.
	$location.url(absUrlHash).replace();
	}
	});

Specifically:

kibana/src/legacy/ui/public/chrome/directives/kbn_chrome.js

Line 80 in 7198a1f

const absUrlHash = url.parse($location.absUrl()).hash.slice(1);

newUrl is correct but absUrlHash decoded the forward slashes.

@spalger you might find this interesting/infuriating :p

[sorenlouv]

Btw even if you don't have any APM data, you should be able to repro this issue by going to the APM app:
/app/apm#/any-service/transactions/request/GET%20%2Fapi%2Fcustomers

[joshdover]

Should these two "corrections" be merged into a single thing?

[sorenlouv]

@joshdover I'm not following?

[sorenlouv]

oh, whether stuff in ui/public/chrome/directives/kbn_chrome.js and ui/public/url/url.js should be merged? Yes, if possible, that sounds like a good improvement 👍

[eliperelman]

@sqren would you mind testing this branch out again? I am using a forked version of angular that avoids the path codecs to test out here.

[sorenlouv]

@eliperelman Just tested this and works well. Didn't notice any decoding issues.
Thanks for doing this!
How are you planning to get this in? Should we run your fork of Angular, or is there a way to apply your changes on top of vanilla angular?

[sorenlouv]

I just recall that this problem is not isolated to Angular but also affects nginx and golang. I'm currently looking into this and seeing if we can give good workarounds to users in these situations. But the best solution might be to leave the legacyEncodeURIComponent in.

Examples of users seeing this problem with nginx:

• https://discuss.elastic.co/t/apm-viewer-in-kibana-broken-when-transaction-name-has-a-slash-in-it/176044/3?u=sqren
• [APM] Transaction names ending in a "/" shows no transactions details #32640 (comment)

[eliperelman]

retest

[sorenlouv]

I have revised the PR to only bring in the angular changes.

Sounds good 👍

I think that since it appears that only APM is needing to use these bandaids it is OK to keep them in since it solves a particular problem that doesn't seem to exist elsewhere.

Just to be clear: this problem affects anybody who has uri-encoded characters in the url and has an nginx or go proxy in front. It's not an APM specific problem unfortunately.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[epixa]

@eliperelman I slapped the reverted label on this PR. Feel free to remove if I misunderstood something.