Conversation
Security: - Add origin validation to switch-provider postMessage handler - Add safeRedirect() helper to validate redirect URL protocols - Always return generic "Authentication failed" in login errors - Log deleteSession errors in callback handler SEO/GEO: - Use 308 permanent redirect in middleware (was 307) - Set canonical on /welcome to https://certified.app/ (preserve backlink equity) - Add disallow rules to robots.ts for auth routes - Add robots noindex + canonical to profile page - Fix manifest start_url to /welcome - Fix sitemap lastModified to fixed dates - Upgrade Organization logo to ImageObject in JSON-LD Accessibility: - Move focus trap ref to backdrop in sign-in modal - Add focus trap to mobile feedback bottom sheet - Replace title with aria-label on sign-out buttons - Add role="menu" to account switcher - Add aria-describedby to feedback email input - Add focus restoration to useFocusTrap hook Performance + CSS: - Add display: "swap" to all font configs - Remove unused React imports from partner-apps.tsx and footer.tsx - Add --color-focus-green and --color-success-icon CSS variables Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Apply safeRedirect() to submitEmail and submitHandle (was missed) - Revert canonical on /welcome back to /welcome (avoids redirect loop for crawlers) - Fix not-found.tsx: link to /welcome, add noindex metadata - Fix auth-guard: redirect to /welcome instead of /?returnTo=... - Add aria-describedby + aria-invalid to sign-in modal input - Add HSTS header to next.config.ts - Add twitter:site and twitter:creator to layout metadata - Fix isMobile SSR hydration: use useState + useEffect instead of render-time check Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…pages - Replace custom hostname-only CSRF check in feedback route with standard checkCsrf() for consistent origin validation - Add explicit OG images to /about, /terms, /privacy, /dsa pages Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughConsolidates SEO/robots/metadata updates, security headers and public URL config, input sanitization and CSRF handling, authentication redirect/refactor, accessibility and focus-trap improvements, API validation/error-extraction utilities, organization APIs refactors, assorted UI/UX and CSS tweaks, and deletes design/audit documents. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/api/feedback/route.ts (1)
19-27:⚠️ Potential issue | 🟠 MajorStrip invisible Unicode characters before validating/storing input.
messageandProposed patch
import { NextRequest, NextResponse } from "next/server"; import { Resend } from "resend"; import { checkCsrf } from "@/lib/auth/csrf"; +const INVISIBLE_UNICODE_REGEX = /[\u200B-\u200D\u2060\uFEFF\u00AD]/g; + if (!process.env.RESEND_API_KEY) { console.warn("RESEND_API_KEY is not set — feedback emails will fail"); } @@ try { - - const { message, email } = await req.json(); + const body = await req.json(); + const message = + typeof body?.message === "string" + ? body.message.replace(INVISIBLE_UNICODE_REGEX, "").trim() + : ""; + const email = + typeof body?.email === "string" + ? body.email.replace(INVISIBLE_UNICODE_REGEX, "").trim() + : ""; if (!message || typeof message !== "string" || !message.trim()) { return NextResponse.json({ error: "Message is required" }, { status: 400 }); }As per coding guidelines,
{src/components/**/*.{tsx,ts},src/app/api/**/*.ts}: Sanitize input by stripping invisible Unicode characters both client-side and server-side (defense in depth).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/feedback/route.ts` around lines 19 - 27, The message and email should be normalized by stripping invisible Unicode characters before any validation or further use: add a small server-side sanitizer (e.g., a helper like stripInvisibleChars) and apply it to the parsed values from await req.json() (the variables message and email) before the checks in route.ts and before constructing the email content; replace uses of message and email with the sanitized versions so the subsequent validations (/^[^\s@]+@[^\s@]+\.[^\s@]+$/ test and message.trim() check) and any outgoing emails operate on the cleaned strings.src/components/ui/feedback-modal.tsx (1)
318-324:⚠️ Potential issue | 🟡 MinorIncorrect ref assignment pattern for focus trap hook.
The callback ref manually assigns
eltomobileFocusTrapRef.current, butuseFocusTrapexpects the returned ref to be attached directly withref={mobileFocusTrapRef}. This pattern defeats the hook's design and uses unnecessary type assertions.Replace the callback ref with a direct ref assignment:
<div className={`bottom-sheet feedback-bottom-sheet ${sheetExpanded ? "bottom-sheet--expanded" : ""}`} - ref={(el) => { (sheetRef as React.MutableRefObject<HTMLDivElement | null>).current = el; (mobileFocusTrapRef as React.MutableRefObject<HTMLDivElement | null>).current = el; }} + ref={mobileFocusTrapRef} role="dialog"However,
sheetRefis used for animations and drag handling throughout the component. Instead of replacing it entirely, keep both refs as separate, properly-attached refs on the same element:- ref={(el) => { (sheetRef as React.MutableRefObject<HTMLDivElement | null>).current = el; (mobileFocusTrapRef as React.MutableRefObject<HTMLDivElement | null>).current = el; }} + ref={sheetRef}Then attach
mobileFocusTrapRefto the focus trap hook properly (or remove the callback pattern and attach it directly to the same DOM element in a second pass if needed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/feedback-modal.tsx` around lines 318 - 324, The callback ref currently assigns el to mobileFocusTrapRef and sheetRef manually; replace that by attaching the focus-trap ref directly and keeping sheetRef in sync: remove the inline callback and use ref={mobileFocusTrapRef} on the element (so useFocusTrap receives the ref as intended), then synchronize sheetRef with the focus-trap DOM node (e.g., set sheetRef.current = mobileFocusTrapRef.current in a useEffect) so the existing animation/drag logic that uses sheetRef continues to work; reference the mobileFocusTrapRef, sheetRef, and useFocusTrap hook to locate the affected code.
🧹 Nitpick comments (2)
src/app/globals.css (1)
781-783: Variable usage is correct; consider updating the adjacent box-shadow for consistency.Line 781 correctly uses the new
--color-focus-greenvariable. However, line 782's box-shadow usesrgba(148, 187, 81, 0.15), which is the alpha-channel version of the same color (#94BB51).♻️ Optional refactor for consistency
For better maintainability, consider using the CSS variable with an alpha channel:
.signin-modal__input:focus { border-color: var(--color-focus-green); - box-shadow: 0 0 0 3px rgba(148, 187, 81, 0.15); + box-shadow: 0 0 0 3px color-mix(in srgb, var(--color-focus-green) 15%, transparent); }Alternatively, if broader browser support is needed, define a separate variable:
--color-focus-green-alpha: rgba(148, 187, 81, 0.15);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/globals.css` around lines 781 - 783, Replace the hard-coded rgba in the box-shadow with a consistent CSS variable usage: update the box-shadow declaration that currently uses rgba(148, 187, 81, 0.15) to use the existing --color-focus-green variable with alpha (e.g., via color-mix() or by introducing a new --color-focus-green-alpha variable set to rgba(148, 187, 81, 0.15)); locate the rule that sets border-color: var(--color-focus-green) and change its adjacent box-shadow to reference the variable (or the new --color-focus-green-alpha) so both border-color and box-shadow derive from the same named color.src/components/ui/feedback-modal.tsx (1)
19-24: isMobile detection has a timing issue and doesn't handle resize.The
isMobilestate starts asfalseand updates only whenisOpenchanges. This causes:
Brief trap flip-flop on mobile: When the modal opens on mobile,
focusTrapRefinitially activates (isOpen && !isMobile=true && !false), then deactivates after the effect setsisMobile = true. This may cause a brief focus restoration side-effect.No resize handling: If a user rotates their device while the modal is open,
isMobilewon't update.♻️ Suggested fix: Initialize synchronously and handle resize
- const [isMobile, setIsMobile] = useState(false) - useEffect(() => { - setIsMobile(window.innerWidth <= 768) - }, [isOpen]) + const [isMobile, setIsMobile] = useState(() => + typeof window !== "undefined" ? window.innerWidth <= 768 : false + ) + useEffect(() => { + const updateMobile = () => setIsMobile(window.innerWidth <= 768) + updateMobile() + window.addEventListener("resize", updateMobile) + return () => window.removeEventListener("resize", updateMobile) + }, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/feedback-modal.tsx` around lines 19 - 24, The isMobile state is initialized to false and only updated on isOpen changes, causing brief focus-trap flip and no resize handling; change initialization to derive from the current viewport (e.g., useState(() => window.innerWidth <= 768) or set synchronously in useLayoutEffect) and add a window resize listener that updates isMobile (with cleanup) so that focusTrapRef and mobileFocusTrapRef (which depend on isOpen && !isMobile and isOpen && isMobile) always get the correct value when the modal opens or the device is rotated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/auth/login/route.ts`:
- Around line 55-57: The catch-all error handler in the login route currently
returns a 400 for any unexpected failures (e.g., getOAuthClient() or
client.authorize()), which misclassifies provider/internal outages; update the
catch block in the route handler (the try/catch surrounding
getOAuthClient/authorize and subsequent auth logic) to return a sanitized 5xx
response (use NextResponse.json with a generic message and status 500) while
keeping the existing explicit validation branches as 400; continue logging the
actual error server-side (console.error or process logger) but do not include
error details in the response body.
In `@src/app/globals.css`:
- Around line 81-83: Replace the hardcoded rgba(148, 187, 81, 0.15) usage with a
CSS variable: add a companion variable (e.g., --color-focus-green-rgb: 148, 187,
81) alongside --color-focus-green in the :root definitions, then update the
occurrence of rgba(148, 187, 81, 0.15) to use rgba(var(--color-focus-green-rgb),
0.15) so the accent opacity variant is derived from the same color token.
In `@src/app/not-found.tsx`:
- Around line 1-7: Remove the unsupported Metadata API export from
not-found.tsx: delete the exported constant metadata and its type import (import
type { Metadata } from "next") since special files like not-found.tsx do not
support Metadata; keep the page UI but remove the metadata object (title/robots)
and any unused imports to avoid type/import errors.
In `@src/app/robots.ts`:
- Around line 9-16: The disallow array in src/app/robots.ts currently contains
"/oauth/*" and "/api/*" but omits the root paths "/oauth" and "/api"; update the
disallow list (the disallow variable/array in robots.ts) to include the exact
entries "/oauth" and "/api" alongside the existing wildcard entries so both the
root and subpaths are disallowed.
In `@src/components/layout/navbar.tsx`:
- Line 212: The account-switcher ARIA implementation is incomplete: the
container with className "account-switcher__menu" has role="menu" but its
interactive child buttons (the sign-out button and the organization switch
buttons rendered in the same block) lack menu item roles; update the button
elements inside that menu to include role="menuitem" (or
role="menuitemcheckbox"/"menuitemradio" if semantics require) so assistive tech
recognizes them as menu items—specifically add role="menuitem" to the sign-out
button and to each organization button rendered in the account-switcher menu in
navbar.tsx.
In `@src/middleware.ts`:
- Line 7: The redirect in middleware currently uses a permanent status code
(308) via NextResponse.redirect which can cause caches to treat the redirect as
permanent and keep redirecting authenticated users to "/welcome"; update the
middleware to use a temporary redirect (change the status from 308 to 307 or
omit the status to use the default temporary redirect) where
NextResponse.redirect(new URL("/welcome", request.url), 308) is called so the
redirect remains state-dependent and not cached as permanent.
---
Outside diff comments:
In `@src/app/api/feedback/route.ts`:
- Around line 19-27: The message and email should be normalized by stripping
invisible Unicode characters before any validation or further use: add a small
server-side sanitizer (e.g., a helper like stripInvisibleChars) and apply it to
the parsed values from await req.json() (the variables message and email) before
the checks in route.ts and before constructing the email content; replace uses
of message and email with the sanitized versions so the subsequent validations
(/^[^\s@]+@[^\s@]+\.[^\s@]+$/ test and message.trim() check) and any outgoing
emails operate on the cleaned strings.
In `@src/components/ui/feedback-modal.tsx`:
- Around line 318-324: The callback ref currently assigns el to
mobileFocusTrapRef and sheetRef manually; replace that by attaching the
focus-trap ref directly and keeping sheetRef in sync: remove the inline callback
and use ref={mobileFocusTrapRef} on the element (so useFocusTrap receives the
ref as intended), then synchronize sheetRef with the focus-trap DOM node (e.g.,
set sheetRef.current = mobileFocusTrapRef.current in a useEffect) so the
existing animation/drag logic that uses sheetRef continues to work; reference
the mobileFocusTrapRef, sheetRef, and useFocusTrap hook to locate the affected
code.
---
Nitpick comments:
In `@src/app/globals.css`:
- Around line 781-783: Replace the hard-coded rgba in the box-shadow with a
consistent CSS variable usage: update the box-shadow declaration that currently
uses rgba(148, 187, 81, 0.15) to use the existing --color-focus-green variable
with alpha (e.g., via color-mix() or by introducing a new
--color-focus-green-alpha variable set to rgba(148, 187, 81, 0.15)); locate the
rule that sets border-color: var(--color-focus-green) and change its adjacent
box-shadow to reference the variable (or the new --color-focus-green-alpha) so
both border-color and box-shadow derive from the same named color.
In `@src/components/ui/feedback-modal.tsx`:
- Around line 19-24: The isMobile state is initialized to false and only updated
on isOpen changes, causing brief focus-trap flip and no resize handling; change
initialization to derive from the current viewport (e.g., useState(() =>
window.innerWidth <= 768) or set synchronously in useLayoutEffect) and add a
window resize listener that updates isMobile (with cleanup) so that focusTrapRef
and mobileFocusTrapRef (which depend on isOpen && !isMobile and isOpen &&
isMobile) always get the correct value when the modal opens or the device is
rotated.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc92d306-d81e-4a0e-b213-7cf50005ab96
📒 Files selected for processing (27)
GEO-AUDIT-REPORT.mdcertified-v2.pencertified.pennext.config.tssrc/app/about/page.tsxsrc/app/api/auth/callback-handler/route.tssrc/app/api/auth/login/route.tssrc/app/api/feedback/route.tssrc/app/dsa/page.tsxsrc/app/globals.csssrc/app/layout.tsxsrc/app/manifest.tssrc/app/not-found.tsxsrc/app/page.tsxsrc/app/privacy/page.tsxsrc/app/robots.tssrc/app/sitemap.tssrc/app/terms/page.tsxsrc/components/landing/sections/partner-apps.tsxsrc/components/layout/auth-guard.tsxsrc/components/layout/footer.tsxsrc/components/layout/navbar.tsxsrc/components/ui/feedback-modal.tsxsrc/components/ui/sign-in-modal.tsxsrc/hooks/use-focus-trap.tssrc/lib/auth/auth-context.tsxsrc/middleware.ts
💤 Files with no reviewable changes (4)
- src/components/landing/sections/partner-apps.tsx
- certified-v2.pen
- certified.pen
- GEO-AUDIT-REPORT.md
Comment on lines
+81
to
+83
| /* Focus / Success accents */ | ||
| --color-focus-green: #94BB51; | ||
| --color-success-icon: #28a745; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for hardcoded instances of the new color values in globals.css
echo "=== Searching for hardcoded `#94BB51` (focus green) ==="
rg -n '#94BB51|rgb\(148,\s*187,\s*81\)|rgba\(148,\s*187,\s*81' src/app/globals.css
echo ""
echo "=== Searching for hardcoded `#28a745` (success icon) ==="
rg -n '#28a745|rgb\(40,\s*167,\s*69\)|rgba\(40,\s*167,\s*69' src/app/globals.css
echo ""
echo "=== Verification complete ==="
echo "Expected: Only variable definitions (lines 82-83) and rgba usage on line 782 should appear"Repository: hypercerts-org/certified-app
Length of output: 428
Update line 782 to use the CSS variable for consistency.
The variable definitions at lines 82–83 are correct, but line 782 still contains a hardcoded rgba(148, 187, 81, 0.15) (the opacity variant of --color-focus-green). Refactor it to use rgba(var(--color-focus-green), 0.15) to maintain consistency with the new variable system.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/globals.css` around lines 81 - 83, Replace the hardcoded rgba(148,
187, 81, 0.15) usage with a CSS variable: add a companion variable (e.g.,
--color-focus-green-rgb: 148, 187, 81) alongside --color-focus-green in the
:root definitions, then update the occurrence of rgba(148, 187, 81, 0.15) to use
rgba(var(--color-focus-green-rgb), 0.15) so the accent opacity variant is
derived from the same color token.
…erns - Strip invisible Unicode chars from feedback message/email (defense in depth) - Remove combined ref callback in sign-in modal — use focusTrapRef directly - Remove unused backdropRef from sign-in modal - Sync mobile focus trap ref with sheetRef via useEffect in feedback modal Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/feedback/route.ts`:
- Around line 17-23: Wrap the call to req.json() in error handling and return a
400 response for malformed JSON instead of letting it bubble into a 500: catch
JSON parse errors thrown by const body = await req.json() in the handler in
route.ts (where message/email are derived), detect a SyntaxError (or JSON parse
failure), and immediately return a response with status 400 and a short error
message (preserving the existing stripInvisible/message/email logic for valid
bodies).
In `@src/components/ui/feedback-modal.tsx`:
- Around line 254-257: The error message id is duplicated because formContent is
mounted in both desktop and mobile variants; change the id and aria-describedby
to include a per-variant suffix (e.g., "feedback-email-error-desktop" and
"feedback-email-error-mobile") instead of the shared "feedback-email-error".
Update the JSX inside the FeedbackModal component where formContent is rendered
and where aria-describedby is set (the input with aria-invalid/aria-describedby
and the <p id=... role="alert">) to compute the suffix from the active variant
flag (or prop) and use the suffixed id consistently so each variant has a unique
error id.
- Around line 19-32: The mobile focus trap can be activated before sheetRef is
wired, causing useFocusTrap to snapshot null; to fix, attach the focus-trap ref
directly to the sheet element instead of copying it later: when rendering the
bottom sheet element set its ref to a combined ref that assigns to both sheetRef
and mobileFocusTrapRef (so mobileFocusTrapRef.current is populated as soon as
the DOM node mounts) and keep useFocusTrap(isOpen && isMobile) usage;
alternatively update useFocusTrap to re-run when the passed ref node changes,
but the preferred fix is to wire mobileFocusTrapRef into the sheet's ref path so
isMobile + isOpen activation sees a non-null node immediately (refer to
isMobile, sheetRef, mobileFocusTrapRef, and useFocusTrap).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47f09c20-a86a-48de-8e67-e0a46d00cc91
📒 Files selected for processing (3)
src/app/api/feedback/route.tssrc/components/ui/feedback-modal.tsxsrc/components/ui/sign-in-modal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/sign-in-modal.tsx
Comment on lines
+254
to
+257
| aria-invalid={emailError ? true : undefined} | ||
| aria-describedby={emailError ? "feedback-email-error" : undefined} | ||
| /> | ||
| {emailError && <p className="feedback-modal__error" role="alert">{emailError}</p>} | ||
| {emailError && <p id="feedback-email-error" className="feedback-modal__error" role="alert">{emailError}</p>} |
There was a problem hiding this comment.
Give the error message id a per-variant suffix.
While the modal is open, formContent is mounted in the desktop dialog at Line 316 and again in the mobile sheet at Line 340. That makes feedback-email-error appear twice whenever emailError is set, so aria-describedby becomes ambiguous. Either render only the active variant or suffix the ids per variant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/feedback-modal.tsx` around lines 254 - 257, The error
message id is duplicated because formContent is mounted in both desktop and
mobile variants; change the id and aria-describedby to include a per-variant
suffix (e.g., "feedback-email-error-desktop" and "feedback-email-error-mobile")
instead of the shared "feedback-email-error". Update the JSX inside the
FeedbackModal component where formContent is rendered and where aria-describedby
is set (the input with aria-invalid/aria-describedby and the <p id=...
role="alert">) to compute the suffix from the active variant flag (or prop) and
use the suffixed id consistently so each variant has a unique error id.
…edup Round 1 (45 issues): - Extract shared utilities: extractError, sanitize, constants, config - Auth context: deduplicate session fetch, memoize context value - XRPC route: replace `as any` with proper AT Protocol types, add input validation - Org registration: change fail-open to fail-closed with logging - BlobRef: add getBlobRefLink() helper, eliminate double type assertions - React hooks: fix useEffect deps, parallelize fetches with Promise.all - Accessibility: keyboard support on upload divs, ARIA roles on modals - Performance: stop rAF on reduced-motion, fix mobile detection listener Round 2 (8 issues): - A11y: aria-label on sign-in buttons, htmlFor/id on form inputs - Dedup: extract fetchDidDocument() in did.ts, toRkey() in api.ts - Config: shared PUBLIC_URL constant for csrf.ts and oauth-client.ts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Removing authLoading from the useCallback deps meant fetchOrgs was created while auth was still loading, and the `if (authLoading) return` guard prevented it from ever fetching. The effect never re-ran after auth finished because the dep array didn't change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previously each org was resolved sequentially in a for loop (3 network calls per org). With N orgs this meant N sequential rounds. Now all N orgs resolve in parallel via Promise.all, reducing wall-clock time from O(N) to O(1) rounds. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The account-switcher user-row had no overflow constraint, and the item inside used width:100% in a flex context which pushed the sign-out button past the viewport edge. Fixed by: - Adding overflow:hidden to user-row - Setting width:auto on the flex item - Adding text-overflow:ellipsis on name/handle text - Adding overflow-x:hidden to bottom-sheet content Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/profile/banner-upload.tsx (1)
26-28:⚠️ Potential issue | 🟡 MinorExpose and enforce disabled state for the custom button while uploading.
The wrapper stays interactive during upload. Add
aria-disabledand guard activation so keyboard/mouse behavior matches state.Suggested patch
const handleClick = () => { + if (isUploading) return; fileInputRef.current?.click(); }; @@ role="button" tabIndex={0} aria-label="Upload banner" + aria-disabled={isUploading} onClick={handleClick} - onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); handleClick(); } }} + onKeyDown={(e) => { + if (isUploading) return; + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + handleClick(); + } + }}Also applies to: 83-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/profile/banner-upload.tsx` around lines 26 - 28, The custom upload button currently remains interactive during uploads; update its activation handlers (e.g., handleClick that calls fileInputRef.current?.click()) to early-return when the upload state is active (use the component's isUploading or similar flag), and add aria-disabled and/or disabled attributes to the button element so assistive tech and styling reflect the state; apply the same guard to any other activation handlers referenced around the 83-89 block so both mouse clicks and keyboard activation are blocked while uploading.src/components/profile/avatar-upload.tsx (1)
29-31:⚠️ Potential issue | 🟡 MinorMatch interactive semantics with upload state on the avatar trigger.
The custom button should expose disabled state and block activation while
isUploadingis true.Suggested patch
const handleClick = () => { + if (isUploading) return; fileInputRef.current?.click(); }; @@ role="button" tabIndex={0} aria-label="Upload avatar" + aria-disabled={isUploading} onClick={handleClick} - onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); handleClick(); } }} + onKeyDown={(e) => { + if (isUploading) return; + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + handleClick(); + } + }}Also applies to: 85-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/profile/avatar-upload.tsx` around lines 29 - 31, The avatar trigger must be disabled while an upload is in progress: update the click handler(s) (e.g., handleClick and the similar handler around lines 85-90) to early-return when isUploading is true (avoid calling fileInputRef.current?.click()), and add the disabled prop plus aria-disabled={isUploading} to the custom trigger button element so it is not focusable/activatable during upload; also consider setting tabIndex={isUploading ? -1 : 0} to fully match interactive semantics.src/lib/auth/auth-context.tsx (1)
82-90:⚠️ Potential issue | 🟠 MajorClear the provider overlay when the iframe callback completes.
src/app/oauth/callback/page.tsx:26-35keeps the parent mounted in the iframe flow. This handler closes the modal on completion, but it never resetsisRedirectingToProvider, soProviderRedirectOverlaycan stay stuck over the app after a successful sign-in.🪟 Suggested fix
} finally { + setIsRedirectingToProvider(false); setIsModalOpen(false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/auth-context.tsx` around lines 82 - 90, The modal-close path after refreshSession currently calls setIsModalOpen(false) but never resets the provider overlay flag; update the finally block in refreshSession handling (the try/catch/finally around refreshSession) to also call setIsRedirectingToProvider(false) so isRedirectingToProvider is cleared and ProviderRedirectOverlay can unmount; locate the refreshSession call and the finally block along with setIsModalOpen and add the setIsRedirectingToProvider(false) call there.
♻️ Duplicate comments (4)
src/app/api/auth/login/route.ts (1)
54-57:⚠️ Potential issue | 🟠 MajorCatch-all auth failures should return sanitized 5xx, not 400.
Unexpected failures (e.g., OAuth provider/internal errors) are being classified as client input errors. Keep the generic message, but return 500 here.
Suggested fix
} catch (err) { console.error("[Auth] Login error:", err) - return NextResponse.json({ error: "Authentication failed" }, { status: 400 }) + return NextResponse.json({ error: "Authentication failed" }, { status: 500 }) }As per coding guidelines,
src/app/api/**/*.ts:5xx errors must be sanitized — never leak upstream error details to clients.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/auth/login/route.ts` around lines 54 - 57, The catch block in the login route is returning a 400 for unexpected/internal auth failures; change it to return a sanitized 5xx (500) instead while still logging the error server-side. In the catch in src/app/api/auth/login/route.ts (the handler containing the catch that currently logs via console.error("[Auth] Login error:", err) and returns NextResponse.json({ error: "Authentication failed" }, { status: 400 })), keep the server-side logging but change the response status to 500 and do not include error details in the response body (leave the generic "Authentication failed" message).src/components/ui/feedback-modal.tsx (2)
257-260:⚠️ Potential issue | 🟡 MinorUse a variant-specific error id.
formContentis mounted in both dialog variants, sofeedback-email-errorexists twice wheneveremailErroris set. That makesaria-describedbyambiguous. Suffix the ids per variant, or render only the active layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/feedback-modal.tsx` around lines 257 - 260, The error ID "feedback-email-error" is duplicated across dialog variants (formContent mounts in both), causing ambiguous aria-describedby; update the code that renders the email input and error (the emailError check, the aria-describedby on the input, and the <p id="feedback-email-error"> element) to use a variant-specific id (e.g., `feedback-email-error-${variant}` or include the active layout name/prop) so each variant gets a unique id, and ensure the input's aria-describedby references that same computed id only when emailError is set.
26-35:⚠️ Potential issue | 🟠 MajorAttach the mobile trap ref in the sheet's
refpath.
useFocusTrap()snapshotscontainerRef.currentwhenactiveflips. Here the hook is activated before Line 33 copiessheetRef.current, so the mobile sheet can open without a working Tab trap for that cycle.Suggested patch
- // Sync mobile focus trap ref with sheet ref so both point to the same element - useEffect(() => { - if (isMobile && sheetRef.current) { - (mobileFocusTrapRef as React.MutableRefObject<HTMLDivElement | null>).current = sheetRef.current; - } - }, [isMobile, isOpen, mobileFocusTrapRef]) - // Bottom sheet drag state (mobile) const sheetRef = useRef<HTMLDivElement>(null) @@ - ref={sheetRef} + ref={(node) => { + sheetRef.current = node + mobileFocusTrapRef.current = node + }}As per coding guidelines:
src/components/**/*{modal,dialog}*.{tsx,ts}: Modal components must implement focus trapping using theuseFocusTraphook fromsrc/hooks/use-focus-trap.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/feedback-modal.tsx` around lines 26 - 35, The mobile focus trap is activated before your effect copies sheetRef into mobileFocusTrapRef, so the trap may snapshot a null container; fix by wiring the sheet element's ref to set both sheetRef.current and mobileFocusTrapRef.current immediately (use a ref callback on the sheet DOM node) instead of syncing in useEffect. Update the component that renders the sheet so its ref assignment does something like a single callback that assigns the element to sheetRef.current and also to (mobileFocusTrapRef as React.MutableRefObject<HTMLDivElement | null>).current, keeping the existing useFocusTrap calls (useFocusTrap, focusTrapRef, mobileFocusTrapRef) unchanged.src/app/api/feedback/route.ts (1)
18-20:⚠️ Potential issue | 🟡 MinorReturn
400for malformed JSON bodies.
await req.json()parse failures still fall into the generic500path, so bad client payloads are reported as server failures.Suggested patch
try { - const body = await req.json(); + let body: Record<string, unknown>; + try { + body = (await req.json()) as Record<string, unknown>; + } catch { + return NextResponse.json({ error: "Invalid JSON body" }, { status: 400 }); + } const message = typeof body?.message === "string" ? stripInvisible(body.message) : ""; const email = typeof body?.email === "string" ? stripInvisible(body.email) : "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/feedback/route.ts` around lines 18 - 20, The handler currently calls await req.json() inside the try and treats any parse failure as a 500; update the request parsing to detect malformed JSON and return a 400 response: wrap the await req.json() call so that a SyntaxError (or any JSON parse error) is caught separately and the function returns an HTTP 400 with a clear error message, then continue normal validation/extraction of body.message (the existing body/message logic using stripInvisible) for valid JSON; reference the req.json() call and the body/message extraction to locate where to add the specific catch and early 400 response.
🧹 Nitpick comments (1)
src/lib/utils/sanitize.ts (1)
8-10:stripInvisible()does not actually preserve whitespace.The docblock says whitespace is preserved, but
.trim()removes leading and trailing whitespace. Either drop the trim or update the contract so callers choose the right helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/sanitize.ts` around lines 8 - 10, The docblock/behavior mismatch: stripInvisible currently trims leading/trailing whitespace despite claiming to preserve whitespace; change stripInvisible (export const stripInvisible) to only remove INVISIBLE_CHARS_RE and NOT call .trim(), and update its docblock to accurately state that it preserves all whitespace; if you need a trimmed variant, add a separate helper (e.g., stripInvisibleAndTrim) so callers can opt-in to trimming instead of changing stripInvisible's contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/organizations/register/route.ts`:
- Around line 126-130: The current handler returns extractError(res, ...)
directly to clients which can leak upstream 5xx details; change the error path
around the NextResponse.json call so that if res.status is >= 500 you call
extractError(res, ...) only for server-side logging (e.g., processLogger.error
or console.error) and return a sanitized NextResponse.json({ error:
"Registration failed due to an upstream service error" }, { status: res.status
}), while preserving the existing behavior for non-5xx statuses (still returning
the extracted error body). Ensure you update the branch that uses extractError
and NextResponse.json so 5xx responses never include upstream error bodies.
In `@src/app/api/xrpc/`[...method]/route.ts:
- Around line 33-41: The xrpcError function currently maps unknown errors to
500, causing malformed JSON SyntaxError from request.json() to be misclassified;
update xrpcError (referenced by function name xrpcError) to detect SyntaxError
errors (err instanceof SyntaxError or err?.name === 'SyntaxError') and return
status 400 with the SyntaxError's message so malformed JSON yields a 400
response instead of 500; ensure other errors keep existing behavior (use
status/statusCode if present, otherwise 500).
In `@src/lib/atproto/did.ts`:
- Around line 58-60: The code currently assumes every item in doc.alsoKnownAs is
a string before calling startsWith, which can throw for non-string entries;
update the atUri lookup (the find used to set atUri) to first ensure each aka is
a string (e.g., typeof aka === "string") and only then call
aka.startsWith("at://"), so non-string members are skipped and no exception is
raised when computing atUri from doc.alsoKnownAs.
- Around line 83-87: The guard currently assumes each service entry has a string
id so s.id.endsWith(...) can throw on malformed entries; update the service
lookup in the doc.service handling used by pdsService to first ensure s &&
typeof s.id === "string" (or coerce to string safely) before calling endsWith,
e.g. change the predicate in the Array.find that references s.id and
s.id.endsWith to explicitly check typeof s.id === "string" (or use
String(s.id).endsWith) so malformed service objects don't cause exceptions when
computing pdsService.
- Around line 20-28: The did:web handling allows untrusted domains/paths and can
cause SSRF when resolvePdsUrl()/fetchDidDocument() are called with user-supplied
DIDs; update validation to enforce a strict DID pattern in isDid() (or add a new
validateDid() used by route handlers and /api/resolve-did) that only accepts
allowed methods (e.g., restrict to did:plc, did:key or explicitly allowlisted
did:web domains), and/or implement an allowlist of trusted hostnames for did:web
before constructing the URL in resolvePdsUrl(); also ensure all routes that
accept groupDid params call this validator (the GET/PUT handlers for
/api/organizations/[groupDid]/profile, /metadata and other [groupDid] routes)
and reject or 400 on invalid/unallowed DIDs to prevent server-side fetches to
attacker-controlled hosts.
In `@src/lib/auth/auth-context.tsx`:
- Around line 19-31: The safeRedirect function currently permits both "https:"
and "http:" unconditionally; update safeRedirect to only accept "http:" when
running in development (e.g., NODE_ENV === "development" or an equivalent
runtime flag) and otherwise require "https:"; if the protocol is disallowed,
throw an Error("Invalid redirect URL") as before, then set window.location.href
to parsed.href. Ensure you reference and modify the safeRedirect function to
implement this environment-gated protocol check.
- Around line 45-54: refreshSession should fail fast on non-OK fetch responses
and explicitly clear stale auth state when the returned JSON has no did; update
the fetch("/api/auth/session") handling in refreshSession to check res.ok and
throw or return early on non-OK, call setIsAuthenticated(false), setDid(null),
and setPdsUrl(null) when data.did is falsy (to clear previous user), and before
calling resolvePdsUrl(data.did) reset setPdsUrl(null) so the old pdsUrl is not
shown while resolving the new one; keep uses of resolvePdsUrl,
setIsAuthenticated, setDid, and setPdsUrl intact.
In `@src/lib/organizations/api.ts`:
- Around line 18-19: The toRkey helper is lossy and can collide (e.g., "a.b" vs
"a-b"); replace its non-reversible replace(/[^a-zA-Z0-9]/g, "-") logic with a
reversible encoding of the full group DID (the toRkey function) such as
base64url or encodeURIComponent-based encoding that produces only safe
characters for an AT rkey (or further normalizes padding/characters to meet rkey
constraints). Update toRkey to consistently encode the entire did and decode
where needed so membership writes/deletes use the exact same reversible mapping
and cannot collide.
In `@src/lib/utils/api.ts`:
- Around line 10-11: The returned value may be non-string because data.error
isn't type-guarded; change the return to only use data.error when it's a string
(e.g., check typeof data.error === "string" and non-empty) otherwise return
fallback so the function preserves its Promise<string> contract; locate the code
around the variables data, res.json(), and fallback and replace the direct
return of data.error with a guarded conditional that returns fallback if
data.error is missing or not a string.
---
Outside diff comments:
In `@src/components/profile/avatar-upload.tsx`:
- Around line 29-31: The avatar trigger must be disabled while an upload is in
progress: update the click handler(s) (e.g., handleClick and the similar handler
around lines 85-90) to early-return when isUploading is true (avoid calling
fileInputRef.current?.click()), and add the disabled prop plus
aria-disabled={isUploading} to the custom trigger button element so it is not
focusable/activatable during upload; also consider setting tabIndex={isUploading
? -1 : 0} to fully match interactive semantics.
In `@src/components/profile/banner-upload.tsx`:
- Around line 26-28: The custom upload button currently remains interactive
during uploads; update its activation handlers (e.g., handleClick that calls
fileInputRef.current?.click()) to early-return when the upload state is active
(use the component's isUploading or similar flag), and add aria-disabled and/or
disabled attributes to the button element so assistive tech and styling reflect
the state; apply the same guard to any other activation handlers referenced
around the 83-89 block so both mouse clicks and keyboard activation are blocked
while uploading.
In `@src/lib/auth/auth-context.tsx`:
- Around line 82-90: The modal-close path after refreshSession currently calls
setIsModalOpen(false) but never resets the provider overlay flag; update the
finally block in refreshSession handling (the try/catch/finally around
refreshSession) to also call setIsRedirectingToProvider(false) so
isRedirectingToProvider is cleared and ProviderRedirectOverlay can unmount;
locate the refreshSession call and the finally block along with setIsModalOpen
and add the setIsRedirectingToProvider(false) call there.
---
Duplicate comments:
In `@src/app/api/auth/login/route.ts`:
- Around line 54-57: The catch block in the login route is returning a 400 for
unexpected/internal auth failures; change it to return a sanitized 5xx (500)
instead while still logging the error server-side. In the catch in
src/app/api/auth/login/route.ts (the handler containing the catch that currently
logs via console.error("[Auth] Login error:", err) and returns
NextResponse.json({ error: "Authentication failed" }, { status: 400 })), keep
the server-side logging but change the response status to 500 and do not include
error details in the response body (leave the generic "Authentication failed"
message).
In `@src/app/api/feedback/route.ts`:
- Around line 18-20: The handler currently calls await req.json() inside the try
and treats any parse failure as a 500; update the request parsing to detect
malformed JSON and return a 400 response: wrap the await req.json() call so that
a SyntaxError (or any JSON parse error) is caught separately and the function
returns an HTTP 400 with a clear error message, then continue normal
validation/extraction of body.message (the existing body/message logic using
stripInvisible) for valid JSON; reference the req.json() call and the
body/message extraction to locate where to add the specific catch and early 400
response.
In `@src/components/ui/feedback-modal.tsx`:
- Around line 257-260: The error ID "feedback-email-error" is duplicated across
dialog variants (formContent mounts in both), causing ambiguous
aria-describedby; update the code that renders the email input and error (the
emailError check, the aria-describedby on the input, and the <p
id="feedback-email-error"> element) to use a variant-specific id (e.g.,
`feedback-email-error-${variant}` or include the active layout name/prop) so
each variant gets a unique id, and ensure the input's aria-describedby
references that same computed id only when emailError is set.
- Around line 26-35: The mobile focus trap is activated before your effect
copies sheetRef into mobileFocusTrapRef, so the trap may snapshot a null
container; fix by wiring the sheet element's ref to set both sheetRef.current
and mobileFocusTrapRef.current immediately (use a ref callback on the sheet DOM
node) instead of syncing in useEffect. Update the component that renders the
sheet so its ref assignment does something like a single callback that assigns
the element to sheetRef.current and also to (mobileFocusTrapRef as
React.MutableRefObject<HTMLDivElement | null>).current, keeping the existing
useFocusTrap calls (useFocusTrap, focusTrapRef, mobileFocusTrapRef) unchanged.
---
Nitpick comments:
In `@src/lib/utils/sanitize.ts`:
- Around line 8-10: The docblock/behavior mismatch: stripInvisible currently
trims leading/trailing whitespace despite claiming to preserve whitespace;
change stripInvisible (export const stripInvisible) to only remove
INVISIBLE_CHARS_RE and NOT call .trim(), and update its docblock to accurately
state that it preserves all whitespace; if you need a trimmed variant, add a
separate helper (e.g., stripInvisibleAndTrim) so callers can opt-in to trimming
instead of changing stripInvisible's contract.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe6e401a-a935-4162-bca4-6ae6f2adcda3
📒 Files selected for processing (38)
src/app/api/auth/login/route.tssrc/app/api/auth/logout/route.tssrc/app/api/feedback/route.tssrc/app/api/organizations/[groupDid]/members/route.tssrc/app/api/organizations/[groupDid]/profile/route.tssrc/app/api/organizations/register/route.tssrc/app/api/xrpc/[...method]/route.tssrc/app/organizations/page.tsxsrc/components/dashboard/custom-domain-modal.tsxsrc/components/dashboard/username-card.tsxsrc/components/landing/hero-signin-button.tsxsrc/components/landing/home-client.tsxsrc/components/landing/orbiting-logos.tsxsrc/components/landing/sections/ready-cta-button.tsxsrc/components/layout/navbar.tsxsrc/components/organizations/add-org-modal.tsxsrc/components/organizations/handle-search.tsxsrc/components/organizations/membership-sync-modal.tsxsrc/components/organizations/org-settings.tsxsrc/components/profile/avatar-upload.tsxsrc/components/profile/banner-upload.tsxsrc/components/profile/profile-edit-form.tsxsrc/components/ui/feedback-modal.tsxsrc/components/ui/sign-in-modal.tsxsrc/hooks/use-org-profile.tssrc/lib/atproto/did.tssrc/lib/atproto/profile.tssrc/lib/atproto/types.tssrc/lib/auth/auth-context.tsxsrc/lib/auth/csrf.tssrc/lib/auth/oauth-client.tssrc/lib/organizations/api.tssrc/lib/organizations/org-context.tsxsrc/lib/utils/api.tssrc/lib/utils/config.tssrc/lib/utils/constants.tssrc/lib/utils/sanitize.tssrc/middleware.ts
✅ Files skipped from review due to trivial changes (11)
- src/components/landing/hero-signin-button.tsx
- src/components/organizations/membership-sync-modal.tsx
- src/components/organizations/handle-search.tsx
- src/components/dashboard/custom-domain-modal.tsx
- src/components/landing/sections/ready-cta-button.tsx
- src/app/api/organizations/[groupDid]/profile/route.ts
- src/components/dashboard/username-card.tsx
- src/components/layout/navbar.tsx
- src/components/organizations/add-org-modal.tsx
- src/components/profile/profile-edit-form.tsx
- src/lib/utils/constants.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/middleware.ts
Comment on lines
+33
to
+41
| /** Extract a usable HTTP status + message from an unknown XRPC error. */ | ||
| function xrpcError(err: unknown): { status: number; message: string } { | ||
| const error = err as { status?: number; statusCode?: number; message?: string } | ||
| const status = error?.status ?? error?.statusCode ?? 500 | ||
| const message = | ||
| status >= 500 | ||
| ? "Internal server error" | ||
| : (error?.message ?? "Internal server error") | ||
| return { status, message } |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
cd src/app/api/xrpc && find . -type f -name "*.ts" | head -20Repository: hypercerts-org/certified-app
Repository: hypercerts-org/certified-app
Exit code: 0
stdout:
./[...method]/route.ts
Script executed:
find . -path "*/src/app/api/xrpc/*/route.ts" -o -path "*/src/app/api/xrpc/**/route.ts"Repository: hypercerts-org/certified-app
Repository: hypercerts-org/certified-app
Exit code: 0
stdout:
./src/app/api/xrpc/[...method]/route.ts
Script executed:
git ls-files | grep -E "src/app/api/xrpc.*route\.ts"Repository: hypercerts-org/certified-app
Repository: hypercerts-org/certified-app
Exit code: 0
stdout:
src/app/api/xrpc/[...method]/route.ts
Script executed:
wc -l src/app/api/xrpc/[...method]/route.tsRepository: hypercerts-org/certified-app
Repository: hypercerts-org/certified-app
Exit code: 0
stdout:
272 src/app/api/xrpc/[...method]/route.ts
Script executed:
cat -n src/app/api/xrpc/[...method]/route.ts | head -100Repository: hypercerts-org/certified-app
Repository: hypercerts-org/certified-app
Exit code: 0
stdout:
1 import { NextRequest, NextResponse } from "next/server"
2 import { Agent } from "@atproto/api"
3 import type {
4 ComAtprotoRepoPutRecord,
5 ComAtprotoRepoDeleteRecord,
6 ComAtprotoIdentityUpdateHandle,
7 ComAtprotoServerRequestPasswordReset,
8 ComAtprotoServerResetPassword,
9 ComAtprotoServerUpdateEmail,
10 } from "@atproto/api"
11 import { getOAuthClient } from "@/lib/auth/oauth-client"
12 import { getSessionDid, deleteSession } from "@/lib/auth/session"
13 import { checkCsrf } from "@/lib/auth/csrf"
14 import { LIMIT_MIN, LIMIT_MAX } from "@/lib/utils/constants"
15
16 const ALLOWED_WRITE_COLLECTIONS = [
17 "org.impactindexer.link.attestation",
18 "app.certified.actor.profile",
19 "app.certified.actor.membership",
20 "app.certified.actor.organization",
21 ]
22
23 const ALLOWED_BLOB_CONTENT_TYPES = [
24 "image/jpeg",
25 "image/png",
26 "image/webp",
27 "image/gif",
28 "image/svg+xml",
29 ]
30
31 const MAX_BLOB_SIZE = 4 * 1024 * 1024 // 4MB — Vercel serverless functions have a ~4.5MB request body limit
32
33 /** Extract a usable HTTP status + message from an unknown XRPC error. */
34 function xrpcError(err: unknown): { status: number; message: string } {
35 const error = err as { status?: number; statusCode?: number; message?: string }
36 const status = error?.status ?? error?.statusCode ?? 500
37 const message =
38 status >= 500
39 ? "Internal server error"
40 : (error?.message ?? "Internal server error")
41 return { status, message }
42 }
43
44 /** Clamp and validate a limit query param. */
45 function parseLimit(raw: string | undefined): number | undefined {
46 if (!raw) return undefined
47 const n = parseInt(raw, 10)
48 if (isNaN(n)) return undefined
49 return Math.min(Math.max(LIMIT_MIN, n), LIMIT_MAX)
50 }
51
52 export async function GET(
53 request: NextRequest,
54 { params }: { params: Promise<{ method: string[] }> }
55 ) {
56 try {
57 const { method } = await params
58 const methodName = method.join(".")
59
60 const did = await getSessionDid()
61 if (!did)
62 return NextResponse.json({ error: "Not authenticated" }, { status: 401 })
63
64 const client = await getOAuthClient()
65 let oauthSession
66 try {
67 oauthSession = await client.restore(did)
68 } catch {
69 await deleteSession()
70 return NextResponse.json({ error: "Session expired" }, { status: 401 })
71 }
72 const agent = new Agent(oauthSession)
73
74 // Query params come as Record<string, string> from URLSearchParams.
75 // AT Protocol SDK expects specific typed params — we validate the required
76 // fields per-method below and cast through unknown for the proxy pattern.
77 const queryParams: Record<string, string> = Object.fromEntries(
78 request.nextUrl.searchParams.entries()
79 )
80
81 switch (methodName) {
82 case "com.atproto.repo.getRecord": {
83 const { repo, collection, rkey, cid } = queryParams
84 if (!repo || !collection || !rkey) {
85 return NextResponse.json({ error: "repo, collection, and rkey are required" }, { status: 400 })
86 }
87 const result = await agent.com.atproto.repo.getRecord({ repo, collection, rkey, cid })
88 return NextResponse.json(result.data)
89 }
90 case "com.atproto.repo.listRecords": {
91 const { repo, collection, cursor, reverse, rkeyEnd, rkeyStart } = queryParams
92 if (!repo || !collection) {
93 return NextResponse.json({ error: "repo and collection are required" }, { status: 400 })
94 }
95 const result = await agent.com.atproto.repo.listRecords({
96 repo,
97 collection,
98 limit: parseLimit(queryParams.limit),
99 cursor,
100 reverse: reverse === "true" ? true : undefined,
Script executed:
cat -n src/app/api/xrpc/[...method]/route.ts | sed -n '100,200p'Repository: hypercerts-org/certified-app
Repository: hypercerts-org/certified-app
Exit code: 0
stdout:
100 reverse: reverse === "true" ? true : undefined,
101 rkeyEnd,
102 rkeyStart,
103 })
104 return NextResponse.json(result.data)
105 }
106 case "com.atproto.server.getSession": {
107 const result = await agent.com.atproto.server.getSession()
108 return NextResponse.json(result.data)
109 }
110 case "com.atproto.sync.getBlob": {
111 const { did: blobDid, cid } = queryParams
112 if (!blobDid || !cid) {
113 return NextResponse.json({ error: "did and cid are required" }, { status: 400 })
114 }
115 const result = await agent.com.atproto.sync.getBlob({ did: blobDid, cid })
116 const blob = result.data as Uint8Array
117 return new NextResponse(Buffer.from(blob), {
118 headers: {
119 "Content-Type":
120 result.headers["content-type"] || "application/octet-stream",
121 },
122 })
123 }
124 default:
125 return NextResponse.json(
126 { error: `Unknown method: ${methodName}` },
127 { status: 400 }
128 )
129 }
130 } catch (err: unknown) {
131 const { status, message } = xrpcError(err)
132 return NextResponse.json({ error: message }, { status })
133 }
134 }
135
136 export async function POST(
137 request: NextRequest,
138 { params }: { params: Promise<{ method: string[] }> }
139 ) {
140 const csrfError = checkCsrf(request)
141 if (csrfError) return csrfError
142
143 try {
144 const { method } = await params
145 const methodName = method.join(".")
146
147 const did = await getSessionDid()
148 if (!did)
149 return NextResponse.json({ error: "Not authenticated" }, { status: 401 })
150
151 const client = await getOAuthClient()
152 let oauthSession
153 try {
154 oauthSession = await client.restore(did)
155 } catch {
156 await deleteSession()
157 return NextResponse.json({ error: "Session expired" }, { status: 401 })
158 }
159 const agent = new Agent(oauthSession)
160
161 // Parse body once (uploadBlob uses arrayBuffer instead)
162 let body: Record<string, unknown> | null = null
163 if (methodName !== "com.atproto.repo.uploadBlob") {
164 body = await request.json()
165 }
166
167 // Validate repo on write methods — reject cross-repo writes
168 const REPO_METHODS = ["com.atproto.repo.putRecord", "com.atproto.repo.deleteRecord"]
169 if (body && REPO_METHODS.includes(methodName)) {
170 if (body.repo && body.repo !== did) {
171 return NextResponse.json(
172 { error: "Forbidden: cannot write to another user's repo" },
173 { status: 403 }
174 )
175 }
176 // Collection allowlist
177 if (
178 body.collection &&
179 !ALLOWED_WRITE_COLLECTIONS.includes(body.collection as string)
180 ) {
181 return NextResponse.json(
182 { error: "Collection not allowed" },
183 { status: 403 }
184 )
185 }
186 }
187
188 switch (methodName) {
189 case "com.atproto.repo.putRecord": {
190 const result = await agent.com.atproto.repo.putRecord(
191 body as ComAtprotoRepoPutRecord.InputSchema
192 )
193 return NextResponse.json(result.data)
194 }
195 case "com.atproto.repo.deleteRecord": {
196 const result = await agent.com.atproto.repo.deleteRecord(
197 body as ComAtprotoRepoDeleteRecord.InputSchema
198 )
199 return NextResponse.json(result.data)
200 }
Script executed:
cat -n src/app/api/xrpc/[...method]/route.ts | sed -n '200,272p'Repository: hypercerts-org/certified-app
Repository: hypercerts-org/certified-app
Exit code: 0
stdout:
200 }
201 case "com.atproto.repo.uploadBlob": {
202 const contentType =
203 request.headers.get("content-type") || "application/octet-stream"
204 // Check content type
205 const mimeType = contentType.split(";")[0].trim()
206 if (!ALLOWED_BLOB_CONTENT_TYPES.includes(mimeType)) {
207 return NextResponse.json(
208 { error: "Unsupported media type" },
209 { status: 415 }
210 )
211 }
212 // Check content length
213 const contentLengthHeader = request.headers.get("content-length")
214 if (contentLengthHeader && Number(contentLengthHeader) > MAX_BLOB_SIZE) {
215 return NextResponse.json(
216 { error: "Payload too large" },
217 { status: 413 }
218 )
219 }
220 const buffer = await request.arrayBuffer()
221 if (buffer.byteLength > MAX_BLOB_SIZE) {
222 return NextResponse.json(
223 { error: "Payload too large" },
224 { status: 413 }
225 )
226 }
227 const result = await agent.com.atproto.repo.uploadBlob(
228 new Uint8Array(buffer),
229 { encoding: contentType }
230 )
231 return NextResponse.json(result.data)
232 }
233 case "com.atproto.identity.updateHandle": {
234 await agent.com.atproto.identity.updateHandle(
235 body as ComAtprotoIdentityUpdateHandle.InputSchema
236 )
237 // Void operation — no data to return
238 return NextResponse.json({})
239 }
240 case "com.atproto.server.requestPasswordReset": {
241 await agent.com.atproto.server.requestPasswordReset(
242 body as ComAtprotoServerRequestPasswordReset.InputSchema
243 )
244 return NextResponse.json({})
245 }
246 case "com.atproto.server.resetPassword": {
247 await agent.com.atproto.server.resetPassword(
248 body as ComAtprotoServerResetPassword.InputSchema
249 )
250 return NextResponse.json({})
251 }
252 case "com.atproto.server.requestEmailUpdate": {
253 const result = await agent.com.atproto.server.requestEmailUpdate()
254 return NextResponse.json(result.data)
255 }
256 case "com.atproto.server.updateEmail": {
257 await agent.com.atproto.server.updateEmail(
258 body as ComAtprotoServerUpdateEmail.InputSchema
259 )
260 return NextResponse.json({})
261 }
262 default:
263 return NextResponse.json(
264 { error: `Unknown method: ${methodName}` },
265 { status: 400 }
266 )
267 }
268 } catch (err: unknown) {
269 const { status, message } = xrpcError(err)
270 return NextResponse.json({ error: message }, { status })
271 }
272 }
Script executed:
cat > /tmp/test_json_error.js << 'EOF'
try {
JSON.parse("invalid json");
} catch (e) {
console.log("Error type:", e.constructor.name);
console.log("Has status?", "status" in e);
console.log("Has statusCode?", "statusCode" in e);
console.log("Message:", e.message);
console.log("Error keys:", Object.keys(e));
}
EOF
node /tmp/test_json_error.jsRepository: hypercerts-org/certified-app
Repository: hypercerts-org/certified-app
Exit code: 0
stdout:
Error type: SyntaxError
Has status? false
Has statusCode? false
Message: Unexpected token 'i', "invalid json" is not valid JSON
Error keys: []
Malformed JSON should return 400, not 500.
For non-blob POSTs, request.json() at line 164 throws SyntaxError on malformed bodies. Without explicit handling, this bubbles to the catch block at lines 268–270, where xrpcError() defaults to status 500 since SyntaxError has no status or statusCode property. This incorrectly classifies client input errors as server errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/xrpc/`[...method]/route.ts around lines 33 - 41, The xrpcError
function currently maps unknown errors to 500, causing malformed JSON
SyntaxError from request.json() to be misclassified; update xrpcError
(referenced by function name xrpcError) to detect SyntaxError errors (err
instanceof SyntaxError or err?.name === 'SyntaxError') and return status 400
with the SyntaxError's message so malformed JSON yields a 400 response instead
of 500; ensure other errors keep existing behavior (use status/statusCode if
present, otherwise 500).
Comment on lines
+20
to
+28
| if (did.startsWith("did:plc:")) { | ||
| url = `https://plc.directory/${did}`; | ||
| } else if (did.startsWith("did:web:")) { | ||
| const withoutPrefix = did.slice("did:web:".length); | ||
| const parts = withoutPrefix.split(":"); | ||
| const domain = parts[0]; | ||
| const path = parts.length > 1 ? parts.slice(1).join("/") : ".well-known"; | ||
| url = `https://${domain}/${path}/did.json`; | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Find all resolver callsites with context
rg -nP --type=ts -C3 '\b(resolveHandle|resolvePdsUrl)\s*\('
# 2) Find routes/handlers where `did` originates from request input
rg -nP --type=ts -C4 '\b(did)\b' src
# 3) Inspect for upstream DID validation helpers (allowlist/sanitization)
rg -nP --type=ts -C3 '\b(validate|sanitize|assert).*(did|DID)|\b(did|DID).*(validate|sanitize|assert)\b' srcRepository: hypercerts-org/certified-app
Length of output: 50385
🏁 Script executed:
#!/bin/bash
# 1. Check the /api/resolve-did route for input validation
echo "=== API route: /api/resolve-did ==="
cat src/app/api/resolve-did/route.ts
echo -e "\n=== API route: /api/organizations/[groupDid]/profile ==="
cat src/app/api/organizations/[groupDid]/profile/route.ts
echo -e "\n=== Check add-org-modal for user input validation ==="
sed -n '50,75p' src/components/organizations/add-org-modal.tsx
echo -e "\n=== Search for DID format validation (regex/validation functions) ==="
rg -n 'did.*format|format.*did|DID.*pattern|pattern.*DID|/did:' src --type=ts -C2Repository: hypercerts-org/certified-app
Length of output: 5306
🏁 Script executed:
#!/bin/bash
echo "=== Find isDid function definition ==="
rg -n 'function isDid|const isDid|export.*isDid' src --type=ts -A3
echo -e "\n=== Check where isDid is imported from in add-org-modal ==="
head -30 src/components/organizations/add-org-modal.tsx | grep -E "import|isDid"
echo -e "\n=== Search for all uses of groupDid in route handlers ==="
rg -n 'groupDid' src/app/api/organizations --type=ts -B2 -A2
echo -e "\n=== Check if there's any middleware or validation for [groupDid] routes ==="
ls -la src/app/api/organizations/ 2>/dev/null || echo "No middleware file found"
find src/app/api -name "middleware*" -o -name "_middleware*" 2>/dev/null || echo "No middleware files"Repository: hypercerts-org/certified-app
Length of output: 24450
Confirm did:web trust boundary to avoid SSRF exposure.
did:web domain/path is derived from untrusted input and fetched server-side without validation. Multiple API routes accept groupDid route parameters that are passed directly to resolvePdsUrl() without format validation:
/api/organizations/[groupDid]/profile(GET/PUT)/api/organizations/[groupDid]/metadata(GET/PUT)- Other
[groupDid]routes
A crafted did:web value like did:web:attacker.com:malicious:path bypasses the minimal isDid() checks and reaches fetchDidDocument(), which constructs https://attacker.com/malicious/path/did.json and fetches it.
Ensure route params and /api/resolve-did?did= inputs are validated against a strict DID format before resolution, or implement an allowlist of trusted DID methods/domains.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/atproto/did.ts` around lines 20 - 28, The did:web handling allows
untrusted domains/paths and can cause SSRF when
resolvePdsUrl()/fetchDidDocument() are called with user-supplied DIDs; update
validation to enforce a strict DID pattern in isDid() (or add a new
validateDid() used by route handlers and /api/resolve-did) that only accepts
allowed methods (e.g., restrict to did:plc, did:key or explicitly allowlisted
did:web domains), and/or implement an allowlist of trusted hostnames for did:web
before constructing the URL in resolvePdsUrl(); also ensure all routes that
accept groupDid params call this validator (the GET/PUT handlers for
/api/organizations/[groupDid]/profile, /metadata and other [groupDid] routes)
and reject or 400 on invalid/unallowed DIDs to prevent server-side fetches to
attacker-controlled hosts.
Reduced margin-right from 12px to 4px on signout button, added min-width:0 on the text container div so flex children can shrink properly. The 12px margin was pushing the button past the viewport. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previous fix used overflow:hidden which clipped the button entirely. Now properly constrains the layout: item uses flex:1 1 0% with no right padding so the signout button fits within the row without needing overflow clipping. Removed all margin from signout button. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Added margin-right: 8px to signout button for breathing room from edge - Active background now covers the entire user row (including signout button) using :has(.account-switcher__item--active) selector - Removed individual item background in user-row to avoid double-up Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owners can never leave a group, period. Replaced the async checkCanLeave (which fired N member-list API calls on mount, causing 400 errors) with a simple synchronous useMemo that checks org.role !== "owner". Eliminates all /members 400s from the groups page. Tooltip now says "Owners can't leave the group". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Full rename of "organizations" to "groups" across: - URL routes: /organizations/* → /groups/* - API routes: /api/organizations/* → /api/groups/* - Directories: src/lib/organizations → src/lib/groups, src/components/organizations → src/components/groups - Types: Organization → Group, OrgOrganization → GroupMetadata - Functions: registerOrganization → registerGroup, resolveOrganizations → resolveGroups - Variables: organizations → groups, setOrganizations → setGroups - All user-facing text, comments, and documentation - robots.ts, README.md, AGENTS.md, test plans Preserved: Schema.org "@type": "Organization", AT Protocol collection names (app.certified.actor.organization), organizationType data field, and legal text in terms/privacy pages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- login/route.ts: catch-all returns 500 instead of 400 for internal errors - register/route.ts: sanitize upstream 5xx — return generic 502, not raw error - middleware.ts: use 307 temporary redirect (not 308 permanent) for auth-conditional - navbar.tsx: add role="menuitem" to all buttons inside role="menu" containers - feedback-modal.tsx: fix focus trap timing (declare sheetRef before useFocusTrap), render formContent only in active variant to avoid duplicate element IDs - did.ts: type-guard alsoKnownAs elements and service.id before string methods - not-found.tsx: remove unsupported metadata export - robots.ts: add bare /oauth and /api to disallow list Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- auth-context: restrict http: redirects to development only - auth-context: refreshSession clears stale auth state on non-OK response or null DID (prevents rendering old user after switch) - groups/api: document toRkey collision safety (no change needed — DIDs use restricted charset, collisions impossible in practice) - utils/api: guard data.error type before returning - feedback/route: catch JSON parse errors as 400, not 500 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previously membership rkeys were derived from the group DID via lossy string replacement. Now the PDS assigns TID-based rkeys automatically: - putMembership: omits rkey so PDS generates a TID, checks for existing records first to avoid duplicates - deleteMembership: lists records, finds by groupDid, deletes by rkey - resolveGroups: gets rkey from local membership record - Removed toRkey() helper entirely Nothing depends on rkey format or its relationship to the DID. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
putRecord requires an explicit rkey — can't omit it (createRecord would auto-generate but isn't in the XRPC proxy allowlist). Added generateTid() that produces AT Protocol TID format (timestamp-based base32-sortable, 13 chars). Duplicate check via listMemberships ensures no duplicate records for the same groupDid. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Added com.atproto.repo.createRecord to XRPC proxy so the PDS generates the TID rkey server-side. Removed client-side generateTid() function. putMembership now uses createRecord (no rkey needed), deleteMembership finds record by groupDid then deletes by rkey. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Results of 4 rounds of 12-reviewer audits + a comprehensive 10-reviewer code audit (2 rounds) + CodeRabbit feedback (2 rounds). Full rename of "organizations" to "groups." Covers security, SSR, SEO, GEO, accessibility, performance, TypeScript, React patterns, API consistency, state management, error handling, and code quality.
Rename: organizations → groups
/organizations/*→/groups/*(pages and API)src/lib/organizations→src/lib/groups,src/components/organizations→src/components/groupsSecurity
safeRedirect(): validates protocols, restrictshttp:to development onlyrefreshSession(): clears stale auth state on non-OK response or null DIDextractError(): guardsdata.erroris string before returningcheckCsrf(), JSON parse errors return 400XRPC Proxy
com.atproto.repo.createRecordto proxy — PDS generates TID rkeys server-sidecreateRecordData Model
createRecord.putMembershipchecks for duplicates before creating,deleteMembershipfinds record bygroupDidthen deletes by rkey. Nothing depends on rkey format.useMemoonorg.rolereplaces async N+1 member-list calls. Eliminates all/members400 errors.SEO / GEO
/to/welcome/oauthand/apidisallow entries addedAccessibility
role="button",tabIndex={0}, keyboard supportrole="dialog",aria-modal,aria-labelon close buttonsrole="menuitem"on all buttons insiderole="menu"htmlFor/idassociationsTypeScript & Code Quality
extractError(),sanitize,getBlobRefLink(),fetchDidDocument()PUBLIC_URLconfigalsoKnownAsandservice.idReact & State Management
refreshSession(), memoized context valueCSS / Mobile
Test plan
/unauthenticated → 307 redirect to/welcome/groups→ groups list loadsnpm run buildpassesnpm run lint— no new errors🤖 Generated with Claude Code