Skip to content

Commit

Permalink
Remove decodeURIComponent method for KVv2 secret path on list view (#…
Browse files Browse the repository at this point in the history
…28698)

* remove encoding for KVv2

* test coverage

* changelog

* validations

* Revert "validations"

This reverts commit d6fd291.

* update subtext for secret path

* Update list.js

* Update secret-edit.js

* test coverage for data-octets

* Update list-directory.js

* fix modelForm test

* amend subText

* test selector things
  • Loading branch information
Monkeychip committed Oct 17, 2024
1 parent d7bfa41 commit 130d5a0
Show file tree
Hide file tree
Showing 6 changed files with 126 additions and 6 deletions.
3 changes: 3 additions & 0 deletions changelog/28698.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
ui: No longer running decodeURIComponent on KVv2 list view allowing percent encoded data-octets in path name.
```
6 changes: 5 additions & 1 deletion ui/app/models/kv/data.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,11 @@ const validations = {
@withFormFields()
export default class KvSecretDataModel extends Model {
@attr('string') backend; // dynamic path of secret -- set on response from value passed to queryRecord.
@attr('string', { label: 'Path for this secret' }) path;
@attr('string', {
label: 'Path for this secret',
subText: 'Names with forward slashes define hierarchical path structures.',
})
path;
@attr('object') secretData; // { key: value } data of the secret version

// Params returned on the GET response.
Expand Down
8 changes: 4 additions & 4 deletions ui/lib/kv/addon/routes/list-directory.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@
import Route from '@ember/routing/route';
import { service } from '@ember/service';
import { hash } from 'rsvp';
import { normalizePath } from 'vault/utils/path-encoding-helpers';
import { breadcrumbsForSecret } from 'kv/utils/kv-breadcrumbs';
import { pathIsDirectory } from 'kv/utils/kv-breadcrumbs';
import { pathIsDirectory, breadcrumbsForSecret } from 'kv/utils/kv-breadcrumbs';

export default class KvSecretsListRoute extends Route {
@service store;
Expand Down Expand Up @@ -48,7 +46,9 @@ export default class KvSecretsListRoute extends Route {
getPathToSecret(pathParam) {
if (!pathParam) return '';
// links and routing assumes pathToParam includes trailing slash
return pathIsDirectory(pathParam) ? normalizePath(pathParam) : normalizePath(`${pathParam}/`);
// users may want to include a percent-encoded octet like %2f in their path. Example: 'foo%2fbar' or non-data octets like 'foo%bar'.
// we are assuming the user intended to include these characters in their path and we should not decode them.
return pathIsDirectory(pathParam) ? pathParam : `${pathParam}/`;
}

model(params) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,117 @@ module('Acceptance | kv-v2 workflow | navigation', function (hooks) {
return;
});

test('KVv2 handles secret with % and space in path correctly', async function (assert) {
// To check this bug no longer happens: https://github.com/hashicorp/vault/issues/11616
await navToBackend(this.backend);
await click(PAGE.list.createSecret);
const pathWithSpace = 'per%centfu ll';
await typeIn(GENERAL.inputByAttr('path'), pathWithSpace);
await fillIn(FORM.keyInput(), 'someKey');
await fillIn(FORM.maskedValueInput(), 'someValue');
await click(FORM.saveBtn);
assert.dom(PAGE.title).hasText(pathWithSpace, 'title is full path without any encoding/decoding.');
assert
.dom(PAGE.breadcrumbAtIdx(1))
.hasText(this.backend, 'breadcrumb before secret path is backend path');
assert
.dom(PAGE.breadcrumbCurrentAtIdx(2))
.hasText('per%centfu ll', 'the current breadcrumb is value of the secret path');

await click(PAGE.breadcrumbAtIdx(1));
assert
.dom(`${PAGE.list.item(pathWithSpace)} [data-test-path]`)
.hasText(pathWithSpace, 'the list item is shown correctly');

await typeIn(PAGE.list.filter, 'per%');
await click('[data-test-kv-list-filter-submit]');
assert
.dom(`${PAGE.list.item(pathWithSpace)} [data-test-path]`)
.hasText(pathWithSpace, 'the list item is shown correctly after filtering');

await click(PAGE.list.item(pathWithSpace));
assert.strictEqual(
currentURL(),
`/vault/secrets/${this.backend}/kv/${encodeURIComponent(pathWithSpace)}`,
'Path is encoded in the URL'
);
});

test('KVv2 handles nested secret with % and space in path correctly', async function (assert) {
await navToBackend(this.backend);
await click(PAGE.list.createSecret);
const nestedPathWithSpace = 'per%/centfu ll';
await typeIn(GENERAL.inputByAttr('path'), nestedPathWithSpace);
await fillIn(FORM.keyInput(), 'someKey');
await fillIn(FORM.maskedValueInput(), 'someValue');
await click(FORM.saveBtn);
assert
.dom(PAGE.title)
.hasText(
nestedPathWithSpace,
'title is of the full nested path (directory included) without any encoding/decoding.'
);
assert.dom(PAGE.breadcrumbAtIdx(2)).hasText('per%');
assert
.dom(PAGE.breadcrumbCurrentAtIdx(3))
.hasText('centfu ll', 'the current breadcrumb is value centfu ll');

await click(PAGE.breadcrumbAtIdx(1));
assert
.dom(`${PAGE.list.item('per%/')} [data-test-path]`)
.hasText('per%/', 'the directory item is shown correctly');

await typeIn(PAGE.list.filter, 'per%/');
await click('[data-test-kv-list-filter-submit]');
assert
.dom(`${PAGE.list.item('centfu ll')} [data-test-path]`)
.hasText('centfu ll', 'the list item is shown correctly after filtering');

await click(PAGE.list.item('centfu ll'));
assert.strictEqual(
currentURL(),
`/vault/secrets/${this.backend}/kv/${encodeURIComponent(nestedPathWithSpace)}`,
'Path is encoded in the URL'
);
});

test('KVv2 handles nested secret with a percent-encoded data octet in path correctly', async function (assert) {
// To check this bug no longer happens: https://github.com/hashicorp/vault/issues/25905
await navToBackend(this.backend);
await click(PAGE.list.createSecret);
const pathDataOctet = 'hello/foo%2fbar/world';
await typeIn(GENERAL.inputByAttr('path'), pathDataOctet);
await fillIn(FORM.keyInput(), 'someKey');
await fillIn(FORM.maskedValueInput(), 'someValue');
await click(FORM.saveBtn);
assert
.dom(PAGE.title)
.hasText(
pathDataOctet,
'title is of the full nested path (directory included) without any encoding/decoding.'
);
assert
.dom(PAGE.breadcrumbAtIdx(2))
.hasText('hello', 'hello is the first directory and shows up as a separate breadcrumb');
assert
.dom(PAGE.breadcrumbAtIdx(3))
.hasText('foo%2fbar', 'foo%2fbar is the second directory and shows up as a separate breadcrumb');
assert.dom(PAGE.breadcrumbCurrentAtIdx(4)).hasText('world', 'the current breadcrumb is value world');

await click(PAGE.breadcrumbAtIdx(2));
assert
.dom(`${PAGE.list.item('foo%2fbar/')} [data-test-path]`)
.hasText('foo%2fbar/', 'the directory item is shown correctly');

await click(PAGE.list.item('foo%2fbar/'));
await click(PAGE.list.item('world'));
assert.strictEqual(
currentURL(),
`/vault/secrets/${this.backend}/kv/${encodeURIComponent(pathDataOctet)}`,
'Path is encoded in the URL'
);
});

module('admin persona', function (hooks) {
hooks.beforeEach(async function () {
const token = await runCmd(
Expand Down
2 changes: 1 addition & 1 deletion ui/tests/acceptance/secrets/backend/kv/secret-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ module('Acceptance | secrets/secret/create, read, delete', function (hooks) {
await deleteEngine(backend, assert);
});

test('UI handles secret with % in path correctly', async function (assert) {
test('KVv1 handles secret with % in path correctly', async function (assert) {
const enginePath = this.backend;
const secretPath = 'per%cent/%fu ll';
const [firstPath, secondPath] = secretPath.split('/');
Expand Down
2 changes: 2 additions & 0 deletions ui/tests/helpers/kv/kv-selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ export const PAGE = {
breadcrumbs: '[data-test-breadcrumbs]',
breadcrumb: '[data-test-breadcrumbs] li',
breadcrumbAtIdx: (idx) => `[data-test-breadcrumbs] li:nth-child(${idx + 1}) a`,
breadcrumbCurrentAtIdx: (idx) =>
`[data-test-breadcrumbs] li:nth-child(${idx + 1}) .hds-breadcrumb__current`,
infoRow: '[data-test-component="info-table-row"]',
infoRowValue: (label) => `[data-test-value-div="${label}"]`,
infoRowToggleMasked: (label) => `[data-test-value-div="${label}"] [data-test-button="toggle-masked"]`,
Expand Down

0 comments on commit 130d5a0

Please sign in to comment.