From 1989480639fad6f62f5e31a519df5fe58b811e5f Mon Sep 17 00:00:00 2001 From: Louis Gregg Date: Sat, 23 Nov 2024 16:57:42 +0100 Subject: [PATCH 1/4] Log session ID instead of client IP in recaptcha flow --- changelog.d/17956.bugfix | 1 + synapse/handlers/ui_auth/checkers.py | 8 +- tests/handlers/test_checkers.py | 124 +++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 changelog.d/17956.bugfix create mode 100644 tests/handlers/test_checkers.py diff --git a/changelog.d/17956.bugfix b/changelog.d/17956.bugfix new file mode 100644 index 00000000000..7afa2d0a7a4 --- /dev/null +++ b/changelog.d/17956.bugfix @@ -0,0 +1 @@ +Don't log client IP during captcha verification (privacy issue fix). Contibuted by Louis Gregg. \ No newline at end of file diff --git a/synapse/handlers/ui_auth/checkers.py b/synapse/handlers/ui_auth/checkers.py index 32dca8c43ba..783db226116 100644 --- a/synapse/handlers/ui_auth/checkers.py +++ b/synapse/handlers/ui_auth/checkers.py @@ -115,9 +115,15 @@ async def check_auth(self, authdict: dict, clientip: str) -> Any: raise LoginError( 400, "Captcha response is required", errcode=Codes.CAPTCHA_NEEDED ) + try: + _session = authdict["session"] + except KeyError: + # Client tried to provide captcha but didn't give the session ID: + # bad request. + raise LoginError(400, "Missing UIA session", Codes.MISSING_PARAM) logger.info( - "Submitting recaptcha response %s with remoteip %s", user_response, clientip + "Submitting recaptcha response %s for session %s", user_response, _session ) # TODO: get this from the homeserver rather than creating a new one for diff --git a/tests/handlers/test_checkers.py b/tests/handlers/test_checkers.py new file mode 100644 index 00000000000..7f943059cc4 --- /dev/null +++ b/tests/handlers/test_checkers.py @@ -0,0 +1,124 @@ +from typing import Dict +from unittest.mock import AsyncMock, Mock + +from twisted.test.proto_helpers import MemoryReactor +from twisted.trial import unittest +from twisted.web.client import PartialDownloadError + +from synapse.api.errors import Codes, LoginError +from synapse.handlers.ui_auth.checkers import RecaptchaAuthChecker +from synapse.server import HomeServer +from synapse.util import json_encoder + + +class TestRecaptchaAuthChecker(unittest.TestCase): + def setUp(self: "TestRecaptchaAuthChecker") -> None: + self.hs = Mock(spec=HomeServer) + self.hs.config = Mock() + self.hs.config.captcha.recaptcha_private_key = "test_private_key" + self.hs.config.captcha.recaptcha_siteverify_api = ( + "https://www.recaptcha.net/recaptcha/api/siteverify" + ) + self.http_client = AsyncMock() + self.hs.get_proxied_http_client.return_value = self.http_client + self.recaptcha_checker = RecaptchaAuthChecker(self.hs) + self.reactor = MemoryReactor() + + def test_is_enabled(self: "TestRecaptchaAuthChecker") -> None: + """Test that the checker is enabled when a private key is configured.""" + self.assertTrue(self.recaptcha_checker.is_enabled()) + + def test_is_disabled(self: "TestRecaptchaAuthChecker") -> None: + """Test that the checker is disabled when no private key is configured.""" + self.hs.config.captcha.recaptcha_private_key = None + recaptcha_checker = RecaptchaAuthChecker(self.hs) + self.assertFalse(recaptcha_checker.is_enabled()) + + def test_check_auth_success(self: "TestRecaptchaAuthChecker") -> None: + """Test that authentication succeeds with a valid recaptcha response.""" + _expected_response = {"success": True} + self.http_client.post_urlencoded_get_json = AsyncMock( + return_value=_expected_response + ) + + authdict = {"response": "captcha_solution", "session": "fake_session_id"} + clientip = "127.0.0.1" + + d = self.recaptcha_checker.check_auth(authdict, clientip) + result = self.successResultOf(d) + self.assertTrue(result) + + self.http_client.post_urlencoded_get_json.assert_called_once_with( + self.hs.config.captcha.recaptcha_siteverify_api, + args={ + "secret": "test_private_key", + "response": "captcha_solution", + "remoteip": clientip, + }, + ) + + def test_check_auth_failure(self: "TestRecaptchaAuthChecker") -> None: + """Test that authentication fails with an invalid recaptcha response.""" + _expected_response = {"success": False} + self.http_client.post_urlencoded_get_json = AsyncMock( + return_value=_expected_response + ) + + authdict = {"response": "invalid_response", "session": "fake_session_id"} + clientip = "127.0.0.1" + + d = self.recaptcha_checker.check_auth(authdict, clientip) + f = self.failureResultOf(d, LoginError) + self.assertEqual(f.value.errcode, Codes.UNAUTHORIZED) + + def test_check_missing_session(self: "TestRecaptchaAuthChecker") -> None: + """Test that authentication fails when the session ID is missing.""" + + authdict = {"response": "invalid_response"} + clientip = "127.0.0.1" + + d = self.recaptcha_checker.check_auth(authdict, clientip) + f = self.failureResultOf(d, LoginError) + self.assertEqual(f.value.errcode, Codes.MISSING_PARAM) + + def test_check_auth_missing_response(self: "TestRecaptchaAuthChecker") -> None: + """Test that authentication fails when the user captcha response is missing.""" + authdict: Dict[str, str] = {} + clientip = "127.0.0.1" + + d = self.recaptcha_checker.check_auth(authdict, clientip) + f = self.failureResultOf(d, LoginError) + self.assertEqual(f.value.errcode, Codes.CAPTCHA_NEEDED) + + def test_check_auth_exception(self: "TestRecaptchaAuthChecker") -> None: + """Test that authentication succeeds when a PartialDownloadError occurs during verification.""" + + partial_download_error = PartialDownloadError(500) + partial_download_error.response = json_encoder.encode({"success": True}).encode( + "utf-8" + ) + self.http_client.post_urlencoded_get_json.side_effect = partial_download_error + + authdict = {"response": "captcha_solution", "session": "fake_session_id"} + clientip = "127.0.0.1" + + d = self.recaptcha_checker.check_auth(authdict, clientip) + result = self.successResultOf(d) + self.assertTrue(result) + + def test_check_auth_logging(self: "TestRecaptchaAuthChecker") -> None: + """Test that the client IP is not logged during authentication.""" + with self.assertLogs(level="INFO") as cm: + authdict = {"response": "captcha_solution", "session": "fake_session_id"} + clientip = "127.0.0.1" + + _expected_response = {"success": True} + self.http_client.post_urlencoded_get_json = AsyncMock( + return_value=_expected_response + ) + + d = self.recaptcha_checker.check_auth(authdict, clientip) + self.successResultOf(d) + + logs = "\n".join(cm.output) + self.assertNotIn(clientip, logs) From 885ec6c3a7df76c5da3ff2c6acd73fa7708f0b2d Mon Sep 17 00:00:00 2001 From: Louis Gregg Date: Sun, 24 Nov 2024 13:20:54 +0100 Subject: [PATCH 2/4] fix for old_deps twisted==18.9.0 --- tests/handlers/test_checkers.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/handlers/test_checkers.py b/tests/handlers/test_checkers.py index 7f943059cc4..268a39f2da8 100644 --- a/tests/handlers/test_checkers.py +++ b/tests/handlers/test_checkers.py @@ -1,6 +1,7 @@ from typing import Dict from unittest.mock import AsyncMock, Mock +from twisted.internet.defer import ensureDeferred from twisted.test.proto_helpers import MemoryReactor from twisted.trial import unittest from twisted.web.client import PartialDownloadError @@ -44,7 +45,8 @@ def test_check_auth_success(self: "TestRecaptchaAuthChecker") -> None: authdict = {"response": "captcha_solution", "session": "fake_session_id"} clientip = "127.0.0.1" - d = self.recaptcha_checker.check_auth(authdict, clientip) + d = ensureDeferred(self.recaptcha_checker.check_auth(authdict, clientip)) + result = self.successResultOf(d) self.assertTrue(result) @@ -67,7 +69,7 @@ def test_check_auth_failure(self: "TestRecaptchaAuthChecker") -> None: authdict = {"response": "invalid_response", "session": "fake_session_id"} clientip = "127.0.0.1" - d = self.recaptcha_checker.check_auth(authdict, clientip) + d = ensureDeferred(self.recaptcha_checker.check_auth(authdict, clientip)) f = self.failureResultOf(d, LoginError) self.assertEqual(f.value.errcode, Codes.UNAUTHORIZED) @@ -77,7 +79,7 @@ def test_check_missing_session(self: "TestRecaptchaAuthChecker") -> None: authdict = {"response": "invalid_response"} clientip = "127.0.0.1" - d = self.recaptcha_checker.check_auth(authdict, clientip) + d = ensureDeferred(self.recaptcha_checker.check_auth(authdict, clientip)) f = self.failureResultOf(d, LoginError) self.assertEqual(f.value.errcode, Codes.MISSING_PARAM) @@ -86,7 +88,7 @@ def test_check_auth_missing_response(self: "TestRecaptchaAuthChecker") -> None: authdict: Dict[str, str] = {} clientip = "127.0.0.1" - d = self.recaptcha_checker.check_auth(authdict, clientip) + d = ensureDeferred(self.recaptcha_checker.check_auth(authdict, clientip)) f = self.failureResultOf(d, LoginError) self.assertEqual(f.value.errcode, Codes.CAPTCHA_NEEDED) @@ -102,7 +104,7 @@ def test_check_auth_exception(self: "TestRecaptchaAuthChecker") -> None: authdict = {"response": "captcha_solution", "session": "fake_session_id"} clientip = "127.0.0.1" - d = self.recaptcha_checker.check_auth(authdict, clientip) + d = ensureDeferred(self.recaptcha_checker.check_auth(authdict, clientip)) result = self.successResultOf(d) self.assertTrue(result) @@ -117,7 +119,7 @@ def test_check_auth_logging(self: "TestRecaptchaAuthChecker") -> None: return_value=_expected_response ) - d = self.recaptcha_checker.check_auth(authdict, clientip) + d = ensureDeferred(self.recaptcha_checker.check_auth(authdict, clientip)) self.successResultOf(d) logs = "\n".join(cm.output) From 3e1968e3e4b8cbf2813cb1f1e04281a8ad8afb5d Mon Sep 17 00:00:00 2001 From: Louis Gregg Date: Tue, 26 Nov 2024 12:15:04 +0100 Subject: [PATCH 3/4] dont log session during captcha flow, either --- synapse/handlers/ui_auth/checkers.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/ui_auth/checkers.py b/synapse/handlers/ui_auth/checkers.py index 783db226116..34c8a56f115 100644 --- a/synapse/handlers/ui_auth/checkers.py +++ b/synapse/handlers/ui_auth/checkers.py @@ -116,15 +116,13 @@ async def check_auth(self, authdict: dict, clientip: str) -> Any: 400, "Captcha response is required", errcode=Codes.CAPTCHA_NEEDED ) try: - _session = authdict["session"] + _ = authdict["session"] except KeyError: # Client tried to provide captcha but didn't give the session ID: # bad request. raise LoginError(400, "Missing UIA session", Codes.MISSING_PARAM) - logger.info( - "Submitting recaptcha response %s for session %s", user_response, _session - ) + logger.debug("Submitting recaptcha response %s", user_response) # TODO: get this from the homeserver rather than creating a new one for # each request From 0cb78f86f268e48f9cd026789517856cc918be4a Mon Sep 17 00:00:00 2001 From: Louis Gregg Date: Sat, 14 Dec 2024 22:50:59 +0100 Subject: [PATCH 4/4] removed check for session in captcha request. sytest 11register.pl doesnt provide this and fails --- synapse/handlers/ui_auth/checkers.py | 6 ------ tests/handlers/test_checkers.py | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/synapse/handlers/ui_auth/checkers.py b/synapse/handlers/ui_auth/checkers.py index 34c8a56f115..cbda76e5ced 100644 --- a/synapse/handlers/ui_auth/checkers.py +++ b/synapse/handlers/ui_auth/checkers.py @@ -115,12 +115,6 @@ async def check_auth(self, authdict: dict, clientip: str) -> Any: raise LoginError( 400, "Captcha response is required", errcode=Codes.CAPTCHA_NEEDED ) - try: - _ = authdict["session"] - except KeyError: - # Client tried to provide captcha but didn't give the session ID: - # bad request. - raise LoginError(400, "Missing UIA session", Codes.MISSING_PARAM) logger.debug("Submitting recaptcha response %s", user_response) diff --git a/tests/handlers/test_checkers.py b/tests/handlers/test_checkers.py index 268a39f2da8..89a5a942af0 100644 --- a/tests/handlers/test_checkers.py +++ b/tests/handlers/test_checkers.py @@ -81,7 +81,7 @@ def test_check_missing_session(self: "TestRecaptchaAuthChecker") -> None: d = ensureDeferred(self.recaptcha_checker.check_auth(authdict, clientip)) f = self.failureResultOf(d, LoginError) - self.assertEqual(f.value.errcode, Codes.MISSING_PARAM) + self.assertEqual(f.value.errcode, Codes.UNAUTHORIZED) def test_check_auth_missing_response(self: "TestRecaptchaAuthChecker") -> None: """Test that authentication fails when the user captcha response is missing."""