Revised content security policy and added WebViewContentType.URL for url webviews

[tjcouch-sil]

Content Security Policy Design decisions and guiding principles at https://github.com/paranext/paranext/wiki/Content-Security-Policy-Design

• Prevent extension webviews from creating iframes and a number of other tags on parent document
• Fixed problem with MutationObserver where it wouldn't remove frames if they were added under another element
• Stopped logging good things from Evil so it doesn't clog up the log so much
• Prevent extension main files from executing arbitrary code through eval and Function
• webview iframe sandbox changes
  • Made 'allow-same-origin' opt-out from WebViewDefinition
  • Made 'allow-scripts' opt-out from WebViewDefinition for HTML and React WebViews, opt-in for URL WebViews
  • Allowed WebView iframes to request fullscreen
• CSP Changes
  • Allow WebAssembly
  • Allow extensions to create iframes whose urls may be in the WebViewDefinition's allowedFrameSources (restricted to only allow urls on papi-extension: and https: at most, but defaults to none)
  • Allow images, media, and fonts from https: and data:
• Added importing images and fonts as data urls in webpack

---

This change is 

[tjcouch-sil]

Note: the MutationObserver changes seem to be fairly insignificant performance-wise. See the following logs:

Before changes to MutationObserver:

After changes to MutationObserver:

Reviewable status: 0 of 11 files reviewed, all discussions resolved

[lyonsil]

I am approving this, but I have a bunch of thoughts and suggestions to consider, too.

We are headed toward a very complicated set of security controls from what I see here. Complexity leads to holes when it comes to security. You've pointed out a number of places where there are holes, and odds are there are probably more you haven't identified.

I think we're going to end up needing to rely far more on trust between people and trust with whomever distributes extensions than our implemented security model to keep "bad" things from happening unless there are other simple, lower level controls that stop bad things from happening. I'm banking somewhat on our protocol handler to provide a place that can cut through all of this, but even that might not cover it.

Having said all of this, we still need to do things included here to "get out of the way" for partners to build extensions with capabilities they are looking for (e.g., WASM). It's probably more important to our overall success to give more capabilities to extensions than to have bulletproof security when running arbitrary code, especially at this point in time.

We're in a spot where I don't think we should give up on putting some security controls in place, but I also don't think we're equipped to build a bullet proof system. We can go really far down the rabbit hole on this, and the more complicated we make things, the harder it is going to be to really understand what we have and fix things if we run into problems.

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @tjcouch-sil)

---

extensions/src/hello-world/web-views/hello-world.web-view.tsx line 175 at r1 (raw file):

      <div className="title">
        Hello World <span className="framework">React</span>
        <img width={16} height={16} src={`${Logo}`} alt="Hello World Logo" />

Is it worth adding a comment here, too, about papi-extensions:// URLs being preferred over embedding data: URLs? I could be wrong, but I would expect people to look at our sample TSX files for guidance before they look at any webpack code/config.

---

src/renderer/index.ejs line 17 at r1 (raw file):

      default-src 'none' so things can't happen unless we allow them
      script-src-elem allows script tags but not in-line attribute scripts. That way, we can remove
        scripts we don't want using monkey-patched `document.createElement and not have to worry

Minor nit: missing a closing quote mark

---

src/renderer/index.ejs line 20 at r1 (raw file):

        about in-line scripts
        'self' so scripts can be loaded from us
        'wasm-unsafe-eval' because webview iframes want to use wasm

We could only grant wasm-unsafe-eval and unsafe-inline to "trusted" extensions like we intend to do with fs and fetch.

---

src/shared/data/web-view.model.ts line 134 at r1 (raw file):

   * It is best practice to set this to `false` where possible.
   *
   * Note: Until we have a message-passing APi for WebViews, there is currently no way to

Minor nit: the lowercase i should be capitalized in API.

---

