Skip to content
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

fix(session-storage): add typing and trust proxy setting #9725

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AMoreaux
Copy link
Contributor

Added explicit typing for session storage options to improve type safety. Enabled 'trust proxy' to ensure proper client IP and protocol detection behind proxies. These changes improve security and reliability in session handling.

Added explicit typing for session storage options to improve type safety. Enabled 'trust proxy' to ensure proper client IP and protocol detection behind proxies. These changes improve security and reliability in session handling.
@AMoreaux AMoreaux requested a review from Weiko January 17, 2025 16:01
@AMoreaux AMoreaux self-assigned this Jan 17, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

Added explicit typing for session storage options and enabled trust proxy settings to improve security and reliability when running behind proxies, particularly for SSO functionality.

  • Added trust proxy setting in packages/twenty-server/src/main.ts to ensure accurate client IP detection
  • Implemented explicit session.SessionOptions typing in session-storage.module-factory.ts for better type safety
  • Set secure cookie option based on HTTPS detection through SERVER_URL environment variable
  • Added Redis connection error handling with descriptive error messages

2 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +89 to +90
// Trust the first proxy to ensure correct client IP and protocol detection.
app.set('trust proxy', 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Ensure this is only enabled when actually running behind a proxy. Consider making this configurable via an environment variable.

packages/twenty-server/src/main.ts Outdated Show resolved Hide resolved
Relocated the session middleware initialization to an earlier point in the code for better alignment and clarity. This change ensures proper setup and avoids redundancy in the middleware application.
packages/twenty-server/src/main.ts Dismissed Show dismissed Hide dismissed
@AMoreaux
Copy link
Contributor Author

Fix #9430

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant