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

remove "x-frame-options" and "content-security-policy" response headers #2963

Merged
merged 2 commits into from
Nov 7, 2022

Conversation

Tom-Hirschberger
Copy link
Contributor

Many users like me do have the problem that they want to embed other sites to their mirror by "iframe".
As some developers set the "x-frame-options" and "content-security-policy" for security reasons these sites can not be embedded.
Electron provides the "webview" element additionally to "iframe" which allows to embed these sites although. The main difference is that a new process is started which handles the "webview" element.
BUT: As the "webview" process needs to be started and is isolated "webview" is slower and the elements can not be accessed from the embedding website.

As an alternative i implemented a small callback function in electron.js which removes the response headers that forbid the embedding.

The removing can be controlled with the new config options:

  • ignoreXOriginHeader
  • ignoreContentSecurityPolicy

…onses if configured;

these headers prevent sites of being embedded into iframes; with the headers being removed the sites can be embedded;
@codecov-commenter
Copy link

Codecov Report

Merging #2963 (8433ed1) into develop (4d47c08) will increase coverage by 0.07%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #2963      +/-   ##
===========================================
+ Coverage    24.00%   24.08%   +0.07%     
===========================================
  Files           49       49              
  Lines        10121    10135      +14     
===========================================
+ Hits          2430     2441      +11     
- Misses        7691     7694       +3     
Impacted Files Coverage Δ
js/electron.js 0.00% <0.00%> (ø)
modules/default/updatenotification/node_helper.js 74.28% <0.00%> (+15.71%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sdetweil
Copy link
Collaborator

sdetweil commented Nov 2, 2022

interesting. but doesn't that violate the source's content policies?

@Tom-Hirschberger
Copy link
Contributor Author

Tom-Hirschberger commented Nov 2, 2022

It might violate but it it does not necessarily.
Most time the header is set to prevent of XSS attacks and not to avoid embedding. But a XSS attack at the electron instance providing the mirrors page is very unlikely.

In my case even my self hosted Node Red dashboard has set the header and I did not find a option to disable it.

@khassel
Copy link
Collaborator

khassel commented Nov 2, 2022

if I remember this correctly we had already such issues and we inserted a config option httpHeaders for this.

Have you tested iframe embedding with these

let config = {
	address: "localhost",
	port: 8080,
	httpHeaders: { contentSecurityPolicy: false, crossOriginOpenerPolicy: false, crossOriginEmbedderPolicy: false, crossOriginResourcePolicy: false, originAgentCluster: false, frameguard: false },
	...
}

httpHeaders?

@Tom-Hirschberger
Copy link
Contributor Author

Did a quick check in server.js and the Changelog.
The httpHeaders control can be used to control if the MagicMirror page can be embedded with a Iframe.
But they can not be used to control the headers of pages that are embedded to the mirror page.

@khassel
Copy link
Collaborator

khassel commented Nov 6, 2022

thanks for clarifying, had the older iframe change in mind and wanted to avoid doing something which is already implemented.

@rejas rejas merged commit b9b7d2c into MagicMirrorOrg:develop Nov 7, 2022
@Tom-Hirschberger
Copy link
Contributor Author

Thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants