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

Add pre-request tab and minimal execution context #7065

Merged
merged 44 commits into from
Feb 9, 2024

Conversation

jackkav
Copy link
Contributor

@jackkav jackkav commented Feb 6, 2024

inspired by: https://www.electronjs.org/docs/latest/tutorial/message-ports/#worker-process

And #6991

provides a promise based API for the insomnia renderer to send a script and request related data to a hidden browser window and receive the request data back mutated by the script.

Goals:

  • to use a similar sandboxing mechanism to insomnia renderer, where we remove require from the render and use preload to allow desired node modules.
  • use a single channel promise to perform operations.
  • add a basic ux and e2e test
  • low code mechanics to make it easier to change

todo

  • run code on another browser and return a promise
  • wire up script tab to request
  • transform request using promise
  • e2e test adding a header to a request with a script
  • errors in the script should be visible in response pane
  • console logs should appear in the timeline, either write from worker or return logs with context
    • delaying until next PR because it requires a network rewrite to make timelines append-able for http requests, eg deterministic names and newlines separated json
  • can require allow listed modules
  • add node built in crypto
  • reload hidden window on change -> remove preload / modify window-utils
  • investigate context isolation

future work

  • timeout and cancel and close()?
  • clean up listeners, requires a close event in the hidden window which might not be needed?
  • find instability triggers, test and handle them

closes INS-3380

@jackkav jackkav changed the title Worker-process-messageport Add pre-request tab and minimal execution context Feb 7, 2024
@jackkav jackkav force-pushed the worker-process-messageport branch 2 times, most recently from 412bcd2 to aa4121b Compare February 7, 2024 18:17
sandbox: true,
contextIsolation: true,
nodeIntegration: false,
contextIsolation: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is awesome to simplify parts such as message channel creation and message handling in preload. And I would propose to enable those flags if possible, as this environment is mainly for those anonymous code and you know it would be difficult for us to turn on them again :). I also spent lots of time to keep them enabled in another PR, please consider this.

Copy link
Contributor Author

@jackkav jackkav Feb 8, 2024

Choose a reason for hiding this comment

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

This is an idea for a developer experience tradeoff. I have also spent many hours writing bridges in the app to prepare for contextIsolation. Its something I want to investigate but it requires more boilerplate and I don't want to add anymore until I can deliver a more seamless dx. The faster it is to work on the more secure we can make it.
So far I enabled hot reload of changes to the hidden window index.ts on change without restarting the app. The next step is to stop vite from reloading the main browser window when we change the hidden window.
After this is will be easier to experiment with the security flags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok thanks for the push @ihexxa ive reenabled contextIsolation, and pointed the index.html directly at the index.ts. The dx is improved and only preload changes require a full reload. However i still havent stopped insomnia from reloading on hidden window changes but i found a workaround. If you logout it doesnt go back to workspace view at reload so the learning loop is faster.


invariant(mainWindow, 'mainWindow is not defined');
mainWindow.webContents.mainFrame.ipc.on('open-channel-to-hidden-browser-window', event => {
const { port1, port2 } = new MessageChannelMain();
Copy link
Contributor

Choose a reason for hiding this comment

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

We may close these ports to avoid leaking and take care of crash of the window, as they are related to stability and user experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to scope these meta aspects out into testable PRs where possible ill add it to future work

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 thanks for understanding, it also makes sense to me and let's test it later as probably it is not necessary.

@jackkav jackkav marked this pull request as ready for review February 9, 2024 00:11
gatzjames
gatzjames previously approved these changes Feb 9, 2024
@jackkav jackkav enabled auto-merge (squash) February 9, 2024 10:37
@jackkav jackkav merged commit c0707c8 into Kong:develop Feb 9, 2024
7 checks passed
@jackkav jackkav deleted the worker-process-messageport branch February 9, 2024 10:51
jackkav added a commit to jackkav/insomnia that referenced this pull request Mar 13, 2024
* works

* add preload

* promise api

* tidy

* create hash works

* return errors from worker

* basic wiring

* remove preload

* move build output to folder

* async execution

* add simple context object

* smoke test

* fix types

* fix unit tests

* remove createHash remote function

* tidy

* naming

* fix test

* add errors to preview and timeline

* basic require support

* fix types

* fix warning

* can get logs

* fix console.log patch

* logs

* fix test

* remove log

* skip mock test

* fix test

* fix types

* can write to timeline

* add watcher

* improved dx

* can console.log in the script

* simplify

* enable contextIsolation

* fix types

* use vite for hidden window build and dev

* fix type

* rename

* move pre request test to critical

* fix typo

* close ports

* unskip test
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.

4 participants