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.

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

Remove decodeURIComponent method for KVv2 secret path on list view #28698

Merged
merged 14 commits into from
Oct 16, 2024
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
1 change: 1 addition & 0 deletions ui/tests/unit/decorators/model-form-fields-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ module('Unit | Decorators | ModelFormFields', function (hooks) {
name: 'path',
options: {
label: 'Path for this secret',
subText: 'Names with forward slashes define hierarchical path structures.',
},
type: 'string',
},
Expand Down
Loading