[20230204026] feat: implement Google OAuth sign-in flow (#243) - #252
Conversation
|
Greetings, @mubasshirahin. Thy scroll hath arrived unblemished — the runes align and no conflict bars the path. The council shall now convene over its contents, weighing each incantation by candlelight. Tarry a while, brave adventurer; we shall investigate and return to thee with our verdict. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesGoogle OAuth sign-in
Shared UI assets
Service worker cache version
Repository maintenance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant GoogleSignInButton
participant API
participant Google
participant AuthService
participant UI
User->>GoogleSignInButton: Click sign-in
GoogleSignInButton->>API: GET /api/auth/google
API->>Google: Redirect for authorization
Google-->>API: Callback with authorization code
API->>AuthService: Exchange and verify code
AuthService-->>API: JWT auth result
API-->>UI: Redirect with token
UI->>UI: Seed session and redirect home
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
packages/api/src/services/auth.service.ts-118-133 (1)
118-133: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize emails before account lookup/linking.
findByEmail(email)is case-sensitive, so a password account saved with different casing can be missed and Google sign-in will create a duplicate user instead of linking. Apply the same normalization in sign-up/sign-in and the Google flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/services/auth.service.ts` around lines 118 - 133, Normalize email addresses consistently in sign-up, sign-in, and the Google authentication flow before persistence or lookup, including the email passed to users.findByEmail in the shown linking logic. Reuse the existing normalization convention or helper so equivalent casing resolves to the same account and Google identity linking does not create duplicates.
🧹 Nitpick comments (3)
packages/api/src/services/debts.service.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 TrivialUse one canonical logger instance across the services layer.
packages/api/src/services/index.tsconstructs a logger separately from the instance exported bypackages/api/src/services/logger.service.ts. These changes make six services use the latter while other barrel consumers may still use the former. Update the barrel to re-export the canonical logger, or verify that separate instances are intentional.
packages/api/src/services/debts.service.ts#L6-L6: keep the direct import only if it resolves to the canonical instance.packages/api/src/services/preferences.service.ts#L5-L5: use the same canonical logger instance.packages/api/src/services/prompt.service.ts#L4-L4: use the same canonical logger instance.packages/api/src/services/recurring.service.ts#L8-L8: use the same canonical logger instance.packages/api/src/services/tags.service.ts#L6-L6: use the same canonical logger instance.packages/api/src/services/transactions.service.ts#L6-L6: use the same canonical logger instance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/services/debts.service.ts` at line 6, Use the logger instance exported by services/logger.service.ts as the single canonical logger throughout the services layer. Update services/index.ts to re-export that instance instead of constructing a separate one, and ensure the imports in packages/api/src/services/debts.service.ts:6-6, packages/api/src/services/preferences.service.ts:5-5, packages/api/src/services/prompt.service.ts:4-4, packages/api/src/services/recurring.service.ts:8-8, packages/api/src/services/tags.service.ts:6-6, and packages/api/src/services/transactions.service.ts:6-6 all resolve to it; keep the direct debts import only if it already references the canonical export.packages/api/src/services/auth.service.ts (1)
23-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInject
googleClientlike the other dependencies for consistency and testability.
usersandvaultsare constructor-injected with defaults (per this class's own doc comment, explicitly for unit-testability), butgoogleClientis instantiated as a fixed class field, so tests can't substitute a mock without reaching intogoogle-auth-libraryinternals.♻️ Proposed refactor
export class AuthService { - private readonly googleClient = new OAuth2Client(GOOGLE_CLIENT_ID); - constructor( private readonly users: UsersRepository = usersRepository, private readonly vaults: VaultsRepository = vaultsRepository, + private readonly googleClient: OAuth2Client = new OAuth2Client(GOOGLE_CLIENT_ID), ) {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/services/auth.service.ts` around lines 23 - 29, Update AuthService to constructor-inject the OAuth2Client dependency like users and vaults, supplying the existing Google client as its default value. Remove the fixed googleClient class-field instantiation and preserve the service’s existing use of the injected googleClient so tests can provide a mock.packages/ui/src/app/signin/page.tsx (1)
49-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated Google sign-in success handler.
Both pages implement an identical
handleGoogleSuccess: clear errors, set loading, callauthApi.google(credential), callsetSessionwith the same field mapping, redirect to/, and set the same fallback error message on failure. This is copy-pasted logic that will drift if the session-handling contract changes.
packages/ui/src/app/signin/page.tsx#L49-L62: replace this handler with a shared hook/helper (e.g.,useGoogleAuthHandleror acompleteGoogleSignIn(credential)helper colocated withAuthApi.ts) that both pages call.packages/ui/src/app/signup/page.tsx#L55-L68: same replacement, reusing the same shared helper instead of a second copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/app/signin/page.tsx` around lines 49 - 62, The Google sign-in success logic is duplicated across both pages. In packages/ui/src/app/signin/page.tsx lines 49-62 and packages/ui/src/app/signup/page.tsx lines 55-68, replace each local handleGoogleSuccess implementation with a shared useGoogleAuthHandler or completeGoogleSignIn helper that preserves error clearing, loading state, authApi.google, setSession mapping, redirect, and fallback error behavior; both sites require the same replacement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/migrations/1783695914180-Migration.ts`:
- Around line 18-21: Update the down migration’s users table recreation and data
copy around the migration’s down method so Google-only users with null passwords
can be restored successfully. Preserve existing password values while allowing
null passwords in the recreated users schema and ensuring the INSERT from
temporary_users does not violate the schema constraint.
In `@packages/ui/src/components/GoogleSignInButton.tsx`:
- Around line 16-36: The handleClick flow in GoogleSignInButton currently relies
on prompt() without handling skipped or suppressed One Tap states. Add a
moment_callback to the Google Identity initialize configuration that detects
isNotDisplayed() or isSkippedMoment(), then invoke the persistent sign-in
fallback via renderButton() or the existing OAuth popup path while preserving
onError for actual prompt failures.
---
Other comments:
In `@packages/api/src/services/auth.service.ts`:
- Around line 118-133: Normalize email addresses consistently in sign-up,
sign-in, and the Google authentication flow before persistence or lookup,
including the email passed to users.findByEmail in the shown linking logic.
Reuse the existing normalization convention or helper so equivalent casing
resolves to the same account and Google identity linking does not create
duplicates.
---
Nitpick comments:
In `@packages/api/src/services/auth.service.ts`:
- Around line 23-29: Update AuthService to constructor-inject the OAuth2Client
dependency like users and vaults, supplying the existing Google client as its
default value. Remove the fixed googleClient class-field instantiation and
preserve the service’s existing use of the injected googleClient so tests can
provide a mock.
In `@packages/api/src/services/debts.service.ts`:
- Line 6: Use the logger instance exported by services/logger.service.ts as the
single canonical logger throughout the services layer. Update services/index.ts
to re-export that instance instead of constructing a separate one, and ensure
the imports in packages/api/src/services/debts.service.ts:6-6,
packages/api/src/services/preferences.service.ts:5-5,
packages/api/src/services/prompt.service.ts:4-4,
packages/api/src/services/recurring.service.ts:8-8,
packages/api/src/services/tags.service.ts:6-6, and
packages/api/src/services/transactions.service.ts:6-6 all resolve to it; keep
the direct debts import only if it already references the canonical export.
In `@packages/ui/src/app/signin/page.tsx`:
- Around line 49-62: The Google sign-in success logic is duplicated across both
pages. In packages/ui/src/app/signin/page.tsx lines 49-62 and
packages/ui/src/app/signup/page.tsx lines 55-68, replace each local
handleGoogleSuccess implementation with a shared useGoogleAuthHandler or
completeGoogleSignIn helper that preserves error clearing, loading state,
authApi.google, setSession mapping, redirect, and fallback error behavior; both
sites require the same replacement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 7659f4d8-525d-49eb-bac4-5a6ad02d8515
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
README.mdpackages/api/.env.examplepackages/api/package.jsonpackages/api/src/entities/User.entity.tspackages/api/src/migrations/1783695914180-Migration.tspackages/api/src/repositories/users.repository.tspackages/api/src/routes/auth.routes.tspackages/api/src/routes/auth/google.route.tspackages/api/src/services/auth.service.tspackages/api/src/services/debts.service.tspackages/api/src/services/preferences.service.tspackages/api/src/services/prompt.service.tspackages/api/src/services/recurring.service.tspackages/api/src/services/tags.service.tspackages/api/src/services/transactions.service.tspackages/api/src/services/users.service.tspackages/api/src/services/vaults.service.tspackages/api/src/tests/auth.service.test.tspackages/api/src/tests/vaults.service.test.tspackages/shared/src/contracts/auth.tspackages/ui/.env.developmentpackages/ui/public/sw.jspackages/ui/src/app/layout.tsxpackages/ui/src/app/signin/page.tsxpackages/ui/src/app/signup/page.tsxpackages/ui/src/components/GoogleAuthProvider.tsxpackages/ui/src/components/GoogleSignInButton.tsxpackages/ui/src/lib/api/AuthApi.ts
| await queryRunner.query(`ALTER TABLE "users" RENAME TO "temporary_users"`); | ||
| await queryRunner.query(`CREATE TABLE "users" ("id" varchar PRIMARY KEY NOT NULL, "name" varchar(100) NOT NULL, "email" varchar(255) NOT NULL, "avatar" varchar(255) NOT NULL DEFAULT (''), "password" varchar(255) NOT NULL, "createdAt" datetime DEFAULT (datetime('now')), "updatedAt" datetime DEFAULT (datetime('now')), "deletedAt" datetime, "disableAiPrompt" boolean NOT NULL DEFAULT (0), "googleId" varchar(255), CONSTRAINT "UQ_97672ac88f789774dd47f7c8be3" UNIQUE ("email"), CONSTRAINT "UQ_93124f92af3bd84a60e424644bb" UNIQUE ("googleId"))`); | ||
| await queryRunner.query(`INSERT INTO "users"("id", "name", "email", "avatar", "password", "createdAt", "updatedAt", "deletedAt", "disableAiPrompt", "googleId") SELECT "id", "name", "email", "avatar", "password", "createdAt", "updatedAt", "deletedAt", "disableAiPrompt", "googleId" FROM "temporary_users"`); | ||
| await queryRunner.query(`DROP TABLE "temporary_users"`); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Migration file ==\n'
sed -n '1,220p' packages/api/src/migrations/1783695914180-Migration.ts
printf '\n== User entity/searches ==\n'
rg -n "password|googleId|disableAiPrompt|deletedAt|isNull|nullable" packages/api/src -g '!**/dist/**' -g '!**/build/**'
printf '\n== Relevant user entity file(s) ==\n'
fd -a "User" packages/api/src | head -n 20
fd -a "user" packages/api/src | head -n 20Repository: ali-ahnaf/pocket_pixel
Length of output: 45453
Down migration breaks for Google-only users. down() recreates users with password NOT NULL and then copies rows straight from temporary_users; any account created via Google with password = null will abort the rollback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/src/migrations/1783695914180-Migration.ts` around lines 18 - 21,
Update the down migration’s users table recreation and data copy around the
migration’s down method so Google-only users with null passwords can be restored
successfully. Preserve existing password values while allowing null passwords in
the recreated users schema and ensuring the INSERT from temporary_users does not
violate the schema constraint.
| const handleClick = useCallback(() => { | ||
| if (!ready || !clientId) { | ||
| onError?.(); | ||
| return; | ||
| } | ||
|
|
||
| if (!initialized.current) { | ||
| window.google!.accounts.id.initialize({ | ||
| client_id: clientId, | ||
| callback: (response) => onSuccess(response.credential), | ||
| cancel_on_tap_outside: true, | ||
| }); | ||
| initialized.current = true; | ||
| } | ||
|
|
||
| try { | ||
| window.google!.accounts.id.prompt(); | ||
| } catch { | ||
| onError?.(); | ||
| } | ||
| }, [ready, clientId, onSuccess, onError]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby references.
ast-grep outline packages/ui/src/components/GoogleSignInButton.tsx --view expanded || true
echo "----"
wc -l packages/ui/src/components/GoogleSignInButton.tsx
echo "----"
cat -n packages/ui/src/components/GoogleSignInButton.tsx | sed -n '1,220p'
echo "----"
rg -n "GoogleSignInButton|accounts\.id\.prompt|accounts\.id\.initialize|renderButton\(" packages/ui/src -SRepository: ali-ahnaf/pocket_pixel
Length of output: 4329
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for any local handling of Google Identity Services prompt notifications / callback refs.
rg -n "isNotDisplayed|isSkippedMoment|prompt\\((notification|.*=>)|onSuccessRef|onErrorRef|initialized\\.current" packages/ui/src -SRepository: ali-ahnaf/pocket_pixel
Length of output: 332
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect signin/signup pages around GoogleSignInButton usage and handler identities.
cat -n packages/ui/src/app/signin/page.tsx | sed -n '1,240p'
echo "----"
cat -n packages/ui/src/app/signup/page.tsx | sed -n '1,280p'Repository: ali-ahnaf/pocket_pixel
Length of output: 20074
🌐 Web query:
Google Identity Services prompt moment listener suppressed after dismissal Safari Firefox ITP renderButton documentation
💡 Result:
When a user manually dismisses or closes the Google Identity Services (GIS) One Tap prompt, the system automatically suppresses future prompts to respect the user's preference and reduce annoyance [1]. This suppression is standard behavior across all browsers, including those subject to Intelligent Tracking Prevention (ITP) like Safari and Firefox [2][3]. Key points regarding this behavior include: 1. Automatic Suppression: If a user explicitly closes the One Tap prompt (e.g., by clicking the 'Close' button or tapping outside the prompt area, depending on configuration), the library flags this state. Consequently, subsequent calls to google.accounts.id.prompt will not display the prompt again for that user session or until the suppression cooldown period resets [1]. 2. Prompt Moment Listener: The prompt method allows you to pass a momentListener to receive notifications about the UI status, such as skipped or dismissed moments [4][5]. - Dismissed Moment: This occurs when a user explicitly closes the prompt, including when you programmatically call google.accounts.id.cancel [4][5]. - Skipped Moment: This occurs due to automatic cancellation, user cancellation, or if the user is not signed into a Google session [4][5]. - Note on FedCM: If FedCM is enabled (use_fedcm_for_prompt: true), the behavior of the momentListener may change, and notifications can be delayed by up to 1 minute to improve user privacy [6]. 3. ITP Browsers (Safari/Firefox): ITP does not independently suppress the GIS prompt in a way that differs from standard browsers; rather, browsers with ITP restrictions (like Safari and Firefox) often trigger a specific "upgraded" UX [2][3]. This UX may begin with a welcome page or a pop-up window [3]. The GIS library is designed to handle these environments, but you must ensure your implementation properly utilizes the moment_callback to handle states where the prompt is not displayed [5][7]. 4. Mitigation and Troubleshooting: - If your prompt is not reappearing during testing, ensure you are not hitting the "cooldown" period. - Verify that your momentListener is correctly handling 'dismissed' and 'skipped' moments, as relying on these callbacks is the recommended way to determine if you should fall back to other identity providers or alternative login methods [5]. - If using FedCM, be aware of the potential for notification delays [6]. For further implementation details, refer to the official GIS documentation on managing prompt UI status notifications and ITP support [5][2].
Citations:
- 1: https://developers.google.com/identity/gsi/web/guides/features
- 2: https://developers.google.com/identity/gsi/web/guides/integrate
- 3: https://developers.google.cn/identity/gsi/web/guides/features
- 4: https://developers.google.com/identity/gsi/web/reference/js-reference
- 5: https://developers.google.com/identity/gsi/web/guides/display-google-one-tap
- 6: https://stackoverflow.com/questions/77526214/prompt-momentlistener-is-not-triggering-when-use-fedcm-for-prompt-is-true
- 7: https://developers.google.com/identity/gsi/web/reference/html-reference
prompt() needs a fallback path for skipped One Tap states.
This button relies on google.accounts.id.prompt() alone, so if One Tap is suppressed or skipped (for example after a manual dismissal) the click can appear to do nothing and never call onError. Add a moment_callback for isNotDisplayed() / isSkippedMoment() and fall back to renderButton() or an OAuth popup for a persistent sign-in button.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/GoogleSignInButton.tsx` around lines 16 - 36, The
handleClick flow in GoogleSignInButton currently relies on prompt() without
handling skipped or suppressed One Tap states. Add a moment_callback to the
Google Identity initialize configuration that detects isNotDisplayed() or
isSkippedMoment(), then invoke the persistent sign-in fallback via
renderButton() or the existing OAuth popup path while preserving onError for
actual prompt failures.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/api/src/services/prompt.service.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
loggerfrom the canonical module.Line 6 still imports
loggerfrom., whilepackages/api/src/services/index.tscreates a separateLoggerinstance from the singleton exported bypackages/api/src/services/logger.service.ts. Keep onlytagsServiceandvaultsServicefrom the barrel.Suggested import split
-import { logger, tagsService, vaultsService } from '.'; +import { logger } from './logger.service'; +import { tagsService, vaultsService } from '.';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/services/prompt.service.ts` at line 6, Update the imports in prompt.service.ts so logger comes from the canonical logger service module, while the barrel import from '.' retains only tagsService and vaultsService. Ensure prompt.service.ts uses the shared logger singleton rather than the separately created barrel instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/services/transactions.service.ts`:
- Around line 1-8: Remove the duplicate transactionsRepository and logger
imports at the top of the transactions service. Keep the combined repository
import from ../repositories containing transactionsRepository and
vaultsRepository, and retain the explicit logger import from ./logger.service.
- Around line 101-106: Update the transfer persistence flow containing the
expense and income saves so both save calls and the conditional replaceTags
calls execute within a single AppDataSource.transaction boundary. Preserve the
existing tagIds behavior and use the transaction-scoped repository/manager for
every write, ensuring any failure rolls back the entire transfer.
---
Nitpick comments:
In `@packages/api/src/services/prompt.service.ts`:
- Line 6: Update the imports in prompt.service.ts so logger comes from the
canonical logger service module, while the barrel import from '.' retains only
tagsService and vaultsService. Ensure prompt.service.ts uses the shared logger
singleton rather than the separately created barrel instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 1e532b5e-1fe4-41a4-bd07-4eacedb36f3a
📒 Files selected for processing (4)
README.mdpackages/api/src/services/debts.service.tspackages/api/src/services/prompt.service.tspackages/api/src/services/transactions.service.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/api/src/services/prompt.service.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
loggerfrom the canonical module.Line 6 still imports
loggerfrom., whilepackages/api/src/services/index.tscreates a separateLoggerinstance from the singleton exported bypackages/api/src/services/logger.service.ts. Keep onlytagsServiceandvaultsServicefrom the barrel.Suggested import split
-import { logger, tagsService, vaultsService } from '.'; +import { logger } from './logger.service'; +import { tagsService, vaultsService } from '.';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/services/prompt.service.ts` at line 6, Update the imports in prompt.service.ts so logger comes from the canonical logger service module, while the barrel import from '.' retains only tagsService and vaultsService. Ensure prompt.service.ts uses the shared logger singleton rather than the separately created barrel instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/services/transactions.service.ts`:
- Around line 1-8: Remove the duplicate transactionsRepository and logger
imports at the top of the transactions service. Keep the combined repository
import from ../repositories containing transactionsRepository and
vaultsRepository, and retain the explicit logger import from ./logger.service.
- Around line 101-106: Update the transfer persistence flow containing the
expense and income saves so both save calls and the conditional replaceTags
calls execute within a single AppDataSource.transaction boundary. Preserve the
existing tagIds behavior and use the transaction-scoped repository/manager for
every write, ensuring any failure rolls back the entire transfer.
---
Nitpick comments:
In `@packages/api/src/services/prompt.service.ts`:
- Line 6: Update the imports in prompt.service.ts so logger comes from the
canonical logger service module, while the barrel import from '.' retains only
tagsService and vaultsService. Ensure prompt.service.ts uses the shared logger
singleton rather than the separately created barrel instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 1e532b5e-1fe4-41a4-bd07-4eacedb36f3a
📒 Files selected for processing (4)
README.mdpackages/api/src/services/debts.service.tspackages/api/src/services/prompt.service.tspackages/api/src/services/transactions.service.ts
🛑 Comments failed to post (2)
packages/api/src/services/transactions.service.ts (2)
1-8: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate imports; this file will not compile.
transactionsRepositoryandloggerare each declared twice. Keep the combined repository import and the explicit./logger.serviceimport only.Suggested cleanup
-import { transactionsRepository } from '../repositories'; import { logger } from './logger.service'; import { transactionsRepository, vaultsRepository } from '../repositories'; -import { logger } from '.';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import { CreateTransactionInput, CreateTransferInput, ListTransactionsQuery, TransactionDto, UpdateTransactionInput } from '`@expense-tracker/shared`'; import { Expense } from '../entities/Expense.entity'; import { AppError } from '../errors/app-error'; import { TransactionsRepository, TransactionDateFilter } from '../repositories/transactions.repository'; import { logger } from './logger.service'; import { transactionsRepository, vaultsRepository } from '../repositories';🧰 Tools
🪛 GitHub Actions: Build / 2_test.txt
[error] 5-5: TS2300: Duplicate identifier 'transactionsRepository'.
[error] 6-6: TS2300: Duplicate identifier 'logger'.
[error] 7-7: TS2300: Duplicate identifier 'transactionsRepository'.
[error] 8-8: TS2300: Duplicate identifier 'logger'.
[error] 1-1: Test step failed:
jestexited with code 1 (npm lifecycle scripttestfailed).🪛 GitHub Actions: Build / test
[error] 5-5: TypeScript TS2300: Duplicate identifier 'transactionsRepository'.
[error] 6-6: TypeScript TS2300: Duplicate identifier 'logger'.
[error] 7-7: TypeScript TS2300: Duplicate identifier 'transactionsRepository'.
[error] 8-8: TypeScript TS2300: Duplicate identifier 'logger'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/services/transactions.service.ts` around lines 1 - 8, Remove the duplicate transactionsRepository and logger imports at the top of the transactions service. Keep the combined repository import from ../repositories containing transactionsRepository and vaultsRepository, and retain the explicit logger import from ./logger.service.
101-106: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== transactions.service.ts outline ==\n' ast-grep outline packages/api/src/services/transactions.service.ts --view expanded || true printf '\n== relevant slices ==\n' sed -n '1,220p' packages/api/src/services/transactions.service.ts printf '\n== search for transaction usage in api services ==\n' rg -n "transaction|manager|QueryRunner|save\\(|replaceTags\\(" packages/api/src/services packages/api/src -g '*.ts' | sed -n '1,220p'Repository: ali-ahnaf/pocket_pixel
Length of output: 32567
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== transactions.repository.ts outline ==\n' ast-grep outline packages/api/src/repositories/transactions.repository.ts --view expanded || true printf '\n== transactions.repository.ts ==\n' sed -n '1,260p' packages/api/src/repositories/transactions.repository.ts printf '\n== search replaceTags implementation and transaction wrappers ==\n' rg -n "replaceTags|AppDataSource\\.transaction|manager\\.save|manager\\.delete|QueryRunner" packages/api/src/repositories packages/api/src/services -g '*.ts' | sed -n '1,220p'Repository: ali-ahnaf/pocket_pixel
Length of output: 4966
Wrap transfer writes in a transaction.
packages/api/src/services/transactions.service.ts:101-106—save(expenseTx),save(incomeTx), and bothreplaceTags()calls are independent; if any later step fails, this can leave a partial transfer or mismatched tags. Run them inside oneAppDataSource.transaction(...)boundary, or equivalent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/services/transactions.service.ts` around lines 101 - 106, Update the transfer persistence flow containing the expense and income saves so both save calls and the conditional replaceTags calls execute within a single AppDataSource.transaction boundary. Preserve the existing tagIds behavior and use the transaction-scoped repository/manager for every write, ensuring any failure rolls back the entire transfer.
df6938f to
d0af0d9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/api/src/migrations/1783695914180-Migration.ts (1)
19-24: 🗄️ Data Integrity & Integration | 🟠 MajorMake
down()safe for Google-only accounts.The supplied
packages/api/src/services/auth.service.tscreates users withpassword: null, but Line 19 recreatesusers.passwordasNOT NULLand Line 20 copies those null values. Even if that copy were relaxed, Lines 23-24 repeat the same failure when restoring the historical schema. A downgrade after a Google-only signup therefore aborts. Define and enforce a downgrade policy, such as rejecting the downgrade with an explicit precondition or converting OAuth-only users to a valid legacy representation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/migrations/1783695914180-Migration.ts` around lines 19 - 24, Make the migration’s down() path explicitly handle users with null passwords before recreating the NOT NULL users.password column. Choose and enforce a clear downgrade policy—either reject when Google-only accounts exist with an explicit precondition, or convert them to a valid legacy representation—and apply it consistently to both users table reconstruction and historical schema restoration around the INSERT statements.
🧹 Nitpick comments (1)
packages/api/src/migrations/1783695914180-Migration.ts (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFormat the migration with Prettier.
Line 1 uses double quotes, and the class declaration uses non-Prettier indentation/semicolon formatting. Run the repository formatter on this file.
As per coding guidelines, TypeScript files must be formatted with Prettier using
singleQuote,trailingComma: all, andprintWidth: 200.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/migrations/1783695914180-Migration.ts` around lines 1 - 4, Run Prettier on Migration1783695914180 and apply the repository’s TypeScript formatting settings, including single quotes, trailing commas where applicable, 200-character print width, and normalized indentation and semicolons.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/api/src/migrations/1783695914180-Migration.ts`:
- Around line 19-24: Make the migration’s down() path explicitly handle users
with null passwords before recreating the NOT NULL users.password column. Choose
and enforce a clear downgrade policy—either reject when Google-only accounts
exist with an explicit precondition, or convert them to a valid legacy
representation—and apply it consistently to both users table reconstruction and
historical schema restoration around the INSERT statements.
---
Nitpick comments:
In `@packages/api/src/migrations/1783695914180-Migration.ts`:
- Around line 1-4: Run Prettier on Migration1783695914180 and apply the
repository’s TypeScript formatting settings, including single quotes, trailing
commas where applicable, 200-character print width, and normalized indentation
and semicolons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 87aa6c11-3c03-457a-8d4c-5503fff7655a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
packages/api/.env.examplepackages/api/package.jsonpackages/api/src/entities/User.entity.tspackages/api/src/migrations/1783695914180-Migration.tspackages/api/src/repositories/users.repository.tspackages/api/src/routes/auth.routes.tspackages/api/src/routes/auth/google.route.tspackages/api/src/services/auth.service.tspackages/api/src/services/debts.service.tspackages/api/src/services/preferences.service.tspackages/api/src/services/prompt.service.tspackages/api/src/services/recurring.service.tspackages/api/src/services/tags.service.tspackages/api/src/services/users.service.tspackages/api/src/services/vaults.service.tspackages/api/src/tests/auth.service.test.tspackages/api/src/tests/vaults.service.test.tspackages/shared/src/contracts/auth.tspackages/ui/.env.developmentpackages/ui/public/sw.jspackages/ui/src/app/layout.tsxpackages/ui/src/app/signin/page.tsxpackages/ui/src/app/signup/page.tsxpackages/ui/src/components/GoogleAuthProvider.tsxpackages/ui/src/components/GoogleSignInButton.tsxpackages/ui/src/lib/api/AuthApi.ts
💤 Files with no reviewable changes (1)
- packages/api/src/services/prompt.service.ts
🚧 Files skipped from review as they are similar to previous changes (21)
- packages/api/package.json
- packages/api/.env.example
- packages/api/src/services/vaults.service.ts
- packages/ui/.env.development
- packages/api/src/routes/auth/google.route.ts
- packages/ui/src/app/signin/page.tsx
- packages/ui/public/sw.js
- packages/api/src/services/tags.service.ts
- packages/api/src/tests/vaults.service.test.ts
- packages/api/src/services/users.service.ts
- packages/ui/src/components/GoogleSignInButton.tsx
- packages/ui/src/components/GoogleAuthProvider.tsx
- packages/api/src/services/preferences.service.ts
- packages/api/src/tests/auth.service.test.ts
- packages/api/src/entities/User.entity.ts
- packages/ui/src/app/signup/page.tsx
- packages/api/src/services/recurring.service.ts
- packages/ui/src/lib/api/AuthApi.ts
- packages/ui/src/app/layout.tsx
- packages/api/src/services/auth.service.ts
- packages/shared/src/contracts/auth.ts
d0af0d9 to
546a27b
Compare
546a27b to
1a43579
Compare
|
The quest is sealed, @mubasshirahin! Thy incantations have been woven into the main path. The realm grows stronger for thy contribution — accept the gratitude of this humble keeper of the codex. May thy commits forever merge cleanly, brave adventurer 💰 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/ui/.env.example (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the required ending blank line.
This leaves a dotenv-linter warning in the changed file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/.env.example` at line 2, Add a trailing blank line after NEXT_PUBLIC_GOOGLE_CLIENT_ID in the environment example file so the file ends with the required newline and passes dotenv-linter validation.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/routes/auth/google-callback.route.ts`:
- Around line 28-32: Update the Google callback handler around
authService.googleSignInWithCode so it does not place result.token in the
redirect URL. Persist the authenticated session using the project’s supported
HttpOnly cookie mechanism, or issue a short-lived single-use exchange code, then
redirect without exposing the JWT in browser history, logs, or referrers.
In `@packages/api/src/routes/auth/google-sign-in.route.ts`:
- Around line 16-27: Update both OAuth routes to follow the approved utilService
response contract: in google-sign-in.route.ts, use an existing redirect-capable
utilService method or add the approved redirect mechanism before returning the
Google authorization result; in google-callback.route.ts, remove the local
try/catch, let asyncHandler forward failures to the global error handler, and
send successful output through the same utilService mechanism.
In `@packages/api/src/services/auth.service.ts`:
- Around line 165-169: Update the user-update flow around users.findByEmail and
users.save so an existing non-null user.googleId is never replaced when it
differs from the incoming googleId. Reject this conflict unless the request is
handled through an authenticated account-linking flow, while preserving linking
for users without a Google ID; add a regression test covering the conflicting
identity case.
- Around line 220-221: Update normalizeEmail and the UsersRepository.findByEmail
lookup so existing mixed-case email records remain discoverable, either by
backfilling legacy users.email values during migration or by making the
repository comparison case-insensitive. Preserve normalized email storage and
ensure both password sign-in and Google sign-in resolve an existing account
instead of creating duplicates.
In `@packages/ui/src/app/auth/google/callback/page.tsx`:
- Around line 42-54: Remove the JWT-based session creation from the Google
callback flow in the callback page, including token parsing and the setSession
call. Replace it with a server-mediated HttpOnly session-cookie completion or a
single-use callback-code exchange bound to the browser’s OAuth initiation
state/cookie, and preserve the existing failure redirect behavior for invalid or
failed callbacks.
---
Nitpick comments:
In `@packages/ui/.env.example`:
- Line 2: Add a trailing blank line after NEXT_PUBLIC_GOOGLE_CLIENT_ID in the
environment example file so the file ends with the required newline and passes
dotenv-linter validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: adde60d7-3207-48b4-aeb0-688c452edef8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
.agents/settings.json.gitignorepackages/api/.env.examplepackages/api/package.jsonpackages/api/src/entities/User.entity.tspackages/api/src/index.tspackages/api/src/migrations/1783695914180-Migration.tspackages/api/src/repositories/users.repository.tspackages/api/src/routes/auth.routes.tspackages/api/src/routes/auth/google-callback.route.tspackages/api/src/routes/auth/google-sign-in.route.tspackages/api/src/services/auth.service.tspackages/api/src/tests/auth.service.google.test.tspackages/api/src/tests/auth.service.test.tspackages/api/src/tests/vaults.service.test.tspackages/shared/src/constants.tspackages/shared/src/index.tspackages/ui/.env.developmentpackages/ui/.env.examplepackages/ui/public/sw.jspackages/ui/src/app/auth/google/callback/page.tsxpackages/ui/src/app/signin/page.tsxpackages/ui/src/app/signup/page.tsxpackages/ui/src/components/AuthGuard.tsxpackages/ui/src/components/GoogleSignInButton.tsxpackages/ui/src/lib/api/ApiClient.tspackages/ui/src/lib/helpers/static.tspackages/ui/test-lucide.ts
💤 Files with no reviewable changes (4)
- packages/ui/test-lucide.ts
- packages/ui/src/lib/api/ApiClient.ts
- .agents/settings.json
- packages/ui/.env.development
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/api/src/routes/auth.routes.ts
- packages/api/src/repositories/users.repository.ts
- packages/api/src/tests/auth.service.test.ts
- packages/api/src/entities/User.entity.ts
- packages/api/src/migrations/1783695914180-Migration.ts
- packages/api/src/tests/vaults.service.test.ts
| try { | ||
| const result = await authService.googleSignInWithCode(code); | ||
| return res.redirect(`${APP_URL}/auth/google/callback/?token=${encodeURIComponent(result.token)}`); | ||
| } catch { | ||
| return res.redirect(`${APP_URL}/auth/google/callback/?error=1`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not place the session JWT in the callback URL.
The token will enter browser history and can be exposed through logs or referrers. Use a secure HttpOnly session cookie with authenticated cookie support, or redirect with a short-lived single-use exchange code instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/src/routes/auth/google-callback.route.ts` around lines 28 - 32,
Update the Google callback handler around authService.googleSignInWithCode so it
does not place result.token in the redirect URL. Persist the authenticated
session using the project’s supported HttpOnly cookie mechanism, or issue a
short-lived single-use exchange code, then redirect without exposing the JWT in
browser history, logs, or referrers.
| router.get( | ||
| '/google', | ||
| asyncHandler(async (_req: Request, res: Response) => { | ||
| const state = randomBytes(16).toString('hex'); | ||
| res.cookie(OAUTH_STATE_COOKIE, state, { | ||
| httpOnly: true, | ||
| secure: isProd, | ||
| sameSite: 'lax', | ||
| maxAge: 10 * 60 * 1000, | ||
| }); | ||
| return res.redirect(authService.getGoogleAuthUrl(state)); | ||
| }), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep OAuth routes within the API response and error-handling contract.
Both routes bypass utilService; the callback also catches service failures locally instead of allowing asyncHandler to forward them to the global handler.
packages/api/src/routes/auth/google-sign-in.route.ts#L16-L27: return the initiation result through the approved API response mechanism, or add an explicit approved redirect mechanism to it.packages/api/src/routes/auth/google-callback.route.ts#L24-L32: remove the route-localtry/catchand route output through the same approved mechanism.
As per coding guidelines, “Do not add try/catch blocks in API routes; wrap handlers with asyncHandler and let the global error handler middleware handle thrown errors.” As per coding guidelines, “Send API responses only through utilService.”
📍 Affects 2 files
packages/api/src/routes/auth/google-sign-in.route.ts#L16-L27(this comment)packages/api/src/routes/auth/google-callback.route.ts#L24-L32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/src/routes/auth/google-sign-in.route.ts` around lines 16 - 27,
Update both OAuth routes to follow the approved utilService response contract:
in google-sign-in.route.ts, use an existing redirect-capable utilService method
or add the approved redirect mechanism before returning the Google authorization
result; in google-callback.route.ts, remove the local try/catch, let
asyncHandler forward failures to the global error handler, and send successful
output through the same utilService mechanism.
Source: Coding guidelines
| user = await this.users.findByEmail(email); | ||
| if (user) { | ||
| user.googleId = googleId; | ||
| if (!user.avatar && profile.picture) user.avatar = profile.picture; | ||
| user = await this.users.save(user); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not overwrite an already-linked Google identity.
When email lookup finds a user whose googleId belongs to another Google subject, this path replaces that association. Reject conflicting non-null IDs and require an authenticated account-linking flow; add a regression test for this case.
Proposed fix
if (user) {
+ if (user.googleId && user.googleId !== googleId) {
+ throw new AppError('Email is already linked to a different Google account', 409);
+ }
user.googleId = googleId;
if (!user.avatar && profile.picture) user.avatar = profile.picture;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| user = await this.users.findByEmail(email); | |
| if (user) { | |
| user.googleId = googleId; | |
| if (!user.avatar && profile.picture) user.avatar = profile.picture; | |
| user = await this.users.save(user); | |
| user = await this.users.findByEmail(email); | |
| if (user) { | |
| if (user.googleId && user.googleId !== googleId) { | |
| throw new AppError('Email is already linked to a different Google account', 409); | |
| } | |
| user.googleId = googleId; | |
| if (!user.avatar && profile.picture) user.avatar = profile.picture; | |
| user = await this.users.save(user); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/src/services/auth.service.ts` around lines 165 - 169, Update the
user-update flow around users.findByEmail and users.save so an existing non-null
user.googleId is never replaced when it differs from the incoming googleId.
Reject this conflict unless the request is handled through an authenticated
account-linking flow, while preserving linking for users without a Google ID;
add a regression test covering the conflicting identity case.
| private normalizeEmail(email: string): string { | ||
| return email.trim().toLowerCase(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline packages/api/src/repositories/users.repository.ts --items all
ast-grep outline packages/api/src/entities/User.entity.ts --items all
rg -n -C3 'findByEmail|LOWER\(|ILIKE|email' packages/api/src/repositories/users.repository.ts packages/api/src/entities/User.entity.ts
fd -a '1783695914180-Migration.ts' packages/api/src/migrations --exec sed -n '1,260p' {}Repository: ali-ahnaf/pocket_pixel
Length of output: 6262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- auth.service relevant lines ---\n'
sed -n '1,320p' packages/api/src/services/auth.service.ts | cat -n | sed -n '1,320p'
printf '\n--- search for email normalization/canonicalization across migrations ---\n'
rg -n -C2 'normalizeEmail|LOWER\(|ILIKE|case[- ]?insensitive|email' packages/api/src/migrations packages/api/src/services packages/api/src/repositories packages/api/src/entities | sed -n '1,240p'Repository: ali-ahnaf/pocket_pixel
Length of output: 31710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- repository lookup implementation ---\n'
sed -n '1,120p' packages/api/src/repositories/users.repository.ts | cat -n
printf '\n--- users entity column definition ---\n'
sed -n '1,80p' packages/api/src/entities/User.entity.ts | cat -nRepository: ali-ahnaf/pocket_pixel
Length of output: 2821
Backfill legacy emails or make lookups case-insensitive. normalizeEmail() only lower-cases the input; UsersRepository.findByEmail() still does an exact match, and the migrations copy existing users.email values as-is. Any mixed-case row will stop matching, which can break password sign-in and cause Google sign-in to create a duplicate account instead of linking it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/src/services/auth.service.ts` around lines 220 - 221, Update
normalizeEmail and the UsersRepository.findByEmail lookup so existing mixed-case
email records remain discoverable, either by backfilling legacy users.email
values during migration or by making the repository comparison case-insensitive.
Preserve normalized email storage and ensure both password sign-in and Google
sign-in resolve an existing account instead of creating duplicates.
| const params = new URLSearchParams(window.location.search); | ||
| const token = params.get('token'); | ||
| const decoded = token ? decodeToken(token) : null; | ||
|
|
||
| if (params.get('error') || !token || !decoded) { | ||
| setFailed(true); | ||
|
|
||
| const timer = setTimeout(() => router.replace('/signin'), 2000); | ||
| return () => clearTimeout(timer); | ||
| } | ||
|
|
||
| setSession(token, { id: decoded.userId, name: decoded.name, email: decoded.email, avatar: decoded.avatar }); | ||
| router.replace('/'); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not establish sessions from a JWT in the callback URL.
A victim can be induced to visit this page with an attacker’s valid JWT, which setSession then persists as the victim’s session. This is login CSRF/session fixation; query-string JWTs can also leak through browser history, logs, and same-origin referrers. Complete the flow with an HttpOnly session cookie, or exchange a single-use callback code bound to the browser’s OAuth-initiation state/cookie instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/app/auth/google/callback/page.tsx` around lines 42 - 54,
Remove the JWT-based session creation from the Google callback flow in the
callback page, including token parsing and the setSession call. Replace it with
a server-mediated HttpOnly session-cookie completion or a single-use
callback-code exchange bound to the browser’s OAuth initiation state/cookie, and
preserve the existing failure redirect behavior for invalid or failed callbacks.
2 participants
Description
Adds Google OAuth sign-in as an alternative to email/password login.
Closes #243
Changes
POST /api/auth/googleroute that verifies Google ID tokensgoogleIdcolumn to User entity (with migration)googleSignIn()handles three cases — existing link, email match (link), new userGoogleSignInButton+GoogleAuthProvidercomponents integrated into sign-in/sign-up pagesRoll Number
20230204026
Summary by CodeRabbit