fix!: reject invalid JWTs immediately instead of falling through to next auth mode

[homanp]

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

When allow is an array (e.g. ['user', 'always']), an invalid JWT (malformed, expired, wrong signature) silently falls through to the next auth mode via catch { return null } in tryMode. A garbage JWT like "this.is.garbage" succeeds with authType: 'always' instead of returning a 401.

This happens because the code does not distinguish between "no credential present" (legitimate fallthrough) and "credential present but invalid" (should reject).

What is the new behavior?

tryMode now returns a three-value result:

• AuthResult credential matched, auth succeeded
• null no relevant credential present, safe to try next mode
• INVALID credential present but failed verification, reject immediately

When a JWT is present but fails verification, verifyCredentials immediately returns an INVALID_CREDENTIALS error instead of trying the next mode.

public/secret modes are unchanged, the apikey header is shared between both, so a key that doesn't match secret should still be allowed to try public.

Note: This PR also changes the behavior when a JWT verifies cryptographically but has no sub claim (or sub isn't a string), previously this returned null (fallthrough to next mode), now it returns INVALID (immediate rejection). This is consistent with the present-but-invalid principle applied throughout the rest of the PR.

Tests added:

• Invalid JWT with ['user', 'always'] → rejected
• Expired JWT with ['user', 'always'] → rejected
• No token with ['user', 'always'] → correctly falls through to always
• Invalid JWT with ['user', 'public'] + valid apikey → rejected

All 112 tests pass. Build and typecheck pass.

[kallebysantos]

First of all, thx for your contribution 💚

From your PR you're assuming that allow list should be AND but its OR — At least one should be valid.
You can check which method passed by looking inside ctx.auth.authType

Applying AND operator doesn't make sense here, since you can't have an allow: [..., "always"] at same time it forces valid user token or anything else.

cc @tomaspozo

[homanp]

First of all, thx for your contribution 💚

From your PR you're assuming that allow list should be AND but its OR — At least one should be valid. You can check which method passed by looking inside ctx.auth.authType

Applying AND operator doesn't make sense here, since you can't have an allow: [..., "always"] at same time it forces valid user token or anything else.

cc @tomaspozo

Thanks for the review! To clarify, the PR does not change the allow list from OR to AND. The OR logic (first match wins) is fully preserved.

The change only affects one case: when a credential is present but invalid. Today, sending a garbage JWT like "this.is.garbage" to an endpoint with allow: ['user', 'always'] silently succeeds with authType: 'always', because the catch {} in the user mode returns null (same as "no credential"), causing fallthrough. At least from tests.

The fix distinguishes between:

• No credential present (token: null) → return null → try next mode (OR behavior, unchanged)
• Credential present but invalid (malformed/expired JWT) → return INVALID → reject immediately

Without this fix, one can send any garbage in the Authorization: Bearer header to downgrade from user to always mode, and if the handler branches on ctx.auth.authType to use supabaseAdmin for always-mode requests, that's an RLS bypass?

[tomaspozo]

Thanks for digging into this @homanp, and for the clear explanation. I agree that a present-but-invalid JWT shouldn't silently downgrade. Distinguishing "credential present but invalid" from "credential absent" is easier to reason about than collapsing both into fallthrough. Good catch!

A couple of small asks before we merge:

1. Test for allow: ['user', 'secret'] with an invalid JWT + valid secret apikey, mirroring the ['user', 'public'] case. This is where the behavior change is most visible to existing callers, today they'd authenticate via secret, after this PR they'd get InvalidCredentialsError, so locking it in with a test would be great.

2. Test for the sub-missing path. The change at verify-credentials.ts:168-170 also flips return null → return INVALID when a JWT verifies cryptographically but has no sub claim (or sub isn't a string). Consistent with the rest of the PR, but it's a second behavior change worth covering. Sign a token without sub and assert InvalidCredentialsError. Worth a brief note in the PR description too.

3. BREAKING CHANGE: footer in the commit. Since this changes the outcome for clients sending an invalid token alongside a valid apikey, something like:

BREAKING CHANGE: when multiple auth modes are allowed, a present-but-invalid
JWT is now rejected with InvalidCredentialsError instead of falling through
to the next mode. Clients that previously relied on silent fallthrough
(e.g., stale token + valid apikey) must now either omit the Authorization
header or refresh the token.

Happy to handle the docs updates on my end once this lands. Thanks again!

[homanp]

Thanks for digging into this @homanp, and for the clear explanation. I agree that a present-but-invalid JWT shouldn't silently downgrade. Distinguishing "credential present but invalid" from "credential absent" is easier to reason about than collapsing both into fallthrough. Good catch!

A couple of small asks before we merge:

1. Test for allow: ['user', 'secret'] with an invalid JWT + valid secret apikey, mirroring the ['user', 'public'] case. This is where the behavior change is most visible to existing callers, today they'd authenticate via secret, after this PR they'd get InvalidCredentialsError, so locking it in with a test would be great.
2. Test for the sub-missing path. The change at verify-credentials.ts:168-170 also flips return null → return INVALID when a JWT verifies cryptographically but has no sub claim (or sub isn't a string). Consistent with the rest of the PR, but it's a second behavior change worth covering. Sign a token without sub and assert InvalidCredentialsError. Worth a brief note in the PR description too.
3. BREAKING CHANGE: footer in the commit. Since this changes the outcome for clients sending an invalid token alongside a valid apikey, something like:

BREAKING CHANGE: when multiple auth modes are allowed, a present-but-invalid
JWT is now rejected with InvalidCredentialsError instead of falling through
to the next mode. Clients that previously relied on silent fallthrough
(e.g., stale token + valid apikey) must now either omit the Authorization
header or refresh the token.

Happy to handle the docs updates on my end once this lands. Thanks again!

Got it, will update shortly.

[homanp]

@tomaspozo done.

[tomaspozo]

Pushed a docs: commit (10f1560) onto this branch with the doc + SKILL updates to match the new no-fallthrough behavior. Scope:

• docs/auth-modes.md, docs/security.md, docs/error-handling.md, docs/api-reference.md, README.md: note that absent credentials still fall through, but a present-but-invalid JWT rejects with InvalidCredentialsError.
• skills/supabase-server/SKILL.md: same note, plus a targeted callout in the allow: 'always' section about the ['user', 'always'] pattern.
• TSDoc on Allow, WithSupabaseConfig.allow, and verifyCredentials.

No behavior changes and no new tests — the existing test suite (114 passing) still covers the runtime side. Bundling the docs into this PR so they ship in the same release as the breaking change.