src/shared/data/web-view.model.ts line 135 at r1 (raw file):

   *
   * Note: Until we have a message-passing APi for WebViews, there is currently no way to
   * interact with the platform via a WebView with `allowSameOrigin: false`.

Let's assume we have a message passing API in place and someone sets this to false. The webview won't fully know who is talking to it, right? So this would still have to expose/share a token for authentication, I think, just not the raw credentials themselves. Or maybe we're thinking of a scenario where someone unknown passes a token to the "secure" webview. That uses the token to prompt the user in some way (e.g., "If this is an expected request, please approve it here." Kind of like how stream services authenticate apps.

---

src/shared/data/web-view.model.ts line 150 at r1 (raw file):

   * is not necessary to run scripts in your WebView, you should set this to `false` to reduce risk.
   */
  // This does not follow our normal pattern of naming booleans because it mirrors the

I don't think we need this comment. There are enough other details and comments to worry about, and this is a super minor point.

---

src/shared/data/web-view.model.ts line 163 at r1 (raw file):

   * attribute in your webview, you must include them in this property.
   *
   * For example, if you specify `allowFrameSources: ['https://example.com/']`, you will be able to

Is this and the RegExp below the only places we expect extensions to list specific URLs for white listing? We'll need something in the manifest to use in our protocol handler, too, right?

[tjcouch-sil]

Yeah, this is hard and very complex security stuff for sure. I appreciate that concern as I see more holes all the time. I don't know what else to do about it right now. We can discuss on a call if you want, but I also think this is worth getting in for now and maybe dreaming up something for next time.

Reviewable status: 4 of 12 files reviewed, 4 unresolved discussions (waiting on @lyonsil)

---

extensions/src/hello-world/web-views/hello-world.web-view.tsx line 175 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Is it worth adding a comment here, too, about papi-extensions:// URLs being preferred over embedding data: URLs? I could be wrong, but I would expect people to look at our sample TSX files for guidance before they look at any webpack code/config.

Done. Sounds good :)

---

src/renderer/index.ejs line 17 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Minor nit: missing a closing quote mark

Done. Thanks!

---

src/renderer/index.ejs line 20 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

We could only grant wasm-unsafe-eval and unsafe-inline to "trusted" extensions like we intend to do with fs and fetch.

Sorry, this comment about fixing this is outdated, and I had already fixed it this morning but just hadn't pushed yet. I have updated the comment. Do you still have concerns? We can talk in a call if so.

---

src/shared/data/web-view.model.ts line 135 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Let's assume we have a message passing API in place and someone sets this to false. The webview won't fully know who is talking to it, right? So this would still have to expose/share a token for authentication, I think, just not the raw credentials themselves. Or maybe we're thinking of a scenario where someone unknown passes a token to the "secure" webview. That uses the token to prompt the user in some way (e.g., "If this is an expected request, please approve it here." Kind of like how stream services authenticate apps.

Sorry, I'm not following on everything you're saying here. I'll explain what I had in mind real quick, and maybe we can follow up with a call if you'd like.

The idea I had was that we would handle webview message posting by routing the messages to and from the web view provider so only the web view provider could communicate through message passing. Not sure exactly what this looks like yet, but it seems reasonably feasible.

---

src/shared/data/web-view.model.ts line 150 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I don't think we need this comment. There are enough other details and comments to worry about, and this is a super minor point.

Ok. I took it out because it does not comply with our style guide and we require comments explaining why not in situations like this, but I'm happy not having it as well. I just figured someone would probably notice and mark it in the code review, so I just threw it in.

---

src/shared/data/web-view.model.ts line 163 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Is this and the RegExp below the only places we expect extensions to list specific URLs for white listing? We'll need something in the manifest to use in our protocol handler, too, right?

I'm not sure we will be able to tell in the protocol handler that something is a request to get an iframe's source, so I'm not sure this is relevant to the protocol handler. If you'd like to discuss further on a call, let me know :)

[tjcouch-sil]

I do think especially bullet 2 in #89 regarding getting the webviews served separately on the same origin so they can have a different CSP will help some. Won't fix everything, but it will help some, especially since it will theoretically let us remove unsafe-inline from the renderer CSP.

Reviewable status: 4 of 12 files reviewed, 3 unresolved discussions (waiting on @lyonsil)

[lyonsil]

Speaking very high level about security (i.e., not about this PR specifically):

• We are playing an asymmetric game here. If someone wants to do something bad, they have to find 1 hole. If we want to prevent something bad, we have to have 0 holes.
• We are working in a system that has had over a thousand security vulnerabilities reported publicly over the last 10 years. Even very large companies are always playing catch up.
• We can't simply ignore security, but given the scope of the problem we can always find "one more thing" to do for security in the code. From a time standpoint, it can be a bottomless pit without ever achieving the necessary goal of 0 holes if we want to be "fully secure."
• It is a common problem for security to get in the way of providing more functionality that users want. I know we're stepping back from that here (e.g., wasm) which is why I'm explicitly not blocking anything in this PR.
• I've generally been advocating that we should change the rules of the game and focus on trust between people as a basis for security more than perfect coding. That to me seems far more achievable and considerate of our resources.

Overall, I worry that the more we go after every little hole we can find, the more we get sucked into the "trying to produce perfect code" trap with security. It doesn't feel like we're all rowing in the same direction here.

Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tjcouch-sil)

---

src/shared/data/web-view.model.ts line 135 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Sorry, I'm not following on everything you're saying here. I'll explain what I had in mind real quick, and maybe we can follow up with a call if you'd like.

The idea I had was that we would handle webview message posting by routing the messages to and from the web view provider so only the web view provider could communicate through message passing. Not sure exactly what this looks like yet, but it seems reasonably feasible.

I thought the idea was message passing between webviews. If we're talking about message passing with the web view provider then that is not a concern. Somehow the unsafe things will have to talk with something safe, though, and we may not really know what the unsafe thing is that we're talking to.

---

src/shared/data/web-view.model.ts line 163 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I'm not sure we will be able to tell in the protocol handler that something is a request to get an iframe's source, so I'm not sure this is relevant to the protocol handler. If you'd like to discuss further on a call, let me know :)

I agree. I don't think the protocol handler will have any idea what the source of a request is that it sees. I wasn't sure if we were going to try to do any whitelisting in the protocol handler or if it's just for an on/off switch for offline mode. For some reason I was thinking the protocol handler would be involved when we add it later, but maybe not.

This comment is not blocking. We can talk more about it separately from the PR. It is more about thoughts for future work that is potentially, but not necessarily, related to what you are adding here.

[tjcouch-sil]

Yes, it seems worth considering more. I share your concerns about the asymmetric game of patching holes. I believe the organization-level verification of extensions will contribute to the trust factor, but it wouldn't hurt to consider further :)

Reviewable status: complete! all files reviewed, all discussions resolved

---

src/shared/data/web-view.model.ts line 135 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I thought the idea was message passing between webviews. If we're talking about message passing with the web view provider then that is not a concern. Somehow the unsafe things will have to talk with something safe, though, and we may not really know what the unsafe thing is that we're talking to.

Indeed. It is worth considering further. Filed #616 :)

---

src/shared/data/web-view.model.ts line 163 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I agree. I don't think the protocol handler will have any idea what the source of a request is that it sees. I wasn't sure if we were going to try to do any whitelisting in the protocol handler or if it's just for an on/off switch for offline mode. For some reason I was thinking the protocol handler would be involved when we add it later, but maybe not.

This comment is not blocking. We can talk more about it separately from the PR. It is more about thoughts for future work that is potentially, but not necessarily, related to what you are adding here.

I think there is an idea to create a blacklist in the protocol handler, but that would be an overall blacklist. Maybe also a whitelist if people want to be secure but still have some access? Something to consider another time. :)