-
Notifications
You must be signed in to change notification settings - Fork 973
feat: ts compatible session store #584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe session store setup in the server code was refactored by introducing a typed options interface and creating a reusable session store instance. Configuration changes include a shorter session pruning interval, dynamic cookie security based on environment, updated session middleware options, and sourcing the session secret from an environment variable with a fallback. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server
participant PgSession (Session Store)
Note over Server: On incoming request
Server->>PgSession (Session Store): Access session using reusable store instance
PgSession (Session Store)-->>Server: Retrieve/store session data
Server-->>User: Respond with session-enabled result
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/server.ts
(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.31.1)
server/src/server.ts
[warning] 62-62: A hard-coded credential was detected. It is not recommended to store credentials in source-code, as this risks secrets being leaked and used by either an internal or external malicious adversary. It is recommended to use environment variables to securely provide credentials or retrieve credentials from a secure vault or HSM (Hardware Security Module).
Context: secret: 'mx-session'
Note: [CWE-798] Use of Hard-coded Credentials. [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html
(express-session-hardcoded-secret-typescript)
🔇 Additional comments (3)
server/src/server.ts (3)
42-48
: Excellent type safety improvement with PgStoreOptions interfaceThe introduction of a properly typed interface for session store options improves type safety and provides better IDE support. This is a good practice for TypeScript projects.
50-58
: Good session store extraction and configuration improvementsCreating a reusable session store instance improves code organization. The reduced pruneSessionInterval (from 1 hour to 15 minutes) will clean up inactive sessions more frequently, which is good for database hygiene. The properly typed error logging that only logs the error message (instead of the entire object) is also an improvement.
62-70
: Security improvements in session configurationThe session configuration has been improved in several ways:
- Using the reusable sessionStore instance
- Setting
saveUninitialized
tofalse
(prevents storing empty sessions)- Making cookie
secure
flag dynamic based on environmentThese changes enhance security, but be aware they might change behavior:
- With
saveUninitialized: false
, uninitialized sessions won't be saved anymore- With dynamic
secure
flag, cookies will only be sent over HTTPS in productionVerify that your application doesn't rely on uninitialized sessions being saved, and that you're serving over HTTPS in production.
🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 62-62: A hard-coded credential was detected. It is not recommended to store credentials in source-code, as this risks secrets being leaked and used by either an internal or external malicious adversary. It is recommended to use environment variables to securely provide credentials or retrieve credentials from a secure vault or HSM (Hardware Security Module).
Context: secret: 'mx-session'
Note: [CWE-798] Use of Hard-coded Credentials. [REFERENCES]
- https://cheatsheetseries.owasp.org/cheatsheets/Secrets_Management_Cheat_Sheet.html(express-session-hardcoded-secret-typescript)
server/src/server.ts
Outdated
}, | ||
} as any), | ||
}), | ||
store: sessionStore, | ||
secret: 'mx-session', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security risk: Hardcoded session secret
Using a hardcoded session secret is a security vulnerability. If compromised, this could lead to session hijacking.
Replace the hardcoded secret with an environment variable:
- secret: 'mx-session',
+ secret: process.env.SESSION_SECRET || 'mx-session-dev',
And add SESSION_SECRET to your environment variables in production. Consider adding this to your README or documentation:
# Required environment variables:
SESSION_SECRET - A strong, random string used to sign session cookies
📝 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.
secret: 'mx-session', | |
secret: process.env.SESSION_SECRET || 'mx-session-dev', |
feat: use env var for session store store
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
101-101
: Clarify default secret value and improve phrasingThe "If Not Set" column could specify the actual fallback secret (
'mx-session'
) and refine the wording. For example:-| `SESSION_SECRET` | No | A strong, random string used to sign session cookies | Uses default secret. Recommended to define your own session secret to avoid session hijacking. | +| `SESSION_SECRET` | No | A strong, random string used to sign session cookies | Uses default secret ('mx-session'). It is recommended to define your own session secret to avoid session hijacking. |This makes the fallback explicit and smooths the phrasing.
🧰 Tools
🪛 LanguageTool
[grammar] ~101-~101: The verb ‘Recommended’ is used with the gerund form.
Context: ... | Uses default secret. Recommended to define your own session secret to avoid sessio...(ADMIT_ENJOY_VB)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ENVEXAMPLE
(1 hunks)README.md
(1 hunks)server/src/server.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ENVEXAMPLE
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/server.ts
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~101-~101: The verb ‘Recommended’ is used with the gerund form.
Context: ... | Uses default secret. Recommended to define your own session secret to avoid sessio...
(ADMIT_ENJOY_VB)
Summary by CodeRabbit