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 support for container tabs #218

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aaronkollasch
Copy link

@aaronkollasch aaronkollasch commented Nov 6, 2021

This PR adds support for Firefox containers (#5, #166).

  • Adds the option to use separate policies for each new container. Container policies initially copy the default policy. Container tab support is disabled by default.
  • The current tab's container permissions can be edited from the NoScript popup.
  • All containers are editable in the "NoScript Options" page. Container permissions can be copied between containers or cleared. Exporting and importing are supported.
  • Adds a ContextStore in nscl to store policies for each container.
  • No changes to the filtering backend, except to make src/bg/RequestGuard.js use the correct policy for each request.
  • No changes to the UI if container tabs are disabled or the contextualIdentities API is not supported, e.g. in Chrome (except for an added option to enable/disable container tabs).

@jtotht
Copy link

jtotht commented Nov 7, 2021

Container policies are empty by default.

Does this mean that if I have set a site trusted, it won’t be trusted anymore in containers? While being able to configure different policies per container is a nice feature, I think in most cases if one trusts a site, and then it trusts it in all containers, the containerless policies should continue to apply in containers unless specifically overridden.

@aaronkollasch
Copy link
Author

Thanks, you make a good point. The reason why I had it that way is that the default permissions list is quite long, and it can be laborious to clear the list for every container if desired. (Some users' containers can number in the 10's or more)

Using the containerless policy is the better default, true. Perhaps I could add a "clear" button to the Per-site Permissions tab, which clears the selected container's rules (after a confirmation prompt).

@aaronkollasch
Copy link
Author

Just added a commit on default container policies. Whenever a new container is created, it will now take on the policies of the default container. However, changes made to any container's policy will not propagate to other containers. I think that is a good balance.

Note that this is more permissive than plugins like Cookie AutoDelete which default to blocking everything for each new container. Since NoScript has a default allowlist, I suppose it makes sense to propagate it to new containers, but it could be unexpected behavior for some users.

@aaronkollasch aaronkollasch force-pushed the container-tabs branch 4 times, most recently from a5e67a6 to cf84091 Compare November 13, 2021 20:33
@jtotht
Copy link

jtotht commented Nov 13, 2021

The reason why I had it that way is that the default permissions list is quite long, and it can be laborious to clear the list for every container if desired.

Just added a commit on default container policies. Whenever a new container is created, it will now take on the policies of the default container. However, changes made to any container's policy will not propagate to other containers. I think that is a good balance.

It can be laborious the other way round as well: I have for example half a dozen containers for Google stuff, and Google has quite some domains (*.google.com, gstatic.com, googlevideo.com, youtube.com, blogger.com etc., let alone country-specific domains like google.co.uk). If Google decides to load things from yet another domain, say alphabet.com, I have to go through each container to add it. If I trust Google won’t load viruses in container A, it won’t load them in container B either.

@aaronkollasch
Copy link
Author

Yeah it can get laborious. Right now the best way to copy configs across containers is to edit the ContextStore json from the debug menu.

I could imagine defining a set of profiles ("Google", etc.) and assigning containers (firefox-container-1, firefox-container-2, etc.) to different profiles. However, this seems needlessly complicated.

Probably a better way is to add a "copy profile from..." dropdown in the options that copies the profile from a selected container. Would that work for you?

There is the question of what to copy – ideally it would append/update rather than replace the current profile's settings, but this makes things tricky in terms of managing conflicts. Perhaps it should merge the site list and replace all other settings.

@aaronkollasch
Copy link
Author

I added a selector in options.js to copy another container's profile to the currently viewed container. The new profile just replaces the current profile right now. It's possible to have the site lists merge/update, but that could be needless complexity.

There is another issue with the settings: each container has a separate Policy instance and thus its own preset customizations. I don't think there's a need to have different presets for each container, so ideally the containers should mirror the default container's presets. I just need to find a good way to make that happen.

@aaronkollasch
Copy link
Author

I reverted commit aaronkollasch@8d8bb4c which switched ns.computeChildPolicy() to get the tab's cookieStoreId from the TabCache, allowing a browser API call to be removed from RequestGuard.injectPolicyScript().

This caused reliability issues as the TabCache didn't always have the tabId in question, so ns.computeChildPolicy() would use the default policy instead of the container's policy. I'd like to use the TabCache but only if I can get it to work reliably.

Perhaps I could add a TabCache update or browser.tabs.get(tabId) fallback if tabId is not in the TabCache. This would require ns.computeChildPolicy() to be an async function, though.

@aaronkollasch
Copy link
Author

aaronkollasch commented Dec 6, 2021

Instead of getting the cookieStoreId from the TabCache or browser.tabs.get, it is now passed from nscl/ service/DocStartInjection.js if computeChildPolicy is called from injectPolicyScript.

The current tab or TabCache are used if computeChildPolicy called through fetchChildPolicy and the sender does not have a cookieStoreId.

@aaronkollasch aaronkollasch force-pushed the container-tabs branch 2 times, most recently from 3522ca6 to 4c451bb Compare December 6, 2021 23:44
@aaronkollasch
Copy link
Author

I've covered the features I can think of, so I'm going to mark this as ready for review.

Some to-do's remaining are:

  • Add internationalization:
    • Button labels in ui/options.html:
      • lbl-containers
      • select-container-label
      • copy-container-label
      • btn-clear-container
    • The name of the default container ("Default"):
      • in ui/popup.html container-id
      • in ui/popup.js
      • in ui/options.html select-container and copy-container
      • in ui/options.js:updateContainerOptions()
    • The alert message for ui/options.js:copyContainer()
    • The alert message for #btn-clear-container
  • Revert .gitmodules after merging nscl

Also, as @jtotht mentioned earlier, adding the contextualIdentities permission enables the Container Tabs feature for all installs, even for users who don't turn on container tab support. This is not ideal, but I think the cost is small if the feature isn't used.

I've been daily driving NoScript with these changes for about a month now, and can say I've found container-specific permissions really useful, as there are a bunch of sites I use but don't want to allow everywhere. So I now find myself reflexively clicking the disable restrictions button less often :)

@aaronkollasch aaronkollasch marked this pull request as ready for review December 11, 2021 07:54
@aaronkollasch aaronkollasch force-pushed the container-tabs branch 2 times, most recently from 15fd4f6 to bda83f4 Compare April 6, 2022 06:58
@aaronkollasch
Copy link
Author

Just rebased on main – I merged all commits to make rebasing easier. The original pull request's commit history, based on 11.2.12rc3 (4cfdebd), is now saved in a separate branch: container-tabs-original and its nscl submodule.

@craigcomstock
Copy link

@aaronkollasch just tried this and so far a simple test seems to do what I was looking for: trust domains per container. Sweet! Let me know if I can help with testing or development.

@Hoeze
Copy link

Hoeze commented Mar 15, 2023

Can this be merged?

@kgkgn
Copy link

kgkgn commented Jan 28, 2024

Any chance we can maybe push this to be reviewed for potential merging? Been following this PR for a while and would love to see it gets merged as I use both container tabs and noscript fairly heavily, and would love to see them working together ;)))

Anything I can do?

@craigcomstock
Copy link

craigcomstock commented Feb 7, 2024

I switched to a multi-instance multi-profile method of working. Made a script to manage it, save cookies in my dotfiles git repo ($HOME/.gitignore = '*'). Been working pretty good. :) Easily shared between linux and macos at least.

$HOME/bin/website:

if [ "$1" = "-l" ]; then
  ls $HOME/website
  exit 0
fi
if [ "$1" = "-s" ]; then # save
  cd $HOME
  for d in $HOME/website/*; do
    git add -f $d/cookies.sqlite
  done
  cd -
  exit 0
fi
if command -v firefox; then
  ff=$(which firefox)
elif command -v firefox-esr; then
  ff=firefox-esr
else
  ff=/Applications/Firefox.app/Contents/MacOS/firefox
fi
# redacted use of pass command to find the password for the site :)
$ff --profile $HOME/website/$1 https://$1  &

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.

None yet

5 participants