Skip to content

Commit 8568be2

Browse files
docs(oauth): add comments addressing code review feedback
- Document intentional RLS bypass in generateCrossOriginToken() - Document intentional RLS bypass in connected_accounts identity check - Add note about SQL identifier sources being trusted metaschema config - Expand isEmailVerified() docs with provider field mappings and TODO Addresses comments from @pyramation on PR #1220 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent f19bc7b commit 8568be2

1 file changed

Lines changed: 25 additions & 0 deletions

File tree

graphql/server/src/middleware/oauth.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@
1515
* All DB queries run through `req.constructive.withPgClient()` which
1616
* applies pgSettings (role, claims, request_id) via SET LOCAL, replacing
1717
* the manual `set_config()` calls in the original implementation.
18+
*
19+
* SQL Identifiers: Schema/table/function names are interpolated into SQL strings.
20+
* These values come exclusively from metaschema module config (trusted, admin-written).
21+
* They are not user input and follow PostgreSQL identifier naming conventions.
1822
*/
1923

2024
import crypto from 'crypto';
@@ -217,6 +221,10 @@ async function generateCrossOriginToken(
217221
RETURNING id
218222
`;
219223

224+
// Intentional RLS bypass: This runs as the connection pool user (postgres/superuser)
225+
// because we're updating a session that was just created server-side during OAuth callback.
226+
// The user hasn't authenticated via JWT yet, so RLS would block this write.
227+
// Security: The accessToken hash ensures we only update the session we just created.
220228
const result = await ctx.pool.query(sql, [otToken, accessToken]);
221229
if (result.rows.length === 0) {
222230
throw new Error('Failed to set cross-origin token');
@@ -239,6 +247,19 @@ function getBaseUrl(req: Request): string {
239247
* Extract email_verified from the raw provider response.
240248
* OAuthProfile.raw contains the original provider data which includes
241249
* email_verified for OIDC providers (Google, etc.).
250+
*
251+
* Known provider field mappings:
252+
* - Google (OIDC): email_verified (boolean)
253+
* - Google (legacy): verified_email (boolean)
254+
* - GitHub: emails[].verified (requires separate API call, not in profile)
255+
* - Microsoft: email_verified (boolean, OIDC)
256+
* - Apple: email_verified (boolean, OIDC)
257+
*
258+
* For providers that don't include email_verified in the profile (e.g. GitHub),
259+
* this returns false. The admin can set oauth_require_verified_email=false
260+
* in app_settings_auth to allow sign-in without verification.
261+
*
262+
* TODO: Consider adding emailVerified to OAuthProfile type upstream in @constructive-io/oauth
242263
*/
243264
function isEmailVerified(profile: OAuthProfile): boolean {
244265
const raw = profile.raw as Record<string, unknown> | null;
@@ -512,6 +533,10 @@ export function createOAuthRoutes(_opts: ConstructiveOptions): Router {
512533
WHERE service = $1 AND identifier = $2
513534
LIMIT 1
514535
`;
536+
// Intentional RLS bypass: This is a pre-auth lookup for an anonymous user who
537+
// cannot query connected_accounts via RLS. We need to check if the OAuth identity
538+
// already exists to decide whether to call sign_in_identity or sign_up_identity.
539+
// Security: Query only checks existence by service+identifier, no sensitive data returned.
515540
const checkResult = await ctx.pool.query(checkSql, [
516541
profile.provider,
517542
profile.providerId,

0 commit comments

Comments
 (0)