From 315b947740a2e3a5ae8d4cbf594042e473c00734 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 3 Mar 2026 23:15:33 -0800 Subject: [PATCH] Harden render iframe open-link handling (#36811) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR hardens the handling of the “open-link” action in render iframes (external rendering iframes). It prevents iframes from triggering unsafe or unintended redirects or opening new windows via postMessage. Additionally, it improves iframe height reporting to reduce scrollbar and height mismatch issues, and adds unit test coverage. --------- Co-authored-by: wxiaoguang --- web_src/js/markup/render-iframe.test.ts | 46 +++++++++++++++++++ web_src/js/markup/render-iframe.ts | 39 ++++++++++++---- .../js/standalone/external-render-iframe.ts | 20 ++++++-- 3 files changed, 92 insertions(+), 13 deletions(-) create mode 100644 web_src/js/markup/render-iframe.test.ts diff --git a/web_src/js/markup/render-iframe.test.ts b/web_src/js/markup/render-iframe.test.ts new file mode 100644 index 0000000000..53c9dc3720 --- /dev/null +++ b/web_src/js/markup/render-iframe.test.ts @@ -0,0 +1,46 @@ +import {navigateToIframeLink} from './render-iframe.ts'; + +describe('navigateToIframeLink', () => { + const openSpy = vi.spyOn(window, 'open').mockImplementation(() => null); + const assignSpy = vi.spyOn(window.location, 'assign').mockImplementation(() => undefined); + + test('safe links', () => { + navigateToIframeLink('http://example.com', '_blank'); + expect(openSpy).toHaveBeenCalledWith('http://example.com/', '_blank', 'noopener,noreferrer'); + vi.clearAllMocks(); + + navigateToIframeLink('https://example.com', '_self'); + expect(assignSpy).toHaveBeenCalledWith('https://example.com/'); + vi.clearAllMocks(); + + navigateToIframeLink('https://example.com', null); + expect(assignSpy).toHaveBeenCalledWith('https://example.com/'); + vi.clearAllMocks(); + + navigateToIframeLink('/path', ''); + expect(assignSpy).toHaveBeenCalledWith('http://localhost:3000/path'); + vi.clearAllMocks(); + + // input can be any type & any value, keep the same behavior as `window.location.href = 0` + navigateToIframeLink(0, {}); + expect(assignSpy).toHaveBeenCalledWith('http://localhost:3000/0'); + vi.clearAllMocks(); + }); + + test('unsafe links', () => { + window.location.href = 'http://localhost:3000/'; + + // eslint-disable-next-line no-script-url + navigateToIframeLink('javascript:void(0);', '_blank'); + expect(openSpy).toHaveBeenCalledTimes(0); + expect(assignSpy).toHaveBeenCalledTimes(0); + expect(window.location.href).toBe('http://localhost:3000/'); + vi.clearAllMocks(); + + navigateToIframeLink('data:image/svg+xml;utf8,', ''); + expect(openSpy).toHaveBeenCalledTimes(0); + expect(assignSpy).toHaveBeenCalledTimes(0); + expect(window.location.href).toBe('http://localhost:3000/'); + vi.clearAllMocks(); + }); +}); diff --git a/web_src/js/markup/render-iframe.ts b/web_src/js/markup/render-iframe.ts index 1291dea4f8..531942e0b1 100644 --- a/web_src/js/markup/render-iframe.ts +++ b/web_src/js/markup/render-iframe.ts @@ -1,23 +1,46 @@ import {generateElemId, queryElemChildren} from '../utils/dom.ts'; import {isDarkTheme} from '../utils.ts'; +function safeRenderIframeLink(link: any): string | null { + try { + const url = new URL(`${link}`, window.location.href); + if (url.protocol !== 'http:' && url.protocol !== 'https:') { + console.error(`Unsupported link protocol: ${link}`); + return null; + } + return url.href; + } catch (e) { + console.error(`Failed to parse link: ${link}, error: ${e}`); + return null; + } +} + +// This function is only designed for "open-link" command from iframe, is not suitable for other contexts. +// Because other link protocols are directly handled by the iframe, but not here. +// Arguments can be any type & any value, they are from "message" event's data which is not controlled by us. +export function navigateToIframeLink(unsafeLink: any, target: any) { + const linkHref = safeRenderIframeLink(unsafeLink); + if (linkHref === null) return; + if (target === '_blank') { + window.open(linkHref, '_blank', 'noopener,noreferrer'); + return; + } + // treat all other targets including ("_top", "_self", etc.) as same tab navigation + window.location.assign(linkHref); +} + async function loadRenderIframeContent(iframe: HTMLIFrameElement) { const iframeSrcUrl = iframe.getAttribute('data-src')!; if (!iframe.id) iframe.id = generateElemId('gitea-iframe-'); window.addEventListener('message', (e) => { + if (e.source !== iframe.contentWindow) return; if (!e.data?.giteaIframeCmd || e.data?.giteaIframeId !== iframe.id) return; const cmd = e.data.giteaIframeCmd; if (cmd === 'resize') { - // TODO: sometimes the reported iframeHeight is not the size we need, need to figure why. Example: openapi swagger. - // As a workaround, add some pixels here. - iframe.style.height = `${e.data.iframeHeight + 2}px`; + iframe.style.height = `${e.data.iframeHeight}px`; } else if (cmd === 'open-link') { - if (e.data.anchorTarget === '_blank') { - window.open(e.data.openLink, '_blank'); - } else { - window.location.href = e.data.openLink; - } + navigateToIframeLink(e.data.openLink, e.data.anchorTarget); } else { throw new Error(`Unknown gitea iframe cmd: ${cmd}`); } diff --git a/web_src/js/standalone/external-render-iframe.ts b/web_src/js/standalone/external-render-iframe.ts index dcfeb50541..f8ec070785 100644 --- a/web_src/js/standalone/external-render-iframe.ts +++ b/web_src/js/standalone/external-render-iframe.ts @@ -20,7 +20,15 @@ function mainExternalRenderIframe() { window.parent.postMessage({giteaIframeCmd: cmd, giteaIframeId: iframeId, ...data}, '*'); }; - const updateIframeHeight = () => postIframeMsg('resize', {iframeHeight: document.documentElement.scrollHeight}); + const updateIframeHeight = () => { + // Don't use integer heights from the DOM node. + // Use getBoundingClientRect(), then ceil the height to avoid fractional pixels which causes incorrect scrollbars. + const rect = document.documentElement.getBoundingClientRect(); + postIframeMsg('resize', {iframeHeight: Math.ceil(rect.height)}); + // As long as the parent page is responsible for the iframe height, the iframe itself doesn't need scrollbars. + // This style should only be dynamically set here when our code can run. + document.documentElement.style.overflowY = 'hidden'; + }; const resizeObserver = new ResizeObserver(() => updateIframeHeight()); resizeObserver.observe(window.document.documentElement); @@ -29,16 +37,18 @@ function mainExternalRenderIframe() { // the easiest way to handle dynamic content changes and easy to debug, can be fine-tuned in the future setInterval(updateIframeHeight, 1000); - // no way to open an absolute link with CSP frame-src, it also needs some tricks like "postMessage" or "copy the link to clipboard" - const openIframeLink = (link: string, target: string) => postIframeMsg('open-link', {openLink: link, anchorTarget: target}); + // no way to open an absolute link with CSP frame-src, it needs some tricks like "postMessage" (let parent window to handle) or "copy the link to clipboard" (let users manually paste it to open). + // here we choose "postMessage" way for better user experience. + const openIframeLink = (link: string, target: string | null) => postIframeMsg('open-link', {openLink: link, anchorTarget: target}); document.addEventListener('click', (e) => { const el = e.target as HTMLAnchorElement; if (el.nodeName !== 'A') return; - const href = el.getAttribute('href') || ''; + const href = el.getAttribute('href') ?? ''; // safe links: "./any", "../any", "/any", "//host/any", "http://host/any", "https://host/any" if (href.startsWith('.') || href.startsWith('/') || href.startsWith('http://') || href.startsWith('https://')) { e.preventDefault(); - openIframeLink(href, el.getAttribute('target')!); + const forceTarget = (e.metaKey || e.ctrlKey) ? '_blank' : null; + openIframeLink(href, forceTarget ?? el.getAttribute('target')); } }); }