Skip to content

Security: sandbox Web Page iframe and enforce http(s) URL allowlist (CWE-1021/CWE-829) #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jakexcosme wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Security: sandbox Web Page iframe and enforce http(s) URL allowlist (CWE-1021/CWE-829) #11

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions src/plugins/webPage/components/WebPage.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,15 @@
-->
<template>
<div class="l-iframe abs">
<iframe :src="url"></iframe>
<iframe :src="url" sandbox="allow-scripts allow-forms" referrerpolicy="no-referrer"></iframe>

@devin-ai-integration devin-ai-integration Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Sandbox omits allow-same-origin, disabling storage/DOM for same-origin relative URLs

The sandbox="allow-scripts allow-forms" attribute deliberately omits allow-same-origin. This means even the allowed same-origin root-relative URLs (/...) matched by isSafeRelativeUrl at src/plugins/webPage/components/WebPage.vue:46 will be loaded into a unique opaque origin, so framed same-origin pages will lose access to cookies, localStorage, and same-origin DOM/XHR. This is the intended security tradeoff per the PR, but reviewers should confirm no existing deployment relies on embedding a same-origin page that needs storage/session access, since such pages may now silently break.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intended. Omitting allow-same-origin is deliberate: with it, an attacker-supplied same-origin (or allow-scripts+allow-same-origin) frame could script its way out of the sandbox, defeating the fix. The Web Page object is designed to embed external content (its url is free-text and, per the finding, attacker-influenceable via Import-from-JSON/shared persistence), so treating the frame as an untrusted opaque origin is correct. There is no known Open MCT feature that relies on embedding a same-origin page needing cookie/localStorage access through this object. Flagging the tradeoff for reviewers, but not changing it.

</div>
</template>

<script>
import { sanitizeUrl } from '@braintree/sanitize-url';

const BLOCKED_URL = 'about:blank';

export default {
inject: ['openmct', 'domainObject'],
data: function () {
Expand All @@ -37,7 +39,18 @@ export default {
},
computed: {
url() {
return sanitizeUrl(this.currentDomainObject.url);
const url = sanitizeUrl(this.currentDomainObject.url);

// Only allow http/https absolute urls and same-origin relative urls
const isSafeAbsoluteUrl = /^https?:\/\//i.test(url);
const isSafeRelativeUrl = /^\/(?!\/)/.test(url);
if (!isSafeAbsoluteUrl && !isSafeRelativeUrl) {
console.warn('Blocked unsafe URL:', url);

return BLOCKED_URL;
}

return url;
Comment on lines +45 to +53

@devin-ai-integration devin-ai-integration Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: Behavior change: previously-rendered scheme-less and relative URLs now blocked

Previously the iframe bound whatever sanitizeUrl returned, which passes through values like 123, example.com/page (scheme-less), or scheme-relative //host/path. The new allowlist at src/plugins/webPage/components/WebPage.vue:45-51 only permits http(s):// absolute URLs and single-slash root-relative paths, rendering everything else as about:blank. Existing persisted Web Page objects whose URLs lack a scheme (a common user habit, e.g. entering www.example.com) will stop rendering and show a blank frame with only a console warning. This is intentional hardening but is a user-visible behavior change worth noting in release notes given the type description invites free-text URLs.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct and intended. Scheme-less values like www.example.com or 123 were already broken/ambiguous as iframe sources; blocking them (rendering about:blank + a console warning) is the safe, explicit behavior and closes the scheme-relative //host bypass. This mirrors the existing allowlist in OpenImageInNewTabAction. I've noted it as a user-visible behavior change; the "notable change / release notes" checkbox is left for the maintainer to confirm. Not changing behavior here.

}
}
};
Expand Down