Skip to content

Commit aff15fe

Browse files
committed
feat: add metrics, logging, and ASCII validation to token flows
Enhance all token validation flows with consistent observability and security improvements: **Metrics & Logging**: - Add increment_auth_failure() to all token type and sub claim validation errors across refresh, verify, and reset flows - Add category_logger.warning() for all validation failures to match get_jwt_user() pattern - Provides consistent observability across all authentication flows **Security Enhancement**: - Add .isascii() check to all user_id string validation - Prevents acceptance of non-ASCII digit characters (e.g., superscript digits) that .isdigit() would accept - Applied consistently across all 4 token validation flows **Test Coverage**: - Add tests for string 'sub' claim conversion (refresh, verify, reset, jwt_auth) - Add test for JWT with invalid signature (jwt.InvalidTokenError path) - Achieve 100% test coverage (663 tests passing) All changes maintain backward compatibility and follow existing patterns in get_jwt_user() implementation.
1 parent 2bac442 commit aff15fe

File tree

3 files changed

+173
-4
lines changed

3 files changed

+173
-4
lines changed

app/managers/auth.py

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,7 @@ async def refresh(
178178
or len(refresh_token.refresh) > MAX_JWT_TOKEN_LENGTH
179179
or not is_valid_jwt_format(refresh_token.refresh)
180180
):
181+
increment_auth_failure("invalid_token", "refresh_token")
181182
raise HTTPException(
182183
status.HTTP_401_UNAUTHORIZED, ResponseMessages.INVALID_TOKEN
183184
)
@@ -195,15 +196,29 @@ async def refresh(
195196
if not isinstance(token_type, str) or not secrets.compare_digest(
196197
token_type, "refresh"
197198
):
199+
increment_auth_failure("invalid_token", "refresh_token")
200+
category_logger.warning(
201+
"Refresh attempted with invalid or missing token type",
202+
LogCategory.AUTH,
203+
)
198204
raise HTTPException(
199205
status.HTTP_401_UNAUTHORIZED, ResponseMessages.INVALID_TOKEN
200206
)
201207

202208
user_id = payload.get("sub")
203209
# Accept int-like strings but reject weird types early
204-
if isinstance(user_id, str) and user_id.isdigit():
210+
if (
211+
isinstance(user_id, str)
212+
and user_id.isascii()
213+
and user_id.isdigit()
214+
):
205215
user_id = int(user_id)
206216
if isinstance(user_id, bool) or not isinstance(user_id, int):
217+
increment_auth_failure("invalid_token", "refresh_token")
218+
category_logger.warning(
219+
"Refresh attempted with invalid or missing 'sub' claim",
220+
LogCategory.AUTH,
221+
)
207222
raise HTTPException(
208223
status.HTTP_401_UNAUTHORIZED, ResponseMessages.INVALID_TOKEN
209224
)
@@ -275,15 +290,29 @@ async def verify(code: str, session: AsyncSession) -> None:
275290
if not isinstance(token_type, str) or not secrets.compare_digest(
276291
token_type, "verify"
277292
):
293+
increment_auth_failure("invalid_token", "verify_email")
294+
category_logger.warning(
295+
"Email verification with invalid/missing token type",
296+
LogCategory.AUTH,
297+
)
278298
raise HTTPException(
279299
status.HTTP_401_UNAUTHORIZED, ResponseMessages.INVALID_TOKEN
280300
)
281301

282302
user_id = payload.get("sub")
283303
# Accept int-like strings but reject weird types early
284-
if isinstance(user_id, str) and user_id.isdigit():
304+
if (
305+
isinstance(user_id, str)
306+
and user_id.isascii()
307+
and user_id.isdigit()
308+
):
285309
user_id = int(user_id)
286310
if isinstance(user_id, bool) or not isinstance(user_id, int):
311+
increment_auth_failure("invalid_token", "verify_email")
312+
category_logger.warning(
313+
"Email verification with invalid/missing 'sub' claim",
314+
LogCategory.AUTH,
315+
)
287316
raise HTTPException(
288317
status.HTTP_401_UNAUTHORIZED, ResponseMessages.INVALID_TOKEN
289318
)
@@ -413,15 +442,29 @@ async def reset_password(
413442
if not isinstance(token_type, str) or not secrets.compare_digest(
414443
token_type, "reset"
415444
):
445+
increment_auth_failure("invalid_token", "password_reset")
446+
category_logger.warning(
447+
"Password reset with invalid/missing token type",
448+
LogCategory.AUTH,
449+
)
416450
raise HTTPException(
417451
status.HTTP_401_UNAUTHORIZED, ResponseMessages.INVALID_TOKEN
418452
)
419453

420454
user_id = payload.get("sub")
421455
# Accept int-like strings but reject weird types early
422-
if isinstance(user_id, str) and user_id.isdigit():
456+
if (
457+
isinstance(user_id, str)
458+
and user_id.isascii()
459+
and user_id.isdigit()
460+
):
423461
user_id = int(user_id)
424462
if isinstance(user_id, bool) or not isinstance(user_id, int):
463+
increment_auth_failure("invalid_token", "password_reset")
464+
category_logger.warning(
465+
"Password reset with invalid/missing 'sub' claim",
466+
LogCategory.AUTH,
467+
)
425468
raise HTTPException(
426469
status.HTTP_401_UNAUTHORIZED, ResponseMessages.INVALID_TOKEN
427470
)
@@ -576,7 +619,7 @@ async def get_jwt_user( # noqa: C901
576619

577620
user_id = payload.get("sub")
578621
# Accept int-like strings but reject weird types early
579-
if isinstance(user_id, str) and user_id.isdigit():
622+
if isinstance(user_id, str) and user_id.isascii() and user_id.isdigit():
580623
user_id = int(user_id)
581624
if isinstance(user_id, bool) or not isinstance(user_id, int):
582625
increment_auth_failure("invalid_token", "jwt")

tests/unit/test_auth_manager.py

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -757,3 +757,73 @@ async def test_reset_password_missing_sub_claim(self, test_db) -> None:
757757
)
758758
assert exc_info.value.status_code == status.HTTP_401_UNAUTHORIZED
759759
assert exc_info.value.detail == ResponseMessages.INVALID_TOKEN
760+
761+
@pytest.mark.asyncio
762+
async def test_refresh_string_sub_claim(self, test_db) -> None:
763+
"""Test refresh accepts string 'sub' claim and converts to int."""
764+
await UserManager.register(self.test_user, test_db)
765+
# Create a JWT with string 'sub' claim
766+
token_with_string_sub = jwt.encode(
767+
{
768+
"typ": "refresh",
769+
"sub": "1", # String instead of int
770+
"exp": datetime.now(tz=timezone.utc).timestamp() + 3600,
771+
},
772+
get_settings().secret_key,
773+
algorithm="HS256",
774+
)
775+
# Should succeed because we convert "1" to 1
776+
result = await AuthManager.refresh(
777+
TokenRefreshRequest(refresh=token_with_string_sub), test_db
778+
)
779+
assert isinstance(result, str)
780+
781+
@pytest.mark.asyncio
782+
async def test_verify_string_sub_claim(self, test_db) -> None:
783+
"""Test verify accepts string 'sub' claim and converts to int."""
784+
# Create unverified user by passing BackgroundTasks
785+
background_tasks = BackgroundTasks()
786+
await UserManager.register(
787+
self.test_user, test_db, background_tasks=background_tasks
788+
)
789+
# Create a JWT with string 'sub' claim
790+
token_with_string_sub = jwt.encode(
791+
{
792+
"typ": "verify",
793+
"sub": "1", # String instead of int
794+
"exp": datetime.now(tz=timezone.utc).timestamp() + 600,
795+
},
796+
get_settings().secret_key,
797+
algorithm="HS256",
798+
)
799+
# Should succeed because we convert "1" to 1
800+
with pytest.raises(HTTPException) as exc_info:
801+
await AuthManager.verify(token_with_string_sub, test_db)
802+
# Verify it succeeded with HTTP 200
803+
assert exc_info.value.status_code == status.HTTP_200_OK
804+
# Verify the user was marked as verified
805+
user = await UserManager.get_user_by_id(1, test_db)
806+
assert user.verified is True
807+
808+
@pytest.mark.asyncio
809+
async def test_reset_password_string_sub_claim(self, test_db) -> None:
810+
"""Test reset_password accepts string 'sub' and converts to int."""
811+
await UserManager.register(self.test_user, test_db)
812+
813+
# Create a JWT with string 'sub' claim
814+
token_with_string_sub = jwt.encode(
815+
{
816+
"typ": "reset",
817+
"sub": "1", # String instead of int
818+
"exp": datetime.now(tz=timezone.utc).timestamp() + 1800,
819+
},
820+
get_settings().secret_key,
821+
algorithm="HS256",
822+
)
823+
new_password = "newpassword123!" # noqa: S105
824+
# Should succeed because we convert "1" to 1
825+
# If this raises an exception, the test will fail
826+
await AuthManager.reset_password(
827+
token_with_string_sub, new_password, test_db
828+
)
829+
# Success - the string "1" was converted to int 1

tests/unit/test_jwt_auth.py

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,62 @@ async def test_jwt_auth_missing_sub_rejected(self, test_db, mocker) -> None:
132132
assert exc.value.status_code == status.HTTP_401_UNAUTHORIZED
133133
assert exc.value.detail == ResponseMessages.INVALID_TOKEN
134134

135+
async def test_jwt_auth_string_sub_claim(self, test_db, mocker) -> None:
136+
"""Test with a token containing string sub claim."""
137+
token, _ = await UserManager.register(self.test_user, test_db)
138+
# Create a JWT with string 'sub' instead of int
139+
token = jwt.encode(
140+
{
141+
"typ": "access",
142+
"sub": "1", # String instead of int
143+
"exp": datetime.datetime.now(tz=datetime.timezone.utc)
144+
+ datetime.timedelta(minutes=10),
145+
},
146+
get_settings().secret_key,
147+
algorithm="HS256",
148+
)
149+
mock_req = mocker.patch(self.mock_request_path)
150+
mock_req.headers = {"Authorization": f"Bearer {token}"}
151+
mock_credentials = HTTPAuthorizationCredentials(
152+
scheme="Bearer", credentials=token
153+
)
154+
155+
result = await get_jwt_user(
156+
request=mock_req, db=test_db, credentials=mock_credentials
157+
)
158+
159+
assert isinstance(result, User)
160+
assert result.email == self.test_user["email"]
161+
assert result.id == 1
162+
163+
async def test_jwt_auth_wrong_signature(self, test_db, mocker) -> None:
164+
"""Test with a token with wrong signature."""
165+
await UserManager.register(self.test_user, test_db)
166+
# Create a JWT with wrong signature
167+
token = jwt.encode(
168+
{
169+
"typ": "access",
170+
"sub": 1,
171+
"exp": datetime.datetime.now(tz=datetime.timezone.utc)
172+
+ datetime.timedelta(minutes=10),
173+
},
174+
"wrong_secret_key",
175+
algorithm="HS256",
176+
)
177+
mock_req = mocker.patch(self.mock_request_path)
178+
mock_req.headers = {"Authorization": f"Bearer {token}"}
179+
mock_credentials = HTTPAuthorizationCredentials(
180+
scheme="Bearer", credentials=token
181+
)
182+
183+
with pytest.raises(HTTPException) as exc:
184+
await get_jwt_user(
185+
request=mock_req, db=test_db, credentials=mock_credentials
186+
)
187+
188+
assert exc.value.status_code == status.HTTP_401_UNAUTHORIZED
189+
assert exc.value.detail == ResponseMessages.INVALID_TOKEN
190+
135191
async def test_jwt_auth_no_auth_header(self, test_db, mocker) -> None:
136192
"""Test with no authorization header."""
137193
mock_req = mocker.patch(self.mock_request_path)

0 commit comments

Comments
 (0)