Skip to content

Commit 697a045

Browse files
gortiz-dotcmszJaaal
gortiz-dotcms
and
zJaaal
authored
fix(pageAPI): Apply Regex and sanitization correctly (dotCMS#31422)
### Problems found 1. Regarding the first mentioned issue, the vanityUrl was trying to redirect to $1, this is because when handling regex in vanityUrl a function was needed to be executed to process the url and get the final one that the page should be redirected to, and in the cases of Temporary and Permanent Redirect that function was not being executed 2. About the second issue, it was a case that was not being contemplated when using vanityUrls. As the uri is `(.*)/index` and the forwardTo `$1` this resulted in an empty string url that was generating a blank page when accesing through browser. 3. Also the regex of the second issue was generating an infinite loop in the UVE, this was because it was trying to forward to an empty string. The frontend of the UVE was handling this case and fallbacking to `index` in the url, then navigating to pageAPI to render `index` but then the backend finds that there is a vanityUrl and forwards to empty string, then the frontend redirects again to `index` and so on. ### Proposed Changes * To solve the problem **1**: Apply a workaround to modify the forwardTo value (executing the function that processes the url) and set it to the CachedVanityUrl which is the entity used in the Temporary and Permanent Redirect case. This way in those cases the forwardTo is going to be correct, and return to the frontend as it should. * To solve the problem **2**: Add an extra check in the `processForward` function that verifies if the forward url is empty (this means that the forward should redirect to the root) then set `/` as the final url. This will make the url work as it should in the browser. * To solve the problem **3**: Modify what the frontend send to the backend in the cases where it receives from the frontend an empty url. Instead of index, send to the backend `/`, this will result in the same behaviour as index, but allowing the user to handle this last case. Now when using a vanityUrl with `index` in the regex, and forwarding to `/` it won't generate an infinite loop. * Additional fix: when using a vanityUrl with regex, and forwarding to an url that matches with that regex, it results in an infinite loop error. So instead of using `DotPreconditions.checkArgument` to throw an error, I made a check to see if the url is self referenced, and if it true, then fallback to the main url. **Note and question:** `final String urlIn = !url.startsWith(StringPool.SLASH) ? StringPool.SLASH + url : url;` Was added in the processForward function, this was because as navigating through the UVE, the url that the backend gets doesn't have any starting slashes, so in the case that the issue specifies, using `(.*)/index` regex (and forwardTo $1) didn't match when navigating to index as the url that the function receives would be `index` (without slash). So as the `resolveVanityUrlIfPresent` does, this will try to check for matches on the vanityUrl but with a `correctedUri` that will have a `/` at the start. Before the addition of the slash and as the url didn't matched, the forwardTo field remained on $1 and then we got the same error as the problem 1. Adding the slash resulted in the problem 3, which was also solved. So the questions would be if this is ok. Should we handle it in a different way? Could be some cases in where the addition of the slash results in a wrong url or redirect? ### Checklist - [x] Tests --------- Co-authored-by: Jalinson Diaz <[email protected]>
1 parent e754349 commit 697a045

File tree

16 files changed

+184
-72
lines changed

16 files changed

+184
-72
lines changed

‎core-web/libs/portlets/edit-ema/portlet/src/lib/dot-ema-shell/dot-ema-shell.component.spec.ts‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -329,9 +329,9 @@ describe('DotEmaShellComponent', () => {
329329
);
330330

331331
spectator.detectChanges();
332-
expect(spyloadPageAsset).toHaveBeenCalledWith({ ...params, url: 'index' });
332+
expect(spyloadPageAsset).toHaveBeenCalledWith({ ...params, url: '/index' });
333333
expect(spyLocation).toHaveBeenCalledWith(
334-
'/?language_id=1&url=index&variantName=DEFAULT&mode=EDIT_MODE'
334+
'/?language_id=1&url=%2Findex&variantName=DEFAULT&mode=EDIT_MODE'
335335
);
336336
});
337337

@@ -342,7 +342,7 @@ describe('DotEmaShellComponent', () => {
342342

343343
const params = {
344344
...INITIAL_PAGE_PARAMS,
345-
url: '/some-url/some-nested-url/'
345+
url: '/some-url/some-nested-url'
346346
};
347347

348348
overrideRouteSnashot(
@@ -353,10 +353,10 @@ describe('DotEmaShellComponent', () => {
353353
spectator.detectChanges();
354354
expect(spyloadPageAsset).toHaveBeenCalledWith({
355355
...params,
356-
url: 'some-url/some-nested-url'
356+
url: '/some-url/some-nested-url'
357357
});
358358
expect(spyLocation).toHaveBeenCalledWith(
359-
'/?language_id=1&url=some-url%2Fsome-nested-url&variantName=DEFAULT&mode=EDIT_MODE'
359+
'/?language_id=1&url=%2Fsome-url%2Fsome-nested-url&variantName=DEFAULT&mode=EDIT_MODE'
360360
);
361361
});
362362

@@ -375,9 +375,12 @@ describe('DotEmaShellComponent', () => {
375375
);
376376

377377
spectator.detectChanges();
378-
expect(spyloadPageAsset).toHaveBeenCalledWith({ ...params, url: 'some-url/' });
378+
expect(spyloadPageAsset).toHaveBeenCalledWith({
379+
...params,
380+
url: '/some-url/index'
381+
});
379382
expect(spyLocation).toHaveBeenCalledWith(
380-
'/?language_id=1&url=some-url%2F&variantName=DEFAULT&mode=EDIT_MODE'
383+
'/?language_id=1&url=%2Fsome-url%2Findex&variantName=DEFAULT&mode=EDIT_MODE'
381384
);
382385
});
383386

@@ -392,7 +395,7 @@ describe('DotEmaShellComponent', () => {
392395
};
393396

394397
const expectedParams = {
395-
url: 'some-url/',
398+
url: '/some-url/index',
396399
[PERSONA_KEY]: 'someCoolDude',
397400
mode: UVE_MODE.EDIT,
398401
language_id: 1
@@ -406,7 +409,7 @@ describe('DotEmaShellComponent', () => {
406409
spectator.detectChanges();
407410
expect(spyloadPageAsset).toHaveBeenCalledWith(expectedParams);
408411
expect(spyLocation).toHaveBeenCalledWith(
409-
'/?url=some-url%2F&language_id=1&mode=EDIT_MODE&personaId=someCoolDude'
412+
'/?url=%2Fsome-url%2Findex&language_id=1&mode=EDIT_MODE&personaId=someCoolDude'
410413
);
411414
});
412415
});
@@ -657,7 +660,7 @@ describe('DotEmaShellComponent', () => {
657660
const baseClientHost = 'http://localhost:3000/';
658661
const params = {
659662
...INITIAL_PAGE_PARAMS,
660-
clientHost: 'http://localhost:3000' // No trailing slash
663+
clientHost: 'http://localhost:3000/' // No trailing slash
661664
};
662665

663666
// Set up route with uveConfig.url that has trailing slash

‎core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/components/dot-uve-toolbar/components/dot-uve-workflow-actions/dot-uve-workflow-actions.component.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ describe('DotUveWorkflowActionsComponent', () => {
204204
expect(spySetWorkflowActionLoading).toHaveBeenCalledWith(true);
205205
expect(spyLoadPageAsset).toHaveBeenCalledWith({
206206
language_id: dotcmsContentletMock.languageId.toString(),
207-
url: dotcmsContentletMock.url
207+
url: '/'
208208
});
209209
expect(spyMessage).toHaveBeenCalledTimes(2);
210210

‎core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,7 @@ describe('EditEmaEditorComponent', () => {
370370

371371
beforeEach(() => {
372372
spectator = createComponent({
373-
queryParams: { language_id: 1, url: 'page-one' },
373+
queryParams: { language_id: 1, url: 'index' },
374374
data: {
375375
data: {
376376
url: 'http://localhost:3000'
@@ -2589,7 +2589,7 @@ describe('EditEmaEditorComponent', () => {
25892589
const iframe = spectator.debugElement.query(By.css('[data-testId="iframe"]'));
25902590

25912591
expect(iframe.nativeElement.src).toBe(
2592-
'http://localhost:3000/page-one?language_id=1'
2592+
'http://localhost:3000/index?language_id=1'
25932593
);
25942594
});
25952595

‎core-web/libs/portlets/edit-ema/portlet/src/lib/services/guards/edit-ema.guard.spec.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -95,12 +95,12 @@ describe('EditEmaGuard', () => {
9595
expect(didEnteredPortlet).toBe(true);
9696
});
9797

98-
it('should navigate to "edit-page" with url as "index" when the initial url queryParam is "/"', () => {
98+
it('should navigate to "edit-page" with url as "/" when the initial url queryParam is ""', () => {
9999
const route: ActivatedRouteSnapshot = {
100100
firstChild: {
101101
url: [{ path: 'content' }]
102102
},
103-
queryParams: { url: '/' }
103+
queryParams: { url: '' }
104104
// eslint-disable-next-line @typescript-eslint/no-explicit-any
105105
} as any;
106106

@@ -110,7 +110,7 @@ describe('EditEmaGuard', () => {
110110
queryParams: {
111111
[PERSONA_KEY]: 'modes.persona.no.persona',
112112
language_id: 1,
113-
url: 'index'
113+
url: '/'
114114
},
115115
replaceUrl: true
116116
});

‎core-web/libs/portlets/edit-ema/portlet/src/lib/services/guards/edit-ema.guard.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,15 +36,15 @@ function confirmQueryParams(queryParams: Params): {
3636
} {
3737
const { missing, ...missingQueryParams } = DEFAULT_QUERY_PARAMS.reduce(
3838
(acc, { key, value }) => {
39-
if (!queryParams[key]) {
40-
acc[key] = value;
39+
if (key === 'url' && queryParams[key]?.trim()?.length === 0) {
40+
acc[key] = '/';
4141
acc.missing = true;
4242

4343
return acc;
4444
}
4545

46-
if (key === 'url' && queryParams[key] === '/') {
47-
acc[key] = 'index';
46+
if (!queryParams[key]) {
47+
acc[key] = value;
4848
acc.missing = true;
4949

5050
return acc;
@@ -77,7 +77,7 @@ const DEFAULT_QUERY_PARAMS = [
7777
},
7878
{
7979
key: 'url',
80-
value: 'index'
80+
value: '/'
8181
},
8282
{
8383
key: 'com.dotmarketing.persona.id',

‎core-web/libs/portlets/edit-ema/portlet/src/lib/store/features/editor/withEditor.spec.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -244,11 +244,17 @@ describe('withEditor', () => {
244244
...MOCK_RESPONSE_HEADLESS.vanityUrl,
245245
url: 'first'
246246
}
247+
},
248+
pageParams: {
249+
language_id: '1',
250+
variantName: 'DEFAULT',
251+
url: 'first',
252+
[PERSONA_KEY]: 'dot:persona'
247253
}
248254
});
249255

250256
expect(store.$iframeURL()).toBe(
251-
'http://localhost:3000/first?language_id=1&variantName=DEFAULT&personaId=dot%3Apersona'
257+
'http://localhost/first?language_id=1&variantName=DEFAULT&personaId=dot%3Apersona'
252258
);
253259
});
254260
});

‎core-web/libs/portlets/edit-ema/portlet/src/lib/store/features/editor/withEditor.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -209,9 +209,7 @@ export function withEditor() {
209209
};
210210
}),
211211
$iframeURL: computed<string | InstanceType<typeof String>>(() => {
212-
const page = store.pageAPIResponse().page;
213-
const vanityURL = store.pageAPIResponse().vanityUrl?.url;
214-
const sanitizedURL = sanitizeURL(vanityURL ?? page?.pageURI);
212+
const sanitizedURL = sanitizeURL(store.pageParams().url);
215213

216214
const url = buildIframeURL({
217215
url: sanitizedURL,

‎core-web/libs/portlets/edit-ema/portlet/src/lib/store/features/load/withLoad.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,9 @@ export function withLoad() {
8282
}
8383

8484
// Maybe we can use retryWhen() instead of this navigate.
85-
const url = vanityUrl.forwardTo.replace('/', '');
8685
router.navigate([], {
8786
queryParamsHandling: 'merge',
88-
queryParams: { url }
87+
queryParams: { url: vanityUrl.forwardTo }
8988
});
9089

9190
// EMPTY is a simple Observable that only emits the complete notification.

‎core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -196,15 +196,19 @@ function insertPositionedContentletInContainer(payload: ActionPayload): {
196196
}
197197

198198
/**
199-
* Remove the index from the end of the url if it's nested and also remove extra slashes
199+
* Sanitizes a URL by:
200+
* 1. Removing extra leading/trailing slashes
201+
* 2. Preserving 'index' in the URL path
200202
*
201203
* @param {string} url
202204
* @return {*} {string}
203205
*/
204206
export function sanitizeURL(url?: string): string {
205-
return url
206-
?.replace(/^\/+|\/+$/g, '') // Remove starting and trailing slashes
207-
?.replace(/^(index)$|(.*?)(?:\/)?index\/?$/, (_, g1, g2) => g1 || `${g2}/`); // Keep 'index' or add slash for paths
207+
if (!url || url === '/') {
208+
return '/';
209+
}
210+
211+
return url.replace(/\/+/g, '/'); // Convert multiple slashes to single slash
208212
}
209213

210214
/**
@@ -292,7 +296,10 @@ export function normalizeQueryParams(params, baseClientHost?: string) {
292296
delete queryParams[PERSONA_KEY];
293297
}
294298

295-
if (baseClientHost && sanitizeURL(baseClientHost) === sanitizeURL(params.clientHost)) {
299+
if (
300+
baseClientHost &&
301+
new URL(baseClientHost).toString() === new URL(params.clientHost).toString()
302+
) {
296303
delete queryParams.clientHost;
297304
}
298305

@@ -633,8 +640,8 @@ export const checkClientHostAccess = (
633640
}
634641

635642
// Most IDEs and terminals add a / at the end of the URL, so we need to sanitize it
636-
const sanitizedClientHost = sanitizeURL(clientHost);
637-
const sanitizedAllowedDevURLs = allowedDevURLs.map(sanitizeURL);
643+
const sanitizedClientHost = new URL(clientHost).toString();
644+
const sanitizedAllowedDevURLs = allowedDevURLs.map((url) => new URL(url).toString());
638645

639646
return sanitizedAllowedDevURLs.includes(sanitizedClientHost);
640647
};

‎core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts‎

Lines changed: 11 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -309,50 +309,30 @@ describe('utils functions', () => {
309309
});
310310

311311
describe('url sanitize', () => {
312-
it('should remove the slash from the start', () => {
313-
expect(sanitizeURL('/cool')).toEqual('cool');
312+
it('should left the / as /', () => {
313+
expect(sanitizeURL('/')).toEqual('/');
314314
});
315315

316-
it("should remove the slash from the end if it's not the only character", () => {
317-
expect(sanitizeURL('super-cool/')).toEqual('super-cool');
316+
it('should clean multiple slashes', () => {
317+
expect(sanitizeURL('//////')).toEqual('/');
318318
});
319319

320-
it('should remove the slash from the end and the beggining', () => {
321-
expect(sanitizeURL('/hello-there/')).toEqual('hello-there');
322-
});
323-
324-
it('should leave as it is for valid url', () => {
325-
expect(sanitizeURL('this-is-where-the-fun-begins')).toEqual(
326-
'this-is-where-the-fun-begins'
327-
);
328-
});
329-
330-
it('should return index if the url is index without nested path', () => {
331-
expect(sanitizeURL('index')).toEqual('index');
332-
expect(sanitizeURL('index/')).toEqual('index');
333-
expect(sanitizeURL('/index/')).toEqual('index');
320+
it('should clean multiple slashes', () => {
321+
expect(sanitizeURL('//index////')).toEqual('/index/');
334322
});
335323

336324
describe('nested url', () => {
337325
it('should leave as it is for a nested valid url', () => {
338-
expect(sanitizeURL('hello-there/general-kenobi')).toEqual(
339-
'hello-there/general-kenobi'
326+
expect(sanitizeURL('hello-there/general-kenobi/')).toEqual(
327+
'hello-there/general-kenobi/'
340328
);
341329
});
342330

343-
it('should remove index from the end of the url but keep the slash if is a nested path', () => {
344-
expect(sanitizeURL('my-nested-path/index/')).toEqual('my-nested-path/');
345-
});
346-
347-
it('should remove the index if a nested path', () => {
348-
expect(sanitizeURL('i-have-the-high-ground/index')).toEqual(
349-
'i-have-the-high-ground/'
331+
it('should clean multiple slashes in a nested url', () => {
332+
expect(sanitizeURL('hello-there////general-kenobi//////')).toEqual(
333+
'hello-there/general-kenobi/'
350334
);
351335
});
352-
353-
it('should remove the index if a nested path with slash', () => {
354-
expect(sanitizeURL('no-index-please/index/')).toEqual('no-index-please/');
355-
});
356336
});
357337
});
358338

0 commit comments

Comments
 (0)