Conversation
6b793df to
5d3e01f
Compare
| * @returns The session | ||
| */ | ||
| export async function getSession(id: string) { | ||
| const session = await prisma.session.findUnique({ |
There was a problem hiding this comment.
can we make funcs like this inline more?
| }, | ||
| }, | ||
| }); | ||
| } No newline at end of file |
There was a problem hiding this comment.
new line at end of file pls
| * @returns If the delete was successful | ||
| */ | ||
| export async function deleteSession(id: string) { | ||
| return prisma.session.delete({ |
There was a problem hiding this comment.
inline with this too, anything that is simple enough to go inline
|
|
||
| export type UpdateSessionData = { | ||
| expiresAt?: Date; | ||
| }; No newline at end of file |
| @@ -0,0 +1,8 @@ | |||
| export type GetSessionsFilters = { | |||
There was a problem hiding this comment.
I'm not sure that these types are complex enough to warrant an externally defined type. Can we just do this inline on the functions that use them?
There was a problem hiding this comment.
I was having this be outside incase we wanna expand on it in the future, i can just leave it in the file if that's better
| * @param filters userId and projectId | ||
| * @returns All sessions associated with the filters | ||
| */ | ||
| export async function getSessions(filters: GetSessionsFilters) { |
There was a problem hiding this comment.
what is the use case for this function and should both params be optional? Or should we require at least one? Or should both be required?
There was a problem hiding this comment.
Youre right, ill make both required
| return crypto.createHash("sha256").update(token).digest("hex"); | ||
| } | ||
|
|
||
| export type GetSessionsFilters = { |
There was a problem hiding this comment.
I think these should be defined inline with the function
| * @param id The session id | ||
| * @returns The session | ||
| */ | ||
| export const getSession = async (id: string) => |
There was a problem hiding this comment.
ah, I would define this with function as regular since defining it as a const makes it different than the rest
| if (!session) return null; | ||
|
|
||
| // don't extend expired sessions | ||
| if (session.expiresAt < new Date()) { |
There was a problem hiding this comment.
similar to above, can make this part of prisma query
There was a problem hiding this comment.
Apparently prisma doesnt support that, i think this should be fine
| } | ||
| return prisma.session.update({ | ||
| where: { id }, | ||
| data: { |
| * @param id The session id | ||
| * @returns If the delete was successful | ||
| */ | ||
| export const deleteSession = async (id: string) => |
There was a problem hiding this comment.
Review written by a Claude agent.
Same framing as the other action-layer PRs open right now: if there's a follow-up ticket for adding caller-identity checks before this is wired into anything, deferring the pure "who's allowed to call this" items is reasonable — separated below so that's explicit.
Must-fix regardless of sequencing (these are functional bugs, not auth gaps):
- Missing
awaitonprisma.session.createmeans sessions created via this function currently can't be validated with the token that's returned (see inline comment) — this looks like it hasn't been exercised end-to-end yet. getSessions's empty-string filter bypass (see inline comment) — a plain truthiness bug, not access-control related.getSession(line 45) is missing areturn— it always resolvesundefinedregardless of whether a matching session exists.updateSessionaccepts an unbounded caller-suppliedexpiresAtwith no cap (see inline comment).- Small one: the
Session.tokencolumn stores a SHA-256 hash (good — not plaintext), but the column name doesn't say so. A rename totokenHashwould help stop a future "fix" from reintroducing plaintext storage when someone's debugging why validation isn't matching.
Pending authz ticket:
createSession,getSessions,deleteSession, andvalidateSessionall takeuserId/projectIdas trusted caller input with no identity check. Related but distinct:createSessionalso doesn't checkUserProjectmembership before issuing a session, which is more of an authorization-scoping question than a pure "is there a caller" one — worth deciding whether that's in scope for the same follow-up.
Worth its own ticket, not introduced by this PR: same RLS gap noted elsewhere — Session sits in the same unprotected public schema.
(Edit: corrected a line reference above and the inline comment anchors below that were off in my first pass — same content, right lines now.)
|
|
||
| const expiresAt = new Date(); | ||
| expiresAt.setDate(expiresAt.getDate() + 1); // 1 day | ||
| const session = prisma.session.create({ |
There was a problem hiding this comment.
prisma.session.create(...) returns a lazy PrismaPromise, and its own enumerable shape includes a then — so { ...session, token: rawToken } below produces a thenable object. When the caller does await createSession(...), that triggers promise assimilation on the returned object rather than treating it as a plain object: the DB write does happen, but the resolved value becomes the created DB record (hashed token), and token: rawToken gets discarded before it ever reaches the caller. Net effect: sessions created this way can't be validated with the token that's actually returned — worth an explicit await here and constructing the return object from the resolved row, independent of anything else in this review.
| }) { | ||
| const sessions = await prisma.session.findMany({ | ||
| where: { | ||
| ...(filters?.userId && { userId: filters.userId }), |
There was a problem hiding this comment.
filters?.userId && { userId: filters.userId } evaluates to "" (falsy) when userId is an empty string rather than undefined, and spreading {...""} contributes nothing — so an empty-string filter is silently dropped rather than applied. Worth checking for undefined/null explicitly (filters.userId != null) rather than relying on truthiness, since this is a plain logic bug that'll under-filter even for a fully authorized caller who passes an empty string by mistake.
| * @param data When the new expireAt should be | ||
| * @returns The updated session | ||
| */ | ||
| export async function updateSession(id: string, data: { expiresAt: Date }) { |
There was a problem hiding this comment.
expiresAt is taken directly from the caller with no upper bound and no check that the session belongs to them — worth clamping to a fixed server-side increment with an absolute cap (e.g. createdAt + N days) rather than trusting the caller's value directly, regardless of what auth ends up gating this.
4 participants
Created lib/session.ts for managing the Session join table which contains:
createSession
getSession
getSessions
deleteSession
validateSession