Skip to content
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.

Sign up for GitHub

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

UI - fix encoding for user-entered paths #6294

Merged
merged 19 commits into from
Mar 1, 2019
Merged

UI - fix encoding for user-entered paths #6294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
405484d
directly depend on route-recognizer
meirish Feb 26, 2019
3f4120e
add path encode helper using route-recognizer normalizer methods
meirish Feb 26, 2019
42f0bcd
encode user-entered paths/ids for places we're not using the built-in…
meirish Feb 26, 2019
0dfd63f
encode secret link params
meirish Feb 26, 2019
f50ef8a
decode params from the url, and encode for linked-block and navigate-…
meirish Feb 26, 2019
bd063f7
add escape-string-regexp
meirish Feb 26, 2019
e852029
use list-controller mixin and escape the string when contructing new …
meirish Feb 26, 2019
87fdc11
update yargs parser
meirish Feb 27, 2019
21d032b
add temp fork of args-parser tokenizer
meirish Feb 27, 2019
8044d0e
use forked tokenizer
meirish Feb 27, 2019
b14249b
encode paths in the console service
meirish Feb 27, 2019
21f4f71
add acceptance tests for kv secrets
meirish Feb 27, 2019
9ede572
revert yargs-parser upgrade
meirish Feb 27, 2019
5d08564
make encoding in linked-block an attribute, and use it on secret lists
meirish Feb 27, 2019
c4af1b7
egp endpoints are enterprise-only, so include 'enterprise' text in th…
meirish Feb 28, 2019
774eaf2
fix routing test and exclude single quote from encoding tests
meirish Feb 28, 2019
3aaa45b
encode cli string before tokenizing
meirish Feb 28, 2019
f5f6605
encode auth_path for use with urlFor
meirish Feb 28, 2019
9e97244
add test for single quote via UI input instead of web cli
meirish Feb 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions ui/app/adapters/auth-method.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ import { assign } from '@ember/polyfills';
import { get, set } from '@ember/object';
import ApplicationAdapter from './application';
import DS from 'ember-data';
import { encodePath } from 'vault/utils/path-encoding-helpers';

export default ApplicationAdapter.extend({
url(path) {
const url = `${this.buildURL()}/auth`;
return path ? url + '/' + path : url;
return path ? url + '/' + encodePath(path) : url;
},

// used in updateRecord on the model#tune action
Expand Down Expand Up @@ -58,6 +59,6 @@ export default ApplicationAdapter.extend({
},

exchangeOIDC(path, state, code) {
return this.ajax(`/v1/auth/${path}/oidc/callback`, 'GET', { data: { state, code } });
return this.ajax(`/v1/auth/${encodePath(path)}/oidc/callback`, 'GET', { data: { state, code } });
},
});
7 changes: 4 additions & 3 deletions ui/app/adapters/lease.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import ApplicationAdapter from './application';
import { encodePath } from 'vault/utils/path-encoding-helpers';

export default ApplicationAdapter.extend({
revokePrefix(prefix) {
let url = this.buildURL() + '/leases/revoke-prefix/' + prefix;
let url = this.buildURL() + '/leases/revoke-prefix/' + encodePath(prefix);
url = url.replace(/\/$/, '');
return this.ajax(url, 'PUT');
},

forceRevokePrefix(prefix) {
let url = this.buildURL() + '/leases/revoke-prefix/' + prefix;
let url = this.buildURL() + '/leases/revoke-prefix/' + encodePath(prefix);
url = url.replace(/\/$/, '');
return this.ajax(url, 'PUT');
},
Expand Down Expand Up @@ -43,7 +44,7 @@ export default ApplicationAdapter.extend({

query(store, type, query) {
const prefix = query.prefix || '';
return this.ajax(this.buildURL() + '/leases/lookup/' + prefix, 'GET', {
return this.ajax(this.buildURL() + '/leases/lookup/' + encodePath(prefix), 'GET', {
data: {
list: true,
},
Expand Down
5 changes: 3 additions & 2 deletions ui/app/adapters/role-aws.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { assign } from '@ember/polyfills';
import ApplicationAdapter from './application';
import { encodePath } from 'vault/utils/path-encoding-helpers';

export default ApplicationAdapter.extend({
namespace: 'v1',
Expand Down Expand Up @@ -31,9 +32,9 @@ export default ApplicationAdapter.extend({
},

urlForRole(backend, id) {
let url = `${this.buildURL()}/${backend}/roles`;
let url = `${this.buildURL()}/${encodePath(backend)}/roles`;
if (id) {
url = url + '/' + id;
url = url + '/' + encodePath(id);
}
return url;
},
Expand Down
2 changes: 2 additions & 0 deletions ui/app/adapters/role-jwt.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import ApplicationAdapter from './application';
import { inject as service } from '@ember/service';
import { get } from '@ember/object';
import { encodePath } from 'vault/utils/path-encoding-helpers';

export default ApplicationAdapter.extend({
router: service(),

findRecord(store, type, id, snapshot) {
let [path, role] = JSON.parse(id);
path = encodePath(path);

let namespace = get(snapshot, 'adapterOptions.namespace');
let url = `/v1/auth/${path}/oidc/auth_url`;
Expand Down
5 changes: 3 additions & 2 deletions ui/app/adapters/role-pki.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { assign } from '@ember/polyfills';
import ApplicationAdapter from './application';
import { encodePath } from 'vault/utils/path-encoding-helpers';

export default ApplicationAdapter.extend({
namespace: 'v1',
Expand Down Expand Up @@ -31,9 +32,9 @@ export default ApplicationAdapter.extend({
},

urlForRole(backend, id) {
let url = `${this.buildURL()}/${backend}/roles`;
let url = `${this.buildURL()}/${encodePath(backend)}/roles`;
if (id) {
url = url + '/' + id;
url = url + '/' + encodePath(id);
}
return url;
},
Expand Down
7 changes: 4 additions & 3 deletions ui/app/adapters/role-ssh.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { assign } from '@ember/polyfills';
import { resolve, allSettled } from 'rsvp';
import ApplicationAdapter from './application';
import { encodePath } from 'vault/utils/path-encoding-helpers';

export default ApplicationAdapter.extend({
namespace: 'v1',
Expand Down Expand Up @@ -34,9 +35,9 @@ export default ApplicationAdapter.extend({
},

urlForRole(backend, id) {
let url = `${this.buildURL()}/${backend}/roles`;
let url = `${this.buildURL()}/${encodePath(backend)}/roles`;
if (id) {
url = url + '/' + id;
url = url + '/' + encodePath(id);
}
return url;
},
Expand Down Expand Up @@ -84,7 +85,7 @@ export default ApplicationAdapter.extend({

findAllZeroAddress(store, query) {
const { backend } = query;
const url = `/v1/${backend}/config/zeroaddress`;
const url = `/v1/${encodePath(backend)}/config/zeroaddress`;
return this.ajax(url, 'GET');
},

Expand Down
17 changes: 9 additions & 8 deletions ui/app/adapters/secret-engine.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
import { assign } from '@ember/polyfills';
import ApplicationAdapter from './application';
import { encodePath } from 'vault/utils/path-encoding-helpers';

export default ApplicationAdapter.extend({
url(path) {
const url = `${this.buildURL()}/mounts`;
return path ? url + '/' + path : url;
return path ? url + '/' + encodePath(path) : url;
},

internalURL(path) {
let url = `/${this.urlPrefix()}/internal/ui/mounts`;
if (path) {
url = `${url}/${path}`;
url = `${url}/${encodePath(path)}`;
}
return url;
},
Expand Down Expand Up @@ -38,14 +39,14 @@ export default ApplicationAdapter.extend({

findRecord(store, type, path, snapshot) {
if (snapshot.attr('type') === 'ssh') {
return this.ajax(`/v1/${path}/config/ca`, 'GET');
return this.ajax(`/v1/${encodePath(path)}/config/ca`, 'GET');
}
return;
},

queryRecord(store, type, query) {
if (query.type === 'aws') {
return this.ajax(`/v1/${query.backend}/config/lease`, 'GET').then(resp => {
return this.ajax(`/v1/${encodePath(query.backend)}/config/lease`, 'GET').then(resp => {
resp.path = query.backend + '/';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again just a sanity check that this doesn't need encoding here, you encode it on 49 but not on 50

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we're only encoding when it goes to the server via ajax, and when it goes to the URL for routing - here it's going to be put in ember data for the model so we want the decoded version for display.

return resp;
});
Expand All @@ -61,25 +62,25 @@ export default ApplicationAdapter.extend({
if (apiPath) {
const serializer = store.serializerFor(type.modelName);
const data = serializer.serialize(snapshot);
const path = snapshot.id;
const path = encodePath(snapshot.id);
return this.ajax(`/v1/${path}/${apiPath}`, options.isDelete ? 'DELETE' : 'POST', { data });
}
},

saveAWSRoot(store, type, snapshot) {
let { data } = snapshot.adapterOptions;
const path = snapshot.id;
const path = encodePath(snapshot.id);
return this.ajax(`/v1/${path}/config/root`, 'POST', { data });
},

saveAWSLease(store, type, snapshot) {
let { data } = snapshot.adapterOptions;
const path = snapshot.id;
const path = encodePath(snapshot.id);
return this.ajax(`/v1/${path}/config/lease`, 'POST', { data });
},

saveZeroAddressConfig(store, type, snapshot) {
const path = snapshot.id;
const path = encodePath(snapshot.id);
const roles = store
.peekAll('role-ssh')
.filterBy('zeroAddress')
Expand Down
5 changes: 3 additions & 2 deletions ui/app/adapters/secret-v2-version.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@ import { isEmpty } from '@ember/utils';
import { get } from '@ember/object';
import ApplicationAdapter from './application';
import DS from 'ember-data';
import { encodePath } from 'vault/utils/path-encoding-helpers';

export default ApplicationAdapter.extend({
namespace: 'v1',
_url(backend, id, infix = 'data') {
let url = `${this.buildURL()}/${backend}/${infix}/`;
let url = `${this.buildURL()}/${encodePath(backend)}/${infix}/`;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah this was the other possibly user generated value here infix. I'm 99.9% sure it's probably hardcoded somewhere else I can see here. But just to triple sanity check with you in case its coming from a user generated value somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it's hardcoded - it'll either be data or metadata - the apis for v2 have that prefix (infix here because there's /v1/backend_path at the start of all of them).

if (!isEmpty(id)) {
url = url + id;
url = url + encodePath(id);
}
return url;
},
Expand Down
5 changes: 3 additions & 2 deletions ui/app/adapters/secret-v2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
/* eslint-disable */
import { isEmpty } from '@ember/utils';
import ApplicationAdapter from './application';
import { encodePath } from 'vault/utils/path-encoding-helpers';

export default ApplicationAdapter.extend({
namespace: 'v1',
_url(backend, id) {
let url = `${this.buildURL()}/${backend}/metadata/`;
let url = `${this.buildURL()}/${encodePath(backend)}/metadata/`;
if (!isEmpty(id)) {
url = url + id;
url = url + encodePath(id);
}
return url;
},
Expand Down
5 changes: 3 additions & 2 deletions ui/app/adapters/secret.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { isEmpty } from '@ember/utils';
import ApplicationAdapter from './application';
import { encodePath } from 'vault/utils/path-encoding-helpers';

export default ApplicationAdapter.extend({
namespace: 'v1',
Expand All @@ -26,9 +27,9 @@ export default ApplicationAdapter.extend({
},

urlForSecret(backend, id) {
let url = `${this.buildURL()}/${backend}/`;
let url = `${this.buildURL()}/${encodePath(backend)}/`;
if (!isEmpty(id)) {
url = url + id;
url = url + encodePath(id);
}

return url;
Expand Down
13 changes: 7 additions & 6 deletions ui/app/adapters/transit-key.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import ApplicationAdapter from './application';
import { pluralize } from 'ember-inflector';
import { encodePath } from 'vault/utils/path-encoding-helpers';

export default ApplicationAdapter.extend({
namespace: 'v1',
Expand Down Expand Up @@ -47,29 +48,29 @@ export default ApplicationAdapter.extend({
},

urlForSecret(backend, id) {
let url = `${this.buildURL()}/${backend}/keys/`;
let url = `${this.buildURL()}/${encodePath(backend)}/keys/`;
if (id) {
url += id;
url += encodePath(id);
}
return url;
},

urlForAction(action, backend, id, param) {
let urlBase = `${this.buildURL()}/${backend}/${action}`;
let urlBase = `${this.buildURL()}/${encodePath(backend)}/${action}`;
// these aren't key-specific
if (action === 'hash' || action === 'random') {
return urlBase;
}
if (action === 'datakey' && param) {
// datakey action has `wrapped` or `plaintext` as part of the url
return `${urlBase}/${param}/${id}`;
return `${urlBase}/${param}/${encodePath(id)}`;
}
if (action === 'export' && param) {
let [type, version] = param;
const exportBase = `${urlBase}/${type}-key/${id}`;
const exportBase = `${urlBase}/${type}-key/${encodePath(id)}`;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure you've checked these but just a triple sanity check that action, param version and type are hardcoded elsewhere/not user generated. There are a couple of others I think down below. I'll just put a little comment next to them so you know where, but same thing, just a triple sanity check.

Actually, just spotted something, 1 question around a slightly different thing here. You treat param as a string on line 66, but then it's an array on line 69? Is that ok? Maybe I read it wrong?

Sorry 1 more tiny teeny nit. I'm not sure what your let/const usage is like. You use const in one place here and let in others. Should this single const be let, or all the lets be consts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the type and action are hard-coded elsewhere in the app, the only thing the user does is name the transit key (which is the id).

Good spot on the array vs string, I'll follow up on that!

Re: const/let - yeah we need to decide as a team how we want to use this. I personally don't think const adds much most of the time (which is also why it's all over the place across the codebase), but we should decide on a standard and stick to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, found the param bit - looks like in export it is an array, and datakey it's a string - I created a ticket internally to track that as we have upcoming work in that area which would be a good time to refactor it.

return version ? `${exportBase}/${version}` : exportBase;
}
return `${urlBase}/${id}`;
return `${urlBase}/${encodePath(id)}`;
},

optionsForQuery(id) {
Expand Down
3 changes: 2 additions & 1 deletion ui/app/components/key-value-header.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { computed } from '@ember/object';
import Component from '@ember/component';
import utils from 'vault/lib/key-utils';
import { encodePath } from 'vault/utils/path-encoding-helpers';

export default Component.extend({
tagName: 'nav',
Expand Down Expand Up @@ -31,7 +32,7 @@ export default Component.extend({
let crumbs = [];
const root = this.get('root');
const baseKey = this.get('baseKey.display') || this.get('baseKey.id');
const baseKeyModel = this.get('baseKey.id');
const baseKeyModel = encodePath(this.get('baseKey.id'));

if (root) {
crumbs.push(root);
Expand Down
13 changes: 12 additions & 1 deletion ui/app/components/linked-block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { inject as service } from '@ember/service';
import Component from '@ember/component';
import hbs from 'htmlbars-inline-precompile';
import { encodePath } from 'vault/utils/path-encoding-helpers';

let LinkedBlockComponent = Component.extend({
router: service(),
Expand All @@ -11,6 +12,8 @@ let LinkedBlockComponent = Component.extend({

queryParams: null,

encode: false,

click(event) {
const $target = this.$(event.target);
const isAnchorOrButton =
Expand All @@ -19,7 +22,15 @@ let LinkedBlockComponent = Component.extend({
$target.closest('button', event.currentTarget).length > 0 ||
$target.closest('a', event.currentTarget).length > 0;
if (!isAnchorOrButton) {
const params = this.get('params');
let params = this.get('params');
if (this.encode) {
params = params.map((param, index) => {
if (index === 0 || typeof param !== 'string') {
return param;
}
return encodePath(param);
});
}
const queryParams = this.get('queryParams');
if (queryParams) {
params.push({ queryParams });
Expand Down
12 changes: 10 additions & 2 deletions ui/app/components/navigate-input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import Component from '@ember/component';
import utils from 'vault/lib/key-utils';
import keys from 'vault/lib/keycodes';
import FocusOnInsertMixin from 'vault/mixins/focus-on-insert';
import { encodePath } from 'vault/utils/path-encoding-helpers';

const routeFor = function(type, mode) {
const MODES = {
Expand Down Expand Up @@ -43,8 +44,15 @@ export default Component.extend(FocusOnInsertMixin, {
filterMatchesKey: null,
firstPartialMatch: null,

transitionToRoute: function() {
this.get('router').transitionTo(...arguments);
transitionToRoute(...args) {
let params = args.map((param, index) => {
if (index === 0 || typeof param !== 'string') {
return param;
}
return encodePath(param);
});

this.get('router').transitionTo(...params);
},

shouldFocus: false,
Expand Down
3 changes: 2 additions & 1 deletion ui/app/components/secret-link.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { computed } from '@ember/object';
import Component from '@ember/component';
import { encodePath } from 'vault/utils/path-encoding-helpers';

export function linkParams({ mode, secret, queryParams }) {
let params;
Expand All @@ -8,7 +9,7 @@ export function linkParams({ mode, secret, queryParams }) {
if (!secret || secret === ' ') {
params = [route + '-root'];
} else {
params = [route, secret];
params = [route, encodePath(secret)];
}

if (queryParams) {
Expand Down
Loading