feat: Implement Google Calendar OAuth connection flow

[AlecKriebel]

Summary

• Implement Google Calendar OAuth2 integration with full token management
• Add secure token storage using AES-GCM encryption via Web Crypto API
• Create API routes for OAuth flow initiation, callback, and connection management
• Include automatic token refresh logic when access tokens expire

Changes

• Database: New calendar_connections table with encrypted token storage
• Encryption: New packages/shared/src/encryption.ts with AES-GCM encrypt/decrypt utilities
• Environment: Added GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET, TOKEN_ENCRYPTION_SECRET variables
• API Routes:
  • GET /api/calendar/authorize - Initiates OAuth flow, redirects to Google
  • GET /api/calendar/callback - Handles OAuth callback, exchanges code for tokens
  • GET /api/calendar/connection - Returns connection status and available calendars
  • PATCH /api/calendar/connection - Updates selected calendar IDs
  • DELETE /api/calendar/connection - Disconnects Google Calendar

Scopes Requested

• calendar.readonly - Read calendar metadata
• calendar.events.readonly - Read calendar events
• userinfo.email - Get user's Google email for display

Test Plan

• Configure Google Cloud OAuth credentials
• Set up environment variables (GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET, TOKEN_ENCRYPTION_SECRET)
• Run database migration (yarn db:migrate-local)
• Test OAuth flow by navigating to /api/calendar/authorize
• Verify callback stores tokens correctly
• Test token refresh by waiting for token expiry
• Test calendar listing and selection

🤖 Generated with Claude Code

[claude]

Code review

Found 1 issue that needs attention:

Token refresh failure is silently ignored

Location: packages/web/lib/calendar/calendar-connection.ts:121-132

Issue:
If refreshAccessToken() returns credentials without the expected access_token or expiry_date fields, the condition on line 121 evaluates to false and the code falls through. The function then returns the original expired accessToken with no indication that refresh failed.

According to the Google APIs library, the Credentials type has optional fields (access_token?: string | null, expiry_date?: number | null), so this scenario is possible.

Impact:

• Silent failure: Callers have no way to detect that token refresh failed
• Expired tokens are returned as if valid
• Downstream API calls will fail with authentication errors

Suggested fix:
Throw an error when refresh fails to provide complete credentials:

if (isExpired) {
  const newCredentials = await refreshAccessToken(refreshToken);

  if (\!newCredentials.access_token || \!newCredentials.expiry_date) {
    throw new Error('Failed to refresh access token: incomplete credentials returned');
  }

  accessToken = newCredentials.access_token;
  // ... rest of update logic
}

Reference:

calendarsync/packages/web/lib/calendar/calendar-connection.ts

Lines 117 to 133 in a7c74fa

	if (isExpired) {
	// Refresh the token
	const newCredentials = await refreshAccessToken(refreshToken);
	if (newCredentials.access_token && newCredentials.expiry_date) {
	accessToken = newCredentials.access_token;
	// Update stored tokens
	await upsertCalendarConnection(userId, {
	accessToken,
	refreshToken: newCredentials.refresh_token || refreshToken,
	expiryDate: newCredentials.expiry_date,
	googleEmail: connection.googleEmail || undefined,
	calendarIds: JSON.parse(connection.calendarIds),
	});
	}
	}

[AlecKriebel]

@superintendent please address review comments

[AlecKriebel]

Review Feedback Addressed

Fixed the token refresh failure issue:

Problem: When refreshAccessToken() returned incomplete credentials (missing access_token or expiry_date), the code silently fell through and returned the original expired token. This would cause downstream API calls to fail with authentication errors without any indication of the root cause.

Solution: Changed the conditional logic to explicitly check for missing credentials and throw an error:

if (!newCredentials.access_token || !newCredentials.expiry_date) {
  throw new Error(
    "Failed to refresh access token: incomplete credentials returned"
  );
}

This ensures callers are notified when token refresh fails, rather than receiving an expired token that will cause authentication failures downstream.

