Support CLI RLS tokens and fix categories index#12
Conversation
WalkthroughThe PR widens token parameters to accept Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant createRlsClient
participant resolveSupabaseToken
participant parseSupabaseAccessToken
participant Database
participant Transaction
Client->>createRlsClient: call with token (string | SupabaseToken)
createRlsClient->>resolveSupabaseToken: resolve token
alt token is string
resolveSupabaseToken->>parseSupabaseAccessToken: parse JWT string
parseSupabaseAccessToken-->>resolveSupabaseToken: SupabaseToken claims
else token is SupabaseToken
resolveSupabaseToken-->>createRlsClient: pass-through claims
end
createRlsClient->>Database: set request.jwt.claims / sub / role
createRlsClient->>Database: optionally apply local role
createRlsClient->>Transaction: execute user transaction
alt success
Transaction-->>createRlsClient: result
else failure
Transaction-->>createRlsClient: error (combined details)
end
createRlsClient->>Database: cleanup jwt claims & role
createRlsClient-->>Client: return result or throw
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (2)packages/*/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/*/src/**/{*.test.ts,__tests__/**/*.ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-10-02T12:40:33.718ZApplied to files:
📚 Learning: 2025-09-30T14:08:23.213ZApplied to files:
🧬 Code graph analysis (3)packages/db/src/schema/index.ts (1)
packages/auth/src/account/provision-account.ts (1)
packages/db/src/index.test.ts (1)
🔇 Additional comments (16)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/db/src/schema/index.ts (1)
100-104: Add validation to prevent SQL injection if constant changes.The
sql.rawinjection assumesDEFAULT_CATEGORY_KINDwill always contain SQL-safe values. While the current value"system"is safe, future changes could introduce SQL metacharacters.Consider adding a runtime validation to ensure safety:
+ // Validate that DEFAULT_CATEGORY_KIND is SQL-safe + if (!/^[a-zA-Z0-9_]+$/.test(DEFAULT_CATEGORY_KIND)) { + throw new Error(`DEFAULT_CATEGORY_KIND must be alphanumeric: ${DEFAULT_CATEGORY_KIND}`); + } uniqueIndex("categories_system_name_idx") .on(table.createdBy, table.name) // drizzle-kit stringifies template parameters as placeholders, so use sql.raw // to inject the literal value without producing $1 in the generated migration. .where( sql`${table.kind} = ${sql.raw(`'${DEFAULT_CATEGORY_KIND}'`)}`, ),Alternatively, if drizzle-kit supports it, consider using a WHERE clause with a proper escaped literal rather than raw SQL injection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/auth/src/account/provision-account.ts(3 hunks)packages/db/src/index.test.ts(9 hunks)packages/db/src/index.ts(4 hunks)packages/db/src/schema/index.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
packages/*/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/*/src/**/*.{ts,tsx}: Avoid implicit any; TypeScript is run with strict enabled
Prefer type guards or the satisfies operator over as casts where appropriate
Prefer unknown for external inputs
Use PascalCase for types and enums
Use camelCase for variables and functions
Ensure source comments are written in English
Files:
packages/auth/src/account/provision-account.tspackages/db/src/schema/index.tspackages/db/src/index.test.tspackages/db/src/index.ts
packages/*/src/**/{*.test.ts,__tests__/**/*.ts}
📄 CodeRabbit inference engine (AGENTS.md)
packages/*/src/**/{*.test.ts,__tests__/**/*.ts}: Write test names and descriptions in English
Co-locate tests as *.test.ts or under tests/ using Bun’s test runner
Files:
packages/db/src/index.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: listee-dev/listee-libs#0
File: AGENTS.md:0-0
Timestamp: 2025-10-02T12:40:33.718Z
Learning: Applies to packages/auth/src/authentication/**/*.ts : Extend Supabase JWT verification (createSupabaseAuthentication using .well-known/jwks.json) only via dedicated modules to keep caching and claim validation centralized
📚 Learning: 2025-09-30T14:08:23.213Z
Learnt from: gentamura
PR: listee-dev/listee-libs#2
File: packages/db/src/index.ts:132-137
Timestamp: 2025-09-30T14:08:23.213Z
Learning: In packages/db/src/index.ts, multi-statement SQL execution using tx.execute(sql`...`) with Drizzle and Postgres.js works correctly, so multiple statements can be combined in a single execute call.
Applied to files:
packages/db/src/index.test.ts
🧬 Code graph analysis (3)
packages/auth/src/account/provision-account.ts (1)
packages/db/src/index.ts (2)
SupabaseToken(154-163)RlsClient(430-432)
packages/db/src/schema/index.ts (1)
packages/db/src/constants/category.ts (1)
DEFAULT_CATEGORY_KIND(2-2)
packages/db/src/index.test.ts (1)
packages/db/src/index.ts (2)
createRlsClient(434-542)parseSupabaseAccessToken(380-387)
🔇 Additional comments (15)
packages/db/src/schema/index.ts (1)
13-13: LGTM: Explicit .js extension follows ESM conventions.The import path now includes the explicit
.jsextension, which is required for proper ESM module resolution.packages/db/src/index.test.ts (4)
212-225: LGTM: Test helpers correctly implement base64url encoding for JWTs.The
encodeSegmentandcreateAccessTokenfunctions properly implement base64url encoding and JWT structure for test purposes. The empty signature (trailing.) is appropriate since these tests focus on payload parsing rather than signature verification.
227-280: LGTM: Comprehensive test coverage for RLS setup and teardown.The test properly validates the expanded RLS transaction sequence (8 queries) including:
- JWT claims configuration (request.jwt.claims, request.jwt.claim.sub, request.jwt.claim.role)
- Local role assignment
- Proper cleanup in reverse order
The test also verifies that invalid role values (containing hyphens) are correctly sanitized to "anon" as expected by the
sanitizeRolefunction.
294-313: LGTM: Good coverage for role permission errors.This test properly simulates a PostgreSQL permission denied error (code 42501) and verifies that the enhanced error handling provides a descriptive message to guide developers toward granting the necessary role membership.
315-340: LGTM: Comprehensive coverage for string token handling.These tests validate both the integration (passing JWT strings to
createRlsClient) and the unit behavior (directparseSupabaseAccessTokencalls), ensuring the new token parsing functionality works correctly.packages/db/src/index.ts (9)
1-1: LGTM: Proper use of Node.js Buffer.Correctly imports Buffer from the
node:bufferbuilt-in module for JWT decoding.
165-218: LGTM: Robust type guards for token validation.The type guards properly validate the structure of Supabase tokens with correct handling of optional fields and the union type for
aud(string or string array).
220-344: LGTM: Well-designed error handling utilities.The error extraction logic properly:
- Traverses error cause chains
- Prevents infinite loops with a visited set
- Deduplicates messages
- Formats PostgreSQL error details comprehensively
This will provide much better error diagnostics for RLS transactions.
346-387: Verify that tokens are already authenticated upstream.The JWT parsing functions correctly decode and validate the structure of Supabase access tokens, but they do not verify cryptographic signatures. This is acceptable only if tokens are already verified by Supabase authentication before reaching this code.
Based on learnings from the authentication module, ensure that JWT verification is handled by the dedicated authentication layer (using
.well-known/jwks.json) before tokens reach the database layer.
389-395: LGTM: Clean union type resolution.The
resolveSupabaseTokenfunction properly handles both token input types, leveraging the type guard to determine which code path to take.
397-420: LGTM: Comprehensive role permission error detection.The function correctly identifies PostgreSQL role permission errors using both message patterns and standard error codes (42501, 0A000, 28000).
434-502: LGTM: Well-structured RLS context setup with proper error handling.The implementation correctly:
- Resolves token claims from either SupabaseToken objects or JWT strings
- Sets all required JWT-related PostgreSQL session parameters
- Registers cleanup tasks immediately after each setup operation
- Provides actionable error messages for role permission failures
- Chains errors properly for debugging
503-522: LGTM: Robust cleanup logic with configurable error propagation.The cleanup mechanism properly:
- Executes tasks in reverse (LIFO) order
- Continues cleanup even if individual tasks fail
- Propagates cleanup errors on the success path
- Suppresses cleanup errors on the failure path to preserve the original error context
524-537: LGTM: Enhanced error reporting for RLS transactions.The error handling properly:
- Attempts cleanup even on transaction failure
- Suppresses cleanup errors to preserve the original failure context
- Aggregates error details from the cause chain
- Formats PostgreSQL-specific error information (code, detail, hint, etc.)
- Maintains error causality for debugging
This will significantly improve the developer experience when debugging RLS issues.
packages/auth/src/account/provision-account.ts (1)
19-19: LGTM: Consistent type widening for flexible token handling.The parameter type changes from
SupabaseTokentoSupabaseToken | stringare consistent across the module and align with the enhanced token handling in the database layer. This maintains backward compatibility while enabling CLI and other consumers to pass JWT strings directly.Also applies to: 29-29, 54-60
| await tx.execute(sql` | ||
| select set_config('request.jwt.claim.role', ${roleSetting}, TRUE); | ||
| `); | ||
| cleanupTasks.push(async () => { | ||
| await tx.execute(sql` | ||
| select set_config('request.jwt.claim.role', NULL, TRUE); | ||
| `); | ||
| }); |
There was a problem hiding this comment.
Supabase’s auth.role() helper reads request.jwt.claim.role, so we mirror PostgREST and populate the claim in addition to set local role.
1 participant
Summary
Testing
Summary by CodeRabbit
New Features
Improvements
Tests