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

Shim internet access out of extensions on extension host and in web view #93

Merged
merged 11 commits into from
Mar 20, 2023

Conversation

tjcouch-sil
Copy link
Member

@tjcouch-sil tjcouch-sil commented Mar 20, 2023

  • Added InternetService to have a place where extensions can go for internet access
  • Changed React webview to a createRoot in an iframe to cut off access to renderer window context
    • Unfortunately, this means React webviews no longer have natural access to our React context
  • Renamed WebviewTypes.ts to WebViewTypes.ts - we should probably decide if we want it to be called WebView or Webview
  • Added content security policies and iframe sandboxing
  • Bumped nvmrc to 18.0.0 to match the introduction of the Node fetch API

Created some issues as follow-ups from this one:
#85
#86
#87
#88
#89
#90
#91

Resolves #49


This change is Reviewable

@tjcouch-sil tjcouch-sil enabled auto-merge March 20, 2023 15:03
Copy link
Contributor

@FoolRunning FoolRunning left a comment

Choose a reason for hiding this comment

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

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


src/extension-host/services/ExtensionService.ts line 146 at r1 (raw file):

    // Disallow any imports within the extension
    // TODO: make this throw so the extension knows what's going on
    // Tell the extension dev if there is an api similar to what they want to import

Is there a reason this wasn't coded as a throw? It would make much more sense. I would expect the current behavior of getting something back from a require call the doesn't contain any of the expected objects/properties would be much more confusing.


src/extension-host/services/ExtensionService.ts line 162 at r1 (raw file):

  globalThis.fetch = () => {
    throw Error('Cannot use fetch! Try using papi.fetch');
  };

I think the shimming should be done consistently (i.e. either using an anonymous function or using a named function as the other two below). Unless there is some subtle difference between the two and is thus a reason to do it the way it was done.


src/shared/services/InternetService.ts line 7 at r1 (raw file):

// TODO: use some more universal cached version of fetch in paranext-core#75
const fetchOriginal = globalThis.fetch;

Is there some kind of guarantee that the code in ExtensionService.ts that replaces fetch won't be run first? Otherwise, this seems very fragile.


src/shared/data/WebViewTypes.ts line 54 at r2 (raw file):

/** Base WebView properties that all WebViews share */
type WebViewContentsBase = { contents: string; title?: string };

Is contents always a string now because passing a ReactNode isn't feasible anymore?

Copy link
Member Author

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 19 of 21 files reviewed, 4 unresolved discussions (waiting on @FoolRunning)


src/extension-host/services/ExtensionService.ts line 146 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Is there a reason this wasn't coded as a throw? It would make much more sense. I would expect the current behavior of getting something back from a require call the doesn't contain any of the expected objects/properties would be much more confusing.

I think I just didn't want to have to handle the exceptions in the extensions. Thanks for calling out lazy TJ. Made a new evil extension that does bad things and removed the bad things from the other extensions.


src/extension-host/services/ExtensionService.ts line 162 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

I think the shimming should be done consistently (i.e. either using an anonymous function or using a named function as the other two below). Unless there is some subtle difference between the two and is thus a reason to do it the way it was done.

Done. I did the others as named functions because you can't use an arrow function as a constructor, but I didn't think to change this one. Thanks!


src/shared/services/InternetService.ts line 7 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Is there some kind of guarantee that the code in ExtensionService.ts that replaces fetch won't be run first? Otherwise, this seems very fragile.

Actually, ExtensionService.ts doesn't fully shim out the globals permanently right now. #75 addresses that issue. But regardless, this code runs before the extension service starts doing its thing because this code happens on import (imports happen before all other code and are even hoisted such that they have to run before other code), whereas the ExtensionService doesn't start doing stuff until its initialize function is executed. But the long-term answer is that we need something better in #75 :)


src/shared/data/WebViewTypes.ts line 54 at r2 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Is contents always a string now because passing a ReactNode isn't feasible anymore?

Yep! No extension code is being run on the renderer window anymore; it's all passed down into the iframe. Before, I was reading the code and running it in the renderer to get a React node, but now we have to pass the component code down as a string.

Copy link
Contributor

@FoolRunning FoolRunning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

Copy link
Contributor

@FoolRunning FoolRunning left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)


src/shared/data/WebViewTypes.ts line 54 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Yep! No extension code is being run on the renderer window anymore; it's all passed down into the iframe. Before, I was reading the code and running it in the renderer to get a React node, but now we have to pass the component code down as a string.

Sounds like a pain, but I guess it's what we have to do. :(

Copy link
Contributor

@FoolRunning FoolRunning left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @tjcouch-sil)

@tjcouch-sil tjcouch-sil merged commit eb012b5 into main Mar 20, 2023
@tjcouch-sil tjcouch-sil deleted the 49-shim-internet branch March 20, 2023 20:38
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.

Create internet access service and shim internet access out
2 participants