From f18232cc2393ba73164a0817bb4adfccf27e0fc0 Mon Sep 17 00:00:00 2001 From: Martin Boulais <31805063+martinboulais@users.noreply.github.com> Date: Thu, 23 Jan 2025 17:22:31 +0100 Subject: [PATCH] Encode URI components in buildURL function (#1824) * Encode URI components in buildURL function * Fix * Use buildUrl instead of URLParams * Fix tests * Fix new tests --- lib/public/utilities/fetch/buildUrl.js | 14 +++++-- .../Overview/EnvironmentOverviewModel.js | 3 +- lib/public/views/Flps/Flps.js | 3 +- .../views/Logs/Overview/LogsOverviewModel.js | 3 +- .../views/Runs/Overview/RunsOverviewModel.js | 4 +- .../views/Tags/Details/TagDetailsModel.js | 3 +- .../views/Tags/Overview/TagsOverviewModel.js | 3 +- lib/utilities/buildUrl.js | 14 +++++-- test/lib/utilities/buildUrl.test.js | 2 + test/public/logs/create.test.js | 37 ++++++++++++------- 10 files changed, 59 insertions(+), 27 deletions(-) diff --git a/lib/public/utilities/fetch/buildUrl.js b/lib/public/utilities/fetch/buildUrl.js index 732f370d17..d94d9304fe 100644 --- a/lib/public/utilities/fetch/buildUrl.js +++ b/lib/public/utilities/fetch/buildUrl.js @@ -90,6 +90,14 @@ export const buildUrl = (baseURL, parameters) => { return url; } + /** + * Sanitize a value to be used as URL parameter key or value + * + * @param {string} value the value to sanitize + * @return {string} the sanitized value + */ + const sanitize = (value) => encodeURIComponent(decodeURIComponent(value)); + /** * Stringify a query parameter to be used in a URL and push it in the serialized query parameters list * @@ -111,16 +119,16 @@ export const buildUrl = (baseURL, parameters) => { if (typeof value === 'object' && value !== null) { for (const [subKey, subValue] of Object.entries(value)) { - formatAndPushQueryParameter(`${key}[${subKey}]`, subValue); + formatAndPushQueryParameter(`${key}[${sanitize(subKey)}]`, subValue); } return; } - serializedQueryParameters.push(`${key}=${value}`); + serializedQueryParameters.push(`${key}=${sanitize(value)}`); }; for (const [key, parameter] of Object.entries(parameters)) { - formatAndPushQueryParameter(key, parameter); + formatAndPushQueryParameter(sanitize(key), parameter); } return `${url}?${serializedQueryParameters.join('&')}`; diff --git a/lib/public/views/Environments/Overview/EnvironmentOverviewModel.js b/lib/public/views/Environments/Overview/EnvironmentOverviewModel.js index f46cfbbb8b..734d567541 100644 --- a/lib/public/views/Environments/Overview/EnvironmentOverviewModel.js +++ b/lib/public/views/Environments/Overview/EnvironmentOverviewModel.js @@ -14,6 +14,7 @@ import { PaginationModel } from '../../../components/Pagination/PaginationModel.js'; import { Observable, RemoteData } from '/js/src/index.js'; import { getRemoteDataSlice } from '../../../utilities/fetch/getRemoteDataSlice.js'; +import { buildUrl } from '../../../utilities/fetch/buildUrl.js'; /** * Environment overview page model @@ -52,7 +53,7 @@ export class EnvironmentOverviewModel extends Observable { 'page[limit]': this._pagination.itemsPerPage, }; - const endpoint = `/api/environments?${new URLSearchParams(params).toString()}`; + const endpoint = buildUrl('/api/environments', params); try { const { items, totalCount } = await getRemoteDataSlice(endpoint); diff --git a/lib/public/views/Flps/Flps.js b/lib/public/views/Flps/Flps.js index 09e3d040a4..9749e4ec0c 100644 --- a/lib/public/views/Flps/Flps.js +++ b/lib/public/views/Flps/Flps.js @@ -13,6 +13,7 @@ import { fetchClient, Observable, RemoteData } from '/js/src/index.js'; import { PaginationModel } from '../../components/Pagination/PaginationModel.js'; +import { buildUrl } from '../../utilities/fetch/buildUrl.js'; /** * Model representing handlers for homePage.js @@ -53,7 +54,7 @@ export default class Overview extends Observable { 'page[limit]': this._pagination.itemsPerPage, }; - const endpoint = `/api/flps?${new URLSearchParams(params).toString()}`; + const endpoint = buildUrl('/api/flps', params); const response = await fetchClient(endpoint, { method: 'GET' }); const result = await response.json(); diff --git a/lib/public/views/Logs/Overview/LogsOverviewModel.js b/lib/public/views/Logs/Overview/LogsOverviewModel.js index 2972678393..d020266cd3 100644 --- a/lib/public/views/Logs/Overview/LogsOverviewModel.js +++ b/lib/public/views/Logs/Overview/LogsOverviewModel.js @@ -19,6 +19,7 @@ import { FilterInputModel } from '../../../components/Filters/common/filters/Fil import { AuthorFilterModel } from '../../../components/Filters/LogsFilter/author/AuthorFilterModel.js'; import { PaginationModel } from '../../../components/Pagination/PaginationModel.js'; import { getRemoteDataSlice } from '../../../utilities/fetch/getRemoteDataSlice.js'; +import { buildUrl } from '../../../utilities/fetch/buildUrl.js'; /** * Model representing handlers for log entries page @@ -90,7 +91,7 @@ export class LogsOverviewModel extends Observable { 'page[limit]': this._pagination.itemsPerPage, }; - const endpoint = `/api/logs?${new URLSearchParams(params).toString()}`; + const endpoint = buildUrl('/api/logs', params); try { const { items, totalCount } = await getRemoteDataSlice(endpoint); diff --git a/lib/public/views/Runs/Overview/RunsOverviewModel.js b/lib/public/views/Runs/Overview/RunsOverviewModel.js index 25084a524e..03f2be92c5 100644 --- a/lib/public/views/Runs/Overview/RunsOverviewModel.js +++ b/lib/public/views/Runs/Overview/RunsOverviewModel.js @@ -26,6 +26,7 @@ import { TimeRangeInputModel } from '../../../components/Filters/common/filters/ import { FloatComparisonFilterModel } from '../../../components/Filters/common/filters/FloatComparisonFilterModel.js'; import { detectorsProvider } from '../../../services/detectors/detectorsProvider.js'; import { AliceL3AndDipoleFilteringModel } from '../../../components/Filters/RunsFilter/AliceL3AndDipoleFilteringModel.js'; +import { buildUrl } from '../../../utilities/fetch/buildUrl.js'; /** * Model representing handlers for runs page @@ -107,8 +108,7 @@ export class RunsOverviewModel extends OverviewPageModel { * @inheritdoc */ getRootEndpoint() { - const paramsString = new URLSearchParams(this._getFilterQueryParams()).toString(); - return `/api/runs?${paramsString}`; + return buildUrl('/api/runs', this._getFilterQueryParams()); } // eslint-disable-next-line valid-jsdoc diff --git a/lib/public/views/Tags/Details/TagDetailsModel.js b/lib/public/views/Tags/Details/TagDetailsModel.js index 2b4b2ca797..574aba6ddb 100644 --- a/lib/public/views/Tags/Details/TagDetailsModel.js +++ b/lib/public/views/Tags/Details/TagDetailsModel.js @@ -20,6 +20,7 @@ import { jsonFetch } from '../../../utilities/fetch/jsonFetch.js'; import { Observable, RemoteData } from '/js/src/index.js'; import { tagsProvider } from '../../../services/tag/tagsProvider.js'; import { TabbedPanelModel } from '../../../components/TabbedPanel/TabbedPanelModel.js'; +import { buildUrl } from '../../../utilities/fetch/buildUrl.js'; export const TAG_DETAILS_PANELS_KEYS = { LOGS: 'logs', @@ -272,7 +273,7 @@ class TagDetailsTabbedPanelModel extends TabbedPanelModel { }; // API call to fetch logs for current tags, include the pagination params. - const { items, totalCount } = await getRemoteDataSlice(`/api/tags/${this._tagId}/logs?${new URLSearchParams(params).toString()}`); + const { items, totalCount } = await getRemoteDataSlice(buildUrl(`/api/tags/${this._tagId}/logs`, params)); this._pagination.itemsCount = totalCount; // Set items count diff --git a/lib/public/views/Tags/Overview/TagsOverviewModel.js b/lib/public/views/Tags/Overview/TagsOverviewModel.js index 6569a1dc95..cd6dec6038 100644 --- a/lib/public/views/Tags/Overview/TagsOverviewModel.js +++ b/lib/public/views/Tags/Overview/TagsOverviewModel.js @@ -13,6 +13,7 @@ import { Observable, RemoteData } from '/js/src/index.js'; import { debounce } from '../../../utilities/debounce.js'; import { getRemoteDataSlice } from '../../../utilities/fetch/getRemoteDataSlice.js'; +import { buildUrl } from '../../../utilities/fetch/buildUrl.js'; /** * Model for tags overview page @@ -59,7 +60,7 @@ export class TagsOverviewModel extends Observable { }, }; - const endpoint = `/api/tags?${new URLSearchParams(params).toString()}`; + const endpoint = buildUrl('/api/tags', params); try { const { items } = await getRemoteDataSlice(endpoint); diff --git a/lib/utilities/buildUrl.js b/lib/utilities/buildUrl.js index b667d81120..787cd3d317 100644 --- a/lib/utilities/buildUrl.js +++ b/lib/utilities/buildUrl.js @@ -90,6 +90,14 @@ exports.buildUrl = (baseURL, parameters) => { return url; } + /** + * Sanitize a value to be used as URL parameter key or value + * + * @param {string} value the value to sanitize + * @return {string} the sanitized value + */ + const sanitize = (value) => encodeURIComponent(decodeURIComponent(value)); + /** * Stringify a query parameter to be used in a URL and push it in the serialized query parameters list * @@ -111,16 +119,16 @@ exports.buildUrl = (baseURL, parameters) => { if (typeof value === 'object' && value !== null) { for (const [subKey, subValue] of Object.entries(value)) { - formatAndPushQueryParameter(`${key}[${subKey}]`, subValue); + formatAndPushQueryParameter(`${key}[${sanitize(subKey)}]`, subValue); } return; } - serializedQueryParameters.push(`${key}=${value}`); + serializedQueryParameters.push(`${key}=${sanitize(value)}`); }; for (const [key, parameter] of Object.entries(parameters)) { - formatAndPushQueryParameter(key, parameter); + formatAndPushQueryParameter(sanitize(key), parameter); } return `${url}?${serializedQueryParameters.join('&')}`; diff --git a/test/lib/utilities/buildUrl.test.js b/test/lib/utilities/buildUrl.test.js index 7a61880a1a..b3612e4b1b 100644 --- a/test/lib/utilities/buildUrl.test.js +++ b/test/lib/utilities/buildUrl.test.js @@ -20,6 +20,7 @@ module.exports = () => { simple: 'hello', key: { nested: [1, 2], + '=': 12, }, }).split('?'); @@ -27,6 +28,7 @@ module.exports = () => { expect(parametersExpressions).to.includes('simple=hello'); expect(parametersExpressions).to.includes('key[nested][]=1'); expect(parametersExpressions).to.includes('key[nested][]=2'); + expect(parametersExpressions).to.includes('key[%3D]=12'); }); it('should successfully build URL by combining existing parameters', () => { const [, fullParametersExpressions] = buildUrl('https://example.com?simple=hello&key1[key2][]=13&key1[key2][]=35', { diff --git a/test/public/logs/create.test.js b/test/public/logs/create.test.js index 9ec56da9cc..b58837c779 100644 --- a/test/public/logs/create.test.js +++ b/test/public/logs/create.test.js @@ -109,7 +109,7 @@ module.exports = () => { }); it('Should successfully display the log-reply form', async () => { - await goToPage(page, 'log-reply&parentLogId=1'); + await goToPage(page, 'log-reply', { queryParameters: { parentLogId: '1' } }); await expectInnerText(page, 'h3', 'Reply to: First entry'); @@ -122,7 +122,7 @@ module.exports = () => { }); it('Should successfully display the autofilled runs, environments and lhcFills when replying', async () => { - await goToPage(page, 'log-reply&parentLogId=119'); + await goToPage(page, 'log-reply', { queryParameters: { parentLogId: '119' } }); await expectInputValue(page, 'input#run-numbers', '2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22'); await expectInputValue(page, 'input#environments', 'Dxi029djX, eZF99lH6'); @@ -130,7 +130,7 @@ module.exports = () => { }); it('Should verify that form autofill all inputs with provided full parameters.', async () => { - await goToPage(page, 'log-create&runNumbers=1,2,3&lhcFillNumbers=1,2,3&environmentIds=1,2,3'); + await goToPage(page, 'log-create', { queryParameters: { runNumbers: '1,2,3', lhcFillNumbers: '1,2,3', environmentIds: '1,2,3' } }); await expectInputValue(page, 'input#run-numbers', '1,2,3'); await expectInputValue(page, 'input#environments', '1,2,3'); @@ -138,7 +138,7 @@ module.exports = () => { }); it('Should verify that form autofill runNumbers only when leaving other parameters empty.', async () => { - await goToPage(page, 'log-create&runNumbers=1,2,3'); + await goToPage(page, 'log-create', { queryParameters: { runNumbers: '1,2,3' } }); await expectInputValue(page, 'input#run-numbers', '1,2,3'); await expectInputValue(page, 'input#environments', ''); @@ -146,7 +146,7 @@ module.exports = () => { }); it('Should verify that form autofill environmentIds only when leaving other parameters empty.', async () => { - await goToPage(page, 'log-create&environmentIds=1,2,3'); + await goToPage(page, 'log-create', { queryParameters: { environmentIds: '1,2,3' } }); await expectInputValue(page, 'input#run-numbers', ''); await expectInputValue(page, 'input#environments', '1,2,3'); @@ -154,7 +154,7 @@ module.exports = () => { }); it('Should verify that form autofill the lhcFillNumbers only when leaving other parameters empty.', async () => { - await goToPage(page, 'log-create&lhcFillNumbers=1,2,3'); + await goToPage(page, 'log-create', { queryParameters: { lhcFillNumbers: '1,2,3' } }); await expectInputValue(page, 'input#run-numbers', ''); await expectInputValue(page, 'input#environments', ''); @@ -162,7 +162,7 @@ module.exports = () => { }); it('Should verify that form autofill the runNumbers and environmentIds when leaving lhcFills empty.', async () => { - await goToPage(page, 'log-create&runNumbers=1,2,3&environmentIds=1,2,3'); + await goToPage(page, 'log-create', { queryParameters: { runNumbers: '1,2,3', environmentIds: '1,2,3' } }); await expectInputValue(page, 'input#run-numbers', '1,2,3'); await expectInputValue(page, 'input#environments', '1,2,3'); @@ -170,7 +170,7 @@ module.exports = () => { }); it('Should verify that form autofill the runNumbers and lhcFillNumbers when leaving environmentIds empty.', async () => { - await goToPage(page, 'log-create&runNumbers=1,2,3&lhcFillNumbers=1,2,3'); + await goToPage(page, 'log-create', { queryParameters: { runNumbers: '1,2,3', lhcFillNumbers: '1,2,3' } }); await expectInputValue(page, 'input#run-numbers', '1,2,3'); await expectInputValue(page, 'input#environments', ''); @@ -178,7 +178,7 @@ module.exports = () => { }); it('Should verify that form autofill the environmentIds and lhcFillNumbers when leaving runNumbers empty.', async () => { - await goToPage(page, 'log-create&environmentIds=1,2,3&lhcFillNumbers=1,2,3'); + await goToPage(page, 'log-create', { queryParameters: { environmentIds: '1,2,3', lhcFillNumbers: '1,2,3' } }); await expectInputValue(page, 'input#run-numbers', ''); await expectInputValue(page, 'input#environments', '1,2,3'); @@ -187,7 +187,7 @@ module.exports = () => { it('Should set the correct template when templateKey is specified.', async () => { const templateKey = 'on-call'; - await goToPage(page, `log-create&templateKey=${templateKey}`); + await goToPage(page, 'log-create', { queryParameters: { templateKey } }); await page.waitForSelector('select'); const selectedOption = await page.evaluate(() => document.querySelector('select').value); @@ -200,8 +200,8 @@ module.exports = () => { const issueDescription = 'This is a sample issue description'; await goToPage( page, - `log-create&templateKey=${templateKey}&detectorOrSubsystem=${detectorOrSubsystem}&` + - `issueDescription=${issueDescription}`, + 'log-create', + { queryParameters: { templateKey, detectorOrSubsystem, issueDescription } }, ); await expectInputValue(page, 'input#run-numbers', ''); @@ -214,8 +214,17 @@ module.exports = () => { it('Should autofill all inputs with provided full parameters.', async () => { await goToPage( page, - 'log-create&runNumbers=1,2,3&lhcFillNumbers=1,2,3&environmentIds=1,2,3&templateKey=on-call&detectorOrSubsystem=ALL&' + - 'issueDescription=This is a sample issue description', + 'log-create', + { + queryParameters: { + runNumbers: '1,2,3', + lhcFillNumbers: '1,2,3', + environmentIds: '1,2,3', + templateKey: 'on-call', + detectorOrSubsystem: 'ALL', + issueDescription: 'This is a sample issue description', + }, + }, ); await expectInputValue(page, 'input#run-numbers', '1,2,3');