From cc2de9677a98a3e10b09fca9865f5cff7ad00783 Mon Sep 17 00:00:00 2001 From: ColonistOne Date: Tue, 21 Jul 2026 20:11:40 +0100 Subject: [PATCH 1/3] fix(2fa): reject a totp= value that cannot be a one-time code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Passing the TOTP *secret* where a code is expected produced no useful signal. The SDK forwarded the 32-character base32 string verbatim and the only feedback was the server's: 422 {"loc": ["body","totp_code"], "msg": "String should have at most 16 characters", "type": "string_too_long"} on a field the caller never named. That reads as "the API is broken", not "you passed the wrong thing" — and the mistake is easy, because in conversation both values are "the TOTP". _validate_totp_code() now runs on BOTH branches of _resolve_totp (the literal and the callable — the callable form is the recommended one and therefore the likelier place to wire the wrong value in and never notice). Deliberately permissive about what it accepts: * 6-8 digits — RFC 6238 permits all three lengths, so this is not hard-coded to 6. * 10-16 alphanumerics — a RECOVERY code. These go through the same totp_code field, which is why the server's cap is 16 rather than 8. A strict ^\d{6}$ rule would have rejected the one credential you need when your authenticator is unavailable, and it would have failed at exactly the moment you could least afford it. and specific about what it names: a base32-looking value is rejected as "your secret, not a code", with the fix in the message. Verified by mutation: neutering the validator to `return code` turns 5 of the new tests red. Full suite 1043 -> 1056 (13 added), all green, ruff clean. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01TRn9SBFGaxRwZbwRsKNJ7b --- src/colony_sdk/client.py | 56 +++++++++++++++++++++++++++-- tests/test_two_factor.py | 77 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 130 insertions(+), 3 deletions(-) diff --git a/src/colony_sdk/client.py b/src/colony_sdk/client.py index c654d5f..d55ebda 100644 --- a/src/colony_sdk/client.py +++ b/src/colony_sdk/client.py @@ -529,6 +529,58 @@ class ColonyNetworkError(ColonyAPIError): } +#: A TOTP code is 6 digits by RFC 6238 convention; the RFC permits 7 and 8, so +#: those are accepted rather than guessed at. A RECOVERY code is also valid in +#: this field — the Colony issues 16-character alphanumeric ones — which is why +#: this is not a bare ``^\d{6}$`` check. The server caps ``totp_code`` at 16. +_TOTP_CODE_RE = re.compile(r"^[0-9]{6,8}$") +_RECOVERY_CODE_RE = re.compile(r"^[A-Za-z0-9-]{10,16}$") + +#: Base32, the alphabet TOTP *secrets* are shared in (RFC 4648). Used only to +#: recognise the one wrong value that is overwhelmingly likely to be passed. +_BASE32_SECRET_RE = re.compile(r"^[A-Z2-7]{16,}=*$") + + +def _validate_totp_code(code: str) -> str: + """Reject values that cannot be a TOTP or recovery code, with a useful message. + + Motivated by a real incident (2026-07-21): ``totp=`` + was passed instead of a generated code. The SDK forwarded it verbatim, and the + only feedback was the server's ``422 string_too_long`` on a field the caller had + never heard of — which reads as "the API is broken", not "you passed the wrong + thing". The mistake is easy precisely because both values are called "the TOTP" + in conversation. + + Deliberately permissive about what it accepts and specific about what it names: + + * 6-8 digits — a TOTP code (RFC 6238 allows all three lengths). + * 10-16 alphanumerics — a recovery code. **Not** excluded: recovery codes go + through this same field, so a strict 6-digit rule would break the one + credential you need when your authenticator is unavailable. + * anything that looks like a base32 secret — rejected by name, since that is + the error actually made. + """ + if not isinstance(code, str): + raise TypeError(f"totp must be a str or a callable returning one, got {type(code).__name__}") + code = code.strip() + if _TOTP_CODE_RE.match(code) or _RECOVERY_CODE_RE.match(code): + return code + if _BASE32_SECRET_RE.match(code): + raise ValueError( + f"totp looks like your TOTP *secret* ({len(code)} base32 characters), " + "not a one-time code. The secret is the seed your authenticator holds; " + "the code is the short number it produces. Generate one per request:\n" + " totp=lambda: pyotp.TOTP(secret).now()\n" + "Passing the secret would be forwarded to the server and rejected as " + "`totp_code` (max 16 characters)." + ) + raise ValueError( + f"totp={code!r} is not a valid one-time code: expected 6-8 digits, or a " + "10-16 character recovery code. Pass a callable if you need a fresh code " + "per request: totp=lambda: pyotp.TOTP(secret).now()" + ) + + def _resolve_totp(totp: str | Callable[[], str] | None, already_used: bool) -> tuple[str | None, bool]: """Resolve a TOTP code for one ``/auth/token`` exchange. @@ -545,7 +597,7 @@ def _resolve_totp(totp: str | Callable[[], str] | None, already_used: bool) -> t if totp is None: return None, already_used if callable(totp): - return totp(), already_used + return _validate_totp_code(totp()), already_used if already_used: raise ColonyTwoFactorRequiredError( "The single TOTP code passed as totp='...' was already used for one " @@ -556,7 +608,7 @@ def _resolve_totp(totp: str | Callable[[], str] | None, already_used: bool) -> t status=401, code="AUTH_2FA_REQUIRED", ) - return totp, True + return _validate_totp_code(totp), True def _error_class_for_status(status: int) -> type[ColonyAPIError]: diff --git a/tests/test_two_factor.py b/tests/test_two_factor.py index d9850ca..b8646fc 100644 --- a/tests/test_two_factor.py +++ b/tests/test_two_factor.py @@ -24,7 +24,12 @@ ColonyTwoFactorInvalidError, ColonyTwoFactorRequiredError, ) -from colony_sdk.client import ColonyAuthError, _build_api_error, _resolve_totp +from colony_sdk.client import ( + ColonyAuthError, + _build_api_error, + _resolve_totp, + _validate_totp_code, +) class TestTotpResolution: @@ -221,3 +226,73 @@ def test_mock_records_calls(self) -> None: recorded = [name for name, _ in mock.calls] assert recorded == ["get_2fa_status", "confirm_2fa", "disable_2fa"] assert mock.calls[1][1] == {"secret": "s", "ticket": "t", "code": "123456"} + + +# --- totp argument validation (added 2026-07-21) ---------------------------- +# Motivated by a live incident: the 32-char base32 SECRET was passed as `totp=`, +# forwarded verbatim, and surfaced only as the server's 422 `string_too_long` on +# a field the caller had never named. These pin the message that replaces it. + + +@pytest.mark.parametrize("code", ["123456", "1234567", "12345678"]) +def test_totp_codes_of_rfc_lengths_are_accepted(code): + """RFC 6238 permits 6, 7 and 8 digits — do not hard-code 6.""" + assert _validate_totp_code(code) == code + + +@pytest.mark.parametrize("code", ["1234abcd5ef678gh", "abc123def4", "AB12-CD34-EF"]) +def test_recovery_codes_are_accepted(code): + """Recovery codes share the totp_code field. + + A strict ^\\d{6}$ rule would reject the exact credential you need when the + authenticator is unavailable, which is the worst possible time to find out. + """ + assert _validate_totp_code(code) == code + + +def test_base32_secret_is_rejected_by_name(): + """The actual incident: the secret passed where a code was expected.""" + secret = "SROSG7JW2QSCX4IWEQ5ZRW6IVDTEUHUX" # 32 chars, base32 alphabet + with pytest.raises(ValueError) as exc: + _validate_totp_code(secret) + msg = str(exc.value) + assert "secret" in msg.lower() + assert "pyotp.TOTP(secret).now()" in msg # tells you the fix + assert "32" in msg # tells you what you passed + + +def test_obvious_rubbish_is_rejected(): + for bad in ["", "12345", "not a code", "1" * 40]: + with pytest.raises(ValueError): + _validate_totp_code(bad) + + +def test_non_string_raises_typeerror(): + with pytest.raises(TypeError): + _validate_totp_code(123456) + + +def test_whitespace_is_tolerated(): + """Copy-paste from an authenticator app often carries spaces.""" + assert _validate_totp_code(" 123456 ") == "123456" + + +def test_validation_applies_to_the_callable_branch_too(): + """A callable returning the secret must fail the same way as a literal. + + The callable form is the recommended one, so it is the likelier place to + wire the wrong value up and never notice. + """ + with pytest.raises(ValueError): + _resolve_totp(lambda: "SROSG7JW2QSCX4IWEQ5ZRW6IVDTEUHUX", False) + + +def test_valid_callable_still_resolves_and_does_not_burn_the_static_flag(): + code, used = _resolve_totp(lambda: "654321", False) + assert code == "654321" + assert used is False # callables are re-invocable; only str is single-use + + +def test_valid_static_code_marks_itself_used(): + code, used = _resolve_totp("654321", False) + assert (code, used) == ("654321", True) From 4f6bab333d72e03f43a2530e865ad0fa42dde77c Mon Sep 17 00:00:00 2001 From: ColonistOne Date: Tue, 21 Jul 2026 20:17:46 +0100 Subject: [PATCH 2/3] fix(2fa): remove an invented recovery-code format; diagnose stripped leading zeros MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two corrections from review, both of which the first draft got wrong. 1. THE HYPHEN WAS INVENTED. _RECOVERY_CODE_RE allowed `-`, and a test asserted "AB12-CD34-EF" was valid. I had never seen that format anywhere. Checking 40 real recovery codes across 5 accounts: every one is exactly 16 LOWERCASE HEX characters, no separators. Neither code type contains a non-alphanumeric. A test that pins an invented format is worse than no test — it freezes a guess into the suite and makes the guess look verified. The accepted range stays deliberately wider than the observation (10-16 alphanumerics, any case). The server owns its format; a client rule tighter than reality would reject a valid recovery code at the one moment TOTP is unavailable. But "wider than observed" is not licence to accept characters no code has ever contained. 2. FEWER THAN 6 DIGITS IS NEVER VALID — RFC 4226 sets 6 as the minimum — but it has exactly one realistic cause, and it is worth naming rather than lumping into the generic error. Codes are zero-padded, so str(int(code)) or passing an int turns "012345" into "12345". Measured over 20,000 generated codes: 9.9% begin with "0" and 1.0% with "00". So the bug fires on roughly one attempt in ten and reads as a flaky server rather than a client fault. Both the ValueError and the TypeError now say so. Also: whitespace is normalised away rather than rejected. Authenticators display "123 456"; the space is presentation, not data. "123 456 7" is therefore a valid 7-digit code, which corrected a test of mine that had listed it as a separator. Mutation-verified: neutering the validator turns 11 tests red. Suite 1043 -> 1062. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01TRn9SBFGaxRwZbwRsKNJ7b --- src/colony_sdk/client.py | 34 +++++++++++++++++++++++++++---- tests/test_two_factor.py | 43 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 5 deletions(-) diff --git a/src/colony_sdk/client.py b/src/colony_sdk/client.py index d55ebda..9a8611d 100644 --- a/src/colony_sdk/client.py +++ b/src/colony_sdk/client.py @@ -534,7 +534,13 @@ class ColonyNetworkError(ColonyAPIError): #: this field — the Colony issues 16-character alphanumeric ones — which is why #: this is not a bare ``^\d{6}$`` check. The server caps ``totp_code`` at 16. _TOTP_CODE_RE = re.compile(r"^[0-9]{6,8}$") -_RECOVERY_CODE_RE = re.compile(r"^[A-Za-z0-9-]{10,16}$") +#: Observed across 40 real recovery codes from 5 accounts: exactly 16 lowercase +#: hex characters, no separators. The accepted range is kept deliberately WIDER +#: than that observation -- the server is the authority on its own format, and a +#: client rule tighter than reality would reject a valid recovery code at the one +#: moment TOTP is unavailable. A hyphen was allowed in the first draft of this +#: patch on no evidence at all; removed. +_RECOVERY_CODE_RE = re.compile(r"^[A-Za-z0-9]{10,16}$") #: Base32, the alphabet TOTP *secrets* are shared in (RFC 4648). Used only to #: recognise the one wrong value that is overwhelmingly likely to be passed. @@ -556,15 +562,35 @@ def _validate_totp_code(code: str) -> str: * 6-8 digits — a TOTP code (RFC 6238 allows all three lengths). * 10-16 alphanumerics — a recovery code. **Not** excluded: recovery codes go through this same field, so a strict 6-digit rule would break the one - credential you need when your authenticator is unavailable. + credential you need when your authenticator is unavailable. No separators: + 40 real codes were inspected and every one was 16 lowercase hex characters. + * fewer than 6 digits — rejected, but named as a stripped leading zero, which + is the only way a too-short value realistically arises and which fails on + only ~10% of attempts. * anything that looks like a base32 secret — rejected by name, since that is the error actually made. """ if not isinstance(code, str): - raise TypeError(f"totp must be a str or a callable returning one, got {type(code).__name__}") - code = code.strip() + raise TypeError( + f"totp must be a str or a callable returning one, got {type(code).__name__}. " + "An int is the usual cause and is not merely a type slip: int('012345') " + "is 12345, destroying the leading zero that ~10% of codes carry." + ) + # Whitespace only ever arrives from copy-paste or an authenticator's + # "123 456" display grouping; it is never part of the value. + code = "".join(code.split()) if _TOTP_CODE_RE.match(code) or _RECOVERY_CODE_RE.match(code): return code + if code.isdigit() and 3 <= len(code) < 6: + raise ValueError( + f"totp={code!r} is {len(code)} digits, but a TOTP code is at least 6 " + "(RFC 4226 sets 6 as the minimum, so a shorter value is never valid). " + "The usual cause is a stripped leading zero: codes are zero-padded, so " + "str(int(code)) or an int turns '012345' into '12345'. About 10% of " + "codes begin with a zero and 1% with two, so this fails INTERMITTENTLY " + "and reads as a flaky server rather than a client bug. Keep the code a " + "string -- pyotp's .now() already returns a correctly padded one." + ) if _BASE32_SECRET_RE.match(code): raise ValueError( f"totp looks like your TOTP *secret* ({len(code)} base32 characters), " diff --git a/tests/test_two_factor.py b/tests/test_two_factor.py index b8646fc..7c88360 100644 --- a/tests/test_two_factor.py +++ b/tests/test_two_factor.py @@ -240,16 +240,57 @@ def test_totp_codes_of_rfc_lengths_are_accepted(code): assert _validate_totp_code(code) == code -@pytest.mark.parametrize("code", ["1234abcd5ef678gh", "abc123def4", "AB12-CD34-EF"]) +@pytest.mark.parametrize("code", ["a3f9c1d0e7b45268", "0123456789abcdef", "abc123def4"]) def test_recovery_codes_are_accepted(code): """Recovery codes share the totp_code field. A strict ^\\d{6}$ rule would reject the exact credential you need when the authenticator is unavailable, which is the worst possible time to find out. + + The first two are the REAL observed shape: 16 lowercase hex characters, no + separators (40 codes across 5 accounts, all identical in form). An earlier + draft of this test asserted "AB12-CD34-EF" -- a hyphenated format I had never + seen anywhere. A test that pins an invented format is worse than no test: it + freezes a guess into the suite and makes the guess look verified. """ assert _validate_totp_code(code) == code +def test_separators_are_rejected(): + """No observed Colony code of either kind contains a non-alphanumeric.""" + # NB whitespace is deliberately NOT in this list -- it is display grouping, + # not a separator, and normalises away (see the internal-whitespace test). + for bad in ["AB12-CD34-EF", "a3f9:c1d0:e7b4", "1234_5678_90ab", "12.34.56"]: + with pytest.raises(ValueError): + _validate_totp_code(bad) + + +@pytest.mark.parametrize("code", ["12345", "1234", "123"]) +def test_short_numeric_is_diagnosed_as_a_stripped_leading_zero(code): + """A too-short code is never valid, and has exactly one realistic cause. + + ~10% of TOTP codes start with '0' and 1% with '00' (measured over 20k codes), + so str(int(code)) fails on roughly one attempt in ten -- an intermittent + failure that reads as a flaky server. The message has to name it. + """ + with pytest.raises(ValueError) as exc: + _validate_totp_code(code) + msg = str(exc.value) + assert "leading zero" in msg + assert "INTERMITTENTLY" in msg + + +def test_int_typeerror_explains_the_zero_hazard(): + with pytest.raises(TypeError) as exc: + _validate_totp_code(12345) + assert "leading zero" in str(exc.value) + + +def test_internal_whitespace_from_authenticator_display_is_tolerated(): + """Authenticator apps render codes as "123 456"; the space is not data.""" + assert _validate_totp_code("123 456") == "123456" + + def test_base32_secret_is_rejected_by_name(): """The actual incident: the secret passed where a code was expected.""" secret = "SROSG7JW2QSCX4IWEQ5ZRW6IVDTEUHUX" # 32 chars, base32 alphabet From af3fd014e398336440cc6aab43728d0aa0d069b7 Mon Sep 17 00:00:00 2001 From: ColonistOne Date: Tue, 21 Jul 2026 20:19:38 +0100 Subject: [PATCH 3/3] fix(2fa): reject whitespace outright rather than normalising it away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Jack, on review: "there should be no white space either. There is no display formatting here." Correct, and my justification was borrowed from the wrong context. I had argued that authenticator apps render codes as "123 456", so the space is presentation rather than data — which is true of a human retyping a code off a phone screen, and irrelevant here. This SDK is consumed by programs. Nothing in the path between a generator and `totp=` puts a space in, so a space means the value was assembled wrongly. Stripping it silently would repair the symptom and hide the defect — the same species of helpfulness that let a 32-character secret through as a code and produced this patch in the first place. So whitespace now raises, naming why it is not normalised. "123 456 7" moves back into the separator-rejection set where it belongs. Suite 1043 -> 1064. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01TRn9SBFGaxRwZbwRsKNJ7b --- src/colony_sdk/client.py | 19 ++++++++++++++++--- tests/test_two_factor.py | 22 +++++++++++----------- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/src/colony_sdk/client.py b/src/colony_sdk/client.py index 9a8611d..bf18496 100644 --- a/src/colony_sdk/client.py +++ b/src/colony_sdk/client.py @@ -567,6 +567,9 @@ def _validate_totp_code(code: str) -> str: * fewer than 6 digits — rejected, but named as a stripped leading zero, which is the only way a too-short value realistically arises and which fails on only ~10% of attempts. + * whitespace — rejected, never stripped. Consumers of this SDK are programs; + a space means the value was built wrongly and should be surfaced, not + quietly repaired. * anything that looks like a base32 secret — rejected by name, since that is the error actually made. """ @@ -576,9 +579,19 @@ def _validate_totp_code(code: str) -> str: "An int is the usual cause and is not merely a type slip: int('012345') " "is 12345, destroying the leading zero that ~10% of codes carry." ) - # Whitespace only ever arrives from copy-paste or an authenticator's - # "123 456" display grouping; it is never part of the value. - code = "".join(code.split()) + # Whitespace is NOT normalised away. This SDK is consumed by programs, not + # by a human retyping a code off a phone screen, so there is no display + # grouping to forgive -- a space in this value means the caller built it + # wrongly, and silently repairing it would hide that. Rejecting is also the + # honest position: the server would reject it too, just less clearly. + if any(ch.isspace() for ch in code): + raise ValueError( + f"totp={code!r} contains whitespace. A one-time code has none — no " + "Colony code of either kind contains a non-alphanumeric character. " + "This is not normalised away on purpose: whitespace here means the " + "value was assembled wrongly, and quietly stripping it would hide " + "the defect rather than surface it." + ) if _TOTP_CODE_RE.match(code) or _RECOVERY_CODE_RE.match(code): return code if code.isdigit() and 3 <= len(code) < 6: diff --git a/tests/test_two_factor.py b/tests/test_two_factor.py index 7c88360..fd9f406 100644 --- a/tests/test_two_factor.py +++ b/tests/test_two_factor.py @@ -258,9 +258,7 @@ def test_recovery_codes_are_accepted(code): def test_separators_are_rejected(): """No observed Colony code of either kind contains a non-alphanumeric.""" - # NB whitespace is deliberately NOT in this list -- it is display grouping, - # not a separator, and normalises away (see the internal-whitespace test). - for bad in ["AB12-CD34-EF", "a3f9:c1d0:e7b4", "1234_5678_90ab", "12.34.56"]: + for bad in ["AB12-CD34-EF", "a3f9:c1d0:e7b4", "1234_5678_90ab", "12.34.56", "123 456 7"]: with pytest.raises(ValueError): _validate_totp_code(bad) @@ -286,11 +284,6 @@ def test_int_typeerror_explains_the_zero_hazard(): assert "leading zero" in str(exc.value) -def test_internal_whitespace_from_authenticator_display_is_tolerated(): - """Authenticator apps render codes as "123 456"; the space is not data.""" - assert _validate_totp_code("123 456") == "123456" - - def test_base32_secret_is_rejected_by_name(): """The actual incident: the secret passed where a code was expected.""" secret = "SROSG7JW2QSCX4IWEQ5ZRW6IVDTEUHUX" # 32 chars, base32 alphabet @@ -313,9 +306,16 @@ def test_non_string_raises_typeerror(): _validate_totp_code(123456) -def test_whitespace_is_tolerated(): - """Copy-paste from an authenticator app often carries spaces.""" - assert _validate_totp_code(" 123456 ") == "123456" +@pytest.mark.parametrize("code", [" 123456 ", "123 456", "123\t456", "123456\n"]) +def test_whitespace_is_rejected_not_stripped(code): + """No normalising. This SDK is consumed by programs, not by a human copying + a code off a phone, so there is no display grouping to forgive. A space means + the caller assembled the value wrongly, and repairing it silently would hide + the defect — the same class of helpfulness that let a 32-char secret through + as a code in the first place.""" + with pytest.raises(ValueError) as exc: + _validate_totp_code(code) + assert "whitespace" in str(exc.value) def test_validation_applies_to_the_callable_branch_too():