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

Change Electron detection to be user agent based only #645

Merged
merged 1 commit into from Nov 8, 2019

Conversation

@Calyhre
Copy link
Contributor

Calyhre commented Nov 8, 2019

The Electron detection is currently base on the presence of window.process OR the user agent string.

This works fine for the mxIsElectron, but not for the Electron > 5 check as then we assume window.process is present. This is false for webviews and iframes. Draw.io is currently crashing if used inside an iframe, inside an Electron app.

This PR change the Electron check to be based solely on the user agent string.

@davidjgraph

This comment has been minimized.

Copy link
Contributor

davidjgraph commented Nov 8, 2019

What about a defensive check on the electron 5 line for process? We've been bitten by useragent checks before...

@Calyhre

This comment has been minimized.

Copy link
Contributor Author

Calyhre commented Nov 8, 2019

I didn't dig this whole things up, but isn't the Electron check just to see if the app is loaded in drawio-desktop? If so, then maybe we should fail the check at all if window.process is absent. If the app is loaded in an iframe there is no need to know about it, as it won't be in the desktop app.

@Calyhre Calyhre force-pushed the Calyhre:fix/electron-detection branch from 4282c6d to 4002057 Nov 8, 2019
@Calyhre

This comment has been minimized.

Copy link
Contributor Author

Calyhre commented Nov 8, 2019

I think it is, later the imports and URL handling are changed depending of this detection. There is no point of doing this if loaded from an iframe. I changed the check to just detect the presence of process.

@davidjgraph

This comment has been minimized.

Copy link
Contributor

davidjgraph commented Nov 8, 2019

OK, I see your point. I'm just thinking about an electron app with node integration enabled that loads draw.io in a web view. We've a lot of security controls based on the electron setting. I'll pull this change and leave this note here as a warning to future readers:

If you load draw.io in an iFrame or WebView (I think process isn't there when node is disabled), you need to look at the places where we tighten security based on this flag. If you have the node integration enabled, you must not load the draw.io site as is. Third-party JS is loaded for some of the cloud integrations and if they were compromised the app could gain access to the local filesystem.

@davidjgraph davidjgraph merged commit e71a6a7 into jgraph:master Nov 8, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@Calyhre Calyhre deleted the Calyhre:fix/electron-detection branch Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.