fix(security): Implement JWKS-based JWT verification

[justanotheratom]

Summary

• Fixed critical security vulnerability where JWT signatures were not being verified in Edge Functions
• Implemented proper JWKS-based JWT verification using the jose library, matching the pattern used in the fly.io backend
• Added fallback for local development where Supabase uses HS256 (symmetric keys) with empty JWKS

Background

Commit 6f41807 (Dec 14) removed working JWKS verification code based on the false assumption that the gateway verifies JWTs. However, verify_jwt = false was still set in config.toml, leaving authentication completely unverified. An attacker could forge tokens with any sub claim and access/delete any user's data.

Changes

File	Change
supabase/functions/import_map.json	Added jose library for JWT verification
supabase/functions/shared/auth.ts	Rewrote with JWKS verification + local dev fallback

Test plan

• Local ping endpoint returns 204 with valid token
• Deploy to staging and verify forged tokens are rejected
• Verify production tokens still work after deployment

🤖 Generated with Claude Code

[claude]

Security Review - PR #18: JWKS-based JWT Verification

Summary

This PR addresses a critical security vulnerability where JWT signatures were not being verified, allowing attackers to forge authentication tokens. The implementation correctly restores JWKS-based verification while maintaining local development compatibility.

---

✅ Positive Aspects

1. Critical Security Fix: Properly addresses the authentication bypass vulnerability introduced in commit 6f41807
2. Correct Implementation: Uses industry-standard jose library with JWKS verification pattern
3. Smart Fallback: Gracefully handles local development where Supabase uses HS256 symmetric keys
4. JWKS Caching: Singleton pattern with caching (5min TTL) prevents unnecessary network requests (supabase/functions/shared/auth.ts:7-16)
5. Claim Validation: Verifies issuer, audience, sub, and exp claims
6. Error Messages: Preserves error context in unauthorized responses

---

🔴 Critical Issues

1. Race Condition in JWKS Initialization (supabase/functions/shared/auth.ts:9-15)

The singleton pattern is not thread-safe in concurrent environments:

function getJwks() {
    if (!_jwks && SUPABASE_URL) {
        _jwks = createRemoteJWKSet(...)  // Multiple threads could create this
    }
    return _jwks
}

Risk: In high-concurrency scenarios, multiple requests could initialize _jwks simultaneously.

Fix: Use double-checked locking or initialize once at module load:

const _jwks = SUPABASE_URL 
    ? createRemoteJWKSet(new URL(`${SUPABASE_URL}/auth/v1/.well-known/jwks.json`))
    : null;

2. Local Development Security Warning (supabase/functions/shared/auth.ts:65-68)

The fallback to unverified decode is dangerous if the local development detection is bypassed.

Recommendation: Add explicit logging when using unverified mode:

if (isJwksError && isLocalDevelopment()) {
    console.warn('[AUTH] Using unverified JWT decode for local development')
    return decodeTokenWithoutVerification(token, ctx)
}

3. Error Message Information Disclosure (supabase/functions/shared/auth.ts:71)

throw new Error(`Unauthorized: ${errorMessage}`)

Risk: Detailed error messages could leak information to attackers about the JWT verification system.

Fix: Log detailed errors server-side but return generic message to client:

console.error('[AUTH] JWT verification failed:', errorMessage)
throw new Error('Unauthorized')

---

⚠️ High Priority Issues

4. Missing Test Coverage for Security-Critical Code

The test plan shows only local testing completed. Production security fixes require comprehensive testing.

Required Tests:

• ✅ Valid token verification (local)
• ❌ Missing: Forged token rejection (critical!)
• ❌ Missing: Expired token rejection
• ❌ Missing: Invalid issuer/audience rejection
• ❌ Missing: Malformed token handling
• ❌ Missing: JWKS fetch failure handling

Action: Add test file supabase/tests/EndToEndTests/jwt_verification.test.ts before merging.

5. Local Development Detection is Too Broad (supabase/functions/shared/auth.ts:75-84)

function isLocalDevelopment(): boolean {
    return SUPABASE_URL.includes('127.0.0.1') ||
           SUPABASE_URL.includes('localhost') ||
           SUPABASE_URL.includes('kong:') || ...
}

Risk: String matching could be fooled by domains like evil-kong.com or if SUPABASE_URL is maliciously configured.

Fix: Use explicit environment variable:

const IS_LOCAL_DEV = Deno.env.get('ENVIRONMENT') === 'local' || 
                     Deno.env.get('SUPABASE_ENV') === 'development';

