UI - fix encoding for user-entered paths #6294

meirish · 2019-02-26T20:48:44Z

By default ember will encode and decode dynamic route segments for you, but there are several places where we're using star dynamic segments which don't come with this behavior so we need to do that encoding and decoding ourselves.

This is also true in ember data - model ids are encoded when you pass them to ember data's buildUrl method. There are several places where we're constructing api URLs via string concatenation, and not properly encoding the user-entered items.

Additionally, when passing the path into the filter box on the secret list page, we were not escaping the path so that we could create a valid Regex object. This meant that any characters that were valid in Regex pattern matching being present in the paths could potentially cause an error.

This PR changes the app to make sure we're encoding when passing elements to star segment urls, decoding when we're pulling params from those same routes, and properly escaping strings before passing them to the Regex constructor in the list-controller mixin.

Fixes #6282 .

… ember data buildUrl method

…input components

…Regex objects

johncowen

Hey Matthew!

There's a load of comments here just to triple check certain vars and whether they should be encoded or not, I'm sure they are fine but its just incase, I'd rather notice and say than not.

Also theres a query in there about the encodeURIComponent vs extra encoding in RouteRecognizer. I'm quibbling about changing consul to use extra encoding also (right now I'm pretty sure we just use encodeURIComponent, what's your thoughts on that?

Other than that mainly just nits

👍

johncowen · 2019-02-28T14:09:33Z

ui/app/adapters/transit-key.js

    }
    if (action === 'export' && param) {
      let [type, version] = param;
-      const exportBase = `${urlBase}/${type}-key/${id}`;
+      const exportBase = `${urlBase}/${type}-key/${encodePath(id)}`;


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?

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.

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.

johncowen · 2019-02-28T14:25:33Z

ui/app/utils/args-tokenizer.js

+  }
+
+  return args;
+}


Just wondering why the change from yargs? Also super nit, you have some vars here not sure if you want to switch those out, did this come from elsewhere?

I have some changes I need to push - we went back to yargs-parser - but the change here had to do with handling " and ' in the web console - the tokenizer tries to be smart about where splitting should occur based on quotes and single / double quote break it. I'm thinking maybe we shouldn't support single or double quotes in paths in the web cli, but haven't landed anywhere yet.

The vars are from the source file - https://github.com/yargs/yargs-parser/blob/master/lib/tokenize-arg-string.js

johncowen · 2019-02-28T14:56:21Z

ui/app/utils/path-encoding-helpers.js

+
+const {
+  Normalizer: { normalizePath, encodePathSegment },
+} = RouteRecognizer;


When I did this in Consul Land I wondered whether to use encodeURIComponent or this slightly customized version. Just wondering if I should change to use this RouteRecognizer one also?

Just been reading this:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent#Description

It mentions something similar to adhere to RFC 3986. I don't follow what impact following this/not following this would mean. Do you know what the ins and outs are? Are ! etc just reserved for future or something?

https://github.com/tildeio/route-recognizer/blob/master/lib/route-recognizer/normalizer.ts#L20 has some good comments - I was thinking it'd be nice to have the same semantics as the router. I think for adapter use it's not strictly necessary, but decoded slashes still look nicer.

johncowen · 2019-02-28T14:57:58Z

ui/tests/acceptance/secrets/backend/kv/secret-test.js

+      ']',
+      '^',
+      '_',
+    ].map(char => `${char}some`);


Nice! Water tight! 😍 I should check all these in consul land aswell really.

😄 - we wanted to recreate the script from the reported issue, and doing it this way meant we got to test the web cli too (and found it buggy) so win, win!

johncowen · 2019-02-28T14:59:08Z

ui/package.json

    "sass-svg-uri": "^1.0.0",
    "string.prototype.endswith": "^0.2.0",
    "string.prototype.startswith": "^0.2.0",
    "text-encoder-lite": "1.0.0",
    "walk-sync": "^0.3.3",
    "xstate": "^3.3.3",
-    "yargs-parser": "^10.0.0"
+    "yargs-parser": "^12.0.0"


Oh weird I thought you'd removed of yargs? Did I read it wrong or are you still using it somewhere?

We tried upgrading first, then pulled in the file we were using - but this has changed in the latest back to 10.

johncowen · 2019-02-28T15:00:09Z

ui/app/controllers/vault/cluster/secrets/backend/list.js

-    setFilterFocus(bool) {
-      this.set('filterFocused', bool);
-    },
-


Lots of removals here, I guess this is all covered in ListController now?

Yep! It was previously, we just weren't using the mixin very much.

johncowen · 2019-02-28T15:04:38Z

ui/app/adapters/secret-v2-version.js


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


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.

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).

johncowen · 2019-02-28T15:05:17Z

