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

Wildcard subdomains v2 - e.g. *.google.com #2352

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

Conversation

mckenfra
Copy link
Contributor

@mckenfra mckenfra commented May 4, 2022

See pull request #1500 and issue #473

This is a second attempt at implementing a wildcard subdomains feature. This time round, I have tried to keep the amount of code that is changing to the bare minimum. This should make code review easier and minimise any risk of introducing regressions. We can potentially cancel the previous pull request if this one looks better.

MAC.mov

How the code works

When you click the www part of www.google.com in the popup, a wildcardHostname property with value google.com is added to the stored siteSettings object for www.google.com in assignManager.js See code here

If you then open a tab and navigate to mail.google.com, we need the code to find the matching google.com wildcard hostname stored in the www.google.com site. This is achieved by storing an additional map of wildcardHostname values to siteStoreKeys. See code here

So when handling the request for mail.google.com, the wildcard hostname map is queried to see if there is a mapping for mail.google.com, then google.com (and then theoretically com). It finds that google.com is mapped to www.google.com, and so uses the www.google.com site settings to handle the request. See code here

Syncing

The sync feature should just work as expected, since the new wildcardHostname property of siteSettings will be synced along with all the other properties of the siteSettings object.

Try it out

I have put up a temporary build here for you to try out the feature without having to build it yourself. Let me know if it looks good or if you find any issues.

I have also included unit tests in this pull request.

@Nomes77
Copy link

Nomes77 commented May 4, 2022

@mckenfra I can't thank you a 1000 times for your work!!
I have tested it works as needed. Thank you so much!!

@Nomes77
Copy link

Nomes77 commented May 4, 2022

@groovecoder can you please review and accept this PR? It is working great.

@krisnrg
Copy link

krisnrg commented May 4, 2022

Will this work with multi-level subdomains?

I wonder if it will do what I've been searching for.

I use these containers for local, staging and live sites. Mainly cause they have an elegant line on the tabs.

But I always have to select. "Always open site in..." so it remembers the specific client site.

Does the following work:
Automatically open in the right container
staging container: any-client.staging.main-domain.com
live container: any-client.main-domain.com
local container: any-client.localhost:1234 (whatever port)

@mckenfra
Copy link
Contributor Author

mckenfra commented May 4, 2022

@krisnrg It should work for your use case, except for the port. I haven't added any support for having a "wildcard" port. Would you want that?

Maybe you can try installing the proof-of-concept "wildcard subdomains" build and see if it does what you need. You can download it here

@krisnrg
Copy link

krisnrg commented May 4, 2022

No wildcard is needed for the ports no. For me, it's always the same port. This is a God-sent!

I happened to come here today to see if the code was something I can edit and modify for my needs. Little did I know there would be a PR today lol

Thanks for the hard work.

I'll prolly try it today after I get free.

@dannycolin
Copy link
Collaborator

dannycolin commented May 11, 2022

First of all, @mckenfra thanks a lot for reviving this idea. I'd definitely like to see this feature implemented in MAC. However, I have a few UX concerns over this implementation.

  1. It isn't clear what happens by clicking on the www for all users depending on their technical knowledge about domains.
  2. Using only colors to indicate the state is a big no no in term of web accessibility.

For 1, you could explore a solution that use a tooltip when you hover over the www. We could also have a short message at the top of the site list that is only shown if the user never used the wildcard feature. Something like, "Hey did you know you could ...," with a button that opens the documentation with a more in-depth how-to.

For 2, we could change the www part to * or [all] that'd be even more clear for those who don't know the meaning of the asterix.

@bruvv
Copy link

bruvv commented May 11, 2022

Totally liking that 2e part, will make a lot more sense! love to see this merged with the changes purposed by @dannycolin

@Nomes77
Copy link

Nomes77 commented May 11, 2022

I support:

For 2, we could change the www part to * or [all] that'd be even more clear for those who don't know the meaning of the asterix.

But for 1, I see personally no need, because I thing that most people who are aware of privacy do have technical knowledge about domains.

@dannycolin
Copy link
Collaborator