6. SUPABASE_URL Validation Missing (supabase/functions/shared/auth.ts:4)

const SUPABASE_URL = Deno.env.get('SUPABASE_URL') ?? ''

Risk: Empty string will cause runtime failures. Should fail fast at startup.

Fix:

const SUPABASE_URL = Deno.env.get('SUPABASE_URL')
if (!SUPABASE_URL) {
    throw new Error('SUPABASE_URL environment variable is required')
}

---

📋 Code Quality Issues

7. String-Based Error Detection is Fragile (supabase/functions/shared/auth.ts:60-63)

const isJwksError = errorMessage.includes('no applicable key found') ||
                   errorMessage.includes('JWKSNoMatchingKey') || ...

Problem: Brittle, version-dependent error detection. jose library could change error messages.

Better Approach: Check error type or error code if available:

const isJwksError = error instanceof JWKSError || 
                   error.code === 'ERR_JWKS_NO_MATCHING_KEY';

8. Inconsistent Error Handling Pattern

• Line 71: Preserves error message
• Line 105: Preserves error message

But these are user-facing and could leak information (see issue #3).

---

🔧 Minor Issues

9. Type Safety for Context State (supabase/functions/shared/auth.ts:28-29)

The state checking logic is defensive but verbose. Consider:

if (typeof ctx.state.userId === 'string' && ctx.state.userId) {
    return ctx.state.userId
}

10. Magic Strings for Audience (supabase/functions/shared/auth.ts:46)

audience: 'authenticated',

Extract to constant:

const JWT_AUDIENCE = 'authenticated'
const JWT_ISSUER_PATH = '/auth/v1'

---

🎯 Performance Considerations

11. JWKS Caching is Good, but Undocumented

The jose library handles JWKS caching internally (5min TTL), but this isn't obvious from the code.

Recommendation: Add comment explaining caching behavior.

---

🔒 Security Assessment

Category	Status	Notes
Authentication	✅ Fixed	JWKS verification properly implemented
Authorization	⚠️ Not in scope	Still relies on RLS policies (verify separately)
Input Validation	✅ Good	Claims validated, expiry checked
Error Handling	⚠️ Needs work	Information disclosure risk (issue #3)
Cryptography	✅ Correct	Using jose library, supports ES256/RS256
Local Dev Security	⚠️ Acceptable	Unverified tokens OK for local-only DB

---

📝 Recommendations for Merging

Before merging to production:

1. ✅ Fix critical issue Add user authentication checks to preference functions #1 (JWKS initialization race condition)
2. ✅ Fix issue Preferencevalidator groq #3 (error message disclosure)
3. ✅ Add comprehensive test coverage (issue Add SSE inventory+analysis stream and shared helpers #4)
4. ✅ Add logging for local dev mode (issue Skip analyzer agent for no preferences #2)
5. ⚠️ Consider issue feat(inventory): add Open Food Facts cache with Deno ingest script #5 (environment detection)
6. ⚠️ Validate SUPABASE_URL at startup (issue Feature/family #6)

Post-merge monitoring:

• Monitor for JWKS fetch failures
• Verify no unauthorized access in logs
• Check for increased latency in auth middleware

---

📚 Configuration Review

The config.toml correctly has:

[functions.ingredicheck]
verify_jwt = false
[functions.background]
verify_jwt = false

This is appropriate since the application code now handles verification. However, document why gateway verification is disabled in comments.

---

Final Verdict

Status: ⚠️ APPROVE WITH REQUIRED CHANGES

This PR fixes a critical security vulnerability and the overall approach is sound. However, critical issues #1-3 and test coverage (issue #4) must be addressed before merging to production.

The code quality is good and demonstrates understanding of JWT security best practices. The local development fallback is pragmatic and well-reasoned.

Great work on identifying and fixing this security issue! 🎉

---

Review conducted on commit 4752920

[claude]

Security Review - PR #18: JWT Verification Implementation

I've reviewed this critical security fix. Overall, this is an excellent security improvement that addresses a serious vulnerability. Here's my detailed analysis:

✅ Strengths

1. Critical Security Issue Fixed: Properly addresses the authentication bypass vulnerability where JWT signatures weren't being verified
2. Correct Implementation: Uses industry-standard jose library with JWKS for asymmetric key verification
3. Well-Documented: The PR description clearly explains the vulnerability, fix, and testing approach
4. Appropriate Caching: JWKS client is cached to avoid repeated fetches (jose handles internal caching with 5min TTL)
5. Proper Error Handling: Clear error messages and appropriate fallback behavior

🔒 Security Concerns & Recommendations

HIGH PRIORITY - Local Development Security Gap

Location: supabase/functions/shared/auth.ts:63-65

The fallback to unverified token decoding in local development creates a potential security risk:

if (isLocalDevelopment()) {
    return decodeTokenWithoutVerification(token, ctx)
}

Issues:

1. The isLocalDevelopment() check is based solely on environment variables that could be manipulated
2. An attacker who can control SUPABASE_URL could force the system into "local dev mode" and bypass verification
3. The detection logic (checking for localhost, 127.0.0.1, kong:) could be bypassed with URL manipulation

Recommendation: Use an explicit environment variable flag:

const IS_LOCAL_DEV = Deno.env.get('EDGE_FUNCTION_ENV') !== 'production'

if (IS_LOCAL_DEV) {
    return decodeTokenWithoutVerification(token, ctx)
}

Set this explicitly in your deployment configuration so it cannot be inferred or manipulated.

MEDIUM PRIORITY - JWKS Error Handling

Location: supabase/functions/shared/auth.ts:57-68

The current implementation falls back to unverified tokens for ANY JWKS error in local development. This is too broad.

Issues:

1. Network errors, malformed responses, or actual attacks could trigger the fallback unintentionally
2. The comment says "JWKS verification will always fail locally" but doesn't explain why this is acceptable in production

Recommendation: Be more specific about which errors should trigger the fallback:

} catch (error) {
    const errorMessage = error instanceof Error ? error.message : String(error)
    
    if (IS_LOCAL_DEV && (
        errorMessage.includes('no applicable key found') || 
        errorMessage.includes('empty JWKS')
    )) {
        return decodeTokenWithoutVerification(token, ctx)
    }
    
    throw new Error(`Unauthorized: ${errorMessage}`)
}

🐛 Potential Issues

Token Expiration Check Consistency

Location: supabase/functions/shared/auth.ts:87-90

The expiration check in decodeTokenWithoutVerification() is good, but note that jwtVerify() already validates expiration. Consider documenting this redundancy or making the expiration tolerance configurable if needed.

🔍 Code Quality

1. Good: Type safety with proper TypeScript usage
2. Good: Clear function separation of concerns
3. Good: Helpful comments explaining the local dev fallback
4. Minor: Consider extracting magic strings (like 'authenticated', '/auth/v1') into constants

⚡ Performance Considerations

1. Good: JWKS client is properly cached at module level
2. Good: Context state is checked before re-verifying tokens (line 28-30)
3. Potential Issue: Each request that hits the JWKS endpoint will make a network call if the cache is cold. The jose library handles this, but monitor JWKS endpoint response times.

🧪 Test Coverage

CRITICAL: The test plan shows only 1 of 3 items completed:

• ✅ Local ping endpoint test
• ❌ Staging deployment test with forged tokens
• ❌ Production token verification test

Recommendations:

1. Before merging: Add a test that verifies forged tokens are rejected (even in local dev)
2. Before deploying: Complete the staging tests
3. Consider adding: Unit tests for isLocalDevelopment() edge cases
4. Consider adding: Tests for expired token rejection

📋 Configuration Review

Location: supabase/config.toml:143-149

# Disable gateway JWT verification - our code handles auth
[functions.ingredicheck]
verify_jwt = false

This comment is now accurate - your code does handle auth properly. However, consider:

1. Is there a security benefit to enabling gateway verification and your verification (defense in depth)?
2. Document why you chose to disable gateway verification instead of using both layers

🎯 Final Recommendations

Before Merge:

1. ✅ Fix the local development detection to use an explicit environment variable
2. ✅ Narrow the error conditions that trigger unverified fallback
3. ✅ Add test for forged token rejection
4. ⚠️ Complete the staging deployment tests

After Merge:

1. Monitor authentication errors in production logs
2. Set up alerts for authentication failures
3. Consider implementing rate limiting on auth failures
4. Document the expected JWKS cache behavior

📊 Overall Assessment

This is a critical security fix that should be merged after addressing the high-priority concerns. The implementation is solid, but the local development fallback needs hardening to prevent potential bypass scenarios.

Severity of Original Vulnerability: 🔴 CRITICAL - Complete authentication bypass
Quality of Fix: 🟢 Good - Proper JWKS implementation with minor improvements needed
Risk of Regression: 🟡 Medium - Needs thorough testing in staging before production

Great work identifying and fixing this vulnerability! 🎉