From 280cd58b9d5ef83cdf23830eb58146cf0126d4a2 Mon Sep 17 00:00:00 2001 From: Hayden <64056131+hay-kot@users.noreply.github.com> Date: Sat, 18 Jul 2026 12:27:10 -0500 Subject: [PATCH] fix: harden LDAP and OIDC authentication providers (#7902) --- .../installation/backend-config.md | 1 + .../core/security/providers/ldap_provider.py | 16 +++- .../security/providers/openid_provider.py | 26 ++++-- mealie/core/settings/settings.py | 1 + tests/e2e/login.spec.ts | 5 + .../providers/test_openid_provider.py | 93 ++++++++++++++++++- tests/unit_tests/test_security.py | 63 +++++++++++++ 7 files changed, 193 insertions(+), 12 deletions(-) diff --git a/docs/docs/documentation/getting-started/installation/backend-config.md b/docs/docs/documentation/getting-started/installation/backend-config.md index 6b030c0fc..1aaaeb25b 100644 --- a/docs/docs/documentation/getting-started/installation/backend-config.md +++ b/docs/docs/documentation/getting-started/installation/backend-config.md @@ -101,6 +101,7 @@ For usage, see [Usage - OpenID Connect](../authentication/oidc-v2.md) | ----------------------------------------------------------------------------------- | :-----: | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | OIDC_AUTH_ENABLED | False | Enables authentication via OpenID Connect | | OIDC_SIGNUP_ENABLED | True | Enables new users to be created when signing in for the first time with OIDC | +| OIDC_REQUIRES_EMAIL_VERIFICATION | True | Requires the `email_verified` claim to be true before a user can sign in. This prevents an unverified email from being used to match an existing account. Only disable this if your identity provider does not emit the `email_verified` claim. | | OIDC_CONFIGURATION_URL[†][secrets] | None | The URL to the OIDC configuration of your provider. This is usually something like https://auth.example.com/.well-known/openid-configuration | | OIDC_CLIENT_ID[†][secrets] | None | The client id of your configured client in your provider | | OIDC_CLIENT_SECRET[†][secrets]
:octicons-tag-24: v2.0.0 | None | The client secret of your configured client in your provider | diff --git a/mealie/core/security/providers/ldap_provider.py b/mealie/core/security/providers/ldap_provider.py index 7211fcfb6..e4bf7c3e8 100644 --- a/mealie/core/security/providers/ldap_provider.py +++ b/mealie/core/security/providers/ldap_provider.py @@ -1,6 +1,7 @@ from datetime import timedelta import ldap +import ldap.filter from ldap.ldapobject import LDAPObject from sqlalchemy.orm.session import Session @@ -45,19 +46,23 @@ class LDAPProvider(CredentialsProvider): return None settings = get_app_settings() + # Escape the user-supplied username so filter metacharacters (*, (, ), \, NUL) + # are treated as literal data rather than LDAP filter syntax. + escaped_username = ldap.filter.escape_filter_chars(self.data.username) + user_filter = "" if settings.LDAP_USER_FILTER: # fill in the template provided by the user to maintain backwards compatibility user_filter = settings.LDAP_USER_FILTER.format( id_attribute=settings.LDAP_ID_ATTRIBUTE, mail_attribute=settings.LDAP_MAIL_ATTRIBUTE, - input=self.data.username, + input=escaped_username, ) # Don't assume the provided search filter has (|({id_attribute}={input})({mail_attribute}={input})) search_filter = "(&(|({id_attribute}={input})({mail_attribute}={input})){filter})".format( id_attribute=settings.LDAP_ID_ATTRIBUTE, mail_attribute=settings.LDAP_MAIL_ATTRIBUTE, - input=self.data.username, + input=escaped_username, filter=user_filter, ) @@ -108,6 +113,13 @@ class LDAPProvider(CredentialsProvider): return None data = self.data + # Reject empty passwords before binding. Many directories treat a bind with + # a valid DN and an empty password as an anonymous/unauthenticated bind that + # succeeds, which would otherwise let anyone log in as a known user. + if not data.password: + self._logger.error("[LDAP] Empty password is not permitted; refusing to bind") + return None + if settings.LDAP_TLS_INSECURE: ldap.set_option(ldap.OPT_X_TLS_REQUIRE_CERT, ldap.OPT_X_TLS_NEVER) diff --git a/mealie/core/security/providers/openid_provider.py b/mealie/core/security/providers/openid_provider.py index ac78b1337..8fb761a64 100644 --- a/mealie/core/security/providers/openid_provider.py +++ b/mealie/core/security/providers/openid_provider.py @@ -48,6 +48,15 @@ class OpenIDProvider(AuthProvider[UserInfo]): self._logger.debug("[OIDC] Required claim '%s' is empty", claim) raise MissingClaimException() + # Never trust an unverified email. An IdP that lets a user self-assert an + # arbitrary, unverified address would otherwise allow that user to match + # (and log into) another account by claiming its email. When the email is + # verified, matching an existing account is legitimate account linking. + # Admins whose IdP does not emit the claim can opt out via this setting. + if settings.OIDC_REQUIRES_EMAIL_VERIFICATION and not claims.get("email_verified", False): + self._logger.warning("[OIDC] email_verified claim is missing or false; refusing to authenticate") + raise MissingClaimException() + repos = get_repositories(self.session, group_id=None, household_id=None) is_admin = False @@ -106,15 +115,14 @@ class OpenIDProvider(AuthProvider[UserInfo]): return self.get_access_token(user, settings.OIDC_REMEMBER_ME) # type: ignore - if user: - if settings.OIDC_ADMIN_GROUP and user.admin != is_admin: - self._logger.debug("[OIDC] %s user as admin", "Setting" if is_admin else "Removing") - user.admin = is_admin - repos.users.update(user.id, user) - return self.get_access_token(user, settings.OIDC_REMEMBER_ME) - - self._logger.warning("[OIDC] Found user but their AuthMethod does not match OIDC") - return None + # A matched account is adopted here (including local/LDAP accounts). This is + # safe because the email that produced the match was verified by the IdP + # above, unless the admin has explicitly disabled that requirement. + if settings.OIDC_ADMIN_GROUP and user.admin != is_admin: + self._logger.debug("[OIDC] %s user as admin", "Setting" if is_admin else "Removing") + user.admin = is_admin + repos.users.update(user.id, user) + return self.get_access_token(user, settings.OIDC_REMEMBER_ME) @property def required_claims(self): diff --git a/mealie/core/settings/settings.py b/mealie/core/settings/settings.py index 474b61648..477f73f5b 100644 --- a/mealie/core/settings/settings.py +++ b/mealie/core/settings/settings.py @@ -368,6 +368,7 @@ class AppSettings(AppLoggingSettings): OIDC_CLIENT_SECRET: MaskedNoneString = None OIDC_CONFIGURATION_URL: str | None = None OIDC_SIGNUP_ENABLED: bool = True + OIDC_REQUIRES_EMAIL_VERIFICATION: bool = True OIDC_USER_GROUP: str | None = None OIDC_ADMIN_GROUP: str | None = None OIDC_AUTO_REDIRECT: bool = False diff --git a/tests/e2e/login.spec.ts b/tests/e2e/login.spec.ts index 11b9f4a0e..e5827ca65 100644 --- a/tests/e2e/login.spec.ts +++ b/tests/e2e/login.spec.ts @@ -51,6 +51,7 @@ test('oidc initial login', async ({ page }) => { const claims = { "sub": username, "email": `${username}@example.com`, + "email_verified": true, "preferred_username": username, "name": name, "groups": ["user"] @@ -73,6 +74,7 @@ test('oidc login with user not in propery group', async ({ page }) => { const claims = { "sub": username, "email": `${username}@example.com`, + "email_verified": true, "preferred_username": username, "name": name, "groups": [] @@ -93,6 +95,7 @@ test('oidc sequential login', async ({ page }) => { const claims = { "sub": username, "email": `${username}@example.com`, + "email_verified": true, "preferred_username": username, "name": name, "groups": ["user"] @@ -121,6 +124,7 @@ test('settings page verify oidc', async ({ page }) => { const claims = { "sub": username, "email": `${username}@example.com`, + "email_verified": true, "preferred_username": username, "name": name, "groups": ["user"] @@ -156,6 +160,7 @@ test('oidc admin user', async ({ page }) => { const claims = { "sub": username, "email": `${username}@example.com`, + "email_verified": true, "preferred_username": username, "name": name, "groups": ["user", "admin"] diff --git a/tests/unit_tests/core/security/providers/test_openid_provider.py b/tests/unit_tests/core/security/providers/test_openid_provider.py index 6b198a294..787bf5b27 100644 --- a/tests/unit_tests/core/security/providers/test_openid_provider.py +++ b/tests/unit_tests/core/security/providers/test_openid_provider.py @@ -53,6 +53,7 @@ def test_missing_groups_claim(monkeypatch: MonkeyPatch): data = { "preferred_username": "dude1", "email": "email@email.com", + "email_verified": True, "name": "Firstname Lastname", } auth_provider = OpenIDProvider(None, data) @@ -68,6 +69,7 @@ def test_missing_groups_claim_admin(monkeypatch: MonkeyPatch): data = { "preferred_username": "dude1", "email": "email@email.com", + "email_verified": True, "name": "Firstname Lastname", } auth_provider = OpenIDProvider(None, data) @@ -83,6 +85,7 @@ def test_missing_groups_claim_with_default(monkeypatch: MonkeyPatch): data = { "preferred_username": "dude1", "email": "email@email.com", + "email_verified": True, "name": "Firstname Lastname", } auth_provider = OpenIDProvider(None, data, True) @@ -97,6 +100,7 @@ def test_missing_groups_claim_admin_group_with_default(monkeypatch: MonkeyPatch, data = { "preferred_username": "dude1", "email": unique_user.email, + "email_verified": True, "name": "Firstname Lastname", } auth_provider = OpenIDProvider(unique_user.repos.session, data, True) @@ -111,6 +115,7 @@ def test_missing_user_group(monkeypatch: MonkeyPatch): data = { "preferred_username": "dude1", "email": "email@email.com", + "email_verified": True, "name": "Firstname Lastname", "groups": ["not_mealie_user"], } @@ -126,6 +131,7 @@ def test_has_user_group_existing_user(monkeypatch: MonkeyPatch, unique_user: Tes data = { "preferred_username": "dude1", "email": unique_user.email, + "email_verified": True, "name": "Firstname Lastname", "groups": ["mealie_user"], } @@ -142,6 +148,7 @@ def test_has_admin_group_existing_user(monkeypatch: MonkeyPatch, unique_user: Te data = { "preferred_username": "dude1", "email": unique_user.email, + "email_verified": True, "name": "Firstname Lastname", "groups": ["mealie_admin"], } @@ -158,6 +165,7 @@ def test_has_user_group_new_user(monkeypatch: MonkeyPatch, session: Session): data = { "preferred_username": "dude1", "email": "dude1@email.com", + "email_verified": True, "name": "Firstname Lastname", "groups": ["mealie_user"], } @@ -179,6 +187,7 @@ def test_has_admin_group_new_user(monkeypatch: MonkeyPatch, session: Session): data = { "preferred_username": "dude2", "email": "dude2@email.com", + "email_verified": True, "name": "Firstname Lastname", "groups": ["mealie_admin"], } @@ -208,6 +217,7 @@ def test_ldap_user_creation_invalid_group_or_household( data = { "preferred_username": random_string(), "email": random_email(), + "email_verified": True, "name": random_string(), "groups": ["mealie_user"], } @@ -227,11 +237,92 @@ def test_ldap_user_creation_invalid_group_or_household( assert user is None -def test_claims_logging(caplog, session: Session): +def test_rejects_unverified_email(monkeypatch: MonkeyPatch, session: Session): + monkeypatch.setenv("OIDC_REQUIRES_EMAIL_VERIFICATION", "true") + get_app_settings.cache_clear() + + data = { + "preferred_username": random_string(), + "email": random_email(), + "email_verified": False, + "name": random_string(), + } + auth_provider = OpenIDProvider(session, data) + + with pytest.raises(MissingClaimException): + auth_provider.authenticate() + + +def test_rejects_missing_email_verified(monkeypatch: MonkeyPatch, session: Session): + monkeypatch.setenv("OIDC_REQUIRES_EMAIL_VERIFICATION", "true") + get_app_settings.cache_clear() + + data = { + "preferred_username": random_string(), + "email": random_email(), + "name": random_string(), + } + auth_provider = OpenIDProvider(session, data) + + with pytest.raises(MissingClaimException): + auth_provider.authenticate() + + +def test_does_not_adopt_existing_account_with_unverified_email(monkeypatch: MonkeyPatch, unique_user: TestUser): + """An unverified email must not be able to log into (take over) an existing account.""" + monkeypatch.setenv("OIDC_REQUIRES_EMAIL_VERIFICATION", "true") + get_app_settings.cache_clear() + + data = { + "preferred_username": random_string(), + "email": unique_user.email, + "email_verified": False, + "name": random_string(), + } + auth_provider = OpenIDProvider(unique_user.repos.session, data) + + with pytest.raises(MissingClaimException): + auth_provider.authenticate() + + +def test_adopts_existing_account_with_verified_email(monkeypatch: MonkeyPatch, unique_user: TestUser): + """A verified email legitimately links to an existing (non-OIDC) account.""" + monkeypatch.setenv("OIDC_REQUIRES_EMAIL_VERIFICATION", "true") + get_app_settings.cache_clear() + + data = { + "preferred_username": random_string(), + "email": unique_user.email, + "email_verified": True, + "name": random_string(), + } + auth_provider = OpenIDProvider(unique_user.repos.session, data) + + assert auth_provider.authenticate() is not None + + +def test_allows_unverified_email_when_verification_disabled(monkeypatch: MonkeyPatch, session: Session): + monkeypatch.setenv("OIDC_REQUIRES_EMAIL_VERIFICATION", "false") + get_app_settings.cache_clear() + + data = { + "preferred_username": random_string(), + "email": random_email(), + "name": random_string(), + } + auth_provider = OpenIDProvider(session, data) + + assert auth_provider.authenticate() is not None + + +def test_claims_logging(monkeypatch: MonkeyPatch, caplog, session: Session): + monkeypatch.setenv("OIDC_REQUIRES_EMAIL_VERIFICATION", "true") + get_app_settings.cache_clear() caplog.set_level(logging.DEBUG) data = { "preferred_username": "testuser", "email": "test@example.com", + "email_verified": True, "name": "Test User", "groups": ["mealie_user"], } diff --git a/tests/unit_tests/test_security.py b/tests/unit_tests/test_security.py index 89412d011..fbd8fa50b 100644 --- a/tests/unit_tests/test_security.py +++ b/tests/unit_tests/test_security.py @@ -344,3 +344,66 @@ def test_user_login_ldap_auth_method(monkeypatch: MonkeyPatch, ldap_user: Privat assert result.full_name == ldap_user.full_name assert result.admin == ldap_user.admin assert result.auth_method == AuthMethod.LDAP + + +class PermissiveBindConnMock(LdapConnMock): + """Simulates a directory that accepts a bind with a valid DN and an empty + password (anonymous/unauthenticated bind) as success.""" + + def simple_bind_s(self, dn, bind_pw): + if bind_pw == "": + return + return super().simple_bind_s(dn, bind_pw) + + +class CapturingConnMock(LdapConnMock): + """Records the search filter passed to search_s and returns no matches.""" + + captured_filter: str | None = None + + def search_s(self, dn, scope, filter, attrlist): + if filter != self.app_settings.LDAP_ADMIN_FILTER: + CapturingConnMock.captured_filter = filter + return [] + + +def test_ldap_rejects_empty_password(monkeypatch: MonkeyPatch): + user, mail, name, _, query_bind, query_password = setup_env(monkeypatch) + + def ldap_initialize_mock(url): + # Even against a directory that would accept the empty-password bind, + # the provider must refuse before binding. + return PermissiveBindConnMock(user, "", False, query_bind, query_password, mail, name) + + monkeypatch.setattr(ldap, "initialize", ldap_initialize_mock) + + get_app_settings.cache_clear() + + with session_context() as session: + provider = get_provider(session, user, "") + result = provider.get_user() + + assert result is None + + +def test_ldap_escapes_username_in_search_filter(monkeypatch: MonkeyPatch): + _, mail, name, password, query_bind, query_password = setup_env(monkeypatch) + injected_username = "a*)(uid=*" + CapturingConnMock.captured_filter = None + + def ldap_initialize_mock(url): + return CapturingConnMock(injected_username, password, False, query_bind, query_password, mail, name) + + monkeypatch.setattr(ldap, "initialize", ldap_initialize_mock) + + get_app_settings.cache_clear() + + with session_context() as session: + provider = get_provider(session, injected_username, password) + provider.get_user() + + captured = CapturingConnMock.captured_filter + assert captured is not None + # Metacharacters from the username must be escaped, never present as raw syntax. + assert "\\2a" in captured # '*' -> \2a + assert "*" not in captured