But for 1, I see personally no need, because I thing that most people who are aware of privacy do have technical knowledge about domains.

For Mozilla, privacy is for everyone. It's at the core of Mozilla's manifesto. Empowering users, whatever their level of knowledge, is something we should strive to especially when it's very easy to implement like the proposition I made. It's even make more sense to do that small extra step since it's an addon managed by Mozilla itself. If it was a third-party addon, I'd be less concerned.

@mckenfra
Copy link
Contributor Author

@dannycolin Thanks a lot for your feedback 👍 Regarding your suggestion no. 2, see video below. Is this better?

Wildcard2.mov

@bruvv
Copy link

bruvv commented May 13, 2022

I would more do it with a . in between too:
*.google.com
And perhaps make it not red that suggest something bad is happening

@mckenfra
Copy link
Contributor Author

@bruvv In the current implementation of this pull request, *.google.com will match both www.google.com and google.com (i.e. without any leading dot or subdomain).

It will not match www.notgoogle.com or notgoogle.com of course!

Is that the behaviour you would expect?

@bruvv
Copy link

bruvv commented May 13, 2022

image
this suggest me that anything can be matched before google.co.uk so I think people will mistake it and can be lead to confusing. Will it also match thisisgoogle.co.uk and or thisisnotgoogle.co.uk? if you add a . (dot) before the main domain it suggest that only subdomains will be matched like www.google.co.uk and or mail.google.co.uk etc.

so it should be *.google.co.uk so the implementation is perfect, the visualization is what I am talking about.

@Nomes77
Copy link

Nomes77 commented May 13, 2022

@mckenfra to respond on your last question i like hat behavior.
For instance when you have https://gab.com and hhtps://media.gab.com only one rule is needed.

@Nomes77
Copy link

Nomes77 commented May 13, 2022

@bruvv

Will it also match thisisgoogle.co.uk and or thisisnotgoogle.co.uk?

No

@mckenfra
Copy link
Contributor Author

@bruvv Yes I take your point about adding the leading dot. Yes the behaviour is that *.google.co.uk will also match google.co.uk, but not thisisgoogle.co.uk or thisisnotgoogle.co.uk.

@mckenfra
Copy link
Contributor Author

And perhaps make it not red that suggest something bad is happening

@bruvv The red colour was used so that it stands out clearly, in case people click the subdomain accidentally... but I will be happy to change this to a different colour if you can suggest one? Maybe blue? Or the same colour as the container icon for that particular container?

@bruvv
Copy link

bruvv commented May 13, 2022

I was talking about the visualization not the functionalist. Functional it is perfect, visually it is just missing the . and I agree same colour as the container icon is perfect!

@mckenfra
Copy link
Contributor Author

See below a screencast showing the following changes:

  1. extra dot after star icon
  2. star icon background colour matches container icon colour

