perf: switch JWT verification from network call to local decode (OPE-75)#253
Conversation
Before: Every authenticated request (23 routes) made a network call
to self.client.auth.get_user(token) on Supabase's API. Added latency
to every request and created a hard dependency on Supabase availability.
After: jwt.decode() with HS256 secret verifies locally in microseconds.
Zero network calls for auth. Falls back to API call only if
SUPABASE_JWT_SECRET is not configured.
Changes:
- verify_jwt() now tries local decode first via _verify_local()
- Falls back to _verify_via_api() when jwt_secret is empty
- Specific error messages for expired, invalid audience, bad signature
- No longer leaks internal error details (was: detail=f'...{str(e)}')
- Removed unused 'created_at' from return dict (no consumer uses it)
- Added logger import for debug-level decode failure logging
Tests: 9 new tests in test_jwt_local_decode.py covering:
- Valid token decode, Bearer prefix stripping
- Expired token, wrong secret, missing sub, wrong audience
- Verifies NO network call made (assert_not_called on get_user)
- API fallback when jwt_secret is empty
- Metadata defaults to empty dict
279 tests pass (270 existing + 9 new).
Closes OPE-75
|
@DevanshuNEU is attempting to deploy a commit to the Dev's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds dual-path JWT verification to the auth service: when SUPABASE_JWT_SECRET is set, tokens are decoded and validated locally (HS256, audience check, expiry); otherwise verification falls back to Supabase API. Includes new unit tests covering local decoding behaviors and the API fallback. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AuthService
participant JWTDecoder
participant SupabaseAPI
Client->>AuthService: verify_jwt(token)
AuthService->>AuthService: Strip "Bearer " prefix (case-insensitive)
alt SUPABASE_JWT_SECRET configured
AuthService->>JWTDecoder: _verify_local(token)
alt Local verification succeeds
JWTDecoder-->>AuthService: {user_id, email, metadata}
AuthService-->>Client: User data (200)
else Local verification fails
JWTDecoder-->>AuthService: Error (expired/invalid/wrong audience/missing sub)
AuthService-->>Client: HTTP 401
end
else No SUPABASE_JWT_SECRET
AuthService->>SupabaseAPI: _verify_via_api(token)
alt API verification succeeds
SupabaseAPI-->>AuthService: {user_id, email, metadata}
AuthService-->>Client: User data (200)
else API verification fails
SupabaseAPI-->>AuthService: Error
AuthService-->>Client: HTTP 401
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/tests/test_jwt_local_decode.py (2)
22-33: Convert fixture toyieldfor proper scope and teardown.Using
returncauses bothpatchcontext managers to exit the moment the fixture function returns, i.e., before any test body runs. It works today because__init__is complete and the MagicMock is already stored asself.client, but the intent of apatchcontext is to remain active for the test's lifetime. Usingyieldkeeps the patches live during the test, prevents issues if a test accidentally triggers env-var re-reads, and allows teardown logic if needed later.♻️ Proposed refactor
`@pytest.fixture` def auth_service(): """Create auth service with local JWT secret configured.""" with patch("services.auth.create_client") as mock_client: mock_client.return_value = MagicMock() with patch.dict("os.environ", { "SUPABASE_URL": "https://test.supabase.co", "SUPABASE_ANON_KEY": "test-key", "SUPABASE_JWT_SECRET": JWT_SECRET, }): from services.auth import SupabaseAuthService - return SupabaseAuthService() + yield SupabaseAuthService()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_jwt_local_decode.py` around lines 22 - 33, The auth_service fixture currently returns a SupabaseAuthService while the patch context managers have already exited; change the fixture to a yield-style fixture so the patches to services.auth.create_client and the patched os.environ remain active for the duration of the test. In the auth_service fixture, create the patches (patch("services.auth.create_client") and patch.dict("os.environ", {...})), instantiate SupabaseAuthService, yield that instance, and then let the context managers tear down after yield (or use try/finally to stop them if you open them manually); ensure the symbol names auth_service and SupabaseAuthService are used so the test uses the yielded instance while patches are active.
97-101:test_metadata_defaults_to_empty_dictdoesn't cover an explicitnulluser_metadatain the payload.This test only exercises the absent-key path. The related bug in
_verify_local(payload.get("user_metadata", {})returnsNonewhen the key is present but set tonull) would not be caught. Consider adding:def test_metadata_defaults_to_empty_dict_when_null(self, auth_service): token = _make_token({"sub": "user-null-meta", "email": "x@y.com", "user_metadata": None}) result = auth_service.verify_jwt(token) assert result["metadata"] == {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_jwt_local_decode.py` around lines 97 - 101, Add a test that covers an explicit null user_metadata and fix _verify_local to handle a present-but-null value: add a new test named test_metadata_defaults_to_empty_dict_when_null that creates a token with "user_metadata": None and asserts auth_service.verify_jwt(token)["metadata"] == {}; and in the authentication code update _verify_local so it uses a falsy-coalescing expression (e.g., metadata = payload.get("user_metadata") or {}) instead of payload.get("user_metadata", {}) so a present None becomes an empty dict.backend/services/auth.py (1)
88-91:_verify_via_apimakes a blocking synchronous network call.
self.client.auth.get_user(token)is a blocking I/O operation. Per coding guidelines, I/O operations should use async/await. While this is pre-existing behavior (not introduced by this PR), the newverify_jwtrouting makes the sync contract more visible.verify_jwtand_verify_via_apishould eventually be madeasyncso the event loop isn't blocked on the fallback path.As per coding guidelines: "Use async/await for I/O operations in Python backend".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/auth.py` around lines 88 - 91, _verify_via_api currently performs blocking I/O via self.client.auth.get_user(token); change its signature to async def _verify_via_api(self, token: str) and make verify_jwt async as well so callers can await it, then execute the blocking client call off the event loop (e.g., use asyncio.get_running_loop().run_in_executor(None, lambda: self.client.auth.get_user(token))) and await that result inside _verify_via_api; update verify_jwt to await self._verify_via_api(token) so the fallback path no longer blocks the event loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/services/auth.py`:
- Around line 65-69: The returned "metadata" currently uses
payload.get("user_metadata", {}) which will still yield None if "user_metadata"
exists but is null; change it to use a null-coalescing pattern like
payload.get("user_metadata") or {} so metadata is always a dict. Update the
return in the function that builds {"user_id": user_id, "email":
payload.get("email"), "metadata": ...} to use payload.get("user_metadata") or {}
(matching the behavior in the _verify_via_api counterpart) so downstream code
can safely index result["metadata"].
- Around line 38-39: The Authorization "Bearer " strip is currently
case-sensitive and will miss variants like "bearer" or "BEARER"; update the
logic that processes the token variable (the block that does if
token.startswith("Bearer "): token = token[7:]) to treat the scheme
case-insensitively—e.g., check token.lower().startswith("bearer ") and then
remove the first len("bearer ") characters (or split on whitespace and take the
credential) before calling jwt.decode(); ensure this change is applied wherever
the raw Authorization header is parsed so jwt.decode() receives only the token.
---
Nitpick comments:
In `@backend/services/auth.py`:
- Around line 88-91: _verify_via_api currently performs blocking I/O via
self.client.auth.get_user(token); change its signature to async def
_verify_via_api(self, token: str) and make verify_jwt async as well so callers
can await it, then execute the blocking client call off the event loop (e.g.,
use asyncio.get_running_loop().run_in_executor(None, lambda:
self.client.auth.get_user(token))) and await that result inside _verify_via_api;
update verify_jwt to await self._verify_via_api(token) so the fallback path no
longer blocks the event loop.
In `@backend/tests/test_jwt_local_decode.py`:
- Around line 22-33: The auth_service fixture currently returns a
SupabaseAuthService while the patch context managers have already exited; change
the fixture to a yield-style fixture so the patches to
services.auth.create_client and the patched os.environ remain active for the
duration of the test. In the auth_service fixture, create the patches
(patch("services.auth.create_client") and patch.dict("os.environ", {...})),
instantiate SupabaseAuthService, yield that instance, and then let the context
managers tear down after yield (or use try/finally to stop them if you open them
manually); ensure the symbol names auth_service and SupabaseAuthService are used
so the test uses the yielded instance while patches are active.
- Around line 97-101: Add a test that covers an explicit null user_metadata and
fix _verify_local to handle a present-but-null value: add a new test named
test_metadata_defaults_to_empty_dict_when_null that creates a token with
"user_metadata": None and asserts auth_service.verify_jwt(token)["metadata"] ==
{}; and in the authentication code update _verify_local so it uses a
falsy-coalescing expression (e.g., metadata = payload.get("user_metadata") or
{}) instead of payload.get("user_metadata", {}) so a present None becomes an
empty dict.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
backend/tests/test_jwt_local_decode.py (2)
22-33:auth_servicefixture is missing a return type annotation.The fixture creates and returns a
SupabaseAuthServiceinstance but has no return type annotation. A-> SupabaseAuthService(or-> Generator[SupabaseAuthService, None, None]if converted toyield) keeps it consistent with the codebase guideline.♻️ Proposed fix
+from services.auth import SupabaseAuthService `@pytest.fixture` -def auth_service(): +def auth_service() -> SupabaseAuthService:As per coding guidelines, "Type hints on all function signatures in Python backend."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_jwt_local_decode.py` around lines 22 - 33, The fixture function auth_service lacks a return type hint; update its signature to include an explicit return annotation for SupabaseAuthService (or Generator[SupabaseAuthService, None, None] if you convert it to a yield-style fixture) so the function is declared as returning SupabaseAuthService, keeping the test suite consistent with the project's "Type hints on all function signatures" guideline and making the returned SupabaseAuthService from the SupabaseAuthService() constructor explicit.
97-101:test_metadata_defaults_to_empty_dictonly covers the absent-key case, not the explicit-nullcase.The currently failing scenario from the unresolved past review comment —
user_metadatakey present but set tonullin the payload — is not exercised. The existing test passes even without the bug fix becausepayload.get("user_metadata", {})does return{}when the key is absent. A separate case for the null value is needed to act as a regression guard once the fix is applied.🧪 Suggested additional test
def test_metadata_null_value_defaults_to_empty_dict(self, auth_service): """user_metadata present but explicitly null should still yield {}.""" token = _make_token({"sub": "user-null-meta", "email": "x@y.com", "user_metadata": None}) result = auth_service.verify_jwt(token) assert result["metadata"] == {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_jwt_local_decode.py` around lines 97 - 101, Add a new test to cover the case where the JWT payload contains "user_metadata" explicitly set to None: create a token with {"sub": "user-null-meta", "email": "x@y.com", "user_metadata": None}, call auth_service.verify_jwt(token) (same as in test_metadata_defaults_to_empty_dict) and assert that the returned result["metadata"] == {}; this ensures verify_jwt handles payload.get("user_metadata", {}) when the key exists but is null and prevents regression once the fix for handling None is applied.backend/services/auth.py (2)
51-56: Consider addingissuervalidation tojwt.decode()for defence-in-depth.
jwt.decodein PyJWT 2.8.0 accepts anissuerkeyword argument that validates theissclaim. Currently, any JWT signed with the same HS256 key but carrying a different issuer passes verification. Supabase tokens setissto{SUPABASE_URL}/auth/v1, andself.supabase_urlis already available in the instance.
jwt.InvalidIssuerErroris a subclass ofjwt.InvalidTokenError, so no additionalexceptclause is needed — the catch-all at line 81 handles it automatically.🛡️ Proposed addition
payload = jwt.decode( token, self.jwt_secret, algorithms=["HS256"], audience="authenticated", + issuer=f"{self.supabase_url}/auth/v1", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/auth.py` around lines 51 - 56, Add issuer validation to the JWT decoding call: update the jwt.decode(...) invocation in auth.py (the call to jwt.decode in the method that uses self.jwt_secret) to pass issuer=self.supabase_url + "/auth/v1" (or the exact issuer format Supabase uses) so the decoder validates the token's "iss" claim; keep the existing exception handling (jwt.InvalidTokenError catch) because jwt.InvalidIssuerError is a subclass and will be handled already.
88-111:_verify_via_apimakes a synchronous blocking network call that violates the async/await guideline.
self.client.auth.get_user(token)is a blocking I/O operation. This method is only called as a fallback whenjwt_secretis not configured (line 42-46), but when invoked, it runs synchronously within async dependency functions (get_current_user,get_optional_user) and async route handlers.The primary local-decode path (
_verify_local) avoids this entirely and is the expected production path whenjwt_secretis properly configured. However, the fallback violates the guideline: "Use async/await for I/O operations in Python backend."Making
_verify_via_apiandverify_jwtasync and using Supabase's async client (if available) would address this fully, though the impact is contained to the fallback scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/auth.py` around lines 88 - 111, The _verify_via_api fallback is performing blocking I/O; make it async and switch to the Supabase async client (or an async http call) and await the get_user call: change def _verify_via_api(self, token: str) -> Dict[...] to async def _verify_via_api(self, token: str) -> Dict[...] (or use an async client attribute like self.async_client.auth.get_user and await it), preserve the same error handling (re-raise HTTPException, catch Exception and log with the error), and update verify_jwt (and its callers get_current_user and get_optional_user) to be async so they await verify_jwt/_verify_via_api; ensure logging uses exception info instead of a blocking stack trace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/services/auth.py`:
- Around line 38-39: The code currently strips only a case-sensitive "Bearer "
prefix using token.startswith("Bearer "), so update the Authorization header
handling to be case-insensitive and robust: check the prefix using
token.lower().startswith("bearer ") (or split on whitespace and compare the
first part lowercased) and then remove the prefix accordingly, also trim
surrounding whitespace from token after stripping; apply this change where the
token variable is processed (the existing token.startswith("Bearer ") block).
- Around line 65-69: The returned dict uses payload.get("user_metadata", {})
which still yields None when the key exists but is null; update the return to
coalesce None to an empty dict (e.g., use payload.get("user_metadata") or {} )
so metadata is never None; locate the return in backend/services/auth.py where
the payload variable is used (the block returning "user_id", "email",
"metadata") and replace the metadata expression accordingly.
---
Nitpick comments:
In `@backend/services/auth.py`:
- Around line 51-56: Add issuer validation to the JWT decoding call: update the
jwt.decode(...) invocation in auth.py (the call to jwt.decode in the method that
uses self.jwt_secret) to pass issuer=self.supabase_url + "/auth/v1" (or the
exact issuer format Supabase uses) so the decoder validates the token's "iss"
claim; keep the existing exception handling (jwt.InvalidTokenError catch)
because jwt.InvalidIssuerError is a subclass and will be handled already.
- Around line 88-111: The _verify_via_api fallback is performing blocking I/O;
make it async and switch to the Supabase async client (or an async http call)
and await the get_user call: change def _verify_via_api(self, token: str) ->
Dict[...] to async def _verify_via_api(self, token: str) -> Dict[...] (or use an
async client attribute like self.async_client.auth.get_user and await it),
preserve the same error handling (re-raise HTTPException, catch Exception and
log with the error), and update verify_jwt (and its callers get_current_user and
get_optional_user) to be async so they await verify_jwt/_verify_via_api; ensure
logging uses exception info instead of a blocking stack trace.
In `@backend/tests/test_jwt_local_decode.py`:
- Around line 22-33: The fixture function auth_service lacks a return type hint;
update its signature to include an explicit return annotation for
SupabaseAuthService (or Generator[SupabaseAuthService, None, None] if you
convert it to a yield-style fixture) so the function is declared as returning
SupabaseAuthService, keeping the test suite consistent with the project's "Type
hints on all function signatures" guideline and making the returned
SupabaseAuthService from the SupabaseAuthService() constructor explicit.
- Around line 97-101: Add a new test to cover the case where the JWT payload
contains "user_metadata" explicitly set to None: create a token with {"sub":
"user-null-meta", "email": "x@y.com", "user_metadata": None}, call
auth_service.verify_jwt(token) (same as in test_metadata_defaults_to_empty_dict)
and assert that the returned result["metadata"] == {}; this ensures verify_jwt
handles payload.get("user_metadata", {}) when the key exists but is null and
prevents regression once the fix for handling None is applied.
…arer, yield fixture
1. metadata: payload.get('user_metadata', {}) returns None when key
exists with null value. Changed to 'or {}' pattern so downstream
code can safely index result['metadata'].
2. Bearer prefix: was case-sensitive ('Bearer '), now handles
'bearer ', 'BEARER ' etc. Defensive -- most callers never hit
this but WebSocket handlers pass raw query params.
3. Test fixture: changed from return to yield so patches stay active
for the full test lifetime (correct pytest pattern).
4. Added 2 tests: null metadata coalescing, case-insensitive Bearer.
281 tests pass (270 existing + 11 new).
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/services/auth.py (2)
51-56: Consider requiringexpclaim and adding clock-skewleeway.
expis only verified if present in the payload; claims not inrequireare silently skipped if absent. A crafted token without anexpclaim passes_verify_localwith no expiration. Supabase always includesexp, but addingoptions={"require": ["exp"]}makes the expectation explicit and guards against non-Supabase tokens signed with the same secret.PyJWT also supports the
leewayparameter, which lets you validate a token whose expiration is in the past but not very far — useful to absorb clock drift between services.🔒 Proposed hardening
payload = jwt.decode( token, self.jwt_secret, algorithms=["HS256"], audience="authenticated", + options={"require": ["exp", "sub"]}, + leeway=10, ) - - user_id = payload.get("sub") - if not user_id: - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail="Token missing subject claim", - )With
"sub"inrequire, PyJWT raisesMissingRequiredClaimError(a subclass ofInvalidTokenError) whensubis absent, so the manualif not user_idguard and itsHTTPExceptioncan be removed — the existingexcept jwt.InvalidTokenErrorcatches it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/auth.py` around lines 51 - 56, The jwt.decode call in _verify_local (using token and self.jwt_secret) should require the exp claim and add leeway to tolerate clock skew: call jwt.decode with options={"require": ["exp"]} and pass a leeway value (e.g., leeway=some_seconds) along with audience="authenticated" and algorithms=["HS256"]; optionally include "sub" in the require list so missing subject raises MissingRequiredClaimError and you can remove the manual user_id missing check — adjust exception handling to continue catching jwt.InvalidTokenError as before.
88-111:_verify_via_apican block the event loop whenSUPABASE_JWT_SECRETis missing.
self.client.auth.get_user(token)is a synchronous blocking call. Although the normal path (withSUPABASE_JWT_SECRETconfigured) takes the fast, non-blocking_verify_localroute, the fallback at line 45-46 will invoke this blocking method if the secret is missing. Sinceverify_jwtisdef(notasync), the blocking call serializes requests through the dependency injection chain when this path is taken.In production with proper configuration this doesn't manifest, but the fallback violates the coding guideline: "Use async/await for I/O operations in Python backend."
To future-proof the fallback path, convert to async using
asyncio.to_thread:♻️ Async wrapper for blocking fallback
+ async def _verify_via_api(self, token: str) -> Dict[str, Any]: - def _verify_via_api(self, token: str) -> Dict[str, Any]: """Fallback: verify via Supabase API call (requires network).""" + import asyncio try: - response = self.client.auth.get_user(token) + response = await asyncio.to_thread(self.client.auth.get_user, token)Also make
verify_jwtasync to propagate the change:+ async def verify_jwt(self, token: str) -> Dict[str, Any]: - def verify_jwt(self, token: str) -> Dict[str, Any]: ... if self.jwt_secret: return self._verify_local(token) - return self._verify_via_api(token) + return await self._verify_via_api(token)And update callers in
middleware/auth.py(lines 70-78, 192-194, 209-211) toawait:- user = auth_service.verify_jwt(token) + user = await auth_service.verify_jwt(token)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/auth.py` around lines 88 - 111, The fallback _verify_via_api currently performs a blocking call to self.client.auth.get_user(token); make _verify_via_api an async def and run the blocking call via asyncio.to_thread (e.g., await asyncio.to_thread(self.client.auth.get_user, token)) and keep the same return/exception behavior; then change verify_jwt to async def and await the new async _verify_via_api where it is invoked; finally update all callers that call verify_jwt (the middleware auth usages) to await verify_jwt so the async flow is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/services/auth.py`:
- Around line 65-69: The return block that sets "metadata":
payload.get("user_metadata") or {} is correct and requires no change; it
properly coalesces an explicitly null user_metadata into an empty dict and
matches the behavior in the _verify_via_api counterpart, so leave the return
dict (the keys "user_id", "email", and "metadata" and the
payload.get("user_metadata") usage) as-is.
- Around line 38-39: No change required: the current case-insensitive Bearer
prefix stripping using if token.lower().startswith("bearer "): token = token[7:]
is correct and safe for all capitalization variants of "Bearer"; leave the token
handling in auth.py as-is.
---
Nitpick comments:
In `@backend/services/auth.py`:
- Around line 51-56: The jwt.decode call in _verify_local (using token and
self.jwt_secret) should require the exp claim and add leeway to tolerate clock
skew: call jwt.decode with options={"require": ["exp"]} and pass a leeway value
(e.g., leeway=some_seconds) along with audience="authenticated" and
algorithms=["HS256"]; optionally include "sub" in the require list so missing
subject raises MissingRequiredClaimError and you can remove the manual user_id
missing check — adjust exception handling to continue catching
jwt.InvalidTokenError as before.
- Around line 88-111: The fallback _verify_via_api currently performs a blocking
call to self.client.auth.get_user(token); make _verify_via_api an async def and
run the blocking call via asyncio.to_thread (e.g., await
asyncio.to_thread(self.client.auth.get_user, token)) and keep the same
return/exception behavior; then change verify_jwt to async def and await the new
async _verify_via_api where it is invoked; finally update all callers that call
verify_jwt (the middleware auth usages) to await verify_jwt so the async flow is
preserved.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/services/auth.py (1)
88-111:_verify_via_apiblocks the event loop — violates the async I/O guideline.
self.client.auth.get_user(token)is a synchronous HTTP call. When any of the 23 async route handlers take the fallback path, this blocks the entire asyncio event loop for the duration of the network round-trip.The least-invasive fix without making
verify_jwtasync is to offload the blocking call to a thread-pool executor:♻️ Proposed refactor
- def _verify_via_api(self, token: str) -> Dict[str, Any]: - """Fallback: verify via Supabase API call (requires network).""" - try: - response = self.client.auth.get_user(token) + async def _verify_via_api(self, token: str) -> Dict[str, Any]: + """Fallback: verify via Supabase API call (requires network).""" + import asyncio + loop = asyncio.get_event_loop() + try: + response = await loop.run_in_executor( + None, self.client.auth.get_user, token + )
verify_jwtwould also need to becomeasync defandawait self._verify_via_api(token), which propagates to the 23 route handlers — but given the primary path is already synchronous and fast, an acceptable middle ground is therun_in_executorapproach above to unblock the loop only in the fallback.As per coding guidelines: "Use async/await for I/O operations in Python backend."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/auth.py` around lines 88 - 111, _verify_via_api currently performs a blocking HTTP call (self.client.auth.get_user) which blocks the asyncio loop; change _verify_via_api to async def _verify_via_api(self, token: str) and offload the blocking call to a threadpool with await asyncio.get_running_loop().run_in_executor(None, lambda: self.client.auth.get_user(token)), then keep the same error handling/return shape; update verify_jwt to await self._verify_via_api(token) and propagate the async change to its callers (the route handlers) so the fallback does not block the event loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/services/auth.py`:
- Around line 51-56: The jwt.decode call in the token verification (the call
that passes token, self.jwt_secret, algorithms=["HS256"],
audience="authenticated") should include a small leeway to tolerate clock skew;
update that call to add a leeway parameter (e.g., leeway=30) so expiry checks
allow a short grace period without changing other verification args.
---
Duplicate comments:
In `@backend/services/auth.py`:
- Around line 38-39: The Bearer-prefix stripping using
token.lower().startswith("bearer ") with token = token[7:] is correct for casing
variants; no code change required—keep the existing check and slice as-is, but
if you want to be defensive ensure the variable token is a non-empty string
before calling .lower() (e.g., guard token is truthy) to avoid AttributeError.
---
Nitpick comments:
In `@backend/services/auth.py`:
- Around line 88-111: _verify_via_api currently performs a blocking HTTP call
(self.client.auth.get_user) which blocks the asyncio loop; change
_verify_via_api to async def _verify_via_api(self, token: str) and offload the
blocking call to a threadpool with await
asyncio.get_running_loop().run_in_executor(None, lambda:
self.client.auth.get_user(token)), then keep the same error handling/return
shape; update verify_jwt to await self._verify_via_api(token) and propagate the
async change to its callers (the route handlers) so the fallback does not block
the event loop.
jwt.decode now tolerates 30s clock drift between servers. Expired token test updated: -10s was within leeway, now -60s. 281 tests pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
… postmortem tests
Production auth outage: SUPABASE_JWT_SECRET was a placeholder
('dev-secret-key') on Railway. PR OpenCodeIntel#253 switched JWT verification
from Supabase API calls to local decode. Local decode against a
wrong secret silently failed, causing 401 for all authenticated users.
Layer 1 -- Graceful fallback (services/auth.py):
When local JWT decode fails with InvalidTokenError, try Supabase
API verification before giving up. Wrong secret now degrades to
slow auth (API call per request) instead of broken auth (401).
Expired tokens and missing claims still fail immediately (no
point retrying those).
Layer 2 -- Startup validation:
- auth.py __init__: detects placeholder secrets (< 32 chars or
known placeholders like 'dev-secret-key'). Nulls them out and
logs error with instructions to get the real secret from
Supabase dashboard. Forces API verification path.
- startup_checks.py: warns if JWT secret is suspiciously short.
Layer 3 -- Postmortem tests (3 new):
- test_wrong_secret_falls_back_to_api: THE production scenario.
JWT secret is set but wrong. Verifies auth succeeds via API
fallback instead of returning 401.
- test_expired_token_does_not_fallback: expired is expired.
Verifies we don't waste an API call on genuinely expired tokens.
- test_placeholder_secret_nulled_at_startup: verifies
'dev-secret-key' gets detected and nulled at init time.
Updated 2 existing tests:
- test_wrong_secret_raises_error -> test_wrong_secret_falls_back_not_raises
- test_wrong_audience_raises_error -> test_wrong_audience_falls_back_not_raises
Both now verify fallback behavior instead of asserting errors.
292 tests pass (289 + 3 new). Zero flake8 errors.
1 participant
What
Every authenticated request (23 routes) was making a network call to Supabase's API via
self.client.auth.get_user(token). This added latency to every API call and created a hard dependency on Supabase availability.Fix
verify_jwt()now decodes the JWT locally usingjwt.decode()with the HS256 signing secret. Zero network calls. Instant verification.Falls back to API-based verification only if
SUPABASE_JWT_SECRETis not configured (graceful degradation).Changes
Details
detail=f'...{str(e)}'removed)created_atfrom return dict (zero consumers)Tests
9 tests covering: valid decode, Bearer prefix, expired token, wrong secret, missing sub, wrong audience, no network call verification, API fallback, metadata defaults.
279 total tests pass (270 existing + 9 new).
Impact
All 23 authenticated routes get faster. Auth no longer depends on Supabase network availability.
Closes OPE-75
Summary by CodeRabbit
Improvements
Tests