From 4004090b4539808bfa5cd1fca8c417c3ff5134ac Mon Sep 17 00:00:00 2001
From: Rasmus Widing <rasmus.widing@gmail.com>
Date: Sat, 16 Aug 2025 00:23:37 +0300
Subject: [PATCH] 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
---
 python/src/server/config/config.py       | 22 +++++++++++++++++++---
 python/tests/test_supabase_validation.py | 24 +++++++++---------------
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/python/src/server/config/config.py b/python/src/server/config/config.py
index c225aebe..560fbd77 100644
--- a/python/src/server/config/config.py
+++ b/python/src/server/config/config.py
@@ -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
diff --git a/python/tests/test_supabase_validation.py b/python/tests/test_supabase_validation.py
index 9035dee2..1e24e91a 100644
--- a/python/tests/test_supabase_validation.py
+++ b/python/tests/test_supabase_validation.py
@@ -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():