(I've edited the video to remove the steps where I changed the container icon colour each time)

Wildcard3.mov

@bruvv
Copy link

bruvv commented May 13, 2022

@mckenfra awesome! I love it. Now hopefully this can be merged.

@mckenfra
Copy link
Contributor Author

@bruvv Great! If there is general consensus on the above design, I'll update the PR to include these UI changes.

@Nomes77
Copy link

Nomes77 commented May 13, 2022

@mckenfra the last design is even better then your first proposal. I agree with it.

@bruvv
Copy link

bruvv commented May 16, 2022

Hopefully @dannycolin agrees too (shameless ping :P )

… far fewer extra async storage requests (if any)
@bruvv
Copy link

bruvv commented May 20, 2022

@bakulf can you review this?

@dannycolin
Copy link
Collaborator

Hey folks,

Since a lot of you have pinged the developers for a review, I just wanted to give you a quick updates on why it's taking more time then initially planned to get this reviewed and merged.

Firstl, we're working on a fast and robust way to detect the domain TLDs based on the publicsuffixlist. This takes more time because:

  1. we need a way that makes sure we keep in sync with the upstream list to avoid any mismatches that could have confusing consequences in terms of where cookies exist and/or get shared.
  2. the solutions we're exploring may need WASM or directly talking to the Firefox codebase

I won't go more into details but I'm sure you can appreciate that this solutions aren't simple as copy/pasting code in the repo :).

Second, we needed to work on the UX of this feature to make sure it's as easy as possible to use and so to limit the chance a user shoot themselve in the foot by setting it wrong because the UI was confusing. I'm sharing below a screenshot of what it could look like.

Keep in mind that it may not cover all the use cases. This is done on purpose. We will most likely implement a basic feature that do something like "Enable everything or nothing from domain X". This doesn't mean that more advanced features won't be implemented but simply that we prefer to do it incrementally so it has more chances the maintainers of this addon will accept to merge it.

Frame 1

@Nomes77
Copy link

Nomes77 commented Jun 9, 2022

@mckenfra Just a quick question.
Currently, when using your code when I add gab.com (without www. in front of it) to a container
and then go to media.gab.com the subdomain isn't assigned.
So I have always to make double rules for one website. (gab.com and ★.gab.com)
In may opinion this is really annoying.
How can I change your code to change that behaviour?

I think it has something to do with the following code in assignManager.js:

    getWildcardStoreKey(wildcardHostname) {
      return `wildcardMap@@_${wildcardHostname}`;
    },

    getWildcardStoreKeys(siteStoreKey) {
      // E.g. "siteContainerMap@@_www.mozilla.org" =>
      // ["wildcardMap@@_www.mozilla.org", "wildcardMap@@_mozilla.org", "wildcardMap@@_org"]
      let previous;
      return siteStoreKey.replace(/^siteContainerMap@@_/, "")
        .split(".")
        .reverse()
        .map((subdomain) => previous = previous ? `${subdomain}.${previous}` : subdomain)
        .map((hostname) => this.getWildcardStoreKey(hostname))
        .reverse();
    },

@mckenfra
Copy link
Contributor Author

mckenfra commented Jun 9, 2022

@BPower0036 You should only need to add media.gab.com and then click the media part to change it to a wildcard. That should then automatically also handle gab.com without having to explicitly add it.

@Nomes77
Copy link

Nomes77 commented Jun 9, 2022

@mckenfra Thanks for that! It works! I expected wrongfully to work the other way around, which is actually not possible.

@Hermholtz
Copy link

Hi, I've tried this version 8.0.7.4-WildcardContainers, but no luck.

  1. After installing .xpi package I got two extensions - the official one and 8.0.7.4 at the same time. It showed me the introduction and then displayed all my containers, as expected.

  2. 8.0.7.4 doesn't show me any entry in any container in the "Manage site list" window.

  3. In the "Google" container where I gathered all Google-related domains, the official addon shows me an empty entry at the top of the list. Whenever I use the trash icon to delete it, the next time it's again there. It's only for that container. It used to do that even before installing 8.0.7.4.

If I can somehow do any diagnostics, please guide me what to do. Thanks.

Please see the screenshots.

image

image

image

@mckenfra
Copy link
Contributor Author

@Hermholtz I tried to make it clear on the Release page that this Wildcard Subdomains build is only a temporary proof-of-concept build - i.e. for testing purposes only! I also stated on the Release page:

When installed, this build of the Multi-Account Containers will co-exist with your normal Firefox Multi-Account Containers addon. However, none of the configuration will be shared between the two versions.

So it is expected that none of your sites are visible in this temporary Wildcard Subdomains build.

Perhaps you should just uninstall the Wildcard Subdomains build? There is little point spending time configuring all your sites again in the Wildcard Subdomains build, because when Mozilla (hopefully at some point) releases an official update to include the wildcard subdomains feature, none of those sites from this temporary build will be carried over.

However, you must be very careful that you uninstall the correct Addon. If you accidentally uninstall the official Mozilla Multi-Account Containers addon, you will lose all of your sites configuration!

So when you are on the Firefox Addons page, make sure you delete the addon called MAC Wildcard Subdomains.

I hope that makes sense!

@Hermholtz
Copy link

OK Thank you for clarification, I really did not read the entire documents, I was so eager to try I immediately jumped to installing it :) Now it makes sense. I've uninstalled your temp build.

