UI: [BUGFIX] Decode/encode urls

[johncowen]

In 858b05f#diff-46ef88aa04507fb9b039344277531584
we removed encoding values in pathnames as we thought they were
eventually being encoded by ember. It looks like this isn't the case.

This commit adds url encoding/decoding 'around' ember so the end user
doesn't have to think about it. This is done in 2 places, in addition to
the reverting of the above commit.

Firstly we add url encoding to the {{href-to}} helper/addon. The
reasoning here is, if we are asking for a 'href/url' then whatever we
receive back should be urlencoded. We've done this by reusing as much
code from the original ember-href-to addon as possible, after this
change every call to the {{href-to}} helper will be urlencoded. We
also added the ability to pass arrays as the 'slug' here for if you need
to pass the helper something with slashes in them (for example kv keys).
If you pass an array, every value in the array will be url encoded and
then joined by a slash to produce the url with url encoded parts.

As all links using {{href-to}} are now properly urlencoded. We also
need to decode them in the correct place 'on the other end', so..

Secondly, we override the default Route.paramsFor method to decode all
params before passing them to the Route.model hook.

Lastly (the above revert), as we almost consistently use url params to
construct API calls, we make sure we re-encode any slugs that have been
passed in by the user/developer. The original API for the createURL
function was to allow you to pass values that didn't need encoding,
values that did need encoding, followed by query params (which again
require url encoding)

All in all this should make the entire ember app url encode/decode safe.

Unit tests are added for the underlying functionality, and acceptance tests are
added for KV's only for the moment. I'll probably add more acceptance testing
across other models once this has been approved, probably on top of this
branch.

Fixes #5194

[johncowen]

I've just been doubled checking all this again, and it seems like there is more to it than I originally thought. It looks like ember does encode paths sometimes.

I need to look into this further before this gets merged but it looks like if you specify what ember calls a 'dynamic segment' in your router it will encode it for you. If you use 'wildcard/globbing' then it won't. I personally haven't seen documentation that explains this so I could be wrong. We are currently using ember 2.18, and I am aware that there has been work on the routing system in ember in later versions, so it may or may not have been addressed there.

I'm assuming this is down to being able to specify a parameter of your route configuration as something containing a '/' without that getting encoded. In our case historically we have always required this for our KV store which uses slashes for the key names. We also fairly recently added this to the service routes when we realized people where using slashes in their service names also.

You can see this in the following 2 links:

consul/ui-v2/app/router.js

Line 15 in 59c010e

this.route('show', { path: '/*name' });

consul/ui-v2/app/router.js

Lines 29 to 32 in 59c010e

	this.route('folder', { path: '/*key' });
	this.route('edit', { path: '/*key/edit' });
	this.route('create', { path: '/*key/create' });
	this.route('root-create', { path: '/create' });

This means that as a template editor if you create a 'href-to' something in your templates sometimes the parameters passed to it will be url encoded and sometimes they won't, depending on the configuration you have in your router.js file.

Right now I've traced the code back to here:

https://github.com/tildeio/route-recognizer/blob/0940966757104ea5297717102b1ae3dc260ee8ee/lib/route-recognizer.ts#L89-L99

Without knowing the reasons behind this code, ideally this would split the 'star' param by slash and then still encode all the parts between the slashes (to encode things like # which is what @Anricx is having issues with), similar to what we reverted back to in this PR here:

consul/ui-v2/app/utils/createURL.js

Line 15 in d86e753

encoded.concat(raw.map(encode)).join('/'),

I'm still looking into this as I'm not 100% sure I'm correct, but I doubt the PR here is the entire solution, if even the ideal way to go about this (quick ping to @Anricx so you know this isn't complete). I would like url encoding issues to be 100% transparent to the user of the framework - it's something that as a framework user I don't want to have to worry about, so I'm going to see if I can amend the PR here to take into account my findings today.

On my travels I also noticed other areas that might cause similar confusion here:

https://github.com/tildeio/route-recognizer/blob/0940966757104ea5297717102b1ae3dc260ee8ee/lib/route-recognizer.ts#L564-L574

Unless I'm mistaken here sometimes keynames will get encoded and sometimes they won't, there may be other areas, but incase anyone else hits upon any similar problems this is where to look.

[johncowen]

P.S. It looks like I can make it never encode any path segments using:

ENCODE_AND_DECODE_PATH_SEGMENTS

https://github.com/tildeio/route-recognizer/blob/0940966757104ea5297717102b1ae3dc260ee8ee/lib/route-recognizer.ts#L91-L92

..which would give me consistency and would mean that the rest of the code in this PR would work as expected.

Right now it looks like whilst this static variable is exposed via the RouteRecognizer module, it is not exposed by ember, neither can you re-import it and set it as this module doesn't seem to available in the ember module registry (at least under the same name), trying to import via an 'npm:route-recognizer' gives you a different copy so this doesn't work either. There are a few related PR's around and about discussing this but none seem to explain how to get to this static variable whilst in an ember environment.

The alternative would be to go with my initial thought and try to detect whether a route has been defined with a dynamic segment or a wildcard segment and then encode/skip encoding depending on that. First glance at this seemed complicated.

@meirish @DingoEatingFuzz any suggestions/recommendations here? I might be getting too far into a rabbit hole and there is a simpler approach? We could just stop making KV urls look nice and use %2F's everywhere instead, but I'd rather avoid that if possible.

I suppose I could just turn all my router definitions into wildcard ones? Not sure what impact that might have now or in the future though. (edit: bad idea 😄 )

[meirish]

Hi @johncowen sorry for the delay on this - I'm wondering if instead of trying to do this application-wide, maybe you do it locally in the href when you know there's going to be a splat or where you know you're allowing slashes. Currently you said it's just in KV ids and services and I see there's some template helpers where the kv is passed to href-to helper - would it make sense to add a helper that encode the kv value there, doing the proper encoding?

It seems like splats aren't common enough to worry about the general purpose solution and disabling encoding globally feels like a really big hammer. I also think it might get tricky / impossible if you ever needed more than one wildcard in various URL patterns.

[johncowen]

Hey @meirish

No prob! Yeah I think I agree with you there on the really big hammer thing. I think that little niggle in my head was what made me come back here and triple check things, it's potentially one of those changes that affects the entire UI.

I got a 100% working thing together yesterday, but it works 'above' the issue. It doesn't disable anything globally, which is better, but as I couldn't find a path within ember to figure out whether the route is a dynamic one or a wildcard one, I worked something over ember that let me figure that out and encode or not depending on the route type.

I was thinking last night that I'm pretty sure I always need to pass an array of a '/' split key name when I'm working with KVs, so I could detect that and only encode those when I do that, which might be better than what I did yesterday.

I'm not entirely sure if that will work for services though - it might/should I think, I'm going to try another branch with that I think and see if that works out.

Thanks for the advice, I may ping you later for a sanity check on this.

John

[johncowen]

A bit more info on this, I was looking into why the service route was a wildcard route, and it looks as if it's come from the v1 ui refactor:

consul/ui/javascripts/app/router.js

Line 24 in 8c87238

this.route("show", { path: "/*name" });

I'm thinking that making this a dynamic route would be best, rather than a wildcard route instead of the change we made here:

#4756

That would mean that the URLs for services with slashes in them would look like:

Service Name: application/service
URL: /ui/dc-1/services/application%2Fservice

That would be a change to how the URLs are constructed currently (and I think where being constructed in the v1 ui)

Service Name: application/service
URL: /ui/dc-1/services/application/service

That would only leave us with wildcards being used for KVs, meaning we can potentially restrict all of this to KVs only (including the extra paramsFor functionality required)

I still would ideally like this to happen automatically within the framework, without having to take into account whether a route is a wildcard or not, I'm just not sure considering everything thats the correct thing to do.

@meirish would love to get a short chat in with you today at somepoint if possible, will ping you in a bit

[johncowen]

This PR is getting a little intricate so I'm going to 'briefly' sum up here:

The Problem

The ember framework we use doesn't always deal with URL encoding/decoding for you. Sometimes, when using dynamic routes e.g. /:slug it will, sometimes, when using wildcard/globbing routes e.g. /*slug it won't. This is due to how wildcard routes need to work, they need to include/match any slashes in the 'slug'. In my mind wildcard routes should still encode/decode all other characters, instead of not applying any encoding/decoding at all.

We currently use wildcard routes for within the Services and the KV areas of the UI, therefore these areas currently have URL encoding bugs as we weren't aware of this undocumented detail.

Possible solutions

1. As an end user of the framework we could apply encode/decode all over our app in the specific areas which lack automatic encoding/decoding, in our case in all the links in all the templates for Services and KVs. We know that we only use wildcard routes in Services and KV so we could only apply these changes in these areas. While this is a possibility we believe moving forwards this will gradually result in less maintainable code, and whilst we are currently aware of this issue, what about other developers that may contribute to the code in future? Overall using this approach it would be fairly likely that the encoding of a link would be missed or a decoding of a parameter skipped producing edge case hard to find bugs. This risk would forever be in our application.

2. We are using a third party helper (href-to) to output template links in a more performant fashion. After looking into this module, we have found a way to fairly easily add urlencoding to this functionality in a fairly unobtrusive manner, meaning we can continue to depend on the work of the author of the module. We can also globally add urldecoding functionality to url parameters by overwriting ember's Route.paramsFor method using embers reopen method (the equivalent of writing to CustomClass.prototype). As we have these points of control we just need to detect whether we are in a route that is either a dynamic route or a wildcard route using the methods that ember makes available to us. I've had a quite a deep dive look into this and it doesn't look like there is any way to get to this information using ember alone. I've noticed in the past that the Router class itself is one of the least documented areas of ember, so it would be risky to do anything here out of the ordinary.

3. We noticed that there have been issues and PR's related to this issue before now in the route-recognizer module that ember uses for routing. As a result of this a ENCODE_AND_DECODE_PATH_SEGMENTS static variable was added so that users of the module could control whether any encoding/decoding should be applied to URLs at all. Whilst this approach is heavy handed, potentially we could use this to disable all encoding/decoding, making everything behave consistently, and then control the encoding/decoding ourselves within href-to and paramsFor. The problem here is, it looks like ember doesn't expose this static variable/setting in anyway, it looks like its impossible to set this when using ember.

4. The key to this is being able to detect the type of route and conditionally applying url encoding/decoding in a centralized place. We have a centralized place (href-to and paramsFor), but using ember alone we have no way to detect the type of route. Therefore another option is to define our routes outside of ember, and then pass these definitions to ember. As we 'own' the definitions we can also then make these available to href-to and paramsFor so we can detect the type of route and apply our extra functionality if needed.

We've chosen option 4.

Potential Solution

We define our routes in a declarative format (for the moment at least JSON) outside of Router.map, and loop through this within Router.map to set all our routes using the standard this.route method. We essentially configure our Router from the outside. As this configuration is now done declaratively outside of Router.map we can also make this data available to href-to and paramsFor, allowing us to detect wildcard routes and therefore apply urlencoding/decoding.

Partly related to this is a decision on whether we urlencode the slashes within service names or not. Whilst historically we haven't done this, we feel its a good time to change this behaviour, so we'll also be changing services to use dynamic routes instead of wildcard routes. So service links will then look like /ui/dc-1/services/application%2Fservice rather than /ui/dc-1/services/application/service

We have a solution already for this and everything seems to work as planned, and after speaking to @meirish and going over the possible solutions - I feel this is the best 'route' 😄 :dadjoke:

I'm going to clean up a little and make sure I'm as happy as I can be with the code and push onto the end of this PR - whoever gets here, thanks for getting through it all!

[hashicorp-cla]

All committers have signed the CLA.

[johncowen]

I've added a further commit onto the end of here that implements what is explained above. Few things to note:

1. I decided to keep the route JSON in the router file for the moment as people would expect to see it here.
2. I added a function to dump the JSON config as traditional Router.map code, yet commented it out. Ideally I'd like to be able to say 'hey ember, please don't compile this into my code if we are doing a production build'. Not entirely sure if thats possible or how to do that, so for the moment comments hide the code from production but make sure its available should anyone need it.

Lastly, there are a couple of things I'd like to add here, but probably in a future PR.

1. Right now it looks for a * anywhere in the URL in order to decide whether the route is a wildcard route or not. It wouldn't take that much more effort to make it look for *s inside URL segments, so things like /:name/*slug would be possible. We don't need this functionality yet at least, but it would be straightforward-ish to add if need be.

2. Potentially href-to could look for slashes in strings for wildcard routes and then split them automatically. Right now we split them beforehand in the template and passing the params as an array (this is essentially explicitly saying 'this string contains slashes, please don't encode these specific slashes) I'm not 100% sure what the best thing to do would be here, but again we don't need this functionality yet at least so its left as is for the moment.

Lastly, please feel free to ask if any of this isn't clear, appreciate this entire PR might be a little difficult to follow now. @meirish @DingoEatingFuzz this is ready for a review again now.

[johncowen]

I decided that the href-to helper should always take strings and split by slashes for you behind the scenes/automatically (see point 2 in the comment before this).

I decided this based on the fact that I want url encoding/decoding to be entirely transparent to the user/engineer/template author, the slash splitting/encoding/joining should be done in the helper. They shouldn't have to think 'oh I need to split the string into an array here as the route is a wildcard'.

The url-encode util I'd previously written to cope with strings or multidimensional arrays is now no longer needed so I deleted that plus its tests here also.

Sorry definitely finished with this for the moment! Apologies for the messy PR!

[meirish]

This branch isn't necessary - Ember will create index routes for you if nest callbacks.

[johncowen]

I think I added this as it's needed from the other side i.e. if I try isWildcard('kv.index') it needs to return false, not throw. Come to think of it, I think it would be caught anyway. I added a try/catch to catch any other auto routes I wasn't aware of, the application route also caught me out whilst testing, so I added this then. I'd rather not have a try/catch but I don't know enough about ember to be sure I auto add all its auto created routes.

Just considering whether I'd remove it now I've realized that, even from the other side, any error would be caught.

I think if anything I'd rather replicate ember's 'auto' routes in in the JSON blob rather than catch them - if anything I'd rather automatically add application in here also if you haven't defined it.

[meirish]

A little sad about encoded slashes in the URL, but get why it's there.

[johncowen]

Yeah same here. The only way I can make myself feel better about it is that slashes in service names aren't as 'natural feeling' as they are for KV, so I figure it's 'ok' here. I think here it's even possibly the 'correct' thing to do. Kind of like waving your kids off to university - sad but inside you know it's best in the long run 😂

[meirish]

Nice work digging around to get to the bottom of this one! I left a comment about one branch that's not necessary in the walk function. I know we talked about this and you're pretty set on doing things like this in one place, but I thought of a couple of things that may make this turn into long-term technical debt instead of a "all in one place" solution.

These are both hypotheticals but I thought they were worth raising for thinking about how the application will grow in the future:

1. If you ever have to split the app apart using ember-engines, and you have routes in your engines, it may prove difficult to continue defining routes as JSON (caveat: I've not looked into this too deeply, I'm just assuming that it wouldn't come for free / would require more integration at the engine layer).
2. This couples the app's routing functionality to the usage of href-to and I wanted to note that both Nomad and Vault had to remove href-to when we started using query params more heavily (I think it was when both apps added namespaces). If you ever need to remove it, you'll have to rework this as well.

For both of these reasons, if you want to, I think it may still be worth looking at a model param or template helper that passes encoded values to the link-to / href-to helper (possibly mixed with usage of the serialize route hook). I understand that you may not want to do that though since this works and is nicely packaged up (code looks 👌✨), but just wanted to mention the above as something to ponder 🤔 before moving ahead!

[johncowen]

Hey!

Cool, thanks for the review and for sharing potential problems with this. I'd definitely not thought of the impact of using engines.

I think overall, it would be relatively simple (famous last words!) to revert this entire thing and approach it differently (whether thats making changes just in the KV templates or something else). I included a commented out dump function so I can convert the JSON/declarative config back to a Router.map imperative approach. That, plus deleting my custom href-to and paramsFor files, would pretty much bring us back to the situation before this PR.

Apart from that:

1. Engines. I've no idea how these are implemented. I've never used them and not read much about them - but I do think they could be incredibly useful from what I know. I've no idea how this would work with them, but in the same sense that they might be difficult to integrate what I've done here, they might not be. It might be a good excuse to have a look into them a little more myself though. I have thought of a usecase for them within what I'm doing, but its very very far down them line, thanks for bringing this up though.

2. Yeah completely agree with this one. If I was using link-to anywhere within a wildcard route template this wouldn't work. I ran a quick check before doing this to make sure I wasn't using it anywhere. Right now I imagine link-to doesn't exist, bit of a cheat really 😄. But if I did need it potentially I could do something similar here... maybe...? I'm not sure as it's an ember core helper and therefore potentially weirder than a user defined helper, but I could potentially override it the same as I've done here. You're right though, potentially it could be more difficult. I might look into this just out of interest - there're a lot of 'potentials' there. I think the need to use query params with href-to is much more likely to happen than any usage of engines.

I just had another look at serialize (https://github.com/emberjs/ember.js/blob/v3.7.2/packages/%40ember/-internals/routing/lib/system/route.ts#L1963), and believe or not, I might have a think about this PR more! I'm pretty sure I will stick with the approach I've done here though. One problem I can immediately see with serialize is, you need to know the 'name' of the route's dynamic segment, so to do this in a generic/all in one place manner would mean automatically figuring these names out and knowing what model parameter to set them to. This would still mean interacting with Router, and I'd probably come to the same conclusion that Router is a black box and I need to do something above that. I'm going to have another look at this though. Just looking again, is serialize only invoked when you pass a model into link-to/href-to? Not sure, will have a little play and see, I've not come across serialize before so it will be a good learning opportunity.

I'll reply inline to the other stuff, and probably give this entire thing a bit more of a tyre-kick before I merge it in later today, thanks again for looking at this short notice 🙌

[johncowen]

😂 Just found a tiny problem with this! Doesn't affect users though, only engineers.

ember generate route tries to add a route to the normal Router!

There is a --skip-router option also, so that's needed if you use generate to generate routes now. Ideally you wouldn't need to know about this though, and this goes against my aim to make this as transparent as possible! Will have a look into it a bit more. It's a tiny detail but would be nice to sort this also.

[johncowen]

@kaxcode here's that thing you spotted yesterday! Notice I must have mistakenly changed it from settings to setting at some point 😬 !! I'm still trying to remember/find why its commented out here and not commented out currently.

I want to bounce around ideas for changing this anyway, ideally the URL would be /~nspace/dc-1/somewhere/in/the/ui#settings as the settings page is almost a modal, I remember trying this at the very start and ember didn't like using #s in URLs. I have bigger ember muscles now though so maybe we could try this.

Another option would be to treat the settings page in the same way that we treat all the other URLs so just /~nspace/dc-1/settings. That way we keep all the info we need in the URL and not in localStorage.

Lets chat later