@claude please re-review

[claude]

Claude finished @AlecKriebel's task in 2m 9s —— View job

---

Re-review Complete ✓

Todo List

• Verify token refresh fix in calendar-connection.ts
• Review encryption implementation
• Review database schema and migrations
• Review OAuth flow implementation
• Review API routes (authorize, callback, connection)
• Check for security issues
• Check for error handling and edge cases
• Provide comprehensive feedback

---

Review Findings

✅ Token Refresh Fix Verified

The original issue has been properly addressed. The code now throws an error when refreshAccessToken() returns incomplete credentials:

if (!newCredentials.access_token || !newCredentials.expiry_date) {
  throw new Error(
    "Failed to refresh access token: incomplete credentials returned"
  );
}

This ensures callers are notified when token refresh fails rather than receiving an expired token. Good fix!

---

Additional Issues Found

1. Hardcoded Salt Weakens Encryption Security ⚠️

Location: packages/shared/src/encryption.ts:25

Issue:
The PBKDF2 key derivation uses a hardcoded salt value:

salt: encoder.encode("calendarsync-token-encryption"),

Impact:

• Hardcoded salts significantly weaken PBKDF2's security
• All tokens across all users derive keys from the same secret+salt combination
• If one token is compromised, cryptanalysis becomes easier for all tokens
• Reduces the effective security benefit of using PBKDF2 vs direct key derivation

Best Practice:
PBKDF2 salts should be random and unique per encryption operation (or at minimum, per user). The salt should be stored alongside the ciphertext.

Recommendation:
Either:

1. Generate a random salt per encryption and prepend it to the output: salt + iv + ciphertext
2. Or derive user-specific salts: salt: encoder.encode(calendarsync-${userId})
3. Or switch to direct key derivation with proper key length validation (simpler, if secret is already 32+ bytes)

Reference: https://github.com/Kriebel-LLC/calendarsync/blob/task/6f8c6516-implement-google-calendar-oaut/packages/shared/src/encryption.ts#L25

---

2. Missing Error Handling for Decryption Failures

Location: packages/web/lib/calendar/calendar-connection.ts:104-111

Issue:
Token decryption can fail (corrupted data, wrong secret, tampered data), but there's no try-catch around the decryption calls:

let accessToken = await decryptToken(
  connection.accessTokenEncrypted,
  encryptionSecret
);
const refreshToken = await decryptToken(
  connection.refreshTokenEncrypted,
  encryptionSecret
);

Impact:

• Decryption failures will throw uncaught exceptions, causing 500 errors
• Users with corrupted tokens will be unable to use the system without manual DB cleanup
• No logging to help diagnose decryption issues

Recommendation:
Wrap in try-catch and handle gracefully:

try {
  let accessToken = await decryptToken(...);
  const refreshToken = await decryptToken(...);
} catch (err) {
  req.log.error("Token decryption failed", { error: err, userId });
  // Delete corrupted connection and return null to force re-auth
  await deleteCalendarConnection(userId);
  return null;
}

Reference: https://github.com/Kriebel-LLC/calendarsync/blob/task/6f8c6516-implement-google-calendar-oaut/packages/web/lib/calendar/calendar-connection.ts#L104-L111

---

3. Race Condition in Token Refresh Logic

Location: packages/web/lib/calendar/calendar-connection.ts:117-136

Issue:
If multiple requests call getCalendarTokens() simultaneously when the token is expired, they will all attempt to refresh the token concurrently. This can cause:

• Multiple refresh requests to Google (rate limiting concerns)
• Race condition where updates overwrite each other
• Potential token invalidation if Google invalidates old refresh tokens

Impact:

• Under high load or with parallel requests, token refresh may fail unpredictably
• Could lead to user sessions being terminated due to refresh failures

Recommendation:
Implement token refresh locking/caching:

• Use in-memory cache/mutex to ensure only one refresh happens at a time per user
• Or check token expiry again after acquiring lock
• Or use optimistic locking with version numbers in DB

This is a common pattern for token refresh - many implementations use a per-user lock or cache the refreshed token briefly.

Reference: https://github.com/Kriebel-LLC/calendarsync/blob/task/6f8c6516-implement-google-calendar-oaut/packages/web/lib/calendar/calendar-connection.ts#L117-L136

---

4. JSON Parsing Without Error Handling

Location: packages/web/lib/calendar/calendar-connection.ts:135,142

Issue:
JSON.parse(connection.calendarIds) is called without error handling. If the stored value is corrupted or invalid JSON, this will throw and crash the request:

calendarIds: JSON.parse(connection.calendarIds),
googleEmail: connection.googleEmail || undefined,

Impact:

• Corrupted calendar IDs data causes 500 errors
• No graceful fallback for invalid JSON

Recommendation:
Add safe JSON parsing with fallback:

let calendarIds: string[] = [];
try {
  calendarIds = JSON.parse(connection.calendarIds);
} catch {
  req.log.warn("Invalid calendarIds JSON", { userId });
  calendarIds = [];
}

Or create a helper function for safe JSON parsing that returns a default value on error.

Reference: https://github.com/Kriebel-LLC/calendarsync/blob/task/6f8c6516-implement-google-calendar-oaut/packages/web/lib/calendar/calendar-connection.ts#L135

---

5. Missing Index on Foreign Key

Location: packages/shared/src/db/schema.ts:133 and packages/shared/src/db/migrations/0002_dusty_mojo.sql

Issue:
The calendar_connections table has a userId column that references the users table, but there's only a unique index, not a proper foreign key constraint:

userId: text("user_id", { length: 191 }).notNull(),

Impact:

• Orphaned records if users are deleted
• No referential integrity enforcement
• Could lead to data inconsistencies

Recommendation:
Consider adding a foreign key constraint or implementing cascade delete logic:

userId: text("user_id", { length: 191 })
  .notNull()
  .references(() => users.id, { onDelete: 'cascade' }),

Note: This depends on whether SQLite foreign keys are enabled in your D1 database configuration.

Reference: https://github.com/Kriebel-LLC/calendarsync/blob/task/6f8c6516-implement-google-calendar-oaut/packages/shared/src/db/schema.ts#L133

---

Minor Suggestions

6. State Parameter Lacks CSRF Token Randomness

Location: packages/web/app/api/calendar/authorize/route.ts:14-19

Current:

const state = Buffer.from(
  JSON.stringify({
    userId: user.uid,
    timestamp: Date.now(),
  })
).toString("base64url");

Suggestion:
Add a random nonce to strengthen CSRF protection:

const state = Buffer.from(
  JSON.stringify({
    userId: user.uid,
    timestamp: Date.now(),
    nonce: nanoid(),
  })
).toString("base64url");

While timestamp provides some uniqueness, adding a random nonce makes state parameters unguessable and strengthens CSRF protection.

Reference: https://github.com/Kriebel-LLC/calendarsync/blob/task/6f8c6516-implement-google-calendar-oaut/packages/web/app/api/calendar/authorize/route.ts#L14-L19

---

7. Missing Logging for Calendar Fetch Failures

Location: packages/web/app/api/calendar/connection/route.ts:32-38

Current behavior:
When listCalendars() fails, it returns an empty array with error logging but continues:

try {
  calendars = await listCalendars(tokens.accessToken);
} catch (err) {
  req.log.error("Failed to list calendars", { error: err });
  calendars = [];
}

Consideration:
This might confuse users who successfully connected but see no calendars. Consider:

• Including an error indicator in the response so the UI can show "Failed to fetch calendars"
• Or rethrowing specific errors (like auth failures) that indicate the connection is invalid

---

Summary

Critical Issues:

1. ⚠️ Hardcoded encryption salt - Weakens PBKDF2 security
2. ⚠️ Missing decryption error handling - Will cause 500 errors on corrupted data
3. ⚠️ Token refresh race condition - Can cause issues under concurrent load

Important Issues:
4. JSON parsing without error handling
5. Missing foreign key constraint on userId

Minor Improvements:
6. Add CSRF nonce to state parameter
7. Consider better UX for calendar fetch failures

Positive Notes:

• Token refresh fix is correct ✓
• OAuth flow is well-structured
• Good security practices (encrypted storage, state validation, token expiry checks)
• Comprehensive error handling in callback route
• Good use of Zod validation

---

[AlecKriebel]

Review Feedback Addressed

I've addressed all the critical and important issues from the re-review:

✅ Critical Issues Fixed

1. Hardcoded encryption salt (packages/shared/src/encryption.ts)

• Now generates a random 16-byte salt per encryption operation
• Salt is prepended to the ciphertext: salt + iv + ciphertext
• Each token gets unique cryptographic parameters

2. Missing decryption error handling (packages/web/lib/calendar/calendar-connection.ts)

• Added try-catch around token decryption
• On failure, deletes corrupted connection and returns null
• User can then re-authenticate cleanly

3. Token refresh race condition (packages/web/lib/calendar/calendar-connection.ts)

• Implemented in-memory lock per user to prevent concurrent refresh attempts
• If a refresh is already in progress, subsequent requests wait for it
• After acquiring lock, re-checks token expiry in case another request refreshed it

✅ Important Issues Fixed

4. JSON parsing without error handling

• Added safeJsonParse() helper that returns fallback value on parse errors
• All JSON.parse(connection.calendarIds) calls now use safe parsing

5. Foreign key constraint on userId

• Reviewed the existing schema - other tables (orgInvites, orgUsers) also don't use foreign key constraints
• This appears to be the project's convention, likely due to SQLite FK behavior
• Kept consistent with existing patterns; the unique index provides lookup optimization

✅ Minor Improvements

6. CSRF nonce added to state parameter (packages/web/app/api/calendar/authorize/route.ts)

• Added random nonce: nanoid() to OAuth state
• Makes state parameters completely unguessable

7. Calendar fetch failure response (packages/web/app/api/calendar/connection/route.ts)

• Added calendarFetchError: boolean to response
• UI can now show appropriate message when calendar list fails to load

@claude please re-review

[claude]

Claude encountered an error —— View job

---

Re-review In Progress

Todo List

• Verify hardcoded encryption salt fix
• Verify decryption error handling fix
• Verify token refresh race condition fix
• Verify JSON parsing error handling fix
• Review foreign key constraint decision
• Verify CSRF nonce addition
• Verify calendar fetch error response
• Check for any new issues introduced by fixes
• Provide comprehensive feedback

---

Progress Notes

✅ Encryption salt fix verified - Random 16-byte salt is now generated per encryption (line 51) and prepended to output.

✅ Decryption error handling verified - Try-catch added around token decryption (lines 122-133), deletes corrupted connection on failure.

✅ Token refresh lock verified - In-memory lock implemented (lines 21-22, 141-153) to prevent concurrent refreshes per user.

✅ Safe JSON parsing verified - Helper function safeJsonParse() (lines 27-33) used throughout to handle corrupted JSON gracefully.

✅ CSRF nonce verified - Random nonce added to state parameter (line 19) for stronger CSRF protection.

✅ Calendar fetch error verified - calendarFetchError boolean added to response (lines 33, 39, 53).

Now checking for any new issues...

---

[AlecKriebel]

@claude please re-review - the previous review was interrupted before completing

[claude]

Claude finished @AlecKriebel's task in 1m 31s —— View job

---

Re-review Complete ✅

Todo List

• Verify hardcoded encryption salt fix
• Verify decryption error handling fix
• Verify token refresh race condition fix
• Verify JSON parsing error handling fix
• Review foreign key constraint decision
• Verify CSRF nonce addition
• Verify calendar fetch error response
• Check for any new issues introduced by fixes
• Provide comprehensive feedback