ui/app/adapters/secret-engine.js

    }
    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 + '/';


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

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.

johncowen · 2019-02-28T15:07:24Z

ui/app/adapters/role-jwt.js

@@ -9,7 +10,7 @@ export default ApplicationAdapter.extend({
    let [path, role] = JSON.parse(id);

    let namespace = get(snapshot, 'adapterOptions.namespace');
-    let url = `/v1/auth/${path}/oidc/auth_url`;
+    let url = `/v1/auth/${encodePath(path)}/oidc/auth_url`;
    let redirect_uri = `${window.location.origin}${this.router.urlFor('vault.cluster.oidc-callback', {
      auth_path: path,


Little sanity check, line 13 you encode line 15 you don't. 99.9% sure thats ok, but just to check.

Oh yep, urlFor is going to want encoded since it's router-related - thanks!

…e test

meirish · 2019-02-28T20:39:33Z

@johncowen Thanks for checking this out! I think I addressed all of the points you raised. The tests here just cover secret stuff as that's where Vault server is most lenient in allowed paths. I dug into the error we saw when I tried to create a transit key named #foo and it turns out that many endpoints in vault are more strict wrt characters, so that's disallowed at the server level.

johncowen · 2019-03-01T11:05:01Z

Ah yeah course! Makes sense. Good stuff, anyway thanks for the walkthrough and chat also, good to catch up!

John

dupainaulevain · 2019-03-01T18:36:05Z

@meirish thanks for the fix!

meirish requested review from a team February 26, 2019 20:49

meirish mentioned this pull request Feb 27, 2019

web UI fails to list secrets when the path contains a #*+[(\ character #6282

Closed

meirish added 12 commits February 27, 2019 14:44

directly depend on route-recognizer

405484d

add path encode helper using route-recognizer normalizer methods

3f4120e

encode user-entered paths/ids for places we're not using the built-in…

42f0bcd

… ember data buildUrl method

encode secret link params

0dfd63f

decode params from the url, and encode for linked-block and navigate-…

f50ef8a

…input components

add escape-string-regexp

bd063f7

use list-controller mixin and escape the string when contructing new …

e852029

…Regex objects

update yargs parser

87fdc11

add temp fork of args-parser tokenizer

21d032b

use forked tokenizer

8044d0e

encode paths in the console service

b14249b

add acceptance tests for kv secrets

21f4f71

meirish force-pushed the b-ui-encode-secrets branch from 2f5131c to 21f4f71 Compare February 27, 2019 20:45

meirish marked this pull request as ready for review February 27, 2019 20:54

meirish added 2 commits February 27, 2019 17:32

revert yargs-parser upgrade

9ede572

make encoding in linked-block an attribute, and use it on secret lists

5d08564

johncowen previously approved these changes Feb 28, 2019

View reviewed changes

meirish added 4 commits February 28, 2019 10:35

egp endpoints are enterprise-only, so include 'enterprise' text in th…

c4af1b7

…e test

fix routing test and exclude single quote from encoding tests

774eaf2

encode cli string before tokenizing

3aaa45b

encode auth_path for use with urlFor

f5f6605

meirish dismissed johncowen’s stale review via f5f6605 February 28, 2019 16:50

add test for single quote via UI input instead of web cli

9e97244

johncowen approved these changes Mar 1, 2019

View reviewed changes

meirish added this to the 1.1 milestone Mar 1, 2019

meirish modified the milestones: 1.1, 1.0.4 Mar 1, 2019

meirish merged commit b585c20 into master Mar 1, 2019

meirish deleted the b-ui-encode-secrets branch March 1, 2019 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI - fix encoding for user-entered paths #6294

UI - fix encoding for user-entered paths #6294

meirish commented Feb 26, 2019

johncowen left a comment

johncowen Feb 28, 2019

meirish Feb 28, 2019

meirish Feb 28, 2019

johncowen Feb 28, 2019

meirish Feb 28, 2019

johncowen Feb 28, 2019

meirish Feb 28, 2019

johncowen Feb 28, 2019

meirish Feb 28, 2019

johncowen Feb 28, 2019

meirish Feb 28, 2019

johncowen Feb 28, 2019

meirish Feb 28, 2019

johncowen Feb 28, 2019

meirish Feb 28, 2019

johncowen Feb 28, 2019

meirish Feb 28, 2019

johncowen Feb 28, 2019

meirish Feb 28, 2019

meirish commented Feb 28, 2019

johncowen commented Mar 1, 2019

dupainaulevain commented Mar 1, 2019

UI - fix encoding for user-entered paths #6294

UI - fix encoding for user-entered paths #6294

Conversation

meirish commented Feb 26, 2019

johncowen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meirish commented Feb 28, 2019

johncowen commented Mar 1, 2019

dupainaulevain commented Mar 1, 2019