Skip to content

[Model Element] Require CORS for loading model sources and environment maps#45447

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
achan00:eng/Model-Element-Require-CORS-for-loading-model-sources-and-environment-maps
May 21, 2025
Merged

[Model Element] Require CORS for loading model sources and environment maps#45447
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
achan00:eng/Model-Element-Require-CORS-for-loading-model-sources-and-environment-maps

Conversation

@achan00
Copy link
Contributor

@achan00 achan00 commented May 15, 2025

74ab595

[Model Element] Require CORS for loading model sources and environment maps
https://bugs.webkit.org/show_bug.cgi?id=293063
rdar://151364606

Reviewed by Youenn Fablet and Anne van Kesteren.

Make sure CORS is always enabled with model source and environment
map requests, by passing a non-null crossorigin attribute to
createPotentialAccessControlRequest().

Also make sure we null out m_model once source is reset.

* LayoutTests/TestExpectations:
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-allowed-with-all-access-header-expected.txt: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-allowed-with-all-access-header.html: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-allowed-with-header-expected.txt: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-allowed-with-header.html: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-allowed-with-same-origin-expected.txt: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-allowed-with-same-origin.html: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-blocked-with-header-expected.txt: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-blocked-with-header.html: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-blocked-with-use-credentials-and-all-access-header-expected.txt: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-blocked-with-use-credentials-and-all-access-header.html: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-blocked-without-header-expected.txt: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-blocked-without-header.html: Added.
* LayoutTests/http/tests/security/resources/cube.usdz: Added.
* LayoutTests/http/tests/security/resources/model-access-control.py: Added.
* Source/WebCore/Modules/model-element/HTMLModelElement.cpp:
(WebCore::HTMLModelElement::setSourceURL):
(WebCore::HTMLModelElement::environmentMapRequestResource):

Canonical link: https://commits.webkit.org/295219@main

2409c48

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug 🧪 wpe-wk2 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ❌ 🛠 wpe-cairo
🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision 🧪 mac-AS-debug-wk2 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
🛠 watch-sim

@achan00 achan00 requested a review from marcoscaceres as a code owner May 15, 2025 21:28
@achan00 achan00 self-assigned this May 15, 2025
@achan00 achan00 added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label May 15, 2025
@achan00 achan00 force-pushed the eng/Model-Element-Require-CORS-for-loading-model-sources-and-environment-maps branch from c45724a to f57ea62 Compare May 15, 2025 21:30
@achan00 achan00 requested review from annevk, eddydas and youennf May 15, 2025 21:31
@achan00
Copy link
Contributor Author

achan00 commented May 16, 2025

Actually, this is not correct yet. I need to look more into this.

@annevk
Copy link
Contributor

annevk commented May 16, 2025

Yeah, this will hit an assert in createPotentialAccessControlRequest(). Not sure what @youennf thinks, but one thing that comes to mind is that when parseCORSSettingsAttribute returns nullString(), you use "anonymous"_s instead.

@achan00 achan00 force-pushed the eng/Model-Element-Require-CORS-for-loading-model-sources-and-environment-maps branch from f57ea62 to 0eb5c74 Compare May 16, 2025 07:56
Copy link
Contributor

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This looks good to me, but let's wait for Youenn as well to be sure.

Copy link
Contributor

@youennf youennf left a comment

Choose a reason for hiding this comment

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

LGTM with a test where load would fail.

@achan00 achan00 force-pushed the eng/Model-Element-Require-CORS-for-loading-model-sources-and-environment-maps branch from 0eb5c74 to 282b48b Compare May 17, 2025 23:33
@achan00 achan00 force-pushed the eng/Model-Element-Require-CORS-for-loading-model-sources-and-environment-maps branch from 282b48b to 3aa42ef Compare May 19, 2025 06:53
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 19, 2025
@achan00 achan00 added merge-queue Applied to send a pull request to merge-queue merging-blocked Applied to prevent a change from being merged and removed merging-blocked Applied to prevent a change from being merged merge-queue Applied to send a pull request to merge-queue labels May 19, 2025
@achan00 achan00 force-pushed the eng/Model-Element-Require-CORS-for-loading-model-sources-and-environment-maps branch from 3aa42ef to 2ec5bbd Compare May 20, 2025 19:02
@achan00 achan00 force-pushed the eng/Model-Element-Require-CORS-for-loading-model-sources-and-environment-maps branch from 2ec5bbd to 40d70b3 Compare May 20, 2025 19:06
Copy link
Contributor

@youennf youennf left a comment

Choose a reason for hiding this comment

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

LGTM.
If you have some time, we could add a few additional tests:

  • a data URL load from a HTTP origin (should succeed)
  • a blob URL load from a HTTP origin (should succeed)
  • a file URL load from a file origin (should succeed)
  • a file URL load with a crossorigin attribute (whatever value, should fail)
  • a HTTP URL load with an empty crossorigin attribute (should be the same as anonymous).

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CachedResourceRequest HTMLModelElement::createResourceRequest(const URL& resourceURL, const FetchOptions::Destination& destination)
CachedResourceRequest HTMLModelElement::createResourceRequest(const URL& resourceURL, FetchOptions::Destination destination)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add more tests in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed: https://bugs.webkit.org/show_bug.cgi?id=293371 (Add more tests around model source loading across different origins)

@achan00 achan00 force-pushed the eng/Model-Element-Require-CORS-for-loading-model-sources-and-environment-maps branch from 40d70b3 to 2409c48 Compare May 21, 2025 16:43
@achan00 achan00 added the merge-queue Applied to send a pull request to merge-queue label May 21, 2025
…t maps

https://bugs.webkit.org/show_bug.cgi?id=293063
rdar://151364606

Reviewed by Youenn Fablet and Anne van Kesteren.

Make sure CORS is always enabled with model source and environment
map requests, by passing a non-null crossorigin attribute to
createPotentialAccessControlRequest().

Also make sure we null out m_model once source is reset.

* LayoutTests/TestExpectations:
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-allowed-with-all-access-header-expected.txt: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-allowed-with-all-access-header.html: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-allowed-with-header-expected.txt: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-allowed-with-header.html: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-allowed-with-same-origin-expected.txt: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-allowed-with-same-origin.html: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-blocked-with-header-expected.txt: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-blocked-with-header.html: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-blocked-with-use-credentials-and-all-access-header-expected.txt: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-blocked-with-use-credentials-and-all-access-header.html: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-blocked-without-header-expected.txt: Added.
* LayoutTests/http/tests/security/model-element/model-element-crossorigin-blocked-without-header.html: Added.
* LayoutTests/http/tests/security/resources/cube.usdz: Added.
* LayoutTests/http/tests/security/resources/model-access-control.py: Added.
* Source/WebCore/Modules/model-element/HTMLModelElement.cpp:
(WebCore::HTMLModelElement::setSourceURL):
(WebCore::HTMLModelElement::environmentMapRequestResource):

Canonical link: https://commits.webkit.org/295219@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Model-Element-Require-CORS-for-loading-model-sources-and-environment-maps branch from 2409c48 to 74ab595 Compare May 21, 2025 18:31
@webkit-commit-queue
Copy link
Collaborator

Committed 295219@main (74ab595): https://commits.webkit.org/295219@main

Reviewed commits have been landed. Closing PR #45447 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 74ab595 into WebKit:main May 21, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants