Fix critical issues from code review
- Use python-jose (already in dependencies) instead of PyJWT for JWT decoding - Make unknown Supabase key roles fail fast per alpha principles - Skip all JWT validations (not just signature) when checking role - Update tests to expect failure for unknown roles Fixes: - No need to add PyJWT dependency - python-jose provides JWT functionality - Unknown key types now raise ConfigurationError instead of warning - JWT decode properly skips all validations to only check role claim
This commit is contained in:
parent
3800280f2e
commit
4004090b45
@ -6,7 +6,7 @@ import os
|
||||
from dataclasses import dataclass
|
||||
from urllib.parse import urlparse
|
||||
|
||||
import jwt
|
||||
from jose import jwt
|
||||
|
||||
|
||||
class ConfigurationError(Exception):
|
||||
@ -64,7 +64,18 @@ def validate_supabase_key(supabase_key: str) -> tuple[bool, str]:
|
||||
try:
|
||||
# Decode JWT without verification to check the 'role' claim
|
||||
# We don't verify the signature since we only need to check the role
|
||||
decoded = jwt.decode(supabase_key, options={"verify_signature": False})
|
||||
# Also skip all other validations (aud, exp, etc) since we only care about the role
|
||||
decoded = jwt.decode(
|
||||
supabase_key,
|
||||
'',
|
||||
options={
|
||||
"verify_signature": False,
|
||||
"verify_aud": False,
|
||||
"verify_exp": False,
|
||||
"verify_nbf": False,
|
||||
"verify_iat": False
|
||||
}
|
||||
)
|
||||
role = decoded.get("role")
|
||||
|
||||
if role == "anon":
|
||||
@ -134,7 +145,12 @@ def load_environment_config() -> EnvironmentConfig:
|
||||
)
|
||||
elif key_message.startswith("UNKNOWN_KEY_TYPE:"):
|
||||
role = key_message.split(":", 1)[1]
|
||||
print(f"WARNING: Unknown Supabase key role '{role}'. Proceeding but may cause issues.")
|
||||
raise ConfigurationError(
|
||||
f"CRITICAL: Unknown Supabase key role '{role}'.\n\n"
|
||||
f"Expected 'service_role' but found '{role}'.\n"
|
||||
f"This key type is not supported and will likely cause failures.\n\n"
|
||||
f"Please use a valid service_role key from your Supabase dashboard."
|
||||
)
|
||||
# For UNABLE_TO_VALIDATE, we continue silently
|
||||
|
||||
# Optional environment variables with defaults
|
||||
|
||||
@ -4,7 +4,7 @@ Tests the JWT-based validation of anon vs service keys.
|
||||
"""
|
||||
|
||||
import pytest
|
||||
import jwt
|
||||
from jose import jwt
|
||||
from unittest.mock import patch, MagicMock
|
||||
|
||||
from src.server.config.config import (
|
||||
@ -131,12 +131,8 @@ def test_config_handles_invalid_jwt():
|
||||
assert config.supabase_service_key == "invalid-jwt-key"
|
||||
|
||||
|
||||
def test_config_warns_on_unknown_role():
|
||||
"""Test that configuration loading warns for unknown roles.
|
||||
|
||||
NOTE: This currently prints a warning but doesn't fail.
|
||||
TODO: Per alpha principles, unknown key types should fail fast, not just warn.
|
||||
"""
|
||||
def test_config_fails_on_unknown_role():
|
||||
"""Test that configuration loading fails fast for unknown roles per alpha principles."""
|
||||
# Create a mock key with unknown role
|
||||
unknown_payload = {"role": "custom_role", "iss": "supabase"}
|
||||
mock_unknown_key = jwt.encode(unknown_payload, "secret", algorithm="HS256")
|
||||
@ -151,15 +147,13 @@ def test_config_warns_on_unknown_role():
|
||||
},
|
||||
clear=True # Clear all env vars to ensure isolation
|
||||
):
|
||||
with patch("builtins.print") as mock_print:
|
||||
# Should not raise an exception but should print warning
|
||||
config = load_environment_config()
|
||||
assert config.supabase_service_key == mock_unknown_key
|
||||
# Should raise ConfigurationError for unknown role
|
||||
with pytest.raises(ConfigurationError) as exc_info:
|
||||
load_environment_config()
|
||||
|
||||
# Check that warning was printed
|
||||
mock_print.assert_called_once()
|
||||
call_args = mock_print.call_args[0][0]
|
||||
assert "WARNING: Unknown Supabase key role 'custom_role'" in call_args
|
||||
error_message = str(exc_info.value)
|
||||
assert "Unknown Supabase key role 'custom_role'" in error_message
|
||||
assert "Expected 'service_role'" in error_message
|
||||
|
||||
|
||||
def test_config_raises_on_anon_key_with_port():
|
||||
|
||||
Loading…
Reference in New Issue
Block a user