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

fix: override csp for frame-src #50

Merged
merged 10 commits into from
May 16, 2019
Merged

fix: override csp for frame-src #50

merged 10 commits into from
May 16, 2019

Conversation

hugomano
Copy link
Contributor

@hugomano hugomano commented Apr 16, 2019

What is this PR

  • Fixed a bug about CSP: trusted protocol chrome-extension:// is not bypassed in CSP check at the iframe document level, so we add it to the frame-src to the directive in the Content Security Policy header
    More here
  • Revamped the protocol handler and added verification for web accessible resources

How to test

  • CSP

  • before this PR: log in Gmail with a @gmail.com account, onboarding iframe is not displayed.

  • after this PR: log in Gmail with a @gmail.com account, onboarding iframe is displayed

  • Protocol Handler
    In the Gmail dev tool with the Mixmax contex console:

  • fetch('chrome-extension://ocpljaamllnldhepankaeljmeeeghnid/manifest.json', { mode: 'no-cors' }).then(console.log) response is net::FAILED

  • fetch('chrome-extension://ocpljaamllnldhepankaeljmeeeghnid/src/iframeProxy/proxy.html', { mode: 'no-cors' }).then(console.log) response is HTTP 200

  • fetch('chrome-extension://ocpljaamllnldhepankaeljmeeeghnid/src/iframeProxy/proxy.js', { mode: 'no-cors' }).then(console.log) response is HTTP 200

@alexstrat
Copy link
Contributor

👍
As discussed, this should be only on the resources listed as web_accessible_resources

@hugomano hugomano mentioned this pull request Apr 24, 2019
# Conflicts:
#	package.json
@hugomano
Copy link
Contributor Author

hugomano commented May 9, 2019

@hugomano hugomano changed the title explo: add access-control-allow-origin to accessible resources fix: override csp for frame-src May 10, 2019
// Regex to capture the src attribute value for scrips, stylesheets...
const resourceLinkRegex = /src\s*=\s*['"](.+?)['"]/g;

const captureWhitelistableResourcesFromProtocolServedFile = (pathname: string) =>
Copy link
Contributor

@alexstrat alexstrat May 15, 2019

Choose a reason for hiding this comment

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

Woo this is quite complex, but I think I did understand it. Few questions:

Allowed HTML pages can include extension assets that don't need to be whitelisted.

Are you sure it is necessary to do that? I see no reference of such practice in the documentation?

How does Chromium handle that?

Since we didn't know the origin of the requests

Just in Electron, or even Chromium does not know?

Copy link
Contributor Author

@hugomano hugomano May 16, 2019

Choose a reason for hiding this comment

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

Are you sure it is necessary to do that? I see no reference of such practice in the documentation?

"A navigation from a web origin to an extension resource will be blocked unless the resource is listed as web accessible". So background page, popups and iFrames with chrome-extension:// protocol bypass check.

How does Chromium handle that?

https://cs.chromium.org/chromium/src/extensions/browser/extension_protocols.cc?rcl=4292bebbd8388070fc8718bb54d793b6f36fe4a6&l=391

Just in Electron, or even Chromium does not know?

We don't know the origin of the requests only in Electron: referrer is always blank (issue: electron/electron#14747). Chromium knows the request context.

--

I added all answers in comments belong to the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But as discussed yesterday, we should drop the check for now.
There is 3 reasons: we whitelist installable extensions so we can trust them, we resolve protocol request in the extension folder only at the user filesystem level and this code block may not cover all cases.

Copy link
Contributor

@alexstrat alexstrat May 16, 2019

Choose a reason for hiding this comment

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

Detail: I don't think electron/electron#14747 is the right issue for this. referrer is an HTTP header that can be overridden in some situation and is the frame URL's domain in another situation. We are rather looking for a strong reference to the frame / webContents from which the request came.

@hugomano hugomano merged commit 376b062 into master May 16, 2019
@hugomano hugomano deleted the explo/mixmax-csp branch May 16, 2019 15:32
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.

2 participants