Skip to content

Commit

Permalink
ui: Make it hard to not URLEncode DataSource srcs/URIs (hashicorp#11117)
Browse files Browse the repository at this point in the history
Our DataSource came in very iteratively, when we first started using it we specifically tried not to use it for things that would require portions of the @src="" attribute to be URL encoded (so things like service names couldn't be used, but dc etc would be fine). We then gradually added an easy way to url encode the @src="" attributes with a uri helper and began to use the DataSource component more and more. This meant that some DataSource usage continued to be used without our uri helper.

Recently we hit hashicorp#10901 which was a direct result of us not encoding @src values/URIs (I didn't realise this was one of the places that required URL encoding) and not going back over things to finish things off once we had implemented our uri helper, resulting in ~half of the codebase using it and ~half of it not.

Now that almost all of the UI uses our DataSource component, this PR makes it even harder to not use the uri helper, by wrapping the string that it requires in a private URI class/object, that is then expected/asserted within the DataSource component/service. This means that as a result of this PR you cannot pass a plain string to the DataSource component without seeing an error in your JS console, which in turn means you have to use the uri helper, and it's very very hard to not URL encode any dynamic/user provided values, which otherwise could lead to bugs/errors similar to the one mentioned above.

The error that you see when you don't use the uri helper is currently a 'soft' dev time only error, but like our other functionality that produces a soft error when you mistakenly pass an undefined value to a uri, at some point soon we will make these hard failing "do not do this" errors.

Both of these 'soft error' DX features have been used this to great effect to implement our Admin Partition feature and these kind of things will minimize the amount of these types of bugs moving forwards in a preventative rather than curative manner. Hopefully these are the some of the kinds of things that get added to our codebase that prevent a multitude of problems and therefore are often never noticed/appreciated.

Additionally here we moved the remaining non-uri using DataSources to use uri (that were now super easy to find), and also fixed up a place where I noticed (due to the soft errors) where we were sometimes passing undefined values to a uri call.

The work here also led me to find another couple of non-important 'bugs' that I've PRed already separately, one of which is yet to be merged (hashicorp#11105), hence the currently failing tests here. I'll rebase that once that PR is in and the tests here should then pass 🤞

Lastly, I didn't go the whole hog here to make DataSink also be this strict with its uri usage, there is a tiny bit more work on DataSink as a result of recently work, so I may (or may not) make DataSink equally as strict as part of that work in a separate PR.
  • Loading branch information
johncowen authored Sep 30, 2021
1 parent a20bc5d commit 35a92e8
Show file tree
Hide file tree
Showing 13 changed files with 61 additions and 38 deletions.
4 changes: 4 additions & 0 deletions .changelog/11117.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
```release-note:improvement
ui: Add uri guard to prevent future URL encoding issues
```

4 changes: 2 additions & 2 deletions ui/packages/consul-ui/app/components/auth-dialog/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@
{{! This DataSource just permanently listens to any changes to the users }}
{{! token, whether thats a new token, a changed token or a deleted token }}
<DataSource
@src="settings://consul:token"
@src={{uri 'settings://consul:token'}}
@onchange={{queue (action (mut token) value="data") (action dispatch "CHANGE") (action (mut previousToken) value="data")}}
/>
{{! This DataSink is just used for logging in from the form, }}
{{! or logging out via the exposed logout function }}
<DataSink
@sink="settings://consul:token"
@sink={{uri "settings://consul:token"}}
as |sink|
>
{{yield}}
Expand Down
17 changes: 10 additions & 7 deletions ui/packages/consul-ui/app/components/data-source/README.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class SomethingRepository extends Service {

```hbs preview-template
<DataSource
@src="/partition/nspace/dc/services"
@src={{uri "/partition/nspace/dc/services"}}
@loading="eager"
@disabled={{false}}
as |source|>
Expand All @@ -26,6 +26,9 @@ as |source|>
</DataSource>
```


Please make sure you use the `uri` helper to specify src URIs, this ensures that it is very difficult to miss any URL encoding problems. If you don't use the `uri` helper then an error will be thrown.

## Attributes

| Argument | Type | Default | Description |
Expand Down Expand Up @@ -55,7 +58,7 @@ Straightforward usage can use `mut` to easily update data within a template usin
```hbs
{{! listen for HTTP API changes}}
<DataSource
@src="/partition/nspace/dc/services"
@src={{uri "/partition/nspace/dc/services"}}
@onchange={{action (mut items) value="data"}}
@onerror={{action (mut error) value="error"}}
/>
Expand All @@ -72,7 +75,7 @@ Straightforward usage can use `mut` to easily update data within a template usin
{{! listen for Settings (local storage) changes}}
<DataSource
@src="settings://consul:token"
@src={{uri "settings://consul:token"}}
@onchange={{action (mut token) value="data"}}
@onerror={{action (mut error) value="error"}}
/>
Expand All @@ -85,7 +88,7 @@ A property approach to easily update data within a template
```hbs
{{! listen for HTTP API changes}}
<DataSource
@src="/partition/nspace/dc/services"
@src={{uri "/partition/nspace/dc/services"}}
as |source|>
{{#if source.error}}
Something went wrong!
Expand Down Expand Up @@ -115,19 +118,19 @@ DataSources can also be recursively nested for loading in series as opposed to i
{{! listen for HTTP API changes}}
<DataSource
@src="/partition/nspace/dc/services"
@src={{uri "/partition/nspace/dc/services"}}
@onerror={{action (mut error) value="error"}}
as |source|>
<source.Source
@src="/partition/nspace/dc/service/{{source.data.firstObject.Name}}"
@src={{uri "/partition/nspace/dc/service/{{source.data.firstObject.Name}}"}}
@onerror={{action (mut error) value="error"}}
as |source|>
{{source.data.Service.Service.Name}} <== Detailed information for the first service
<source.Source
@src="/partition/nspace/dc/proxy/for-service/{{source.data.Service.ID}}"
@src={{uri "/partition/nspace/dc/proxy/for-service/{{source.data.Service.ID}}"}}
@onerror={{action (mut error) value="error"}}
@onchange={{action (mut loaded) true}}
as |source|>
Expand Down
2 changes: 1 addition & 1 deletion ui/packages/consul-ui/app/components/empty-state/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
{{on "click" login}}
>
<DataSource
@src="settings://consul:token"
@src={{uri 'settings://consul:token'}}
@onchange={{action (mut token) value="data"}}
/>
{{#if token.AccessorID}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
<BlockSlot @name="menu">
{{#let components.MenuItem components.MenuSeparator as |MenuItem MenuSeparator|}}
<DataSource
@src="/*/*/*/datacenters"
@src={{uri '/*/*/*/datacenters'}}
@onchange={{action (mut @dcs) value="data"}}
@loading="lazy"
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@
<DataSource
@src={{uri '/${partition}/${nspace}/${dc}/policy/${id}'
(hash
partition=item.Partition
nspace=item.Namespace
partition=partition
nspace=nspace
dc=dc
id=item.ID
)
Expand Down
6 changes: 5 additions & 1 deletion ui/packages/consul-ui/app/components/token-source/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@
{{#let (concat '/' (or partition '') '/' (or nspace '') '/' dc) as |path|}}
<State @matches="secret">
<DataSource
@src={{concat path '/token/self/' value}}
@src={{uri (concat path '/token/self/${value}')
(hash
value=value
)
}}
@onchange={{action 'change'}}
@onerror={{action onerror}}
/>
Expand Down
4 changes: 3 additions & 1 deletion ui/packages/consul-ui/app/helpers/uri.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ const templateRe = /\${([A-Za-z.0-9_-]+)}/g;
let render;
export default class UriHelper extends Helper {
@service('encoder') encoder;
@service('data-source/service') data;

constructor() {
super(...arguments);
if (typeof render !== 'function') {
Expand All @@ -13,6 +15,6 @@ export default class UriHelper extends Helper {
}

compute([template, vars]) {
return render(template, vars);
return this.data.uri(render(template, vars));
}
}
8 changes: 3 additions & 5 deletions ui/packages/consul-ui/app/services/data-sink/service.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,15 @@
import Service, { inject as service } from '@ember/service';

const parts = function(uri) {
uri = uri.toString();
if (uri.indexOf('://') === -1) {
uri = `consul://${uri}`;
}
return uri.split('://');
};
export default class DataSinkService extends Service {
@service('data-sink/protocols/http')
consul;

@service('data-sink/protocols/local-storage')
settings;
@service('data-sink/protocols/http') consul;
@service('data-sink/protocols/local-storage') settings;

prepare(uri, data, assign) {
const [providerName, pathname] = parts(uri);
Expand Down
38 changes: 26 additions & 12 deletions ui/packages/consul-ui/app/services/data-source/service.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Service, { inject as service } from '@ember/service';
import { runInDebug } from '@ember/debug';
import { proxy } from 'consul-ui/utils/dom/event-source';
import { schedule } from '@ember/runloop';

Expand All @@ -12,18 +13,19 @@ let cache = null;
let sources = null;
// keeps a count of currently in use EventSources
let usage = null;
class URI {
constructor(uri) {
this.uri = uri;
}
toString() {
return this.uri;
}
}
export default class DataSourceService extends Service {
@service('dom')
dom;

@service('encoder')
encoder;

@service('data-source/protocols/http')
consul;

@service('data-source/protocols/local-storage')
settings;
@service('dom') dom;
@service('encoder') encoder;
@service('data-source/protocols/http') consul;
@service('data-source/protocols/local-storage') settings;

init() {
super.init(...arguments);
Expand Down Expand Up @@ -86,10 +88,22 @@ export default class DataSourceService extends Service {
return source;
}

uri(str) {
return new URI(str);
}

open(uri, ref, open = false) {
if (typeof uri !== 'string') {
if (!(uri instanceof URI) && typeof uri !== 'string') {
return this.unwrap(uri, ref);
}
runInDebug(
_ => {
if(!(uri instanceof URI)) {
console.error(new Error(`DataSource '${uri}' does not use the uri helper. Please ensure you use the uri helper to ensure correct encoding`))
}
}
);
uri = uri.toString();
let source;
// Check the cache for an EventSource that is already being used
// for this uri. If we don't have one, set one up.
Expand Down
4 changes: 2 additions & 2 deletions ui/packages/consul-ui/app/templates/application.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ as |route|>

{{! Tell CSS about our theme }}
<DataSource
@src="settings://consul:theme"
@src={{uri 'settings://consul:theme'}}
as |source|>
{{#each-in source.data as |key value|}}
{{#if (and value (contains key (array "color-scheme" "contrast")))}}
Expand All @@ -31,7 +31,7 @@ as |source|>
{{! If ACLs are enabled try get a token }}
{{#if (can "use acls")}}
<DataSource
@src="settings://consul:token"
@src={{uri 'settings://consul:token'}}
@onchange={{action (mut token) value="data"}}
/>
{{/if}}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
<Route
@name={{routeName}}
as |route|>
<DataLoader @src={{
uri '/${partition}/${nspace}/${dc}/sessions/for-node/${node}'
<DataLoader @src={{uri '/${partition}/${nspace}/${dc}/sessions/for-node/${node}'
(hash
partition=route.params.partition
nspace=route.params.nspace
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
@name={{routeName}}
as |route|>
<DataLoader
@src={{uri
'/${partition}/${nspace}/${dc}/intentions/for-service/${slug}'
@src={{uri '/${partition}/${nspace}/${dc}/intentions/for-service/${slug}'
(hash
partition=route.params.partition
nspace=route.params.nspace
Expand Down

0 comments on commit 35a92e8

Please sign in to comment.