diff --git a/src/colony_sdk/client.py b/src/colony_sdk/client.py index c654d5f..bf18496 100644 --- a/src/colony_sdk/client.py +++ b/src/colony_sdk/client.py @@ -529,6 +529,97 @@ 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}$") +#: 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. +_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. 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. + * 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. + """ + if not isinstance(code, str): + 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 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: + 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), " + "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 +636,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 +647,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..fd9f406 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,114 @@ 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", ["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.""" + 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) + + +@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_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) + + +@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(): + """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)