feat: retained member sessions - #6
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe PR replaces wallet-balance and signed-message authorization with Gnosis SIWE authentication, JWT-backed member sessions, session-protected archive APIs, client session state, rate limiting, and updated access-page layouts. ChangesSIWE session authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Wallet
participant AuthMessage
participant AuthVerify
participant MemberAuth
participant SessionCookie
participant ValhallaPage
participant ArchiveAPI
Wallet->>AuthMessage: Request SIWE challenge
AuthMessage->>SessionCookie: Store challenge cookie
Wallet->>AuthVerify: Submit message and signature
AuthVerify->>MemberAuth: Verify signature and eligibility
AuthVerify->>SessionCookie: Store member session
ValhallaPage->>ArchiveAPI: Request archive data
ArchiveAPI-->>ValhallaPage: Return session-authorized data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
Pull request overview
This PR migrates Valhalla’s access gating from an ad-hoc “sign a message” flow to an EIP-4361 (SIWE) flow with retained, HttpOnly cookie sessions, and updates the client and protected API routes to rely on those sessions for file/channel access.
Changes:
- Added SIWE message + verification endpoints and JWT-backed cookie helpers for challenge/session management.
- Updated
/api/filesand/api/channelto require a valid member session instead of a request-body signature. - Refactored the client-side flow to restore sessions, run SIWE, and normalize API error handling (including 401/403 recovery/logout).
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sample.env | Updates JWT secret guidance to require a 32+ byte random value. |
| pnpm-lock.yaml | Removes ethers and locks new jose dependency. |
| package.json | Swaps ethers for jose to support JWT/session signing. |
| docs/session-workflow.md | Adds branch naming guidance (conventional prefixes). |
| app/utils/requests.ts | Introduces session/auth request helpers and an ApiRequestError with HTTP status. |
| app/page.tsx | Reworks the UI flow to restore sessions, run SIWE, and handle session invalidation/logout. |
| app/globals.css | Adjusts layout/styles for the new hero + gated access presentation. |
| app/api/shared/session.ts | Adds JWT signing/verification helpers and secure cookie management for challenge/session tokens. |
| app/api/shared/memberAuth.ts | Shifts membership enforcement to session-based authorization and adds a shares threshold filter in the member query. |
| app/api/shared/authRateLimit.ts | Adds in-memory rate limiting utilities and a standard 429 response. |
| app/api/files/route.ts | Protects file listing behind requireMemberSession() instead of signature verification. |
| app/api/channel/route.ts | Protects signed URL issuance behind requireMemberSession() instead of signature verification. |
| app/api/auth/verify/route.ts | Verifies SIWE message/signature and establishes a session cookie on success. |
| app/api/auth/session/route.ts | Restores/refreshes existing sessions and clears invalid/ineligible sessions. |
| app/api/auth/message/route.ts | Generates a SIWE message and stores a short-lived challenge cookie. |
| app/api/auth/logout/route.ts | Clears auth cookies to end a retained session. |
| AGENTS.md | Updates repo guidance to reflect SIWE + retained-session architecture and JWT_SECRET requirements. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
app/api/files/route.ts:65
- The response from this session-protected endpoint can include sensitive membership-gated data (the S3 object keys). Consider explicitly setting
Cache-Control: no-storeto prevent intermediary/proxy caching once auth is cookie-based.
return NextResponse.json({ response: files });
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
app/api/auth/verify/route.ts (1)
173-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
NOT_MEMBER_ERRORinstead of repeating the string.
app/api/shared/memberAuth.tsline 5 exportsNOT_MEMBER_ERRORwith the same text. This file already imports from that module. Two copies will drift when the threshold changes.♻️ Suggested change
import { isEligibleMemberAddress, logServerError, + NOT_MEMBER_ERROR, } from "../../shared/memberAuth";if (!(await isEligibleMemberAddress(parsedMessage.address))) { return NextResponse.json( - { error: "This wallet does not hold at least 100 RaidGuild shares." }, + { error: NOT_MEMBER_ERROR }, { status: 403 }, ); }🤖 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 `@app/api/auth/verify/route.ts` around lines 173 - 178, Replace the duplicated membership error string in the isEligibleMemberAddress rejection with the imported NOT_MEMBER_ERROR constant from memberAuth.ts, preserving the existing 403 response behavior.app/api/auth/message/route.ts (1)
63-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConstruct the origin URL once.
new URL(origin)runs twice on lines 68 and 72.♻️ Suggested change
const nonce = generateSiweNonce(); const now = new Date(); + const originUrl = new URL(origin); const message = createSiweMessage({ address, chainId: gnosis.id, - domain: new URL(origin).host, + domain: originUrl.host, expirationTime: new Date(now.getTime() + 5 * 60 * 1000), issuedAt: now, nonce, - scheme: new URL(origin).protocol.slice(0, -1), + scheme: originUrl.protocol.slice(0, -1),🤖 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 `@app/api/auth/message/route.ts` around lines 63 - 76, Construct the origin URL once before createSiweMessage, store it in a local variable, and reuse that variable for both the domain and scheme fields instead of calling new URL(origin) twice.app/api/files/route.ts (1)
67-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared
MemberSessionErrorresponse mapping.
app/api/channel/route.tslines 48-53 contain the identical block. A small helper inapp/api/shared/memberAuth.tskeeps the status contract in one place as more protected routes appear.♻️ Suggested helper
// app/api/shared/memberAuth.ts export function memberSessionErrorResponse(error: unknown) { if (error instanceof MemberSessionError) { return NextResponse.json({ error: error.message }, { status: error.status }); } return null; }🤖 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 `@app/api/files/route.ts` around lines 67 - 73, Extract the duplicated MemberSessionError handling from the route-level catch blocks into a shared memberSessionErrorResponse helper in memberAuth.ts. Update both the files route and channel route to call the helper and return its response when non-null, preserving the existing error message and status contract.app/api/channel/route.ts (1)
14-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCheck the session before parsing the body.
requireMemberSession()runs on line 31, after the request body is read and validated. An unauthenticated caller currently receives 400 for a malformed body instead of 401. Moving the session check first gives consistent responses and avoids work for unauthenticated callers.♻️ Suggested change
export async function POST(req: Request) { + try { + await requireMemberSession(); + } catch (error: unknown) { + if (error instanceof MemberSessionError) { + return NextResponse.json( + { error: error.message }, + { status: error.status }, + ); + } + logServerError("Error fetching channel", error); + return NextResponse.json({ error: "Failed to fetch data" }, { status: 500 }); + } + let requestBody: ChannelRequestBody;Keep the existing
MemberSessionErrorbranch for the S3 block, or extract a small helper to avoid repeating the mapping.🤖 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 `@app/api/channel/route.ts` around lines 14 - 31, Move the requireMemberSession() call and its existing MemberSessionError handling to the beginning of POST, before req.json() parsing and validation. Preserve the 401 response mapping and keep the current request-body validation and S3 error handling behavior unchanged, reusing a helper only if needed to avoid duplicating the session-error mapping.app/api/shared/authRateLimit.ts (1)
11-13: 📐 Maintainability & Code Quality | 🔵 TrivialIn-memory limits apply per process only.
rateLimitEntriesandrpcBudgetlive in process memory. On serverless or multi-instance deployments, each instance keeps its own counters, so the effective limit multiplies by the instance count. Counters also reset on cold start.This is acceptable as a first defense layer. If the deployment scales horizontally, plan a shared store or an edge/WAF rate limit. Document the tradeoff before adding a new dependency.
🤖 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 `@app/api/shared/authRateLimit.ts` around lines 11 - 13, Document near rateLimitEntries and rpcBudget that these in-memory counters are process-local, reset on cold starts, and multiply effective limits across serverless or multi-instance deployments. Explicitly note that this is an intentional first defense layer and that horizontally scaled deployments should use a shared store or edge/WAF rate limiting, without adding a dependency.AGENTS.md (1)
38-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the rate-limit module to the repository map.
The map lists
app/api/shared/memberAuth.tsandapp/api/shared/session.ts. This PR also addsapp/api/shared/authRateLimit.ts, which controls authentication throttling and429responses. Document it so contributors do not bypass the limiter.📝 Proposed documentation addition
- `app/api/shared/session.ts`: signed challenge/session tokens and secure cookie helpers backed by `JWT_SECRET`. +- `app/api/shared/authRateLimit.ts`: request throttling for authentication and + RPC routes, including `429` responses with `Retry-After`.🤖 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 `@AGENTS.md` around lines 38 - 43, Add app/api/shared/authRateLimit.ts to the repository map alongside memberAuth.ts and session.ts, describing that it provides authentication throttling and 429 responses.app/page.tsx (2)
354-384: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplain the
queueMicrotaskdeferral.The effect calls
resetAuthenticationandresetFileRequest, then deferssetAuthPhaseandsetActionErrorinto a microtask. The ordering requirement is not obvious. Without a comment, a later edit can inline the two calls and lose the error message that the resets clear.Add a short comment that states why the state updates must run after the resets.
🤖 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 `@app/page.tsx` around lines 354 - 384, Add a concise comment immediately before the queueMicrotask call in the protected-access error effect, explaining that resetAuthentication and resetFileRequest clear related state, so setAuthPhase and setActionError must be deferred to preserve the error message after those resets. Do not change the existing ordering or behavior.
304-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the nested ternary chains with a derived helper.
accessStateuses a ten-level ternary chain.statusAnnouncementthen repeats almost the same branch order. A future state change must be applied in both places, and the two chains can drift.Extract the state resolution into a small function and map the announcement from a lookup keyed by
accessState.♻️ Sketch of the refactor
+const resolveAccessState = (input: { + chainId?: number; + hasAddress: boolean; + hasVerifiedAccess: boolean; + filesError: unknown; + isConnecting: boolean; + isEndingSession: boolean; + isFilesLoading: boolean; + isSessionLoading: boolean; + logoutError: string; + sessionError: unknown; +}) => { + if (input.hasVerifiedAccess) { + if (input.isFilesLoading) return "archive-loading"; + return input.filesError ? "archive-error" : "archive"; + } + if (input.isEndingSession) return "signing-out"; + if (input.logoutError) return "logout-error"; + if (input.isSessionLoading || input.isConnecting) return "loading"; + if (input.sessionError) return "session-error"; + if (!input.hasAddress) return "idle"; + return input.chainId !== gnosis.id ? "network-error" : "check-in"; +};Then build
statusAnnouncementfrom aRecord<AccessState, string>, and keep only thecheck-incase dependent onauthPhase.🤖 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 `@app/page.tsx` around lines 304 - 352, Replace the nested ternary chains defining accessState and statusAnnouncement with a derived state-resolution helper and a Record keyed by AccessState for announcements. Ensure the helper preserves the existing branch precedence, and keep only the check-in announcement dynamic by deriving it from authPhase while all other states use lookup values.app/utils/requests.ts (1)
83-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the channel response.
axios.posthas no generic here, sodata.data.channelresolves toany.app/page.tsxdeclaresuseMutation<string, Error, string>, which relies on that untyped value. Add the response type so the boundary stays typed.As per coding guidelines: "Prefer typed request validation at API boundaries."♻️ Proposed typing
export const getValhallaFile = async (key: string) => { try { - const data = await axios.post("/api/channel", { + const data = await axios.post<{ channel: string }>("/api/channel", { key, }); return data.data.channel;🤖 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 `@app/utils/requests.ts` around lines 83 - 92, Update getValhallaFile to provide an explicit response type to axios.post, defining the expected channel payload as a string. Return the typed channel value so the useMutation<string, Error, string> contract remains enforced without any.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/auth/message/route.ts`:
- Around line 57-61: Update the rate-limit flow in the message route to key the
primary limit on the client’s network identity rather than the
attacker-controlled address, while retaining the address-based limit as a
secondary check. Derive forwarded client identity only when the request comes
through a trusted proxy; otherwise use the direct connection identity, and apply
the existing authRateLimitResponse behavior for either limit.
In `@app/api/auth/session/route.ts`:
- Around line 43-49: Update the error response in the session route’s catch
block to include the Cache-Control: no-store header, matching the successful
response paths and preventing intermediaries from storing the authentication
error response.
- Around line 26-42: The session route must enforce per-address rate limiting
before calling isEligibleMemberAddress. Add the session-specific
checkAuthRateLimit invocation near the start of the handler, and return
authRateLimitResponse immediately when the address exceeds the limit, preserving
the existing eligibility and session-token flow for allowed requests.
In `@app/api/auth/verify/route.ts`:
- Around line 41-44: Update the createPublicClient configuration for
gnosisClient to pass the GNOSIS_RPC_URL environment variable to http() and
configure an explicit request timeout; preserve the existing Gnosis chain setup.
Add GNOSIS_RPC_URL to sample.env without committing an endpoint value.
In `@app/api/shared/authRateLimit.ts`:
- Around line 63-86: Update checkAuthRpcBudget to accept an identity parameter
and track independent counters and reset times per identity while preserving the
existing global cap. In the auth verify route, pass the challenge address as
identity when invoking checkAuthRpcBudget so one smart-account cannot exhaust
another caller’s budget.
In `@app/api/shared/memberAuth.ts`:
- Around line 24-25: Update MEMBERSHIP_MIN_SHARES to the intended whole-share
threshold used directly by the subgraph shares_gte filter, then align the
related membership error messages with that threshold so they report the actual
required share count.
In `@app/api/shared/session.ts`:
- Around line 167-189: Update getSameOrigin to derive the expected protocol from
x-forwarded-proto when the request is confirmed to come through a known trusted
proxy, while retaining requestUrl.protocol for direct or untrusted requests.
Compare the parsed Origin protocol against that trusted protocol and preserve
the existing same-host validation and null-on-invalid behavior.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 38-43: Add app/api/shared/authRateLimit.ts to the repository map
alongside memberAuth.ts and session.ts, describing that it provides
authentication throttling and 429 responses.
In `@app/api/auth/message/route.ts`:
- Around line 63-76: Construct the origin URL once before createSiweMessage,
store it in a local variable, and reuse that variable for both the domain and
scheme fields instead of calling new URL(origin) twice.
In `@app/api/auth/verify/route.ts`:
- Around line 173-178: Replace the duplicated membership error string in the
isEligibleMemberAddress rejection with the imported NOT_MEMBER_ERROR constant
from memberAuth.ts, preserving the existing 403 response behavior.
In `@app/api/channel/route.ts`:
- Around line 14-31: Move the requireMemberSession() call and its existing
MemberSessionError handling to the beginning of POST, before req.json() parsing
and validation. Preserve the 401 response mapping and keep the current
request-body validation and S3 error handling behavior unchanged, reusing a
helper only if needed to avoid duplicating the session-error mapping.
In `@app/api/files/route.ts`:
- Around line 67-73: Extract the duplicated MemberSessionError handling from the
route-level catch blocks into a shared memberSessionErrorResponse helper in
memberAuth.ts. Update both the files route and channel route to call the helper
and return its response when non-null, preserving the existing error message and
status contract.
In `@app/api/shared/authRateLimit.ts`:
- Around line 11-13: Document near rateLimitEntries and rpcBudget that these
in-memory counters are process-local, reset on cold starts, and multiply
effective limits across serverless or multi-instance deployments. Explicitly
note that this is an intentional first defense layer and that horizontally
scaled deployments should use a shared store or edge/WAF rate limiting, without
adding a dependency.
In `@app/page.tsx`:
- Around line 354-384: Add a concise comment immediately before the
queueMicrotask call in the protected-access error effect, explaining that
resetAuthentication and resetFileRequest clear related state, so setAuthPhase
and setActionError must be deferred to preserve the error message after those
resets. Do not change the existing ordering or behavior.
- Around line 304-352: Replace the nested ternary chains defining accessState
and statusAnnouncement with a derived state-resolution helper and a Record keyed
by AccessState for announcements. Ensure the helper preserves the existing
branch precedence, and keep only the check-in announcement dynamic by deriving
it from authPhase while all other states use lookup values.
In `@app/utils/requests.ts`:
- Around line 83-92: Update getValhallaFile to provide an explicit response type
to axios.post, defining the expected channel payload as a string. Return the
typed channel value so the useMutation<string, Error, string> contract remains
enforced without any.
🪄 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 Plus
Run ID: 6b78aa8f-9834-4e23-9886-0a79ec864a1f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
AGENTS.mdapp/api/auth/logout/route.tsapp/api/auth/message/route.tsapp/api/auth/session/route.tsapp/api/auth/verify/route.tsapp/api/channel/route.tsapp/api/files/route.tsapp/api/shared/authRateLimit.tsapp/api/shared/memberAuth.tsapp/api/shared/session.tsapp/globals.cssapp/page.tsxapp/utils/requests.tsdocs/session-workflow.mdpackage.jsonsample.env
This pull request migrates the authentication and authorization system from a basic signature-based flow to a standards-based Sign-In with Ethereum (SIWE) flow, with secure session management and improved rate limiting. It introduces new API endpoints for SIWE message generation, verification, session restoration, and logout, and updates file/channel access endpoints to require a valid member session. The documentation and environment variable requirements are also updated to reflect these changes.
Authentication and Session Management Overhaul
app/api/auth/message/route.ts(SIWE message generation),app/api/auth/verify/route.ts(SIWE verification and session establishment),app/api/auth/session/route.ts(session restoration), andapp/api/auth/logout/route.ts(logout and session clearing). These endpoints manage secure, short-lived sessions using HttpOnly cookies and JWTs. [1] [2] [3] [4]app/api/shared/session.ts(referenced in docs) to handle challenge/session tokens and secure cookies, backed byJWT_SECRET.API Route Updates
app/api/files/route.tsandapp/api/channel/route.tsto require a valid member session instead of verifying signatures in the request body. Errors related to session/auth are now handled consistently. [1] [2] [3] [4] [5] [6]app/api/shared/memberAuth.ts. [1] [2] [3]Membership Verification and Subgraph Query
shares_gte) for eligibility, and clarified error messages for ineligible wallets. [1] [2]Security and Rate Limiting
app/api/shared/authRateLimit.ts, returning appropriate 429 responses withRetry-Afterheaders.JWT_SECRETmust be a cryptographically random value and described the new authentication/session system. [1] [2]These changes enhance security, improve user experience with session persistence, and lay the groundwork for more scalable and standards-compliant authentication.
Summary by CodeRabbit
New Features
Bug Fixes
Style