I hope Mozilla can make it working soon, this entire issue is more or less 5 years old already...

@mckenfra
Copy link
Contributor Author

I hope Mozilla can make it working soon, this entire issue is more or less 5 years old already...

No problem! And I can see from your screenshot that you must have spent a lot of time adding many many sites to your Google container. So this wildcard subdomains feature is going to be a big help for you as well as others like you have been doing the same thing!

@jacktose
Copy link

Bug 1315558 was mentioned as supporting this feature.

@celluj34
Copy link

celluj34 commented Jun 23, 2022

Does this mean that I cannot map *.google.com and *google.com separately? For instance, mail.google.com vs thisisnotgoogle.com would both be mapped my *.google.com? Or do I not understand properly?

@mckenfra
Copy link
Contributor Author

Does this mean that I cannot map *.google.com and *google.com separately? For instance, mail.google.com vs thisisnotgoogle.com would both be mapped my *.google.com? Or do I not understand properly?

@celluj34 You cannot map *google.com. Wildcards only apply to entire subdomains, *.google.com

@celluj34
Copy link

Can they? I myself don't have a use case for it but I'm sure someone does. Perhaps that's out of the scope of this immediate feature.

@mckenfra
Copy link
Contributor Author

@celluj34 Yes sorry I meant the way it is currently coded, *google.com is not possible. Theoretically it would be possible to change the code to support this use case if people needed it.

@achernyakevich-sc
Copy link

@celluj34 Yes sorry I meant the way it is currently coded, *google.com is not possible. Theoretically it would be possible to change the code to support this use case if people needed it.

@celluj34 It is quite tricky case for regular users. Maybe there is no need to implement support for it. This case could be handled by using Containerise add-on together with MAC - it provide possibility to use RegExp to handle opening in dedicated containers.

@dannycolin
Copy link
Collaborator

Since this issue only let you trigger the wildcard by flipping a switch, I'd say it's out-of-scope. However, it'd fit perfectly in the PR to manually add an URL. This PR could add support for regular expressions.

@celluj34
Copy link

celluj34 commented Sep 8, 2022

What's status on this PR? I would love to see this merged / released.

@dannycolin
Copy link
Collaborator

What's status on this PR? I would love to see this merged / released.

We decided to implement this feature only if we have access to the PublicSuffixList shipped by the browser For this to happen, we need to implement a new API (bug 1315558). However, both @mckenfra and I are working on this on our free time as volunteers so we can't promise you when this is going to be merged. Sorry for not having better news.

@Gunni

This comment was marked as off-topic.

@celluj34
Copy link

We decided to implement this feature only if we have access to the PublicSuffixList shipped by the browser For this to happen, we need to implement a new API (bug 1315558). However, both @mckenfra and I are working on this on our free time as volunteers so we can't promise you when this is going to be merged. Sorry for not having better news.

Why can't the addon be released by providing its own PublicSuffixList, and updating later if/when it does become available?

@dannycolin
Copy link
Collaborator

@celluj34 See w3c/webextensions#231 (comment).

I won't go into the technical details because that'd be too off-topic but tl;dr inconsistencies between the list shipped by the browser and the addon could have potential security impacts.

@Gunni

This comment was marked as off-topic.

@philsnow
Copy link

philsnow commented Dec 5, 2022

Today I was alarmed to discover that my default container has been logged in to Google for I don't know how long. I tried to add a wildcard + apex "always open this container in" rule and found it's not implemented, and then found this PR.

We decided to implement this feature only if we have access to the PublicSuffixList shipped by the browser

I agree that shipping a hard-coded suffix list in the extension that doesn't come from somewhere more authoritative is a non-starter, but why wait to merge this PR as-is? It would improve MAC and thus user safety today for everybody who uses e.g. Google.

Adding support later for the public suffix list will improve the experience even further, but I don't see why that is blocking, especially since this PR is already in a shippable state.

@mozilla mozilla locked and limited conversation to collaborators Dec 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet