Merge pull request #4 from hngprojects/feature/auth-helpers#12
Merge pull request #4 from hngprojects/feature/auth-helpers#12Rayflix55 wants to merge 5 commits into
Conversation
Feature/auth helpers
📝 WalkthroughWalkthroughThis PR adds three independent subsystems: a complete CI/CD deployment pipeline (build artifact creation and automated staging/production deployments), a redesigned authentication experience with a new forgot-password page and enhanced login UI, and a comprehensive cross-platform security scanner tool for detecting supply-chain IOCs and persistence mechanisms. ChangesCI/CD Deployment Pipeline
Authentication UI Enhancement
Supply-Chain Security Scanner
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 Warning |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive security scanner script (lazarus_scanner.py) designed to detect and mitigate Lazarus supply-chain attacks across multiple platforms. Additionally, it implements a new 'Forgot Password' page and refactors the 'Login' page to include Zod validation, OAuth integration, and enhanced UI components. The review feedback highlights several areas for improvement: ensuring consistent validation schemas, avoiding brittle navigation patterns using setTimeout, moving the Toaster component to a root layout, adhering to standard Tailwind CSS classes, and preventing layout shifts in form inputs. There is also a suggestion to improve the uniqueness of backup file naming in the scanner script.
| const LoginSchema = z.object({ | ||
| email: z.string().email({ message: 'Invalid email address' }), | ||
| password: z | ||
| .string() | ||
| .min(6, { message: 'Password must be at least 6 characters' }), | ||
| rememberMe: z.boolean(), | ||
| }) |
There was a problem hiding this comment.
The LoginSchema is redefined locally with a minimum password length of 6, which is inconsistent with the shared LoginSchema in src/schemas/auth.ts (which requires min(1)) and the RegisterSchema (which requires min(8)). It is recommended to import and extend the shared schema to ensure consistent validation rules across the application.
| toast.success('Login success', { | ||
| description: 'Redirecting to workspace', | ||
| }) | ||
| setTimeout(() => router.push('/'), 1000) |
There was a problem hiding this comment.
Using setTimeout to delay navigation after a success toast is a brittle pattern. If the component unmounts or the navigation takes longer than expected, it can lead to a poor user experience. Consider using the toast's callback functions or navigating immediately, as modern toast libraries like sonner handle persistence across routes.
|
|
||
| {/* Right panel */} | ||
| <div className="flex w-full items-center justify-center p-6 sm:p-12 lg:w-[60%]"> | ||
| <Toaster position="top-center" /> |
There was a problem hiding this comment.
The Toaster component should generally be placed in the root layout (e.g., src/app/layout.tsx) rather than in individual page components. This ensures that toast notifications persist during page transitions and prevents multiple toaster instances from being mounted.
| <motion.div | ||
| initial={{ opacity: 0, y: 10 }} | ||
| animate={{ opacity: 1, y: 0 }} | ||
| className="flex w-full max-w-130 flex-col rounded-[40px] bg-white p-8 shadow-[0_8px_30px_rgb(0,0,0,0.04)] sm:p-12 md:p-16" |
There was a problem hiding this comment.
max-w-130 is not a standard Tailwind CSS utility class. If this is not a custom class defined in your configuration, consider using a standard size like max-w-lg or an arbitrary value like max-w-[520px].
References
- Adherence to standard Tailwind CSS utility classes for maintainability. (link)
| <motion.div | ||
| initial={{ opacity: 0, y: 10 }} | ||
| animate={{ opacity: 1, y: 0 }} | ||
| className="flex w-full max-w-130 flex-col rounded-[40px] bg-white p-8 shadow-[0_8px_30px_rgb(0,0,0,0.04)] sm:p-12 md:p-16" |
There was a problem hiding this comment.
max-w-130 is not a standard Tailwind CSS utility class. If this is not a custom class defined in your configuration, consider using a standard size like max-w-lg or an arbitrary value like max-w-[520px].
References
- Adherence to standard Tailwind CSS utility classes for maintainability. (link)
| className={`w-full border bg-[#FFFFFF] ${ | ||
| status === 'error' | ||
| ? 'border-[#FF5630]' | ||
| : 'border-[#919EAB]/30' | ||
| } rounded-xl ${status === 'error' ? 'pl-4' : 'pl-12'} py-3.5 pr-12 text-[#1A1A1A] placeholder-[#919EAB] transition-all focus:ring-1 focus:ring-[#007B55] focus:outline-none`} |
There was a problem hiding this comment.
The conditional padding (pl-4 vs pl-12) causes a layout shift when the input enters an error state because the Mail icon is hidden. It is better to keep the padding consistent and keep the icon visible (perhaps with a different color) to avoid the text jumping.
|
|
||
|
|
||
| def quarantine(path: Path) -> Path: | ||
| backup = path.with_suffix(path.suffix + f".malware-backup-{int(time.time())}") |
There was a problem hiding this comment.
Using int(time.time()) as a suffix for the backup file name can lead to collisions if the script processes the same file multiple times within the same second. Consider using a more precise timestamp or appending a short random string to ensure the backup path is unique.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 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 @.github/workflows/build-and-upload.yml:
- Around line 25-26: Update the CI install step so pnpm fails if the lockfile is
out of sync: in the workflow step named "Install dependencies" replace the
current run command that executes "pnpm install" with a run that uses the
frozen-lockfile flag (i.e., use "pnpm install --frozen-lockfile") so CI will
error if pnpm-lock.yaml would be modified.
- Around line 31-32: The tar command in the "Archive production artifacts" step
(the job named "Archive production artifacts" that runs tar -czf
agentforge-build.tar.gz .next public package.json pnpm-lock.yaml) omits
next.config.ts, which Next.js needs at runtime; update that tar invocation to
include next.config.ts so the production archive contains the runtime Next
config (add next.config.ts to the list of files passed to tar).
In @.github/workflows/production-deployment.yml:
- Around line 17-19: The deployment is currently un-gated; configure the GitHub
Environment named "production" (the environment referenced by environment: name:
"production" in the workflow) to require manual approval by adding one or more
Required reviewers under Settings → Environments → production → Required
reviewers so the deploy job will pause and require an approver to click
"Approve" before the job runs; after adding reviewers, verify the workflow still
references environment: "production" (and its url if needed) so GitHub enforces
the approval gate.
In @.github/workflows/staging-deployment.yml:
- Around line 17-19: The environment.url values currently use hostnames without
a TLD (e.g., "https://staging.agent-forge") which will not resolve; update the
environment.url entries for both environment.name "staging" (in this file) and
the equivalent entry in production-deployment.yml (environment.name
"production") to the full deployed hostnames including your domain (e.g.,
"https://staging.agent-forge.<your-domain>" and
"https://agent-forge.<your-domain>") so the GitHub Environments "View
deployment" links resolve correctly.
- Around line 34-57: Replace password-based auth in the
appleboy/scp-action@v0.1.7 and appleboy/ssh-action@v1.2.1 steps: stop using the
password: field and instead use a private key from secrets (e.g.
secrets.SSH_PRIVATE_KEY) and the corresponding fields the actions support
(private_key / key or key_path) while keeping host and username as-is; add host
fingerprint pinning using the fingerprint: field to the ssh/scp actions to
mitigate MITM; ensure the server’s authorized_keys contains the matching public
key; make the same change in production-deployment.yml so both workflows use SSH
key authentication and host pinning instead of password authentication.
- Around line 3-15: The deploy job's if condition currently requires
github.event.workflow_run.conclusion == 'success', which is null for manual
workflow_dispatch runs; update the deploy job conditional (job name: deploy) to
gate on the event source so it runs either when the triggering workflow_run
completed successfully OR when the event is workflow_dispatch (e.g. use a
condition that checks github.event_name == 'workflow_run' &&
github.event.workflow_run.conclusion == 'success' || github.event_name ==
'workflow_dispatch'), and also guard the "Download build artifact" step's run-id
input so it is omitted or sourced from a workflow_dispatch input when
github.event_name == 'workflow_dispatch' to avoid using
github.event.workflow_run.id which is absent for manual runs.
- Around line 52-57: Change the pm2 invocation to be resilient to first-time
deploys by replacing the brittle "pm2 restart frontend-staging --update-env"
usage with a reload-or-start pattern (e.g., "pm2 reload frontend-staging
--update-env || pm2 start ...") so the process is created if it doesn't exist;
also ensure the non-interactive SSH shell can find pnpm by sourcing the user's
shell rc (or otherwise adding pnpm to PATH) before running "pnpm install --prod"
so pnpm is available in the CI job. Target the script block containing the "pnpm
install --prod" and "pm2 restart frontend-staging --update-env" lines when
making these edits.
In `@lazarus_scanner.py`:
- Around line 174-194: The truncation in clean_file is unsafe: the regexes for
patterns 3/4 use non-greedy brace matching and the code picks the earliest match
across end_markers (cut = m.end() ... min), which can truncate nested objects or
hit false-positives; change clean_file to (1) for patterns that match object
literals (currently the \{[\s\S]*?\} entries) stop using non-greedy regex and
instead implement a small brace-balanced scanner that walks the text from each
candidate "{", tracking braces while respecting strings/comments so you get the
correct closing brace, and (2) compute cut using the last-valid occurrence per
pattern (use finditer and take max(m.end()) per pattern) and only accept a cut
that is after the IOC offset returned by scan_file/quarantine; only then apply
the trim and keep the existing safety checks (50B threshold, writing via
path.write_text).
In `@src/app/forgot-password/ForgotPassword.tsx`:
- Around line 224-228: The "Contact support" anchor in ForgotPassword.tsx (and
the similar instance around lines 300-304) uses href="#" which is inaccessible
and causes page jumps; update the anchor to either a real route (e.g.,
href="/support") or a mailto link (e.g., href="mailto:support@example.com") if
it should open the user's email client, or convert it to a button element with
an onClick handler that opens the support modal/route and include role="button"
and tabIndex={0} for accessibility; ensure the changed element in the
ForgotPassword component maintains visible focus styles and keyboard activation
(Enter/Space) for screen reader and keyboard users.
- Around line 64-74: The catch block in ForgotPassword.tsx currently swallows
all errors and runs mock/demo logic; update the catch to accept an error
parameter (e.g., catch (err)) and remove the setTimeout mock flow, instead
handle real errors by inspecting err (network/response) and setStatus('error')
and setErrorMessage with a meaningful message (or err.message) and optionally
log the error; ensure the promise rejection path for the function (e.g., the
submit/handleForgotPassword function) surfaces failures appropriately rather
than returning mocked success.
- Around line 208-211: Remove the GitHub OAuth button JSX from the
ForgotPassword component: delete the <button ...> block that contains <Github
className="h-5 w-5" /> and "Sign in with GitHub" so the forgot-password flow
only shows recovery actions; also remove any now-unused references/imports such
as the Github icon import and any OAuth handler (e.g., signInWithGitHub or
onGithubSignIn) from the same file to avoid unused symbols and adjust
surrounding layout/styling if needed to preserve spacing.
- Around line 282-287: The Resend Email button allows spamming; add a
client-side cooldown by introducing a cooldown state (e.g., cooldownSeconds via
useState) inside the ForgotPassword component and disable the button while
cooldownSeconds > 0; update the onClick handler referenced (currently calling
setStatus('idle')) to also start the cooldown (setCooldownSeconds to a
configured value), and use a useEffect with a timer to decrement cooldownSeconds
every second and clear the interval on unmount; also change the button rendering
to show disabled styling and the remaining seconds (e.g., "Resend Email (xxs)")
when cooldown is active so users get feedback.
- Around line 282-287: The "Resend Email" button in ForgotPassword currently
only calls setStatus('idle') which clears UI but doesn't trigger the API; update
it to call the same submission logic used by the form (e.g., invoke the submit
handler or extracted resendEmail/sendResetEmail function) so clicking "Resend
Email" actually sends the request again; if submission logic is inline in
handleSubmit, extract that logic into a reusable function (resendResetEmail or
sendResetEmail) and call it from both handleSubmit and the Resend Email button,
preserving status updates and loading/disabled states.
In `@src/app/login/page.tsx`:
- Around line 80-95: The login flow currently implemented in onSubmit (and
related OAuth handlers) lacks CSRF protection; update the client and server flow
to generate and validate CSRF tokens: implement a Server Action or middleware
that issues a cryptographically strong CSRF token (or use NextAuth/Auth.js to
handle this automatically), store the token in a secure httpOnly cookie (or use
double-submit cookie) and include the token in the login form submission (refer
to onSubmit and LoginFormValues for the form payload), then validate that token
server‑side before processing authentication and before completing any OAuth
callback handlers; ensure tokens are rotated/expired and validation logic is
invoked in the server endpoint that handles the login/OAuth exchange.
- Around line 80-95: The onSubmit function currently contains mock logic
(setTimeout and special-case 'error@example.com'); replace it with a real async
authentication call: call your auth API/client (e.g., await auth.login(values)
or fetch POST to your login endpoint) inside the startTransition block, handle
success by showing toast.success and calling router.push('/'), and handle
failures by catching the error and calling toast.error with the error message;
remove the hardcoded setTimeout and conditional email check and ensure you
return/await the real promise and properly type errors for LoginFormValues,
startTransition, toast, and router.push usage.
- Around line 37-39: The password Zod schema currently enforces .min(6) which is
too weak; update the validator for the password field (the password
z.string().min(...) call) to .min(8) (or higher), and adjust the associated
error message from "Password must be at least 6 characters" to "Password must be
at least 8 characters" (and update any related client/server validation or tests
that expect the old minimum).
- Line 96: The variable rememberMe returned from useWatch is unused; remove the
redundant call "const rememberMe = useWatch({ control: form.control, name:
'rememberMe' })" from the component so state is only read/updated via the
existing checkbox handlers that call form.setValue; ensure no other code
references rememberMe before deleting.
- Around line 331-337: Links for "Terms of Service" and "Privacy Policy" are
using href="#" which causes jumps and is inaccessible; replace the dummy anchors
with proper routes (e.g., href="/terms" and href="/privacy") and use Next.js
<Link> if available (or regular <a> with real hrefs) for the "Terms of Service"
and "Privacy Policy" elements in the login page (the anchors containing the text
"Terms of Service" and "Privacy Policy"); ensure the className and hover styles
are preserved and remove the '#' handlers so clicks navigate to the correct
pages.
- Line 92: Replace the untracked setTimeout call that calls router.push('/')
with either an immediate navigation or a cancellable timer: remove the bare
setTimeout(() => router.push('/'), 1000) and call router.push('/') directly, or
if you need the delay, store the timer id returned by setTimeout in a ref/state
and clear it in a useEffect cleanup (clearTimeout(timerId)) so the callback
won't run after the component unmounts; refer to the existing setTimeout and
router.push('/') usage in the page component to locate where to apply this
change.
- Around line 164-186: The OAuth and magic-link buttons in page.tsx (the Button
elements rendering GoogleLogo and Github and their onClick handlers) are
placeholders that only fire toast messages; implement real flows or remove them.
Either wire these buttons to real auth functions in src/lib/api/auth.ts (e.g.,
add startGoogleOAuth(), startGithubOAuth(), sendMagicLinkEmail()) and update
.env.sample with CLIENT_ID/CLIENT_SECRET and redirect URIs, or remove the Button
nodes and related UI (GoogleLogo, Github usage) to avoid exposing dead actions;
ensure isLoading handling and disabled state remain correct and update any
imports/usages accordingly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: fc6ea27d-662d-48f2-9c33-ac33938c41b4
📒 Files selected for processing (7)
.github/workflows/build-and-upload.yml.github/workflows/production-deployment.yml.github/workflows/staging-deployment.ymllazarus_scanner.pysrc/app/forgot-password/ForgotPassword.tsxsrc/app/forgot-password/page.tsxsrc/app/login/page.tsx
| - name: Install dependencies | ||
| run: pnpm install |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use --frozen-lockfile for reproducible CI installs.
Without --frozen-lockfile, pnpm will silently rewrite pnpm-lock.yaml if it drifts from package.json, producing non-reproducible build artifacts and masking lockfile mistakes in PRs. CI installs should fail loudly instead.
♻️ Proposed fix
- name: Install dependencies
- run: pnpm install
+ run: pnpm install --frozen-lockfile🤖 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 @.github/workflows/build-and-upload.yml around lines 25 - 26, Update the CI
install step so pnpm fails if the lockfile is out of sync: in the workflow step
named "Install dependencies" replace the current run command that executes "pnpm
install" with a run that uses the frozen-lockfile flag (i.e., use "pnpm install
--frozen-lockfile") so CI will error if pnpm-lock.yaml would be modified.
| - name: Archive production artifacts | ||
| run: tar -czf agentforge-build.tar.gz .next public package.json pnpm-lock.yaml |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which root-level files Next.js will need at runtime.
fd -HI -d 1 '^(next\.config\.(js|ts|mjs|cjs)|\.npmrc|\.env\.production|instrumentation\.(t|j)s)$'Repository: hngprojects/agentforge-fe
Length of output: 84
Add next.config.ts to the tar archive.
The repository contains next.config.ts at the root, but it is not included in the current tar archive. Since next start requires this file to read runtime configuration (rewrites, redirects, output mode, basePath, images config), the production server will fail to apply these settings and behave differently from local builds.
♻️ Proposed fix
- name: Archive production artifacts
- run: tar -czf agentforge-build.tar.gz .next public package.json pnpm-lock.yaml
+ run: tar -czf agentforge-build.tar.gz .next public package.json pnpm-lock.yaml next.config.ts📝 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.
| - name: Archive production artifacts | |
| run: tar -czf agentforge-build.tar.gz .next public package.json pnpm-lock.yaml | |
| - name: Archive production artifacts | |
| run: tar -czf agentforge-build.tar.gz .next public package.json pnpm-lock.yaml next.config.ts |
🤖 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 @.github/workflows/build-and-upload.yml around lines 31 - 32, The tar command
in the "Archive production artifacts" step (the job named "Archive production
artifacts" that runs tar -czf agentforge-build.tar.gz .next public package.json
pnpm-lock.yaml) omits next.config.ts, which Next.js needs at runtime; update
that tar invocation to include next.config.ts so the production archive contains
the runtime Next config (add next.config.ts to the list of files passed to tar).
| environment: | ||
| name: "production" | ||
| url: "https://agent-forge" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Gate production deploys with a manual approval.
Right now, every successful build on main deploys straight to production with no human in the loop. Configure the production GitHub Environment with required reviewers (Settings → Environments → production → Required reviewers) so an approver must click "Approve" before the deploy job runs. This is the single highest-leverage safety control for a prod pipeline triggered automatically off main.
🤖 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 @.github/workflows/production-deployment.yml around lines 17 - 19, The
deployment is currently un-gated; configure the GitHub Environment named
"production" (the environment referenced by environment: name: "production" in
the workflow) to require manual approval by adding one or more Required
reviewers under Settings → Environments → production → Required reviewers so the
deploy job will pause and require an approver to click "Approve" before the job
runs; after adding reviewers, verify the workflow still references environment:
"production" (and its url if needed) so GitHub enforces the approval gate.
| on: | ||
| workflow_dispatch: | ||
| workflow_run: | ||
| workflows: ["Build and Upload"] | ||
| types: | ||
| - completed | ||
| branches: | ||
| - staging | ||
|
|
||
| jobs: | ||
| deploy: | ||
| runs-on: ubuntu-latest | ||
| if: ${{ github.event.workflow_run.conclusion == 'success' }} |
There was a problem hiding this comment.
Manual workflow_dispatch runs are silently skipped.
On workflow_dispatch, github.event.workflow_run is null, so github.event.workflow_run.conclusion == 'success' evaluates to false and the deploy job never runs. The "manual deploy" trigger is effectively dead code. Gate the condition on the event source.
🐛 Proposed fix
deploy:
runs-on: ubuntu-latest
- if: ${{ github.event.workflow_run.conclusion == 'success' }}
+ if: ${{ github.event_name == 'workflow_dispatch' || github.event.workflow_run.conclusion == 'success' }}Note: when triggered by workflow_dispatch, github.event.workflow_run.id is also unavailable, so the "Download build artifact" step's run-id input must be guarded too (e.g. omit run-id for manual runs to fall back to the latest successful run, or accept a workflow_dispatch input for the run 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 @.github/workflows/staging-deployment.yml around lines 3 - 15, The deploy
job's if condition currently requires github.event.workflow_run.conclusion ==
'success', which is null for manual workflow_dispatch runs; update the deploy
job conditional (job name: deploy) to gate on the event source so it runs either
when the triggering workflow_run completed successfully OR when the event is
workflow_dispatch (e.g. use a condition that checks github.event_name ==
'workflow_run' && github.event.workflow_run.conclusion == 'success' ||
github.event_name == 'workflow_dispatch'), and also guard the "Download build
artifact" step's run-id input so it is omitted or sourced from a
workflow_dispatch input when github.event_name == 'workflow_dispatch' to avoid
using github.event.workflow_run.id which is absent for manual runs.
| environment: | ||
| name: "staging" | ||
| url: "https://staging.agent-forge" |
There was a problem hiding this comment.
Environment URL is missing a TLD.
https://staging.agent-forge isn't a resolvable hostname — the "View deployment" link in the GitHub Environments UI will 404 / DNS-fail. Same in production-deployment.yml (https://agent-forge). Use the actual deployed hostnames (e.g. https://staging.agent-forge.<your-domain>).
🤖 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 @.github/workflows/staging-deployment.yml around lines 17 - 19, The
environment.url values currently use hostnames without a TLD (e.g.,
"https://staging.agent-forge") which will not resolve; update the
environment.url entries for both environment.name "staging" (in this file) and
the equivalent entry in production-deployment.yml (environment.name
"production") to the full deployed hostnames including your domain (e.g.,
"https://staging.agent-forge.<your-domain>" and
"https://agent-forge.<your-domain>") so the GitHub Environments "View
deployment" links resolve correctly.
| const onSubmit = (values: LoginFormValues) => { | ||
| startTransition(async () => { | ||
| await new Promise((resolve) => setTimeout(resolve, 1500)) | ||
|
|
||
| if (values.email === 'error@example.com') { | ||
| toast.error('Login failed', { | ||
| description: 'An error occurred during login', | ||
| }) | ||
| } else { | ||
| toast.success('Login success', { | ||
| description: 'Redirecting to workspace', | ||
| }) | ||
| setTimeout(() => router.push('/'), 1000) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy lift
No CSRF protection for authentication actions.
The login form and OAuth flows do not include CSRF tokens or other anti-CSRF measures. Modern authentication systems should protect against cross-site request forgery attacks.
Consider implementing CSRF protection for authentication endpoints:
- Use Next.js middleware to generate and validate CSRF tokens
- Include CSRF tokens in all authentication forms
- Validate tokens server-side before processing login/OAuth requests
- Consider using established auth libraries like NextAuth.js/Auth.js that handle CSRF automatically
For Next.js App Router, you can:
- Generate CSRF tokens in Server Actions
- Use secure, httpOnly cookies for session management
- Implement the Synchronizer Token Pattern or Double Submit Cookie pattern
Also applies to: 164-186
🤖 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 `@src/app/login/page.tsx` around lines 80 - 95, The login flow currently
implemented in onSubmit (and related OAuth handlers) lacks CSRF protection;
update the client and server flow to generate and validate CSRF tokens:
implement a Server Action or middleware that issues a cryptographically strong
CSRF token (or use NextAuth/Auth.js to handle this automatically), store the
token in a secure httpOnly cookie (or use double-submit cookie) and include the
token in the login form submission (refer to onSubmit and LoginFormValues for
the form payload), then validate that token server‑side before processing
authentication and before completing any OAuth callback handlers; ensure tokens
are rotated/expired and validation logic is invoked in the server endpoint that
handles the login/OAuth exchange.
Critical: Remove mock login logic from production code.
The onSubmit function contains hardcoded mock logic with setTimeout and special handling for error@example.com. This will break in production when integrated with a real authentication API.
🔧 Proposed fix: Implement proper API integration
const onSubmit = (values: LoginFormValues) => {
startTransition(async () => {
- await new Promise((resolve) => setTimeout(resolve, 1500))
-
- if (values.email === 'error@example.com') {
- toast.error('Login failed', {
- description: 'An error occurred during login',
- })
- } else {
- toast.success('Login success', {
- description: 'Redirecting to workspace',
- })
- setTimeout(() => router.push('/'), 1000)
- }
+ try {
+ const response = await fetch('/api/v1/auth/login', {
+ method: 'POST',
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify({
+ email: values.email,
+ password: values.password,
+ rememberMe: values.rememberMe,
+ }),
+ })
+
+ if (!response.ok) {
+ const error = await response.json()
+ toast.error('Login failed', {
+ description: error.message || 'Invalid credentials',
+ })
+ return
+ }
+
+ const data = await response.json()
+ // Store auth token/session as needed
+
+ toast.success('Login success', {
+ description: 'Redirecting to workspace',
+ })
+ router.push('/')
+ } catch (error) {
+ console.error('Login error:', error)
+ toast.error('Login failed', {
+ description: 'Unable to connect to server',
+ })
+ }
})
}📝 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.
| const onSubmit = (values: LoginFormValues) => { | |
| startTransition(async () => { | |
| await new Promise((resolve) => setTimeout(resolve, 1500)) | |
| if (values.email === 'error@example.com') { | |
| toast.error('Login failed', { | |
| description: 'An error occurred during login', | |
| }) | |
| } else { | |
| toast.success('Login success', { | |
| description: 'Redirecting to workspace', | |
| }) | |
| setTimeout(() => router.push('/'), 1000) | |
| } | |
| }) | |
| } | |
| const onSubmit = (values: LoginFormValues) => { | |
| startTransition(async () => { | |
| try { | |
| const response = await fetch('/api/v1/auth/login', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ | |
| email: values.email, | |
| password: values.password, | |
| rememberMe: values.rememberMe, | |
| }), | |
| }) | |
| if (!response.ok) { | |
| const error = await response.json() | |
| toast.error('Login failed', { | |
| description: error.message || 'Invalid credentials', | |
| }) | |
| return | |
| } | |
| const data = await response.json() | |
| // Store auth token/session as needed | |
| toast.success('Login success', { | |
| description: 'Redirecting to workspace', | |
| }) | |
| router.push('/') | |
| } catch (error) { | |
| console.error('Login error:', error) | |
| toast.error('Login failed', { | |
| description: 'Unable to connect to server', | |
| }) | |
| } | |
| }) | |
| } |
🤖 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 `@src/app/login/page.tsx` around lines 80 - 95, The onSubmit function currently
contains mock logic (setTimeout and special-case 'error@example.com'); replace
it with a real async authentication call: call your auth API/client (e.g., await
auth.login(values) or fetch POST to your login endpoint) inside the
startTransition block, handle success by showing toast.success and calling
router.push('/'), and handle failures by catching the error and calling
toast.error with the error message; remove the hardcoded setTimeout and
conditional email check and ensure you return/await the real promise and
properly type errors for LoginFormValues, startTransition, toast, and
router.push usage.
| toast.success('Login success', { | ||
| description: 'Redirecting to workspace', | ||
| }) | ||
| setTimeout(() => router.push('/'), 1000) |
There was a problem hiding this comment.
Potential memory leak: router.push inside setTimeout without cleanup.
The component may unmount before the setTimeout callback fires, causing router.push('/') to execute on an unmounted component. While Next.js router is generally safe, this pattern can cause issues and should use proper cleanup or immediate navigation.
🔧 Proposed fix: Navigate immediately or add cleanup
Option 1 - Navigate immediately (preferred):
toast.success('Login success', {
description: 'Redirecting to workspace',
})
- setTimeout(() => router.push('/'), 1000)
+ router.push('/')Option 2 - Add proper cleanup if delay is needed:
+ useEffect(() => {
+ return () => {
+ // Component cleanup
+ }
+ }, [])
const onSubmit = (values: LoginFormValues) => {
startTransition(async () => {
+ let timeoutId: NodeJS.Timeout
// ... success case
toast.success('Login success', {
description: 'Redirecting to workspace',
})
- setTimeout(() => router.push('/'), 1000)
+ timeoutId = setTimeout(() => router.push('/'), 1000)
+ // Note: still needs proper cleanup strategy
})
}📝 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.
| setTimeout(() => router.push('/'), 1000) | |
| toast.success('Login success', { | |
| description: 'Redirecting to workspace', | |
| }) | |
| router.push('/') |
🤖 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 `@src/app/login/page.tsx` at line 92, Replace the untracked setTimeout call
that calls router.push('/') with either an immediate navigation or a cancellable
timer: remove the bare setTimeout(() => router.push('/'), 1000) and call
router.push('/') directly, or if you need the delay, store the timer id returned
by setTimeout in a ref/state and clear it in a useEffect cleanup
(clearTimeout(timerId)) so the callback won't run after the component unmounts;
refer to the existing setTimeout and router.push('/') usage in the page
component to locate where to apply this change.
| } | ||
| }) | ||
| } | ||
| const rememberMe = useWatch({ control: form.control, name: 'rememberMe' }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Unused value: rememberMe from useWatch.
The rememberMe value is extracted using useWatch but never used in the component. The checkbox already reads and writes directly via form.setValue, making this line redundant.
🧹 Remove unused variable
- const rememberMe = useWatch({ control: form.control, name: 'rememberMe' })📝 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.
| const rememberMe = useWatch({ control: form.control, name: 'rememberMe' }) |
🤖 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 `@src/app/login/page.tsx` at line 96, The variable rememberMe returned from
useWatch is unused; remove the redundant call "const rememberMe = useWatch({
control: form.control, name: 'rememberMe' })" from the component so state is
only read/updated via the existing checkbox handlers that call form.setValue;
ensure no other code references rememberMe before deleting.
| <Button | ||
| type="button" | ||
| onClick={() => toast.info('Google login initiated')} | ||
| disabled={isLoading} | ||
| variant="outline" | ||
| className="h-12 w-full gap-3 rounded-xl border-[#919EAB]/30 font-semibold text-[#454F5B]" | ||
| > | ||
| <GoogleLogo /> | ||
| <span className="mx-1 h-4 w-px bg-[#919EAB]/30" /> | ||
| <span>Continue with Google</span> | ||
| </Button> | ||
| <Button | ||
| type="button" | ||
| onClick={() => toast.info('GitHub login initiated')} | ||
| disabled={isLoading} | ||
| variant="outline" | ||
| className="h-12 w-full gap-3 rounded-xl border-[#919EAB]/30 font-semibold text-[#454F5B]" | ||
| > | ||
| <Github className="h-5 w-5" /> | ||
| <span className="mx-1 h-4 w-px bg-[#919EAB]/30" /> | ||
| <span>Continue with GitHub</span> | ||
| </Button> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if OAuth/magic link API routes exist
fd -t f "route.ts|route.js" src/app/api | xargs rg -l "auth.*(google|github|magic)"
# Check for auth provider configuration
rg -i "google.*client.*id|github.*client.*id|oauth.*provider" --type ts --type js -g '!node_modules'Repository: hngprojects/agentforge-fe
Length of output: 168
🏁 Script executed:
# First, explore the repository structure
git ls-files src/ | head -20Repository: hngprojects/agentforge-fe
Length of output: 638
🏁 Script executed:
# Look for the login page file
fd -t f "page.tsx" src/ | grep -i loginRepository: hngprojects/agentforge-fe
Length of output: 92
🏁 Script executed:
# Check for any auth-related files or directories
fd -t f -i "auth" src/Repository: hngprojects/agentforge-fe
Length of output: 155
🏁 Script executed:
# Read the login page to see the button implementations
cat -n src/app/login/page.tsx | sed -n '160,190p'Repository: hngprojects/agentforge-fe
Length of output: 1494
🏁 Script executed:
# Also check the other lines mentioned
cat -n src/app/login/page.tsx | sed -n '303,320p'Repository: hngprojects/agentforge-fe
Length of output: 769
🏁 Script executed:
# Check the auth API implementation
cat -n src/lib/api/auth.tsRepository: hngprojects/agentforge-fe
Length of output: 5091
🏁 Script executed:
# Check for environment variables related to OAuth
find . -name ".env*" -type f | grep -v node_modulesRepository: hngprojects/agentforge-fe
Length of output: 83
🏁 Script executed:
# Search for OAuth-related packages in package.json
cat package.json | grep -i "oauth\|google\|github" | head -20Repository: hngprojects/agentforge-fe
Length of output: 51
🏁 Script executed:
# Search for any OAuth implementation in the codebase
rg "oauth|googleLogin|githubLogin|magicLink" --type ts --type js --type tsx --type jsx -iRepository: hngprojects/agentforge-fe
Length of output: 97
🏁 Script executed:
# Read the .env.sample to see what OAuth configuration is expected
cat .env.sampleRepository: hngprojects/agentforge-fe
Length of output: 97
🏁 Script executed:
# Retry OAuth search with correct file types
rg "oauth|googleLogin|githubLogin|magicLink" --type ts --type js -iRepository: hngprojects/agentforge-fe
Length of output: 124
Connect OAuth and magic link flows, or remove these placeholder buttons.
The Google OAuth, GitHub OAuth, and magic link buttons display toast notifications only, with no actual authentication implementations. The src/lib/api/auth.ts file contains no corresponding OAuth or magic link functions, and the .env.sample lacks OAuth provider credentials. Either integrate these flows with real authentication providers and a magic link email system before release, or remove the buttons to prevent user confusion.
Also applies to: 307-314
🤖 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 `@src/app/login/page.tsx` around lines 164 - 186, The OAuth and magic-link
buttons in page.tsx (the Button elements rendering GoogleLogo and Github and
their onClick handlers) are placeholders that only fire toast messages;
implement real flows or remove them. Either wire these buttons to real auth
functions in src/lib/api/auth.ts (e.g., add startGoogleOAuth(),
startGithubOAuth(), sendMagicLinkEmail()) and update .env.sample with
CLIENT_ID/CLIENT_SECRET and redirect URIs, or remove the Button nodes and
related UI (GoogleLogo, Github usage) to avoid exposing dead actions; ensure
isLoading handling and disabled state remain correct and update any
imports/usages accordingly.
| <a href="#" className="font-bold text-[#007B55] hover:underline"> | ||
| Terms of Service | ||
| </a>{' '} | ||
| and{' '} | ||
| <a href="#" className="font-bold text-[#007B55] hover:underline"> | ||
| Privacy Policy | ||
| </a> |
There was a problem hiding this comment.
Fix inaccessible Terms of Service and Privacy Policy links.
The ToS and Privacy Policy links use href="#" without proper handlers, making them inaccessible and causing the page to jump to the top on click.
🔧 Proposed fix: Add proper routes
<a
- href="#"
+ href="/terms-of-service"
className="font-bold text-[`#007B55`] hover:underline"
>
Terms of Service
</a>{' '}
and{' '}
<a
- href="#"
+ href="/privacy-policy"
className="font-bold text-[`#007B55`] hover:underline"
>
Privacy Policy
</a>🤖 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 `@src/app/login/page.tsx` around lines 331 - 337, Links for "Terms of Service"
and "Privacy Policy" are using href="#" which causes jumps and is inaccessible;
replace the dummy anchors with proper routes (e.g., href="/terms" and
href="/privacy") and use Next.js <Link> if available (or regular <a> with real
hrefs) for the "Terms of Service" and "Privacy Policy" elements in the login
page (the anchors containing the text "Terms of Service" and "Privacy Policy");
ensure the className and hover styles are preserved and remove the '#' handlers
so clicks navigate to the correct pages.
2 participants
forgot password and login page
Summary by CodeRabbit
New Features
Chores