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

[PM-2083] Subdomains exclusion #2289

Closed
wants to merge 1 commit into from

Conversation

jotak
Copy link

@jotak jotak commented Jan 25, 2022

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

Objective

Feature request to allow all subdomains exclusion, based on domain suffix

See also: https://community.bitwarden.com/t/excluded-sub-domains/30985

Note: I'm not sure if backward compatibility would be expected for excluded domains, ie by default NOT excluding all subdomains when a specific excluded domain is set. If so, probably there could be a checkbox in the domain exclusion config screen, to explicitly ask for turning this feature on.
I can try to do that if requested, however would appreciate some guidance.

Code changes

In notificationBar.ts, instead of checking for the whole hostname in excluded domains, checks that hostname endsWidth any of the excluded domains.

Screenshots

n/a

Testing requirements

non-regression on excluded domains / eventually add new test cases for subdomains exclusion.

Before you submit

  • I have checked for linting errors (npm run lint) (required)
  • This change requires a documentation update (notify the documentation team) => https://bitwarden.com/help/exclude-domains/
  • This change has particular deployment requirements (notify the DevOps team)

@CLAassistant
Copy link

CLAassistant commented Jan 25, 2022

CLA assistant check
All committers have signed the CLA.

@jotak
Copy link
Author

jotak commented Feb 15, 2022

hey , any chance to have a review on this? Thanks

@djsmith85
Copy link
Contributor

Hi @jotak, Thank you for your contribution. This has been added to our internal Community PR board for review.

Due to some recent changes to that part of the code, there is unfortunately a merge-conflict. Could please have a look at it.

Feature request to allow all subdomains exclusion, based on domain suffix
See also: https://community.bitwarden.com/t/excluded-sub-domains/30985

Signed-off-by: Joel Takvorian <jtakvori@redhat.com>
@gdlx
Copy link

gdlx commented Apr 17, 2023

@djsmith85 More than a year since @jotak updated his branch 11 minutes after you requested it and nothing happened...

@djsmith85 djsmith85 changed the title Subdomains exclusion [PM-2083] Subdomains exclusion May 3, 2023
@programmerHaaks
Copy link

Any chance this could get merged? It's annoying to have to click Never for every single service I access at work

@mauricioklein
Copy link

mauricioklein commented Jun 19, 2023

This is very needed.

It's super annoying having to dismiss these popups all the time (those using Gitpod knows the pain)

@micahblut
Copy link
Member

@jotak thank you for your contribution. As implemented, we will not merge this feature. Rather than making subdomain exclusion the default behavior, I think following our community member's suggestion of implementing some sort of wildcard (*.example.com) would be a more flexible solution.

@micahblut micahblut closed this Jul 28, 2023
@zevisert
Copy link
Contributor

Is there another PR for this using Regex yet? Nothing new is referencing this PR since it's been closed.. I'd be happy to contribute the feature if not

@ibcht
Copy link

ibcht commented Sep 15, 2023

Is there another PR for this using Regex yet? Nothing new is referencing this PR since it's been closed.. I'd be happy to contribute the feature if not

Maybe wildcard domain exclusion would be a simple yet powerful addition, regex might be overkill I don't know. Anyway, I would personaly love such kind of feature to exclude any applications hosted under a corporate subdomain.

@martinbra
Copy link

A wildcard would also benefit those who access devices by their IPs , i.e.: "10.42.0.*"

@ForsakenRei
Copy link

Is there another PR for this using Regex yet? Nothing new is referencing this PR since it's been closed.. I'd be happy to contribute the feature if not

I would like to see a wildcard option as well, besides the ip address example above and some of my self-hosted services, there are a lot other cases for example Citibank they can have different subdomains and I don't want to click them every time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet