fix: session cookie not being created on http sites#112
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a new session management system using Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Server
participant sessionHandler
participant authHandler
Browser->>Server: HTTP Request
Server->>sessionHandler: Process session (parse/set cookies, etc.)
sessionHandler-->>Server: Attach session to event.locals
Server->>authHandler: Check authentication, fetch user, enforce permissions
authHandler-->>Server: Continue or redirect based on session/user
Server-->>Browser: Response (page, redirect, etc.)
sequenceDiagram
participant User
participant LoginPage
participant Server
participant sessionHandler
User->>LoginPage: Submit login form
LoginPage->>Server: POST /auth/login
Server->>sessionHandler: Initialize session
Server->>Server: Authenticate user credentials
alt Success
Server->>sessionHandler: Set session data (userId, etc.)
Server-->>LoginPage: Redirect to dashboard or onboarding
else Failure
Server-->>LoginPage: Return error message
end
sequenceDiagram
participant User
participant LogoutPage
participant Server
participant sessionHandler
User->>LogoutPage: Visit /auth/logout or submit logout action
LogoutPage->>Server: Request to logout
Server->>sessionHandler: Destroy session
Server-->>LogoutPage: Redirect to login
Assessment against linked issues
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| event.locals.user = await getUserByUsername(session.username); | ||
| try { | ||
| event.locals.user = await getUserByUsername(session.username); | ||
| } catch (error) { |
Check failure
Code scanning / ESLint
Disallow unused variables Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
src/hooks.server.ts (1)
34-37: 🧹 Nitpick (assertive)Add trailing slash to public
/imgpath
/imgmatches exactly that path only. Static assets are usually served from/img/.... A missing slash will block legitimate requests.-const publicPaths = ['/auth/login', '/img']; +const publicPaths = ['/auth/login', '/img/'];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.json(1 hunks)src/app.d.ts(1 hunks)src/hooks.server.ts(4 hunks)src/lib/constants.ts(1 hunks)src/lib/services/session-handler.ts(1 hunks)src/lib/services/session-service.ts(2 hunks)src/routes/auth/login/+page.server.ts(2 hunks)src/routes/auth/logout/+page.server.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/lib/services/session-service.ts (1)
src/lib/types/settings.type.ts (1)
Settings(19-30)
src/routes/auth/logout/+page.server.ts (1)
src/routes/auth/login/+page.server.ts (2)
load(6-24)actions(26-87)
src/hooks.server.ts (2)
src/lib/services/user-service.ts (1)
getUserByUsername(16-40)src/lib/services/session-handler.ts (1)
sessionHandler(23-32)
🪛 GitHub Check: ESLint
src/hooks.server.ts
[failure] 56-56: Disallow unused variables
'error' is defined but never used.
🔇 Additional comments (2)
src/lib/constants.ts (1)
1-7: Good simplification of environment detectionNice refactoring to use SvelteKit's built-in
devflag instead of manually checking environment variables. This approach is more idiomatic and reliable.Verify that hardcoding
isTesttofalsewon't affect any test-specific behavior in your application, especially since your package.json still contains test scripts that setAPP_ENV=TEST.package.json (1)
69-69:Details
❓ Verification inconclusive
Appropriate dependency for session management
Adding
svelte-kit-cookie-sessionis a good choice for implementing structured session management, which aligns with the PR's goal to fix session cookies on HTTP sites.Please verify that version 4.1.1 is compatible with your SvelteKit version (^2.20.8). You can run this command to check for any compatibility issues:
🏁 Script executed:
#!/bin/bash # Check for compatibility issues between svelte-kit-cookie-session and your SvelteKit version npm ls @sveltejs/kit svelte-kit-cookie-sessionLength of output: 93
Verify svelte-kit-cookie-session compatibility with SvelteKit
Adding
svelte-kit-cookie-session@4.1.1is appropriate for structured session management and aligns with the PR’s goal. However,npm lsdid not report any compatibility info, so please manually confirm that this version supports your SvelteKit (^2.20.8) by inspecting its peer dependencies:# Check which @sveltejs/kit versions this package expects npm view svelte-kit-cookie-session@4.1.1 peerDependenciesEnsure the output includes a matching
@sveltejs/kitrange that covers your installed version.
src/lib/services/session-service.ts
Outdated
| const sessionTimeout = settings.auth?.sessionTimeout || 60; // minutes | ||
|
|
||
| // More reliable HTTPS detection | ||
| const isSecureConnection = request.url.includes('https:') || request.headers?.get('x-forwarded-proto') === 'https'; | ||
|
|
||
| // Log what's happening | ||
| console.log(`Setting session cookie with secure=${isSecureConnection}`); | ||
|
|
||
| cookies.set('session_id', sessionId, { | ||
| path: '/', | ||
| httpOnly: true, | ||
| secure: isSecureConnection, // Only use secure when actually on HTTPS | ||
| maxAge: sessionTimeout * 60, | ||
| sameSite: 'lax' // Change to lax for better compatibility | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalise timeout value & harden HTTPS detection
- Default timeout is
60 minhere but1440 min(24 h) elsewhere (createSession,getSession). Divergent expiries will confuse users and break “remember me” expectations. request.url.includes('https:')may yield false-positives (e.g./proxy/https://…).- Console logs leak potentially sensitive information; prefer a structured logger or remove entirely.
-const sessionTimeout = settings.auth?.sessionTimeout || 60; // minutes
+const sessionTimeout =
+ settings.auth?.sessionTimeout ??
+ 1440; // minutes – keep in sync with createSession()
-const isSecureConnection =
- request.url.includes('https:') ||
- request.headers?.get('x-forwarded-proto') === 'https';
+const isSecureConnection =
+ new URL(request.url).protocol === 'https:' ||
+ request.headers?.get('x-forwarded-proto') === 'https';
-// Log what's happening
-console.log(`Setting session cookie with secure=${isSecureConnection}`);
+// Optional: use a proper logger here📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sessionTimeout = settings.auth?.sessionTimeout || 60; // minutes | |
| // More reliable HTTPS detection | |
| const isSecureConnection = request.url.includes('https:') || request.headers?.get('x-forwarded-proto') === 'https'; | |
| // Log what's happening | |
| console.log(`Setting session cookie with secure=${isSecureConnection}`); | |
| cookies.set('session_id', sessionId, { | |
| path: '/', | |
| httpOnly: true, | |
| secure: isSecureConnection, // Only use secure when actually on HTTPS | |
| maxAge: sessionTimeout * 60, | |
| sameSite: 'lax' // Change to lax for better compatibility | |
| }); | |
| const sessionTimeout = | |
| settings.auth?.sessionTimeout ?? | |
| 1440; // minutes – keep in sync with createSession() | |
| // More reliable HTTPS detection | |
| const isSecureConnection = | |
| new URL(request.url).protocol === 'https:' || | |
| request.headers?.get('x-forwarded-proto') === 'https'; | |
| // Optional: use a proper logger here | |
| cookies.set('session_id', sessionId, { | |
| path: '/', | |
| httpOnly: true, | |
| secure: isSecureConnection, // Only use secure when actually on HTTPS | |
| maxAge: sessionTimeout * 60, | |
| sameSite: 'lax' // Change to lax for better compatibility | |
| }); |
🤖 Prompt for AI Agents (early access)
In src/lib/services/session-service.ts around lines 194 to 208, unify the session timeout default value with the 1440 minutes used elsewhere to avoid inconsistent expiries. Replace the HTTPS detection logic by removing the unreliable request.url.includes('https:') check and rely solely on the 'x-forwarded-proto' header or other secure indicators. Remove the console.log statement to prevent leaking sensitive information or replace it with a structured logger if logging is necessary.
Deploying arcane with
|
| Latest commit: |
550cad2
|
| Status: | ⚡️ Build in progress... |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/routes/auth/login/+page.server.ts (2)
60-64:⚠️ Potential issueReplace return-object redirect with
throw redirect()In SvelteKit actions a plain object is serialised as JSON; the browser never navigates.
You already importredirect, so swap to the idiomatic pattern:- return { - status: 302, - location: redirectTo - }; + throw redirect(302, redirectTo);Leaving this unchanged will leave the user on the login page after a “successful” login.
37-45: 🧹 Nitpick (assertive)Remove or gate debug
console.logstatements that leak PII
console.log(\User not found: ${username}`)` and the “Password verification failed” log both reveal usernames. Combined with the redirect log at line 60 they may expose sensitive info and crowd production logs.Guard them behind
if (DEV)or switch to a structured logger with log-level control:- console.log(`User not found: ${username}`); + if (import.meta.env.DEV) console.debug(`User not found: ${username}`);Repeat for the other occurrences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.env.example(1 hunks)docker-compose.yml(1 hunks)docs/docs/getting-started/configuration.md(1 hunks)docs/docs/getting-started/quickstart.md(2 hunks)package.json(1 hunks)src/lib/components/navbar.svelte(2 hunks)src/lib/services/paths-service.ts(1 hunks)src/lib/services/session-handler.ts(1 hunks)src/lib/services/session-service.ts(0 hunks)src/routes/auth/login/+page.server.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/lib/services/session-service.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lib/services/paths-service.ts (1)
src/lib/constants.ts (2)
isTest(7-7)isDev(4-4)
🪛 LanguageTool
docs/docs/getting-started/quickstart.md
[uncategorized] ~52-~52: Loose punctuation mark.
Context: ...ns (Important):** - APP_ENV: This should be set to production when...
(UNLIKELY_OPENING_PUNCTUATION)
docs/docs/getting-started/configuration.md
[uncategorized] ~108-~108: Loose punctuation mark.
Context: ...Docker Socket Access - DOCKER_GID: Required. Set this to the group ID ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~112-~112: Loose punctuation mark.
Context: ...Application Environment - APP_ENV: Required for Docker. Controls which...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~120-~120: Loose punctuation mark.
Context: ... Security - PUBLIC_SESSION_SECRET: Required. This secret is used to si...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~128-~128: Loose punctuation mark.
Context: ...* - PUBLIC_ALLOW_INSECURE_COOKIES: Optional (Use with Caution). By def...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
docs/docs/getting-started/configuration.md
113-113: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
🔇 Additional comments (16)
package.json (1)
69-69: Appropriate addition of session management dependency.The addition of
svelte-kit-cookie-sessionaligns with the PR objective of fixing session cookie issues on HTTP sites. This package provides standardized cookie-based session handling with proper security features.src/lib/components/navbar.svelte (2)
2-2: Appropriate import addition.The addition of the
LogOuticon is correctly imported alongside other icons.
140-151: Well-implemented logout functionality.The logout button implementation is well-structured:
- Properly positioned at the bottom of the sidebar with a separator
- Uses a POST form submission (more secure than a GET request)
- Adapts properly to both collapsed and expanded sidebar states
- Correctly labeled with appropriate icon
This change directly addresses the PR objective of fixing logout button functionality.
docker-compose.yml (2)
5-5: Consider security implications of using root user.While running as
rootmay resolve certain permission issues, it's generally considered a security best practice to use the principle of least privilege with a non-root user. Unless there's a specific reason requiring root access, consider using a less privileged user.
12-15: Well-configured environment variables for session management.The environment variables are properly configured for session security:
APP_ENV=productionensures correct production pathsDOCKER_GIDis clearly documented with instructions for finding the correct valuePUBLIC_SESSION_SECRETincludes clear instructions for generating a secure random string- The commented
PUBLIC_ALLOW_INSECURE_COOKIESoption provides a solution for HTTP testing while being disabled by default for securityThese changes directly address the PR objective of fixing session cookie creation on HTTP sites by providing the necessary configuration options.
docs/docs/getting-started/quickstart.md (3)
23-33: Security improvements with proper environment configurationThe updated Docker Compose example correctly configures the container to run as root and adds essential environment variables for session security and data persistence.
60-69: Clear instructions for session security configurationThe added documentation for session secret generation and security settings is clear and necessary for secure operation.
50-52: 🧹 Nitpick (assertive)Fix inconsistent formatting on bullet points
There's inconsistent spacing between the bullet point and the text content.
- - **`APP_ENV`**: This should be set to `production` when running in Docker. This ensures the application uses the correct data paths (`/app/data`) rather than development paths. Without this, the application might incorrectly use development paths (`.dev-data`), causing data persistence issues. + - **`APP_ENV`**: This should be set to `production` when running in Docker. This ensures the application uses the correct data paths (`/app/data`) rather than development paths. Without this, the application might incorrectly use development paths (`.dev-data`), causing data persistence issues.Likely an incorrect or invalid review comment.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~52-~52: Loose punctuation mark.
Context: ...ns (Important):** -APP_ENV: This should be set toproductionwhen...(UNLIKELY_OPENING_PUNCTUATION)
src/lib/services/session-handler.ts (5)
5-5: Imported but unused variableThe
devimport is not being used directly in this file, but it's needed as part of theuseSecureCookiecalculation.
11-14: Good security practice using SHA-256The
createSecretfunction correctly hashes the session secret using SHA-256, providing a secure fixed-length key for session management.
16-20: Proper validation of required environment variableThis code correctly checks for the presence of the required
PUBLIC_SESSION_SECRETenvironment variable and throws an error if it's missing, preventing the application from starting with an insecure configuration.
24-26: Clear logic for determining cookie securityThe cookie security logic is correct and well-documented. It ensures cookies are secure by default unless explicitly configured otherwise or in development mode.
28-39: Session handler configuration with appropriate defaultsThe session handler is properly configured with the secure secret, appropriate timeout, and cookie settings. The
secure: useSecureCookieflag correctly applies the security determination logic.src/lib/services/paths-service.ts (1)
4-4: Proper use of SvelteKit's private environment systemThe change correctly uses SvelteKit's private environment system with a helpful comment.
docs/docs/getting-started/configuration.md (2)
102-130: Comprehensive documentation for environment variablesThe added documentation for environment variables provides clear guidance on required and optional settings, including security considerations.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~108-~108: Loose punctuation mark.
Context: ...Docker Socket Access -DOCKER_GID: Required. Set this to the group ID ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~112-~112: Loose punctuation mark.
Context: ...Application Environment -APP_ENV: Required for Docker. Controls which...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~120-~120: Loose punctuation mark.
Context: ... Security -PUBLIC_SESSION_SECRET: Required. This secret is used to si...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~128-~128: Loose punctuation mark.
Context: ...* -PUBLIC_ALLOW_INSECURE_COOKIES: Optional (Use with Caution). By def...(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
113-113: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
155-156: Important additional security recommendationsGood additions regarding backup considerations and HTTPS recommendation for security.
| - **`APP_ENV`**: **Required for Docker.** Controls which file system paths Arcane uses for data storage. When running in Docker, this must be set to `production` to ensure all data is stored in `/app/data` where the volume is mounted. Without this setting, the application may incorrectly use development paths (`.dev-data`) resulting in data not being persisted across container restarts. | ||
| ```yaml | ||
| environment: | ||
| - APP_ENV=production # Ensures proper data paths in Docker | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix markdown formatting
There's inconsistent formatting in the markdown code block. Add blank lines before and after the code block.
- **`APP_ENV`**: **Required for Docker.** Controls which file system paths Arcane uses for data storage. When running in Docker, this must be set to `production` to ensure all data is stored in `/app/data` where the volume is mounted. Without this setting, the application may incorrectly use development paths (`.dev-data`) resulting in data not being persisted across container restarts.
- ```yaml
- environment:
- - APP_ENV=production # Ensures proper data paths in Docker
- ```
+ **`APP_ENV`**: **Required for Docker.** Controls which file system paths Arcane uses for data storage. When running in Docker, this must be set to `production` to ensure all data is stored in `/app/data` where the volume is mounted. Without this setting, the application may incorrectly use development paths (`.dev-data`) resulting in data not being persisted across container restarts.
+
+ ```yaml
+ environment:
+ - APP_ENV=production # Ensures proper data paths in Docker
+ ```🧰 Tools
🪛 LanguageTool
[uncategorized] ~112-~112: Loose punctuation mark.
Context: ...Application Environment - APP_ENV: Required for Docker. Controls which...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
113-113: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents (early access)
In docs/docs/getting-started/configuration.md around lines 112 to 116, the markdown code block for the APP_ENV environment variable lacks blank lines before and after it, causing inconsistent formatting. Add a blank line before the opening ```yaml and another blank line after the closing ``` to properly separate the code block from the surrounding text.
1 participant
Fixes: #108
Fixes: #118
Fixes: #117 for the logout button and handler
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation
UI