From 476798523ccc494e816f8cf59708a0556b906363 Mon Sep 17 00:00:00 2001 From: Daniel Silva Date: Thu, 6 Aug 2026 21:03:25 +0200 Subject: [PATCH 1/7] fix(uve): open file-asset links in a new tab instead of loading them as pages (#35504) handleInternalNav treated every same-host link as an HTMLPage and fed it to the Page API, so a link to a PDF resolved to a 404 "Page not found" in both edit and preview mode. Add an isAssetPath() predicate that mirrors the backend extension heuristic, and route hrefs resolving to a file asset to a new tab instead. Refs: #35504, FD #36746 Co-Authored-By: Claude Opus 5 (1M context) --- .../edit-ema-editor.component.spec.ts | 35 ++++++++++++ .../edit-ema-editor.component.ts | 11 ++++ .../edit-ema/portlet/src/lib/utils/index.ts | 54 +++++++++++++++++++ .../portlet/src/lib/utils/utils.spec.ts | 40 +++++++++++++- 4 files changed, 139 insertions(+), 1 deletion(-) diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts index 259ee5c1209a..23dfa84c6cc5 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts @@ -3115,6 +3115,41 @@ describe('EditEmaEditorComponent', () => { expect(mockEvent.preventDefault).toHaveBeenCalled(); }); + it('should open a same-host PDF link in a new tab instead of loading it as a page', () => { + const pdfUrl = 'http://localhost:3000/application/files/report.pdf'; + const mockEvent = createMockEvent(pdfUrl); + + spectator.component.handleInternalNav(mockEvent); + + expect(windowOpenSpy).toHaveBeenCalledWith(pdfUrl, '_blank'); + expect(pageLoadSpy).not.toHaveBeenCalled(); + expect(mockEvent.preventDefault).toHaveBeenCalled(); + }); + + it('should open a /dA/ asset link in a new tab instead of loading it as a page', () => { + const assetUrl = 'http://localhost:3000/dA/abc123/asset/report.pdf'; + const mockEvent = createMockEvent(assetUrl); + + spectator.component.handleInternalNav(mockEvent); + + expect(windowOpenSpy).toHaveBeenCalledWith(assetUrl, '_blank'); + expect(pageLoadSpy).not.toHaveBeenCalled(); + expect(mockEvent.preventDefault).toHaveBeenCalled(); + }); + + it('should still load a page when the URL uses the page extension', () => { + const pageUrl = 'http://localhost:3000/test-page/index.html'; + const mockEvent = createMockEvent(pageUrl); + + spectator.component.handleInternalNav(mockEvent); + + expect(windowOpenSpy).not.toHaveBeenCalled(); + expect(pageLoadSpy).toHaveBeenCalledWith({ + url: '/test-page/index.html' + }); + expect(mockEvent.preventDefault).toHaveBeenCalled(); + }); + it('should extract and pass query parameters from URL', () => { const urlWithParams = 'http://localhost:3000/test-page?param1=value1¶m2=value2'; diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts index 22f7e6c36920..6bc1b588e9df 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts @@ -118,6 +118,7 @@ import { deleteContentletFromContainer, getTargetUrl, insertContentletInContainer, + isAssetPath, isSamePageNavigation, measureCanvasAvailableSize, shouldNavigate @@ -768,6 +769,16 @@ export class EditEmaEditorComponent implements OnDestroy, AfterViewInit { return; } + // Files (PDFs, images, docs…) are not pages: the Page API cannot resolve + // them and the editor would show "Page not found". Open them in a new tab + // so the author can verify the link without leaving the editor. + if (isAssetPath(url.pathname)) { + this.window.open(href, '_blank'); + e.preventDefault(); + + return; + } + // Same pathname (any hash/query): let the browser handle it (anchors, query-driven UI) if (isSamePageNavigation(href, this.uveStore.pageParams()?.url)) { return; diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts index e94e86fc9429..f42cb0e17d55 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts @@ -1168,3 +1168,57 @@ export const isSamePageNavigation = (incomingUrl: string, currentUrl: string): b return target.pathname === current.pathname; }; + +/** dotCMS path prefixes that stream a binary asset instead of rendering a page. */ +const ASSET_PATH_PREFIXES = ['/dA/', '/dotAsset/', '/contentAsset/']; + +/** + * Extensions that still resolve to an HTMLPage. `html` is the default + * `VELOCITY_PAGE_EXTENSION`; `dot` is the legacy fallback the backend uses + * (see `Identifier#setURI`). + */ +const PAGE_PATH_EXTENSIONS = new Set(['html', 'htm', 'dot']); + +/** + * Matches a plausible file extension: letter-initial, up to 8 alphanumerics. + * Guards URL-map slugs such as `/blog/release-v1.2`, whose trailing `2` must + * not be mistaken for a file extension. + */ +const FILE_EXTENSION_PATTERN = /^[a-z][a-z0-9]{0,7}$/; + +/** + * Checks whether a pathname targets a file asset rather than an HTMLPage. + * + * Mirrors the backend's own extension heuristic: no extension (or the page + * extension) means a page; any other real extension means a file. + * + * @param {string} pathname - The pathname to check (query and hash excluded) + * @returns {boolean} True when the pathname points at a file asset + * + * @example + * isAssetPath('/application/files/doc.pdf') // true + * isAssetPath('/dA/abc123/asset/doc.pdf') // true + * isAssetPath('/about-us/index') // false + * isAssetPath('/about-us/index.html') // false + * isAssetPath('/blog/release-v1.2') // false + */ +export const isAssetPath = (pathname: string): boolean => { + if (!pathname) { + return false; + } + + if (ASSET_PATH_PREFIXES.some((prefix) => pathname.startsWith(prefix))) { + return true; + } + + const lastSegment = pathname.slice(pathname.lastIndexOf('/') + 1); + const dotIndex = lastSegment.lastIndexOf('.'); + + if (dotIndex === -1) { + return false; + } + + const extension = lastSegment.slice(dotIndex + 1).toLowerCase(); + + return FILE_EXTENSION_PATTERN.test(extension) && !PAGE_PATH_EXTENSIONS.has(extension); +}; diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts index 8f8718e357c6..064fddd20a1a 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts @@ -31,7 +31,8 @@ import { normalizeQueryParams, convertUTCToLocalTime, escapeHtmlAttributeValue, - isSamePageNavigation + isSamePageNavigation, + isAssetPath } from '.'; import { DEFAULT_PERSONA, PERSONA_KEY } from '../shared/consts'; @@ -1648,4 +1649,41 @@ describe('utils functions', () => { expect(result.getHours()).toBe(12); }); }); + + describe('isAssetPath', () => { + it.each([ + ['/dA/abc123/asset/report.pdf', true], + ['/dA/abc123/asset/no-extension', true], + ['/dotAsset/abc123', true], + ['/contentAsset/raw-data/abc123/asset', true], + ['/application/files/report.pdf', true], + ['/files/quarterly.docx', true], + ['/media/promo.mp4', true], + ['/backups/site.tar.gz', true], + ['/files/REPORT.PDF', true] + ])('should treat %s as a file asset', (pathname, expected) => { + expect(isAssetPath(pathname as string)).toBe(expected); + }); + + it.each([ + ['/about-us/index', false], + ['/about-us/index.html', false], + ['/about-us/index.htm', false], + ['/legacy/index.dot', false], + ['/blog/', false], + ['/', false], + ['/blog/release-v1.2', false], + ['/news/2024.10', false] + ])('should treat %s as a page', (pathname, expected) => { + expect(isAssetPath(pathname as string)).toBe(expected); + }); + + it('should return false for an empty pathname', () => { + expect(isAssetPath('')).toBe(false); + }); + + it('should return false for a nullish pathname', () => { + expect(isAssetPath(undefined as unknown as string)).toBe(false); + }); + }); }); From 6cfcf1eeb57a498c84c3577e27f951e3addeea09 Mon Sep 17 00:00:00 2001 From: Jalinson Diaz Date: Mon, 10 Aug 2026 11:23:38 -0300 Subject: [PATCH 2/7] fix(uve): correct the file-asset path heuristic and resolve the opened URL (#35504) Applies review feedback on the isAssetPath heuristic: - Drop `htm` from PAGE_PATH_EXTENSIONS. VELOCITY_PAGE_EXTENSION ships as `html` with `dot` as the backend fallback; `htm` is an ordinary file asset in dotCMS, so a .htm upload was still routed to the Page API. - Accept digit-initial extensions (7z, 3gp). The URL-map slug guard only needs to reject all-digit trailing tokens, not digit-initial ones. - Open url.href rather than the raw href, which can be a relative attribute when the click lands on a child of the anchor, and add noopener since the host check compares hostname only. - Record that the backend resolves page vs file by identifier lookup, and that dotted page slugs are a known false positive. Co-Authored-By: Claude Opus 5 (1M context) --- .../edit-ema-editor.component.spec.ts | 29 +++++++++++- .../edit-ema-editor.component.ts | 6 ++- .../edit-ema/portlet/src/lib/utils/index.ts | 34 ++++++++++---- .../portlet/src/lib/utils/utils.spec.ts | 47 ++++++++++--------- 4 files changed, 82 insertions(+), 34 deletions(-) diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts index 23dfa84c6cc5..16d1118def7d 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts @@ -3121,7 +3121,7 @@ describe('EditEmaEditorComponent', () => { spectator.component.handleInternalNav(mockEvent); - expect(windowOpenSpy).toHaveBeenCalledWith(pdfUrl, '_blank'); + expect(windowOpenSpy).toHaveBeenCalledWith(pdfUrl, '_blank', 'noopener'); expect(pageLoadSpy).not.toHaveBeenCalled(); expect(mockEvent.preventDefault).toHaveBeenCalled(); }); @@ -3132,7 +3132,32 @@ describe('EditEmaEditorComponent', () => { spectator.component.handleInternalNav(mockEvent); - expect(windowOpenSpy).toHaveBeenCalledWith(assetUrl, '_blank'); + expect(windowOpenSpy).toHaveBeenCalledWith(assetUrl, '_blank', 'noopener'); + expect(pageLoadSpy).not.toHaveBeenCalled(); + expect(mockEvent.preventDefault).toHaveBeenCalled(); + }); + + it('should resolve a relative asset href against the site origin, not the admin path', () => { + // The click lands on a child of the anchor, so `target.href` is undefined + // and the raw (relative) href attribute is what reaches the handler. + const mockEvent = { + target: { + closest: jest.fn().mockReturnValue({ + getAttribute: () => 'files/report.pdf' + }) + }, + preventDefault: jest.fn() + } as unknown as MouseEvent; + + jest.spyOn(store, 'editorState').mockReturnValue(EDITOR_STATE.IDLE); + + spectator.component.handleInternalNav(mockEvent); + + expect(windowOpenSpy).toHaveBeenCalledWith( + 'http://localhost:3000/files/report.pdf', + '_blank', + 'noopener' + ); expect(pageLoadSpy).not.toHaveBeenCalled(); expect(mockEvent.preventDefault).toHaveBeenCalled(); }); diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts index 6bc1b588e9df..2172fcc93138 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts @@ -773,7 +773,11 @@ export class EditEmaEditorComponent implements OnDestroy, AfterViewInit { // them and the editor would show "Page not found". Open them in a new tab // so the author can verify the link without leaving the editor. if (isAssetPath(url.pathname)) { - this.window.open(href, '_blank'); + // `url` is the origin-resolved form of `href`, which can still be a raw + // relative attribute when the click lands on a child of the anchor. + // `noopener` because the host check above compares hostname only, so + // this branch can still be cross-origin on another scheme or port. + this.window.open(url.href, '_blank', 'noopener'); e.preventDefault(); return; diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts index f42cb0e17d55..8a38997e6b8e 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts @@ -1173,24 +1173,37 @@ export const isSamePageNavigation = (incomingUrl: string, currentUrl: string): b const ASSET_PATH_PREFIXES = ['/dA/', '/dotAsset/', '/contentAsset/']; /** - * Extensions that still resolve to an HTMLPage. `html` is the default - * `VELOCITY_PAGE_EXTENSION`; `dot` is the legacy fallback the backend uses - * (see `Identifier#setURI`). + * Extensions that still resolve to an HTMLPage. `html` is the shipped + * `VELOCITY_PAGE_EXTENSION` and `dot` is the fallback the backend defaults to + * when the property is unset (see `Identifier#setURI`). Deliberately excludes + * `htm`, which dotCMS treats as an ordinary file asset, not a page. + * + * `VELOCITY_PAGE_EXTENSION` is configurable, so a site that overrides it to + * something else will see its page links open in a new tab. The value is not + * exposed to the client, so it cannot be read here. */ -const PAGE_PATH_EXTENSIONS = new Set(['html', 'htm', 'dot']); +const PAGE_PATH_EXTENSIONS = new Set(['html', 'dot']); /** - * Matches a plausible file extension: letter-initial, up to 8 alphanumerics. - * Guards URL-map slugs such as `/blog/release-v1.2`, whose trailing `2` must - * not be mistaken for a file extension. + * Matches a plausible file extension: 1-8 alphanumerics containing at least one + * letter. The letter requirement is what guards URL-map slugs such as + * `/blog/release-v1.2` and `/news/2024.10`, whose all-digit trailing token must + * not be mistaken for a file extension. Digit-initial extensions such as `7z` + * and `3gp` are real and must still match. */ -const FILE_EXTENSION_PATTERN = /^[a-z][a-z0-9]{0,7}$/; +const FILE_EXTENSION_PATTERN = /^(?=.*[a-z])[a-z0-9]{1,8}$/; /** * Checks whether a pathname targets a file asset rather than an HTMLPage. * - * Mirrors the backend's own extension heuristic: no extension (or the page - * extension) means a page; any other real extension means a file. + * No extension (or the page extension) means a page; any other real extension + * means a file. This is a client-side approximation: the backend resolves the + * two by identifier lookup (`CMSUrlUtil#resolveResourceType`), not by + * extension, so an authoritative answer would cost a round-trip per link click. + * + * Known limitation: a page whose last segment carries a dot followed by a short + * alpha token is read as a file, so `/store/product.detail` opens in a new tab + * instead of navigating. Reachable through author-controlled URL-map slugs. * * @param {string} pathname - The pathname to check (query and hash excluded) * @returns {boolean} True when the pathname points at a file asset @@ -1198,6 +1211,7 @@ const FILE_EXTENSION_PATTERN = /^[a-z][a-z0-9]{0,7}$/; * @example * isAssetPath('/application/files/doc.pdf') // true * isAssetPath('/dA/abc123/asset/doc.pdf') // true + * isAssetPath('/backups/archive.7z') // true * isAssetPath('/about-us/index') // false * isAssetPath('/about-us/index.html') // false * isAssetPath('/blog/release-v1.2') // false diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts index 064fddd20a1a..38a14c535a17 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts @@ -1652,30 +1652,35 @@ describe('utils functions', () => { describe('isAssetPath', () => { it.each([ - ['/dA/abc123/asset/report.pdf', true], - ['/dA/abc123/asset/no-extension', true], - ['/dotAsset/abc123', true], - ['/contentAsset/raw-data/abc123/asset', true], - ['/application/files/report.pdf', true], - ['/files/quarterly.docx', true], - ['/media/promo.mp4', true], - ['/backups/site.tar.gz', true], - ['/files/REPORT.PDF', true] - ])('should treat %s as a file asset', (pathname, expected) => { - expect(isAssetPath(pathname as string)).toBe(expected); + '/dA/abc123/asset/report.pdf', + '/dA/abc123/asset/no-extension', + '/dotAsset/abc123', + '/contentAsset/raw-data/abc123/asset', + '/application/files/report.pdf', + '/files/quarterly.docx', + '/media/promo.mp4', + '/backups/site.tar.gz', + '/files/REPORT.PDF', + // `htm` is not a dotCMS page extension: VELOCITY_PAGE_EXTENSION is `html` + // and `dot` is its legacy fallback, so a `.htm` upload is a file asset. + '/uploads/legacy-page.htm', + // Digit-initial extensions are real; only all-digit trailing tokens are slugs. + '/backups/archive.7z', + '/media/clip.3gp' + ])('should treat %s as a file asset', (pathname) => { + expect(isAssetPath(pathname)).toBe(true); }); it.each([ - ['/about-us/index', false], - ['/about-us/index.html', false], - ['/about-us/index.htm', false], - ['/legacy/index.dot', false], - ['/blog/', false], - ['/', false], - ['/blog/release-v1.2', false], - ['/news/2024.10', false] - ])('should treat %s as a page', (pathname, expected) => { - expect(isAssetPath(pathname as string)).toBe(expected); + '/about-us/index', + '/about-us/index.html', + '/legacy/index.dot', + '/blog/', + '/', + '/blog/release-v1.2', + '/news/2024.10' + ])('should treat %s as a page', (pathname) => { + expect(isAssetPath(pathname)).toBe(false); }); it('should return false for an empty pathname', () => { From 96b01be680943f3eafd32ba124c5a02ca78dd27f Mon Sep 17 00:00:00 2001 From: Jalinson Diaz Date: Mon, 10 Aug 2026 11:37:02 -0300 Subject: [PATCH 3/7] docs(uve): record why isAssetPath biases toward opening a new tab (#35504) The dotted-page-slug false positive is a deliberate choice, not an oversight. Reading a page as a file opens a new tab, which is visible and recoverable; reading a file as a page strands the editor on "Page not found", the defect this guards against. An extension allowlist would fix the false positive but invert that bias, so the test stays permissive. Co-Authored-By: Claude Opus 5 (1M context) --- .../portlets/edit-ema/portlet/src/lib/utils/index.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts index 8a38997e6b8e..785003ce22e6 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts @@ -1203,7 +1203,15 @@ const FILE_EXTENSION_PATTERN = /^(?=.*[a-z])[a-z0-9]{1,8}$/; * * Known limitation: a page whose last segment carries a dot followed by a short * alpha token is read as a file, so `/store/product.detail` opens in a new tab - * instead of navigating. Reachable through author-controlled URL-map slugs. + * instead of navigating. Reachable through author-controlled slugs, since the + * page `url` is a plain text field (`HTMLPageAssetAPIImpl`). + * + * That direction is deliberate. Reading a page as a file opens it in a new tab, + * which is visible and recoverable; reading a file as a page hands it to the + * Page API and strands the editor on "Page not found", the defect this guards + * against. So the extension test stays permissive rather than matching against + * a known-extension allowlist, which would invert the bias and make every + * uncommon file type fail the worse way. * * @param {string} pathname - The pathname to check (query and hash excluded) * @returns {boolean} True when the pathname points at a file asset From 76fc64ca6d324a66223fa2707abef35d16bfe776 Mon Sep 17 00:00:00 2001 From: Jalinson Diaz Date: Mon, 10 Aug 2026 11:38:28 -0300 Subject: [PATCH 4/7] test(uve): pin the accepted dotted-page-slug misclassification (#35504) Locks in the documented trade so it stays a conscious choice: a page slug carrying a dot plus a short alpha token reads as a file asset. Flipping any of these to false means the bias was changed, which would send uncommon file extensions to the Page API instead, the failure the guard prevents. Co-Authored-By: Claude Opus 5 (1M context) --- .../edit-ema/portlet/src/lib/utils/utils.spec.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts index 38a14c535a17..e92fc72baad8 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts @@ -1690,5 +1690,18 @@ describe('utils functions', () => { it('should return false for a nullish pathname', () => { expect(isAssetPath(undefined as unknown as string)).toBe(false); }); + + // These pathnames are pages, and the heuristic knowingly reads them as file + // assets: a dot plus a short alpha token is indistinguishable from a real + // extension without asking the backend. Pinned deliberately, because the + // alternative (an extension allowlist) would send uncommon file types to the + // Page API instead, which is the failure this whole guard exists to prevent. + // Flipping any of these to `false` means that trade was changed, not fixed. + it.each(['/store/product.detail', '/pages/about.us', '/docs/getting.started'])( + 'should knowingly misread the page %s as a file asset', + (pathname) => { + expect(isAssetPath(pathname)).toBe(true); + } + ); }); }); From a4c880e47f98f3273ab20b6d4fa0a47b773cb901 Mon Sep 17 00:00:00 2001 From: Jalinson Diaz Date: Mon, 10 Aug 2026 12:00:51 -0300 Subject: [PATCH 5/7] fix(uve): cancel the click before opening the asset tab (#35504) Firefox raises "DOMException: The operation is insecure" when window.open is given a windowFeatures string from a gesture that originated in the sandboxed iframe, which is declared without allow-popups. The noopener argument added earlier turned a permitted tab-open into a rejected popup request, and because the throw happened before preventDefault(), the anchor's default action ran and navigated the iframe to the asset. - Call preventDefault() first, so the page under edit stays put whatever window.open does. - Drop the windowFeatures string, matching the pre-existing external-host branch, which Firefox accepts. - Guard the open call: an escaping throw would reach the RxJS subscriber driving this handler and kill the click listener for the whole session. Co-Authored-By: Claude Opus 5 (1M context) --- .../edit-ema-editor.component.spec.ts | 25 ++++++++++++++++--- .../edit-ema-editor.component.ts | 21 ++++++++++++---- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts index 16d1118def7d..a152ae7c5ca5 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts @@ -3121,7 +3121,7 @@ describe('EditEmaEditorComponent', () => { spectator.component.handleInternalNav(mockEvent); - expect(windowOpenSpy).toHaveBeenCalledWith(pdfUrl, '_blank', 'noopener'); + expect(windowOpenSpy).toHaveBeenCalledWith(pdfUrl, '_blank'); expect(pageLoadSpy).not.toHaveBeenCalled(); expect(mockEvent.preventDefault).toHaveBeenCalled(); }); @@ -3132,7 +3132,7 @@ describe('EditEmaEditorComponent', () => { spectator.component.handleInternalNav(mockEvent); - expect(windowOpenSpy).toHaveBeenCalledWith(assetUrl, '_blank', 'noopener'); + expect(windowOpenSpy).toHaveBeenCalledWith(assetUrl, '_blank'); expect(pageLoadSpy).not.toHaveBeenCalled(); expect(mockEvent.preventDefault).toHaveBeenCalled(); }); @@ -3155,13 +3155,30 @@ describe('EditEmaEditorComponent', () => { expect(windowOpenSpy).toHaveBeenCalledWith( 'http://localhost:3000/files/report.pdf', - '_blank', - 'noopener' + '_blank' ); expect(pageLoadSpy).not.toHaveBeenCalled(); expect(mockEvent.preventDefault).toHaveBeenCalled(); }); + it('should keep the iframe on the page when opening a new tab throws', () => { + // Firefox raises "The operation is insecure" when the sandboxed + // iframe's gesture is used to open a popup. The click must still + // be cancelled, or the anchor navigates the iframe to the asset. + windowOpenSpy.mockImplementation(() => { + throw new DOMException('The operation is insecure.'); + }); + + const mockEvent = createMockEvent( + 'http://localhost:3000/dA/abc123/asset/photo.jpg' + ); + + expect(() => spectator.component.handleInternalNav(mockEvent)).not.toThrow(); + + expect(mockEvent.preventDefault).toHaveBeenCalled(); + expect(pageLoadSpy).not.toHaveBeenCalled(); + }); + it('should still load a page when the URL uses the page extension', () => { const pageUrl = 'http://localhost:3000/test-page/index.html'; const mockEvent = createMockEvent(pageUrl); diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts index 2172fcc93138..9a3f97f8d384 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts @@ -773,13 +773,24 @@ export class EditEmaEditorComponent implements OnDestroy, AfterViewInit { // them and the editor would show "Page not found". Open them in a new tab // so the author can verify the link without leaving the editor. if (isAssetPath(url.pathname)) { - // `url` is the origin-resolved form of `href`, which can still be a raw - // relative attribute when the click lands on a child of the anchor. - // `noopener` because the host check above compares hostname only, so - // this branch can still be cross-origin on another scheme or port. - this.window.open(url.href, '_blank', 'noopener'); + // Cancel before opening. The iframe must stay on the page being edited + // even if the open below fails, and it can: the iframe is sandboxed + // without `allow-popups`, so Firefox rejects a popup raised from its + // gesture with "The operation is insecure". e.preventDefault(); + try { + // `url` is the origin-resolved form of `href`, which can still be a + // raw relative attribute when the click lands on a child of the + // anchor. Two arguments only: a windowFeatures string turns this + // into a popup request, which is what the sandbox rejects. + this.window.open(url.href, '_blank'); + } catch { + // Swallow. A throw here would otherwise reach the RxJS subscriber + // that feeds this handler and kill the click listener for the rest + // of the session, so every later link click would fall through. + } + return; } From d04e7cd20b9a78e0718ff64f49fd9652d647ab5b Mon Sep 17 00:00:00 2001 From: Jalinson Diaz Date: Mon, 10 Aug 2026 12:06:08 -0300 Subject: [PATCH 6/7] fix(uve): keep noopener by opening assets through a rel=noopener anchor (#35504) window.open with a windowFeatures string makes Firefox classify the call as a popup request, and the iframe raising the gesture is sandboxed without allow-popups, so it threw "The operation is insecure". Dropping the string lost the opener guarantee, which is not acceptable. A rel="noopener" anchor is an ordinary tab navigation, so the sandbox permits it, and it severs the opener even for cross-origin targets, where assigning opener = null on a returned window would not be allowed. The host check above compares hostname only, so cross-origin is reachable. preventDefault() still runs first, so the iframe stays on the page under edit whatever the open does. Co-Authored-By: Claude Opus 5 (1M context) --- .../edit-ema-editor.component.spec.ts | 58 +++++++++++++++---- .../edit-ema-editor.component.ts | 54 ++++++++++++----- 2 files changed, 85 insertions(+), 27 deletions(-) diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts index a152ae7c5ca5..0ef645021ab5 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts @@ -3031,18 +3031,40 @@ describe('EditEmaEditorComponent', () => { describe('handleInternalNav', () => { let pageLoadSpy: jest.SpyInstance; let windowOpenSpy: jest.Mock; + let openedLink: { + href: string; + target: string; + rel: string; + click: jest.Mock; + remove: jest.Mock; + }; let mockWindow: { location: { origin: string; hostname: string }; open: jest.Mock; + document: { createElement: jest.Mock; body: { appendChild: jest.Mock } }; }; beforeEach(() => { + // Asset links are opened through a `rel="noopener"` anchor rather + // than window.open, so the sandboxed iframe's gesture is not + // treated as a popup request. See EditEmaEditorComponent. + openedLink = { + href: '', + target: '', + rel: '', + click: jest.fn(), + remove: jest.fn() + }; mockWindow = { location: { origin: 'http://localhost:3000', hostname: 'localhost' }, - open: jest.fn() + open: jest.fn(), + document: { + createElement: jest.fn().mockReturnValue(openedLink), + body: { appendChild: jest.fn() } + } }; (spectator.component as unknown as { window: typeof mockWindow }).window = mockWindow; @@ -3121,7 +3143,9 @@ describe('EditEmaEditorComponent', () => { spectator.component.handleInternalNav(mockEvent); - expect(windowOpenSpy).toHaveBeenCalledWith(pdfUrl, '_blank'); + expect(openedLink.href).toBe(pdfUrl); + expect(openedLink.target).toBe('_blank'); + expect(openedLink.click).toHaveBeenCalled(); expect(pageLoadSpy).not.toHaveBeenCalled(); expect(mockEvent.preventDefault).toHaveBeenCalled(); }); @@ -3132,11 +3156,25 @@ describe('EditEmaEditorComponent', () => { spectator.component.handleInternalNav(mockEvent); - expect(windowOpenSpy).toHaveBeenCalledWith(assetUrl, '_blank'); + expect(openedLink.href).toBe(assetUrl); + expect(openedLink.click).toHaveBeenCalled(); expect(pageLoadSpy).not.toHaveBeenCalled(); expect(mockEvent.preventDefault).toHaveBeenCalled(); }); + it('should sever the opener without a windowFeatures popup request', () => { + // A windowFeatures string makes Firefox treat the call as a popup, + // which the iframe sandbox rejects with "The operation is insecure". + const mockEvent = createMockEvent( + 'http://localhost:3000/dA/abc123/asset/photo.jpg' + ); + + spectator.component.handleInternalNav(mockEvent); + + expect(openedLink.rel).toBe('noopener'); + expect(windowOpenSpy).not.toHaveBeenCalled(); + }); + it('should resolve a relative asset href against the site origin, not the admin path', () => { // The click lands on a child of the anchor, so `target.href` is undefined // and the raw (relative) href attribute is what reaches the handler. @@ -3153,19 +3191,17 @@ describe('EditEmaEditorComponent', () => { spectator.component.handleInternalNav(mockEvent); - expect(windowOpenSpy).toHaveBeenCalledWith( - 'http://localhost:3000/files/report.pdf', - '_blank' - ); + expect(openedLink.href).toBe('http://localhost:3000/files/report.pdf'); + expect(openedLink.click).toHaveBeenCalled(); expect(pageLoadSpy).not.toHaveBeenCalled(); expect(mockEvent.preventDefault).toHaveBeenCalled(); }); it('should keep the iframe on the page when opening a new tab throws', () => { - // Firefox raises "The operation is insecure" when the sandboxed - // iframe's gesture is used to open a popup. The click must still - // be cancelled, or the anchor navigates the iframe to the asset. - windowOpenSpy.mockImplementation(() => { + // The click must still be cancelled if the open fails for any + // reason, or the anchor navigates the iframe to the asset, and the + // throw must not escape into the RxJS subscriber driving this. + openedLink.click.mockImplementation(() => { throw new DOMException('The operation is insecure.'); }); diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts index 9a3f97f8d384..962e0a6aace8 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts @@ -773,23 +773,12 @@ export class EditEmaEditorComponent implements OnDestroy, AfterViewInit { // them and the editor would show "Page not found". Open them in a new tab // so the author can verify the link without leaving the editor. if (isAssetPath(url.pathname)) { - // Cancel before opening. The iframe must stay on the page being edited - // even if the open below fails, and it can: the iframe is sandboxed - // without `allow-popups`, so Firefox rejects a popup raised from its - // gesture with "The operation is insecure". + // Cancel before opening, so the page under edit stays put whatever the + // open does. `url` is the origin-resolved form of `href`, which can + // still be a raw relative attribute when the click lands on a child of + // the anchor. e.preventDefault(); - - try { - // `url` is the origin-resolved form of `href`, which can still be a - // raw relative attribute when the click lands on a child of the - // anchor. Two arguments only: a windowFeatures string turns this - // into a popup request, which is what the sandbox rejects. - this.window.open(url.href, '_blank'); - } catch { - // Swallow. A throw here would otherwise reach the RxJS subscriber - // that feeds this handler and kill the click listener for the rest - // of the session, so every later link click would fall through. - } + this.#openInNewTab(url.href); return; } @@ -803,6 +792,39 @@ export class EditEmaEditorComponent implements OnDestroy, AfterViewInit { e.preventDefault(); } + /** + * Opens a URL in a new tab with the opener severed. + * + * Deliberately not `window.open(url, '_blank', 'noopener')`. A windowFeatures + * string makes Firefox classify the call as a popup request, and the iframe + * raising the gesture is sandboxed without `allow-popups`, so Firefox throws + * "DOMException: The operation is insecure". A `rel="noopener"` anchor is an + * ordinary tab navigation, which the sandbox permits, and carries the same + * opener guarantee, including for cross-origin targets where assigning + * `opener = null` on the returned window would not be allowed. + * + * @param {string} href - Absolute URL to open + * @memberof EditEmaEditorComponent + */ + #openInNewTab(href: string): void { + try { + const doc = this.window.document; + const link = doc.createElement('a'); + + link.href = href; + link.target = '_blank'; + link.rel = 'noopener'; + + doc.body.appendChild(link); + link.click(); + link.remove(); + } catch { + // Swallow. This runs inside the RxJS subscriber that feeds the iframe + // click handler, so an escaping throw would complete the subscription + // and kill link handling for the rest of the session. + } + } + /** * Handles the inline editing functionality triggered by a mouse event. * @param e - The mouse event that triggered the inline editing. From 0bb949348d511ed059c2757e29a61038c6d85897 Mon Sep 17 00:00:00 2001 From: Jalinson Diaz Date: Mon, 10 Aug 2026 13:16:08 -0300 Subject: [PATCH 7/7] fix(uve): treat .dot as a file asset and always clean up the temp anchor (#35504) Applies Copilot review feedback. - Drop `dot` from PAGE_PATH_EXTENSIONS. It is only the fallback that Config.getStringProperty("VELOCITY_PAGE_EXTENSION", "dot") reaches for when the property is absent, and dotmarketing-config.properties:91 ships it as `html`, so `dot` is never the active page extension. It is also the Word 97-2003 template extension, so listing it sent a real upload type to the Page API. This is the same bias already applied to `htm`. - Move the .dot case from the page table to the asset table. - Restructure #openInNewTab so the anchor is removed in a finally block. click() is the call that throws when the open is refused, so the old ordering stranded an anchor in the admin document on every failure. Co-Authored-By: Claude Opus 5 (1M context) --- .../edit-ema-editor.component.spec.ts | 3 +++ .../edit-ema-editor.component.ts | 10 +++++++-- .../edit-ema/portlet/src/lib/utils/index.ts | 22 ++++++++++++------- .../portlet/src/lib/utils/utils.spec.ts | 5 ++++- 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts index 9795afc5ccdc..75d0ab7517ca 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.spec.ts @@ -3313,6 +3313,9 @@ describe('EditEmaEditorComponent', () => { expect(mockEvent.preventDefault).toHaveBeenCalled(); expect(pageLoadSpy).not.toHaveBeenCalled(); + // click() is what throws, so cleanup has to be unconditional or + // every refused open strands an anchor in the admin document. + expect(openedLink.remove).toHaveBeenCalled(); }); it('should still load a page when the URL uses the page extension', () => { diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts index 4c09082bad66..302e1e49c86f 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/edit-ema-editor/edit-ema-editor.component.ts @@ -830,21 +830,27 @@ export class EditEmaEditorComponent implements OnDestroy, AfterViewInit { * @memberof EditEmaEditorComponent */ #openInNewTab(href: string): void { + let link: HTMLAnchorElement | null = null; + try { const doc = this.window.document; - const link = doc.createElement('a'); + link = doc.createElement('a'); link.href = href; link.target = '_blank'; link.rel = 'noopener'; doc.body.appendChild(link); link.click(); - link.remove(); } catch { // Swallow. This runs inside the RxJS subscriber that feeds the iframe // click handler, so an escaping throw would complete the subscription // and kill link handling for the rest of the session. + } finally { + // `click()` is the call that throws when the open is refused, so + // cleanup has to be unconditional or every failed attempt strands an + // anchor in the admin document. + link?.remove(); } } diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts index 785003ce22e6..264ede1128e5 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/index.ts @@ -1173,16 +1173,22 @@ export const isSamePageNavigation = (incomingUrl: string, currentUrl: string): b const ASSET_PATH_PREFIXES = ['/dA/', '/dotAsset/', '/contentAsset/']; /** - * Extensions that still resolve to an HTMLPage. `html` is the shipped - * `VELOCITY_PAGE_EXTENSION` and `dot` is the fallback the backend defaults to - * when the property is unset (see `Identifier#setURI`). Deliberately excludes - * `htm`, which dotCMS treats as an ordinary file asset, not a page. + * Extensions that still resolve to an HTMLPage. Only `html`, the shipped + * `VELOCITY_PAGE_EXTENSION` (`dotmarketing-config.properties:91`). * - * `VELOCITY_PAGE_EXTENSION` is configurable, so a site that overrides it to - * something else will see its page links open in a new tab. The value is not - * exposed to the client, so it cannot be read here. + * Deliberately excludes two extensions that look like candidates. `htm` is an + * ordinary file asset in dotCMS, never a page. `dot` is only the fallback + * `Config.getStringProperty("VELOCITY_PAGE_EXTENSION", "dot")` reaches for when + * the property is absent, which it never is in a standard install, and it is a + * real upload type (the Word 97-2003 template), so listing it would send those + * files to the Page API. + * + * `VELOCITY_PAGE_EXTENSION` is configurable and its value is not exposed to the + * client, so a site that overrides it sees its page links open in a new tab. + * That is the mild failure of the two, consistent with the bias documented on + * `isAssetPath`. */ -const PAGE_PATH_EXTENSIONS = new Set(['html', 'dot']); +const PAGE_PATH_EXTENSIONS = new Set(['html']); /** * Matches a plausible file extension: 1-8 alphanumerics containing at least one diff --git a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts index e92fc72baad8..db9a78fd5ec6 100644 --- a/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts +++ b/core-web/libs/portlets/edit-ema/portlet/src/lib/utils/utils.spec.ts @@ -1664,6 +1664,10 @@ describe('utils functions', () => { // `htm` is not a dotCMS page extension: VELOCITY_PAGE_EXTENSION is `html` // and `dot` is its legacy fallback, so a `.htm` upload is a file asset. '/uploads/legacy-page.htm', + // `dot` is only the fallback VELOCITY_PAGE_EXTENSION for when the + // property is unset, which it never is; it is also the Word template + // extension, so a `.dot` upload is a file asset. + '/templates/letterhead.dot', // Digit-initial extensions are real; only all-digit trailing tokens are slugs. '/backups/archive.7z', '/media/clip.3gp' @@ -1674,7 +1678,6 @@ describe('utils functions', () => { it.each([ '/about-us/index', '/about-us/index.html', - '/legacy/index.dot', '/blog/', '/', '/blog/release-v1.2',