Conversation
There was a problem hiding this comment.
Pull request overview
This PR represents a major refactor of the fresh-session library to adapt to Fresh v2. The changes involve a complete rewrite of the session management system, replacing the old Fresh v1 middleware approach with a new architecture based on Fresh v2's core middleware system.
Changes:
- Complete rewrite of session management using Fresh v2 middleware patterns
- New modular storage architecture with five backend implementations (Memory, Cookie, KV, Redis, SQL)
- Replaced iron-webcrypto with native Web Crypto API (AES-GCM) for encryption
- Modernized API with improved TypeScript support and cleaner interface design
Reviewed changes
Copilot reviewed 67 out of 77 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | New common type definitions and ISessionStore interface |
| src/session.ts | Core SessionManager class and middleware function |
| src/crypto.ts | AES-GCM encryption implementation using Web Crypto API |
| src/cookie.ts | Cookie utility functions for session management |
| src/config.ts | Session configuration with defaults and merging logic |
| src/storage/memory.ts | In-memory session store implementation |
| src/storage/cookie.ts | Cookie-based session store (client-side storage) |
| src/storage/kv.ts | Deno KV session store with auto-expiration |
| src/storage/redis.ts | Redis session store with TTL support |
| src/storage/sql.ts | SQL session store with MySQL-compatible syntax |
| sample/main.ts | Updated demo application using Fresh v2 |
| README.md | Comprehensive documentation rewrite |
| deno.json | Updated dependencies and configuration for Fresh v2 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async cleanup(): Promise<void> { | ||
| await this.#client.execute( | ||
| `DELETE FROM ${this.#tableName} WHERE expires_at IS NOT NULL AND expires_at < NOW()`, | ||
| ); |
There was a problem hiding this comment.
The SQL cleanup query uses MySQL-specific NOW() function. This should be documented or made database-agnostic. PostgreSQL and SQLite use different syntax (PostgreSQL: NOW() works, SQLite: datetime('now')). Consider parameterizing the current timestamp for better portability.
src/storage/sql.ts
Outdated
| await this.#client.execute( | ||
| `INSERT INTO ${this.#tableName} (session_id, data, expires_at) | ||
| VALUES (?, ?, ?) | ||
| ON DUPLICATE KEY UPDATE data = VALUES(data), expires_at = VALUES(expires_at)`, | ||
| [sessionId, dataJson, expiresAtStr], | ||
| ); |
There was a problem hiding this comment.
The table name in SQL queries is directly interpolated without proper escaping or quoting. This could lead to SQL injection if the table name is user-controlled (though unlikely in typical usage). Consider using parameterized table names or validating/escaping the table name. For example, use backticks for MySQL or double quotes for PostgreSQL to properly quote table identifiers.
sample/session.ts
Outdated
| @@ -0,0 +1,24 @@ | |||
| import { MemorySessionStore, session } from "../../fresh-session/mod.ts"; | |||
There was a problem hiding this comment.
The import path "../../fresh-session/mod.ts" appears incorrect. This should likely be "../mod.ts" to import from the parent directory, or use the JSR package name if published. The current path suggests looking for a "fresh-session" directory two levels up, which doesn't exist in this repository structure.
| // Expiry check | ||
| if (entry.value.expiresAt && new Date(entry.value.expiresAt) < new Date()) { | ||
| // Delete if expired | ||
| await kv.delete(this.getKey(cookieValue)); | ||
| return { | ||
| sessionId: crypto.randomUUID(), | ||
| data: {}, | ||
| isNew: true, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The KvSessionStore has both application-level expiry checking (line 84) and KV-level expiration (line 120 with expireIn). This is redundant. Since Deno KV automatically removes expired entries, the manual expiry check and deletion on line 84-86 is unnecessary and could cause race conditions. Consider removing the manual expiry check or documenting why both are needed.
| if (this.#doSessionDestroy) { | ||
| await this.store.destroy(this.#sessionId); | ||
| deleteSessionCookie(response.headers, this.config.cookieName); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When session.destroy() is called, flash messages set in the same request via session.flash.set() are lost because the session is destroyed before they can be saved. This could be confusing for developers who want to set a flash message before destroying the session (e.g., "Session has been cleared!"). Consider documenting this behavior or saving flash messages to the cookie even when destroying the session, so they appear on the next request.
| export function mergeSessionConfig( | ||
| inputSessionConfig?: PartialSessionConfig, | ||
| ): SessionConfig { | ||
| // Merge input config with default config | ||
| if (!inputSessionConfig) { | ||
| return { ...defaultSessionConfig }; | ||
| } | ||
|
|
||
| return { | ||
| cookieName: inputSessionConfig.cookieName ?? | ||
| defaultSessionConfig.cookieName, | ||
| cookieOptions: { | ||
| ...defaultSessionConfig.cookieOptions, | ||
| ...inputSessionConfig.cookieOptions, | ||
| }, | ||
| sessionExpires: inputSessionConfig.sessionExpires ?? | ||
| defaultSessionConfig.sessionExpires, | ||
| }; |
There was a problem hiding this comment.
The mergeSessionConfig function creates a shallow copy of defaultSessionConfig but then spreads both default and input cookieOptions. If a user provides partial cookie options (e.g., only secure: false), they will still get all the default values merged in. However, this doesn't deep-clone the nested object, so modifying the returned config could affect the default. Consider using a more defensive deep merge or freezing the default config object.
src/storage/sql.ts
Outdated
| await this.#client.execute( | ||
| `INSERT INTO ${this.#tableName} (session_id, data, expires_at) | ||
| VALUES (?, ?, ?) | ||
| ON DUPLICATE KEY UPDATE data = VALUES(data), expires_at = VALUES(expires_at)`, | ||
| [sessionId, dataJson, expiresAtStr], | ||
| ); |
There was a problem hiding this comment.
The SQL UPSERT syntax used here (INSERT ... ON DUPLICATE KEY UPDATE) is MySQL-specific and will not work with PostgreSQL, SQLite, or other SQL databases. Consider using a more portable approach or documenting this limitation clearly. PostgreSQL uses INSERT ... ON CONFLICT DO UPDATE, and SQLite uses INSERT OR REPLACE or INSERT ... ON CONFLICT.
adaut fresh v2.