Skip to content

Commit 1fc71d2

Browse files
committed
Bound the length of a logged error reason
1 parent e992884 commit 1fc71d2

4 files changed

Lines changed: 53 additions & 3 deletions

File tree

‎app/logging/rejections.py‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,17 @@
66
logger = logging.getLogger(__name__)
77

88
_DENIAL_STATUSES = frozenset({401, 403})
9+
MAX_REASON_LENGTH = 500
10+
_TRUNCATED = "..."
911

1012

11-
def summarize_reason(exc: Exception) -> str:
13+
def _bounded(reason: str) -> str:
14+
if len(reason) <= MAX_REASON_LENGTH:
15+
return reason
16+
return reason[: MAX_REASON_LENGTH - len(_TRUNCATED)] + _TRUNCATED
17+
18+
19+
def _describe(exc: Exception) -> str:
1220
if isinstance(exc, (RequestValidationError, ValidationError)):
1321
reasons = [
1422
f"{'.'.join(str(loc) for loc in err.get('loc', ()))}: {err.get('msg', '')}".strip(" :")
@@ -18,6 +26,10 @@ def summarize_reason(exc: Exception) -> str:
1826
return str(exc)
1927

2028

29+
def summarize_reason(exc: Exception) -> str:
30+
return _bounded(_describe(exc))
31+
32+
2133
def log_rejection(status_code: int, exc: Exception, reason: str) -> None:
2234
level = logging.WARNING if status_code in _DENIAL_STATUSES else logging.INFO
2335
logger.log(level, "Request rejected with %s (%s): %s", status_code, type(exc).__name__, reason)

‎app/services/pseudonym_referral_service.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
from app.db.models.referral import ReferralEntity
88
from app.errors.mapping import should_log, spec_for
99
from app.logging.events import Log
10+
from app.logging.rejections import summarize_reason
1011
from app.models.pseudonym import EncryptedPseudonym, OprfPseudonym
1112
from app.models.ura import UraNumber
1213
from app.services.pseudonym_resolver import PseudonymResolver
@@ -21,7 +22,7 @@ def _log_failure(event: LogEvent, message: str, exc: Exception) -> None:
2122
logger,
2223
event,
2324
message,
24-
fields={"http_status": spec_for(exc).http_status, "error_reason": str(exc)},
25+
fields={"http_status": spec_for(exc).http_status, "error_reason": summarize_reason(exc)},
2526
)
2627

2728

‎tests/logging/test_rejections.py‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import pytest
22
from pydantic import BaseModel, ValidationError
33

4-
from app.logging.rejections import summarize_reason
4+
from app.logging.rejections import MAX_REASON_LENGTH, summarize_reason
55

66

77
class _Model(BaseModel):
@@ -22,3 +22,21 @@ def test_summarizes_a_validation_error_without_its_input() -> None:
2222

2323
def test_summarizes_a_plain_exception_by_its_message() -> None:
2424
assert summarize_reason(RuntimeError("boom")) == "boom"
25+
26+
27+
@pytest.mark.parametrize(
28+
"exc",
29+
[RuntimeError("x" * (MAX_REASON_LENGTH * 4)), _validation_error(["a"] * 1000)],
30+
ids=["long-message", "many-errors"],
31+
)
32+
def test_bounds_the_length_of_a_reason(exc: Exception) -> None:
33+
reason = summarize_reason(exc)
34+
35+
assert len(reason) == MAX_REASON_LENGTH
36+
assert reason.endswith("...")
37+
38+
39+
def test_keeps_a_reason_of_the_maximum_length_whole() -> None:
40+
message = "x" * MAX_REASON_LENGTH
41+
42+
assert summarize_reason(RuntimeError(message)) == message

‎tests/services/test_pseudonym_referral_service.py‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
from app.debug.crypto_service_api_client_mock import CryptoServiceApiClientMock
88
from app.logging.events import Log
9+
from app.logging.rejections import MAX_REASON_LENGTH
910
from app.models.pseudonym import OprfPseudonym, PseudonymResponse
1011
from app.models.ura import UraNumber
1112
from app.services.exceptions import CryptoServiceError, InvalidKeyInfoError, PseudonymExchangeError
@@ -99,6 +100,24 @@ def test_register_logs_an_error_when_the_crypto_service_fails(
99100
assert fields["http_status"] == 502
100101

101102

103+
def test_register_bounds_the_logged_failure_reason(
104+
referral_service: ReferralService,
105+
key_info_service: KeyInfoService,
106+
ura_number: UraNumber,
107+
caplog: pytest.LogCaptureFixture,
108+
) -> None:
109+
class VerboseFailingCryptoClient(CryptoServiceApiClientMock):
110+
def exchange(self, pseudonym: OprfPseudonym, label: str, mechanism: str) -> PseudonymResponse:
111+
raise RuntimeError("x" * (MAX_REASON_LENGTH * 4))
112+
113+
service = _service_with(VerboseFailingCryptoClient(), referral_service, key_info_service)
114+
with caplog.at_level(logging.INFO), pytest.raises(RuntimeError):
115+
service.register(PSEUDONYM, ura_number, SOURCE)
116+
117+
(fields,) = logged_fields(caplog, Log.REFERRAL_REGISTRATION_ERROR)
118+
assert len(fields["error_reason"]) == MAX_REASON_LENGTH
119+
120+
102121
def test_register_does_not_log_an_expected_failure(
103122
pseudonym_referral_service: PseudonymReferralService,
104123
ura_number: UraNumber,

0 commit comments

Comments
 (0)