From 579896d40b417b9fbf4d5d4fff68412c8cfbef37 Mon Sep 17 00:00:00 2001 From: Martin Varga Date: Mon, 17 Aug 2026 17:27:56 +0200 Subject: [PATCH 1/3] Remove special response for locked account error --- server/mergin/auth/api.yaml | 8 -------- server/mergin/auth/app.py | 5 +++-- server/mergin/auth/controller.py | 16 +++------------- server/mergin/auth/errors.py | 10 ---------- server/mergin/tests/test_auth.py | 18 +++++++++++++----- web-app/packages/lib/src/modules/user/store.ts | 12 ------------ web-app/packages/lib/src/modules/user/types.ts | 2 -- 7 files changed, 19 insertions(+), 52 deletions(-) delete mode 100644 server/mergin/auth/errors.py diff --git a/server/mergin/auth/api.yaml b/server/mergin/auth/api.yaml index fdcba129..b19abf22 100644 --- a/server/mergin/auth/api.yaml +++ b/server/mergin/auth/api.yaml @@ -360,8 +360,6 @@ paths: $ref: "#/components/responses/BadStatusResp" "401": $ref: "#/components/responses/UnauthorizedError" - "423": - $ref: "#/components/responses/LockedResp" /app/auth/logout: get: summary: Logout @@ -639,8 +637,6 @@ paths: $ref: "#/components/responses/NotFoundResp" "415": $ref: "#/components/responses/UnsupportedMediaType" - "423": - $ref: "#/components/responses/LockedResp" x-openapi-router-controller: mergin.auth.controller /app/admin/login: post: @@ -670,8 +666,6 @@ paths: $ref: "#/components/responses/UnauthorizedError" "403": $ref: "#/components/responses/Forbidden" - "423": - $ref: "#/components/responses/LockedResp" /v2/users: post: tags: @@ -744,8 +738,6 @@ components: description: Request could not be processed becuase of conflict in resources UnprocessableEntity: description: Request was correct and yet server could not process it - LockedResp: - description: Account is temporarily locked due to too many failed login attempts. NoContent: description: Success. No content returned. schemas: diff --git a/server/mergin/auth/app.py b/server/mergin/auth/app.py index a8217e97..67efed88 100644 --- a/server/mergin/auth/app.py +++ b/server/mergin/auth/app.py @@ -3,6 +3,7 @@ # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial import functools +import logging from typing import Optional from blinker import signal from flask import current_app, render_template, Flask @@ -13,7 +14,6 @@ from .commands import add_commands from .config import Configuration from .models import User -from .errors import AccountLockedError # signal for other versions to listen to user_account_closed = signal("user_account_closed") @@ -103,7 +103,8 @@ def authenticate(login, password): if user is None: return None if user.is_locked_out(): - raise AccountLockedError() + logging.info(f"Rejected login attempt for locked-out user {user.id}") + return None needs_commit = False # reset non-null locked_until as it has already expired if user.locked_until is not None: diff --git a/server/mergin/auth/controller.py b/server/mergin/auth/controller.py index 56e99cd8..736a5bd1 100644 --- a/server/mergin/auth/controller.py +++ b/server/mergin/auth/controller.py @@ -26,7 +26,6 @@ ) from .bearer import encode_token from .models import User, LoginHistory -from .errors import AccountLockedError from .schemas import UserSchema, UserSearchSchema, UserProfileSchema, UserInfoSchema from .forms import ( LoginForm, @@ -140,10 +139,7 @@ def login_public(): # noqa: E501 """ form = ApiLoginForm() if form.validate(): - try: - user = authenticate(form.login.data, form.password.data) - except AccountLockedError as e: - return e.response(423) + user = authenticate(form.login.data, form.password.data) if user and user.active: expire = datetime.now(pytz.utc) + timedelta( seconds=current_app.config["BEARER_TOKEN_EXPIRATION"] @@ -227,10 +223,7 @@ def search_users(): # pylint: disable=W0613,W0612 def login(): # pylint: disable=W0613,W0612 form = LoginForm() if form.validate(): - try: - user = authenticate(form.login.data, form.password.data) - except AccountLockedError as e: - return e.response(423) + user = authenticate(form.login.data, form.password.data) if user and user.active: login_user(user) if not os.path.isfile(current_app.config["MAINTENANCE_FILE"]): @@ -247,10 +240,7 @@ def admin_login(): # pylint: disable=W0613,W0612 if not form.validate(): return jsonify(form.errors), 400 - try: - user = authenticate(form.login.data, form.password.data) - except AccountLockedError as e: - return e.response(423) + user = authenticate(form.login.data, form.password.data) if user: if user.active and user.is_admin: login_user(user) diff --git a/server/mergin/auth/errors.py b/server/mergin/auth/errors.py deleted file mode 100644 index 99907948..00000000 --- a/server/mergin/auth/errors.py +++ /dev/null @@ -1,10 +0,0 @@ -# Copyright (C) Lutra Consulting Limited -# -# SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial - -from ..app import ResponseError - - -class AccountLockedError(Exception, ResponseError): - code = "AccountLocked" - detail = "Account temporarily locked due to too many failed login attempts" diff --git a/server/mergin/tests/test_auth.py b/server/mergin/tests/test_auth.py index 601060e3..d8b85379 100644 --- a/server/mergin/tests/test_auth.py +++ b/server/mergin/tests/test_auth.py @@ -111,14 +111,16 @@ def test_login_lockout(send_email_mock, client): """ user = add_user("lockoutuser", "correctpassword") since = datetime.utcnow() - timedelta(hours=1) + baseline = None def assert_locked(): + # a locked-out attempt must be byte-identical to an ordinary wrong-password response resp = client.post( url_for("/.mergin_auth_controller_login"), json={"login": "lockoutuser", "password": "wrong"}, ) - assert resp.status_code == 423 - assert resp.json["code"] == "AccountLocked" + assert resp.status_code == baseline.status_code + assert resp.json == baseline.json with patch.dict(client.application.config, {"LOCKOUT_POLICY": "3:60,4:3600"}): # tier 1: 3 failures → 60s lock @@ -128,24 +130,30 @@ def assert_locked(): json={"login": "lockoutuser", "password": "wrong"}, ) assert resp.status_code == 401 + if baseline is None: + # capture the ordinary wrong-password shape before any lock + # kicks in, to compare later locked-out responses against + baseline = resp # lockout email dispatched exactly once, at the moment the lock triggers assert send_email_mock.call_count == 1 assert_locked() - # correct password is also blocked while locked + # correct password is also blocked while locked, and still + # indistinguishable from a wrong-password response resp = client.post( url_for("/.mergin_auth_controller_login"), json={"login": "lockoutuser", "password": "correctpassword"}, ) - assert resp.status_code == 423 + assert resp.status_code == baseline.status_code + assert resp.json == baseline.json # no new failures recorded while already locked out assert LoginHistory.count_recent_failures(user.id, since) == 3 assert user.locked_until is not None - # no further emails while already locked out (attempts above were all 423s) + # no further emails while already locked out (attempts above were all masked 401s) assert send_email_mock.call_count == 1 # tier 2 escalation: one more failure after tier-1 expiry diff --git a/web-app/packages/lib/src/modules/user/store.ts b/web-app/packages/lib/src/modules/user/store.ts index 51c4cde8..ef918e0b 100644 --- a/web-app/packages/lib/src/modules/user/store.ts +++ b/web-app/packages/lib/src/modules/user/store.ts @@ -2,7 +2,6 @@ // // SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial -import axios from 'axios' import { defineStore, getActivePinia } from 'pinia' import { isNavigationFailure } from 'vue-router' @@ -19,7 +18,6 @@ import { ResetPasswordPayload, ChangePasswordWithTokenPayload, ChangePasswordPayload, - ErrorCodes, IsWorkspaceAdminPayload, LoginPayload, UserDetailResponse, @@ -240,21 +238,11 @@ export const useUserStore = defineStore('userModule', { async userLogin(payload: LoginPayload) { const instanceStore = useInstanceStore() const formStore = useFormStore() - const notificationStore = useNotificationStore() try { await UserApi.login(payload.data) await instanceStore.initApp() } catch (err) { - if (axios.isAxiosError(err)) { - const code = err.response?.data?.code as ErrorCodes - if (code === 'AccountLocked') { - await notificationStore.error({ - text: 'Your account is temporarily locked due to too many failed login attempts. Please check your email for a link to unlock it.' - }) - return - } - } await formStore.handleError({ componentId: payload.componentId, error: err, diff --git a/web-app/packages/lib/src/modules/user/types.ts b/web-app/packages/lib/src/modules/user/types.ts index c7c03609..2bf668c6 100644 --- a/web-app/packages/lib/src/modules/user/types.ts +++ b/web-app/packages/lib/src/modules/user/types.ts @@ -152,6 +152,4 @@ export interface UserRouteParams { reset?: string } -export type ErrorCodes = 'AccountLocked' - /* eslint-enable camelcase */ From 48a57dfe3580f3632ae0e9b4c8dfbf5965a694d6 Mon Sep 17 00:00:00 2001 From: Martin Varga Date: Mon, 17 Aug 2026 18:12:54 +0200 Subject: [PATCH 2/3] Make response uniform regardless of account existence Co-Authored-By: Claude Sonnet 5 --- server/mergin/auth/api.yaml | 4 ---- server/mergin/auth/controller.py | 25 +++++++++---------------- server/mergin/tests/test_auth.py | 32 +++++++++++++++++++++++++------- 3 files changed, 34 insertions(+), 27 deletions(-) diff --git a/server/mergin/auth/api.yaml b/server/mergin/auth/api.yaml index b19abf22..6fba1355 100644 --- a/server/mergin/auth/api.yaml +++ b/server/mergin/auth/api.yaml @@ -428,10 +428,6 @@ paths: description: OK "400": $ref: "#/components/responses/BadStatusResp" - "403": - $ref: "#/components/responses/Forbidden" - "404": - $ref: "#/components/responses/NotFoundResp" /app/auth/reset-password/{token}: post: summary: Confirm reset password diff --git a/server/mergin/auth/controller.py b/server/mergin/auth/controller.py index 736a5bd1..36c3c62c 100644 --- a/server/mergin/auth/controller.py +++ b/server/mergin/auth/controller.py @@ -290,25 +290,18 @@ def password_reset(): # pylint: disable=W0613,W0612 if not form.validate(): return jsonify(form.errors), 400 + # respond the same regardless of account existence/state (enumeration) user = User.query.filter( func.lower(User.email) == func.lower(form.email.data.strip()) ).one_or_none() - if not user: - return jsonify({"email": ["Account with given email does not exist"]}), 404 - if not user.active: - # user should confirm email first - return jsonify({"email": ["Account is not active"]}), 400 - if not user.can_edit_profile: - # using SSO - abort(403, CANNOT_EDIT_PROFILE_MSG) - - send_confirmation_email( - current_app, - user, - "change-password", - "email/password_reset.html", - "Password reset", - ) + if user and user.active and user.can_edit_profile: + send_confirmation_email( + current_app, + user, + "change-password", + "email/password_reset.html", + "Password reset", + ) return "", 200 diff --git a/server/mergin/tests/test_auth.py b/server/mergin/tests/test_auth.py index d8b85379..58a4c47f 100644 --- a/server/mergin/tests/test_auth.py +++ b/server/mergin/tests/test_auth.py @@ -114,7 +114,7 @@ def test_login_lockout(send_email_mock, client): baseline = None def assert_locked(): - # a locked-out attempt must be byte-identical to an ordinary wrong-password response + # must be byte-identical to an ordinary wrong-password response resp = client.post( url_for("/.mergin_auth_controller_login"), json={"login": "lockoutuser", "password": "wrong"}, @@ -131,8 +131,7 @@ def assert_locked(): ) assert resp.status_code == 401 if baseline is None: - # capture the ordinary wrong-password shape before any lock - # kicks in, to compare later locked-out responses against + # baseline shape before any lock kicks in baseline = resp # lockout email dispatched exactly once, at the moment the lock triggers @@ -140,8 +139,7 @@ def assert_locked(): assert_locked() - # correct password is also blocked while locked, and still - # indistinguishable from a wrong-password response + # correct password is also blocked while locked resp = client.post( url_for("/.mergin_auth_controller_login"), json={"login": "lockoutuser", "password": "correctpassword"}, @@ -548,12 +546,12 @@ def test_confirm_password(app, client): assert resp.status_code == 400 -# reset password tests: success, no email, not-existing user +# reset password tests: success, no email, not-existing user (200 - masked) test_reset_data = [ ({"email": "mergin@mergin.com"}, 200), ({"email": "Mergin@mergin.com"}, 200), # case insensitive ({}, 400), - ({"email": "tests@mergin.com"}, 404), + ({"email": "tests@mergin.com"}, 200), ] @@ -567,6 +565,26 @@ def test_reset_password(client, data, expected): assert resp.status_code == expected +@patch("mergin.celery.send_email_async.apply_async") +def test_reset_password_masks_account_existence(send_email_mock, client): + """Response must be identical whether or not the account exists.""" + resp_existing = client.post( + url_for("/.mergin_auth_controller_password_reset"), + json={"email": "mergin@mergin.com"}, + ) + assert resp_existing.status_code == 200 + assert send_email_mock.call_count == 1 + + resp_missing = client.post( + url_for("/.mergin_auth_controller_password_reset"), + json={"email": "no-such-user@mergin.com"}, + ) + assert resp_missing.status_code == resp_existing.status_code + assert resp_missing.json == resp_existing.json + # no email dispatched for a nonexistent account + assert send_email_mock.call_count == 1 + + def test_change_password(client): username = "user_test" old_password = "user_password" From cab687fe9b500822a52fd0c4f827aab9be4f179f Mon Sep 17 00:00:00 2001 From: Martin Varga Date: Tue, 18 Aug 2026 10:34:25 +0200 Subject: [PATCH 3/3] Ensure the same timing for all auth routes Call dummy bcrypt hashing if necessary. Co-Authored-By: Claude Sonnet 5 --- server/mergin/auth/app.py | 4 +++- server/mergin/auth/models.py | 9 +++++++- server/mergin/tests/test_auth.py | 37 ++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 2 deletions(-) diff --git a/server/mergin/auth/app.py b/server/mergin/auth/app.py index 67efed88..09bab97e 100644 --- a/server/mergin/auth/app.py +++ b/server/mergin/auth/app.py @@ -13,7 +13,7 @@ from .commands import add_commands from .config import Configuration -from .models import User +from .models import User, _check_dummy_password # signal for other versions to listen to user_account_closed = signal("user_account_closed") @@ -101,9 +101,11 @@ def authenticate(login, password): query = func.lower(User.username) == func.lower(login) user = User.query.filter(query).one_or_none() if user is None: + _check_dummy_password(password) return None if user.is_locked_out(): logging.info(f"Rejected login attempt for locked-out user {user.id}") + _check_dummy_password(password) return None needs_commit = False # reset non-null locked_until as it has already expired diff --git a/server/mergin/auth/models.py b/server/mergin/auth/models.py index 1d749da7..36174e2a 100644 --- a/server/mergin/auth/models.py +++ b/server/mergin/auth/models.py @@ -17,6 +17,12 @@ MAX_USERNAME_LENGTH = 50 +def _check_dummy_password(password: str) -> None: + """Burn the same bcrypt cost as a real check, without an actual user.""" + rounds = current_app.config.get("BCRYPT_LOG_ROUNDS", 12) + bcrypt.hashpw(password.encode("utf-8"), bcrypt.gensalt(rounds)) + + def _parse_lockout_policy(policy_str: str) -> list: """Parse "5:300,10:3600" into [(5, 300), (10, 3600)] sorted ascending by threshold.""" result = [] @@ -66,7 +72,8 @@ def __repr__(self): def check_password(self, password): # users created through SSO if self.passwd is None: - return + _check_dummy_password(password) + return False if isinstance(password, str): password = password.encode("utf-8") return bcrypt.checkpw(password, self.passwd.encode("utf-8")) diff --git a/server/mergin/tests/test_auth.py b/server/mergin/tests/test_auth.py index 58a4c47f..84346b99 100644 --- a/server/mergin/tests/test_auth.py +++ b/server/mergin/tests/test_auth.py @@ -346,6 +346,43 @@ def test_login_history_records_failures(client): assert user.last_signed_in == last_signed_in +@patch("mergin.celery.send_email_async.apply_async") +def test_invalid_login_timing(send_email_mock, client): + """A bcrypt operation must run for every login outcome - nonexistent + user, locked-out user, SSO account, and real wrong password. + """ + import bcrypt + + def login_attempt(login, password="dummy"): + client.post( + url_for("/.mergin_auth_controller_login"), + json={"login": login, "password": password}, + ) + + locked_user = add_user("timinguser", "correctpassword") + with patch( + "mergin.auth.models.bcrypt.hashpw", wraps=bcrypt.hashpw + ) as mock_hashpw, patch( + "mergin.auth.models.bcrypt.checkpw", wraps=bcrypt.checkpw + ) as mock_checkpw: + login_attempt("no-such-user") + assert mock_hashpw.call_count + mock_checkpw.call_count == 1 + + with patch.dict(client.application.config, {"LOCKOUT_POLICY": "1:3600"}): + login_attempt("timinguser", "wrong") # real check, also triggers the lock + assert mock_hashpw.call_count + mock_checkpw.call_count == 2 + assert locked_user.is_locked_out() + + login_attempt("timinguser", "wrong") # now locked - dummy path + assert mock_hashpw.call_count + mock_checkpw.call_count == 3 + + sso_user = User("ssouser", "sso@test.com") + db.session.add(sso_user) + db.session.commit() + login_attempt("ssouser") # SSO - dummy path + assert mock_hashpw.call_count + mock_checkpw.call_count == 4 + + def test_bcrypt_lazy_rehash(app): """Password is transparently rehashed on login when the cost factor changes.""" import bcrypt