---

Review Summary

✅ All Previous Issues Successfully Addressed

The implementation fixes are excellent and properly address all 7 issues from the previous review:

Critical Issues Fixed:

1. ✅ Encryption salt - Now generates random 16-byte salt per encryption (encryption.ts:51), prepended to ciphertext
2. ✅ Decryption error handling - Try-catch added (calendar-connection.ts:122-133), deletes corrupted connection on failure
3. ✅ Token refresh race condition - In-memory lock prevents concurrent refreshes per user (calendar-connection.ts:141-198)

Important Issues Fixed:
4. ✅ JSON parsing errors - safeJsonParse() helper handles corrupted data gracefully (calendar-connection.ts:27-33)
5. ✅ Foreign key constraint - Consistent with existing patterns in the codebase; unique index provides lookup optimization

Minor Improvements Added:
6. ✅ CSRF nonce - Random nonce added to state parameter (authorize/route.ts:19)
7. ✅ Calendar fetch error - calendarFetchError boolean in response (connection/route.ts:33,39,53)

---

New Issue Found

⚠️ CSRF Nonce Not Validated in Callback

Location: packages/web/app/api/calendar/callback/route.ts:10-13,55-63

Issue:
A random nonce is now generated and included in the state parameter (authorize/route.ts:19), but the callback route doesn't validate it. The OAuthState interface doesn't include the nonce field:

interface OAuthState {
  userId: string;
  timestamp: number;
  // nonce field is missing!
}

When the state is parsed (line 57), the nonce is silently ignored, defeating its purpose as a CSRF protection mechanism.

Impact:

• The nonce provides no actual CSRF protection since it's never verified
• An attacker could potentially craft a valid state parameter if they know a user ID and recent timestamp

Recommended Fix:

1. Add nonce to the OAuthState interface
2. The nonce doesn't need explicit validation beyond being present - the fact that it was decoded from the state parameter proves it matches what was sent

interface OAuthState {
  userId: string;
  timestamp: number;
  nonce: string; // Add this field
}

Since the nonce is random and included in the state, an attacker cannot forge a valid state parameter without knowing the nonce value. Just parsing it successfully is sufficient validation.

Reference: callback/route.ts:10-13

---

Code Quality Assessment

Strengths:

• ✅ Excellent security practices overall (encrypted storage, token expiry, CSRF protection)
• ✅ Comprehensive error handling in OAuth callback flow
• ✅ Good use of Zod validation for API inputs
• ✅ Well-structured code with clear separation of concerns
• ✅ Thoughtful implementation of token refresh locking mechanism
• ✅ Good error recovery (corrupted tokens trigger re-authentication)

Architecture:

• ✅ Clean separation between OAuth utilities, connection management, and API routes
• ✅ Proper use of encryption abstraction with Web Crypto API
• ✅ Good database schema design with appropriate indexes

Documentation:

• ✅ Clear JSDoc comments on all major functions
• ✅ Helpful inline comments explaining key decisions
• ✅ Well-documented encryption format in header comment

---

Final Recommendation

Status: Approve with minor suggestion

The PR successfully implements Google Calendar OAuth integration with strong security practices. All previous review feedback has been properly addressed with high-quality fixes.

The one remaining issue (nonce validation) is minor and easily fixed by adding the nonce field to the OAuthState interface. The current implementation is secure enough for merging, but I recommend addressing this for completeness.

Suggested Next Steps:

1. Add nonce: string to the OAuthState interface in callback/route.ts
2. Test the OAuth flow end-to-end with the environment variables configured
3. Merge when ready

Great work on addressing all the review feedback thoroughly! The implementation quality is excellent.

---

[AlecKriebel]

Resolved merge conflicts by rebasing on main. The migration is now correctly numbered as 0003 (after the Stripe subscription changes in 0002). All previous review feedback has been incorporated into this single squashed commit.