Skip to content
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.

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

set COOP, COEP, and CORP headers for desktop #145539

Closed
wants to merge 138 commits into from
Closed

Conversation

jrieken
Copy link
Member

@jrieken jrieken commented Mar 21, 2022

This PR is for #116715 and sets COOP, COEP and CORP headers so that the renderer and worker extension host are cross origin isolated. Tho, for webviews this doesn't work yet

@jrieken jrieken self-assigned this Mar 21, 2022
@jrieken jrieken requested a review from deepak1556 March 21, 2022 09:47
@jrieken jrieken added this to the April 2022 milestone Mar 21, 2022
headers: {
'Cross-Origin-Opener-Policy': 'same-origin',
'Cross-Origin-Embedder-Policy': 'require-corp',
'Cross-Origin-Resource-Policy': 'cross-origin'
Copy link
Contributor

Choose a reason for hiding this comment

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

For webviews, the corp header needs to be added to

const headers = {
'Content-Type': entry.mime,
'Content-Length': entry.data.byteLength.toString(),
'Access-Control-Allow-Origin': '*',
};
. Webviews on desktop are structured like below,

<outer iframe> -> serves the static resources https://github.com/microsoft/vscode/tree/main/src/vs/workbench/contrib/webview/browser/pre
<inner iframe> -> hosts the extension and resources are served via the service worker of the outer frame

webviewProtocolProvider only handles serving the static resources for the outer iframe

/cc @mjbvz

Copy link
Collaborator

Choose a reason for hiding this comment

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

@deepak1556 Yes that sounds correct to me

Will adding this policy risk breaking existing webviews though? I'm not very familiar with cross origin embedder policy so I just want to make sure that if a webview loads an external resource today, adding this new header won't break them

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not very familiar with cross origin embedder policy so I just want to make sure that if a webview loads an external resource today, adding this new header won't break them

Yeah, that's the question here. When a page runs with COEP set to require-corp all its external resources must use the CORP header. I am not sure if web views always and only fetch resources from within their extension bundle or if they also fetch external resources, like from a CDN. Iff so we need to make the crossOriginIsolation opt-in for webviews

Copy link
Contributor

@deepak1556 deepak1556 Mar 22, 2022

Choose a reason for hiding this comment

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

I'm not very familiar with cross origin embedder policy so I just want to make sure that if a webview loads an external resource today, adding this new header won't break them

https://docs.google.com/document/d/1zDlfvfTJ_9e8Jdc8ehuV4zMEu9ySMCiTGMS9y0GU92k/edit is a good primer on these headers

When COEP is set, external resources can also allow themselves to be loaded either with CORP header or CORS Access-Control-Allow-Origin header.

I like the idea of having this feature opt-in, COOP-COEP headers are necessary to gate access to exploitable web API (SharedArrayBuffers, performance.memory) and having extensions explicitly opt-in ensures that they validate external resources loaded obey the CORP/CORS rules and also these headers will isolate them from other extensions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed a change to use 'Cross-Origin-Embedder-Policy': 'credentialless' for web views. That was the only combination of headers allowing the iframes to be loaded while them still being able to load "outside" content, like a random image from our homepage.

@jrieken jrieken marked this pull request as ready for review April 5, 2022 09:34
@jrieken jrieken requested review from mjbvz and deepak1556 April 6, 2022 10:03
@jrieken
Copy link
Member Author

jrieken commented Apr 6, 2022

Please review. The renderer and the web worker extension host will be cross origin isolated with this PR. Web views won't. I still needed to add headers to them to load in the stricter context (CORP, COEP). I have tested with our markdown preview, our images preview, luna paint, and a webview panel that loads resources from the internet (not from within the extension) but please review/test carefully.

@mjbvz
Copy link
Collaborator

mjbvz commented Apr 6, 2022

Thanks @jrieken! I just tested a few webview extensions. Here's what I've found so far:

  • ✅ Md preview seem to work (including references to external images)
  • ✅ Git lens welcome views seem to work
  • ❌ Live preview extension doesn't seem to work. I see [main 2022-04-06T21:38:38.049Z] CodeWindow: failed to load (reason: ERR_BLOCKED_BY_RESPONSE, code: -27)

I'm taking a look into the live preview issue now

@mjbvz
Copy link
Collaborator

mjbvz commented Apr 6, 2022

The request that fails:http://127.0.0.1:3000/index.html. This is for the iframe that the live preview extension creates (which is inside the two iframes that we create for each webview). This request does not set a cross-origin-resource-policy, which causes it to be blocked by the browser

I think this document may be relevant: https://github.com/camillelamy/explainers/blob/main/anonymous_iframes.md

andreamah and others added 27 commits July 18, 2022 16:57
Fixed SignatureInformation.activeParameter comment
Button separator color on high-contrast themes (Fixes #155285)
* Cleanup expansion context key

* Fix #155131
For now only when checking for tabs, not yet for opening tabs
…n elements (#155317)

Remove tabIndex from the label and description elements
Add ability to continue desktop edit session in vscode.dev
…ely (#155485)

[themes] When opening a new window, product icons don't load immediatel. Fixes #142236
* replace list type filter and tree type label controller with list type
navigation and tree find. use proper FindInput widget

* make sure vim doesn't break

* polish outline use case

* 💄

* remove unused import
* add telemetry comments

* update comments
@jrieken jrieken closed this Sep 2, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet