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

Client can be mis-configured due to config and boot-script injection being non-atomic #1179

Closed
robertknight opened this issue Apr 6, 2023 · 3 comments

Comments

@robertknight
Copy link
Member

robertknight commented Apr 6, 2023

When testing changes to the integration between bouncer (hyp.is) and the extension locally, I encountered a situation where the development extension would sometimes be activated in the toolbar yet the client showed annotations from the production instance of h.

After adding some logging I realized the issue is that the extension injects the client's configuration (which tells it where to load resources from) in a separate step to starting the client, and that it is possible for the tab to navigate in-between these steps. When this happens the client can end up starting without the configuration being present on the page, causing it to load assets from the wrong location.

The sequence of events when this happens was:

  1. Bouncer page A loads
  2. Bouncer page A sends a message to the extension and asks it activate itself and navigate to URL B
  3. The extension attempts to activate itself in the tab before the navigation has started (SidebarInjector.injectIntoTab)
  4. The extension injects a configuration <script> tag into the page (start of injectIntoRemoteDocument function)
  5. The tab navigates from A to B
  6. The extension injects the client's boot script into the page (next step of injectIntoRemoteDocument function)
  7. The extension gets a notification about the navigation from A to B having happened.
  8. The extension re-injects the client configuration
  9. The extension re-injects the client's boot script

Client startup logging

In this sequence, steps 4 and 6 are different steps of the same async function in injectIntoRemoteDocument, and are performed in the same browser tab, but they end up being executed in different documents since a navigation is happening. As a result when the client loads in step 6, it doesn't find its configuration and ends up loading with the default production configuration. Step 8 has no effect, since it happens after the client's boot script has already loaded. Step 9 has no effect, since the boot script will detect the presence of another Hypothesis client in the page and bail out.

Although this situation involved some local changes to the bouncer, I believe the same outcome is possible in normal usage if the page triggers a navigation at the same time as the extension is activated.

Possible solutions

  1. Somehow make steps 4 and 6 above a single atomic operation which are guaranteed to execute in the same document
  2. Allow the re-injected client in step 9 to remove and replace the client injected in step 6
  3. Make step 6 abort if the client configuration is not found in the page
@robertknight
Copy link
Member Author

robertknight commented Apr 6, 2023

Chrome 106+ (Sep 2022) provides a general solution for this problem, which is to specify a document ID as the target rather than just a tab ID when executing a script via chrome.scripting.executeScript. When a navigation occurs, the tab ID will remain the same, and the main frame ID is always 0, but the document ID should change.

As well as not being available in older Chrome releases, document IDs are also not currently available in other browsers that support Web Extensions.

@robertknight
Copy link
Member Author

robertknight commented Apr 6, 2023

See also w3c/webextensions#8 and this Chrome design doc.

@acelaya
Copy link
Contributor

acelaya commented Apr 19, 2023

After some tests on the different suggestions, we have finally addressed this by making sure the client's boot script aborts when it detects it has been executed by the browser extension, but it does not find a config script generated by that extension (it would be the third suggested option).

The first naive attempt was to make sure the boot script "exits" if there's no config at all and the extension executed it: hypothesis/client#5392

If the page has other "hardcoded" configs, this could result in a false positive, so we improved it by:

@acelaya acelaya closed this as completed Apr 19, 2023
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

No branches or pull requests

2 participants