Skip to content

Implement the IDL security check#28583

Merged
jdm merged 6 commits into
servo:mainfrom
yvt:feat-webidl-security-check
Apr 4, 2026
Merged

Implement the IDL security check#28583
jdm merged 6 commits into
servo:mainfrom
yvt:feat-webidl-security-check

Conversation

@yvt
Copy link
Copy Markdown
Contributor

@yvt yvt commented Aug 18, 2021

https://heycam.github.io/webidl/#es-operations
https://html.spec.whatwg.org/multipage/#integration-with-idl

This PR implements the IDL security check, which is an important (but not only) mechanism to restrict cross-origin DOM accesses.

This implementation follows WebKit's behavior and not the specification for the following reasons:

  • Neither Gecko nor WebKit implements the specification's behavior.
  • This would require a relatively elaborate mechanism just to have access to the target object's CrossOriginProperties.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)

  • There are tests for these changes OR
  • These changes do not require tests because ___

@highfive
Copy link
Copy Markdown

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/utils.rs, components/script/dom/bindings/proxyhandler.rs
  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/utils.rs, components/script/dom/bindings/proxyhandler.rs

@highfive
Copy link
Copy Markdown

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 18, 2021
Comment thread components/script/dom/bindings/codegen/CodegenRust.py Outdated
@yvt yvt marked this pull request as draft August 20, 2021 03:36
@yvt yvt force-pushed the feat-webidl-security-check branch from 5abae38 to 6388d17 Compare August 21, 2021 15:40
@yvt yvt marked this pull request as ready for review August 21, 2021 15:40
@sagudev sagudev mentioned this pull request Feb 19, 2023
5 tasks
@bors-servo
Copy link
Copy Markdown
Contributor

☔ The latest upstream changes (presumably #29384) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Copy Markdown
Member

jdm commented Apr 30, 2025

This is great, and I love the CallPolicy additions. I'm going to rebase these changes and get them merged.

@jdm jdm self-requested a review April 30, 2025 05:12
@jdm jdm force-pushed the feat-webidl-security-check branch from 6388d17 to 3bbd757 Compare April 4, 2026 03:48
yvt added 4 commits April 3, 2026 23:50
…ion parameters

This trait's associated constant `INFO` provides fields that control
various details of an IDL operation, such as whether a
`[LegacyLenientThis]` attribute is specified on the corresponding
interface member.

The `generic_(method|getter|setter)` functions are now generic over
`Policy: CallPolicy`, making the `generic_lenient_*` methods
unnecessary. Currently, they pass `Policy::INFO` to the inner
`generic_method` function as a runtime parameter, but we can change it
later if we find it beneficial to make generic the inner function.

Signed-off-by: Josh Matthews <[email protected]>
…avior

<https://heycam.github.io/webidl/#es-operations>
<https://html.spec.whatwg.org/multipage/#integration-with-idl>

Not implementing the specification's behavior for the following reasons:

 - Neither Gecko nor WebKit implements this behavior.
 - This would require an elaborate mechanism to have access to the
   target object's `CrossOriginProperties`.

Signed-off-by: Josh Matthews <[email protected]>
Signed-off-by: Josh Matthews <[email protected]>
@jdm jdm force-pushed the feat-webidl-security-check branch from 3bbd757 to b42ebb0 Compare April 4, 2026 03:50
@jdm
Copy link
Copy Markdown
Member

jdm commented Apr 4, 2026

After a detour to fix javascript: URL evaluation which was causing lots of tests to fail intermittently, this is now ready to merge!

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Apr 4, 2026
@jdm jdm enabled auto-merge April 4, 2026 03:52
Signed-off-by: Josh Matthews <[email protected]>
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 4, 2026
Signed-off-by: Josh Matthews <[email protected]>
@jdm jdm added this pull request to the merge queue Apr 4, 2026
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 4, 2026
Merged via the queue into servo:main with commit c6bb3cc Apr 4, 2026
30 checks passed
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 4, 2026
@yvt yvt deleted the feat-webidl-security-check branch April 4, 2026 07:53
@delan
Copy link
Copy Markdown
Member

delan commented May 30, 2026

hi! i’m working on the monthly update. i did a bunch of testing to understand exactly what this patch fixes and what it doesn’t fix, but i’m still not 100% sure. it seems like before this patch, cross-site DOM accesses were already correctly rejected by servo, but same-site cross-origin DOM accesses were incorrectly allowed by servo. and after this patch, we fix one kind of incorrect access, specifically those that involve binding a Window or Location method in this document with a this from the other document. is that correct?

<!--
    1. save this file as test.html
    2. serve the directory with both `python -m http.server 8000` and `8001`
    3. go to <http://localhost:8000/test.html>
-->
<!-- case 1: same-origin -->
<!-- <iframe id=i src="http://localhost:8000/"></iframe> -->
<!-- case 2: same-site cross-origin -->
<iframe id=i src="http://localhost:8001/"></iframe>
<!-- case 3: cross-site -->
<!-- <iframe id=i src="https://servo.org/"></iframe> -->
<script>
    setTimeout(()=>{
        try {
            // for case 2...
            // servo 0.1.0: returns `"#document"`
            // servo 0.2.0: returns `"#document"`
            // firefox 150: throws DOMException
            alert(i.contentWindow.document.nodeName);
        } catch (e) {
            console.log(e);
        }
        try {
            // for case 2...
            // servo 0.1.0: returns `undefined`
            // servo 0.2.0: returns `undefined`
            // firefox 150: throws DOMException
            alert(i.contentWindow.stop());
        } catch (e) {
            console.log(e);
        }
        try {
            // for case 2...
            // servo 0.1.0: returns `undefined`
            // servo 0.2.0: throws SecurityError
            // firefox 150: throws DOMException
            const d = Object.getOwnPropertyDescriptor(window,"stop");
            alert(d.value.bind(i.contentWindow)());
        } catch (e) {
            console.log(e);
        }
    },250)
</script>

@jdm
Copy link
Copy Markdown
Member

jdm commented May 30, 2026

That is correct. This only affects the exotic WebIDL interfaces, Window and Location. #44649 is another big missing piece here.

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

Labels

S-awaiting-review There is new code that needs to be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants