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 f3866b385931..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 @@ -3131,18 +3131,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; @@ -3215,6 +3237,100 @@ 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(openedLink.href).toBe(pdfUrl); + expect(openedLink.target).toBe('_blank'); + expect(openedLink.click).toHaveBeenCalled(); + 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(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. + 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(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', () => { + // 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.'); + }); + + 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(); + // 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', () => { + 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 2a50d446fe68..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 @@ -123,6 +123,7 @@ import { deleteContentletFromContainer, getTargetUrl, insertContentletInContainer, + isAssetPath, isSamePageNavigation, measureCanvasAvailableSize, shouldNavigate @@ -791,6 +792,20 @@ 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)) { + // 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(); + this.#openInNewTab(url.href); + + return; + } + // Same pathname (any hash/query): let the browser handle it (anchors, query-driven UI) if (isSamePageNavigation(href, this.uveStore.pageParams()?.url)) { return; @@ -800,6 +815,45 @@ 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 { + let link: HTMLAnchorElement | null = null; + + try { + const doc = this.window.document; + + link = doc.createElement('a'); + link.href = href; + link.target = '_blank'; + link.rel = 'noopener'; + + doc.body.appendChild(link); + link.click(); + } 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(); + } + } + /** * Handles the inline editing functionality triggered by a mouse event. * @param e - The mouse event that triggered the inline editing. 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..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 @@ -1168,3 +1168,85 @@ 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. Only `html`, the shipped + * `VELOCITY_PAGE_EXTENSION` (`dotmarketing-config.properties:91`). + * + * 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']); + +/** + * 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]{1,8}$/; + +/** + * Checks whether a pathname targets a file asset rather than an HTMLPage. + * + * 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 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 + * + * @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 + */ +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..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 @@ -31,7 +31,8 @@ import { normalizeQueryParams, convertUTCToLocalTime, escapeHtmlAttributeValue, - isSamePageNavigation + isSamePageNavigation, + isAssetPath } from '.'; import { DEFAULT_PERSONA, PERSONA_KEY } from '../shared/consts'; @@ -1648,4 +1649,62 @@ describe('utils functions', () => { expect(result.getHours()).toBe(12); }); }); + + describe('isAssetPath', () => { + it.each([ + '/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', + // `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' + ])('should treat %s as a file asset', (pathname) => { + expect(isAssetPath(pathname)).toBe(true); + }); + + it.each([ + '/about-us/index', + '/about-us/index.html', + '/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', () => { + expect(isAssetPath('')).toBe(false); + }); + + 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); + } + ); + }); });