Skip to content

Fix uve height issue 2#35178

Open
fmontes wants to merge 3 commits intouve-experimentfrom
fix-uve-height-issue-2
Open

Fix uve height issue 2#35178
fmontes wants to merge 3 commits intouve-experimentfrom
fix-uve-height-issue-2

Conversation

@fmontes
Copy link
Copy Markdown
Member

@fmontes fmontes commented Apr 1, 2026

Proposed Changes

  • change 1
  • change 2

Checklist

  • Tests
  • Translations
  • Security Implications Contemplated (add notes if applicable)

Additional Info

** any additional useful context or info **

Screenshots

Original Updated
** original screenshot ** ** updated screenshot **

This PR fixes: #2

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 1, 2026

Claude finished @fmontes's task in 2m 22s —— View job


PR Review

  • Gather context
  • Read changed files
  • Post review findings

Summary

The PR extracts iframe height observation into a shared observeDocumentHeight utility (used by both the SDK for cross-origin reporting and the Angular component for same-origin tracking). The design is sound. A few issues worth addressing:


Issues

1. No height deduplication — redundant messages on every resize/mutation

document-height-observer.ts:32-39measureAndNotify emits on every settled observation without checking whether the height actually changed:

const measureAndNotify = () => {
    const height = html.offsetHeight;
    if (!height) { return; }
    onHeightChange(height); // always fires, even when height === previous height
};

ResizeObserver fires on font-size changes, scrollbar appearance, etc. MutationObserver with subtree: true fires on any DOM mutation in the body (tooltip shows, dropdown opens, etc.). With debounce at 50ms this may fire frequently, causing redundant patchState calls on viewZoomIframeDocHeight and unnecessary iframe resizes. Add a lastHeight variable and only call onHeightChange when the value changes.

Fix this →


2. CSS injection via localStorage in injectEmptyStateStyles

utils.ts:235-236:

const messages = JSON.parse(localStorage.getItem('dotMessagesKeys') ?? '{}');
emptyContainerLabel = messages['editpage.container.is.empty'] ?? emptyContainerLabel;

Then at line 259:

content: '${emptyContainerLabel}';

The label is interpolated directly into a CSS content string with no sanitization. An attacker who can write to localStorage (e.g., a stored XSS elsewhere in the app) can inject CSS. A value like '); color:red; x: (' would break the block. The blast radius is the editor's own iframe, but it's still a CSS injection. Sanitize the value — at minimum strip/escape single quotes and backslashes before interpolation.

Fix this →


3. Inconsistent window vs dotWindow in getIframeAccessMode

withPageApi.ts:113-116pageUpdateParams (first withMethods block, no DI) calls:

iframeAccessMode: getIframeAccessMode(nextPageParams.clientHost)
// uses window.location.origin as default

pageLoad (second withMethods block, with DI) calls:

iframeAccessMode: getIframeAccessMode(pageParams.clientHost, dotWindow.location.origin)

If WINDOW token resolves to a different object than the global window (e.g., in SSR or test environments), these two paths compute different access modes for the same URL. The first call was added in this PR. Either pass dotWindow.location.origin via the deps interface or consolidate the access mode computation to only happen in pageLoad.


4. MutationObserver with subtree: true is very broad

document-height-observer.ts:87-88:

const mutationObserver = new MutationObserver(() => scheduleNotify());
mutationObserver.observe(documentRef.body ?? html, { childList: true, subtree: true });

This fires scheduleNotify on every DOM insertion/removal anywhere in the document — including editor overlays, tooltips, and dropdowns added by the UVE itself. Debouncing helps, but content-heavy pages with frequent DOM activity (live widgets, etc.) will repeatedly schedule double-rAF measurements. The comment justifies it for "DOM removals that shrink the page," but subtree: false (observe direct body children only) would catch most of those cases with far fewer false firings.


5. Minor: indentation in reportIframeHeight

utils.ts:208-213:

const { destroy } = observeDocumentHeight({
    onHeightChange: (height) => {
    sendMessageToUVE({   // <-- not indented relative to arrow function body
        action: DotCMSUVEAction.IFRAME_HEIGHT,
        payload: { height }
    });
    }
});

Misaligned indentation — sendMessageToUVE should be indented one level inside the arrow function.


Not flagged

  • The dual-observer strategy (ResizeObserver for layout, MutationObserver for DOM removals) is sound.
  • destroy() cleanup of timers and both observers is complete.
  • shouldReportIframeHeightToParent logic (same-origin short-circuit) is correct.
  • Splitting height tracking responsibilities between SDK (cross-origin) and Angular component (same-origin) is the right architecture.
  • Test coverage for observeDocumentHeight is thorough.

@semgrep-code-dotcms-test
Copy link
Copy Markdown

Legal Risk

The following dependencies were released under a license that
has been flagged by your organization for consideration.

Recommendation

While merging is not directly blocked, it's best to pause and consider what it means to use this license before continuing. If you are unsure, reach out to your security team or Semgrep admin to address this issue.

GPL-2.0

MPL-2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant