fix(uve): open file-asset links in a new tab instead of loading them as pages (#35504)

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&param2=value2';

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.

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);
+};

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);
+            }
+        );
+    });
 });