-
-
Notifications
You must be signed in to change notification settings - Fork 482
feat: allow for postgres ssl connections #607
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
base: master
Are you sure you want to change the base?
Conversation
|
@JGaudette is attempting to deploy a commit to the goldflag's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe PR adds PostgreSQL SSL configuration support to the application by introducing a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 (2)
.env.example (1)
26-26: Consider reordering for consistency.The static analysis tool suggests placing
POSTGRES_SSLbeforePOSTGRES_USERto maintain alphabetical ordering within the PostgreSQL configuration section.Apply this diff to reorder the keys:
# PostgreSQL Database Configuration POSTGRES_DB=analytics +POSTGRES_SSL=false POSTGRES_USER=frog POSTGRES_PASSWORD=frog -POSTGRES_SSL=falseserver/drizzle.config.ts (1)
16-16: Simplify the boolean expression.The ternary operator is redundant since the equality check already returns a boolean.
Apply this diff to simplify:
- ssl: process.env.POSTGRES_SSL === "true" ? true : false, + ssl: process.env.POSTGRES_SSL === "true",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.env.example(1 hunks)docker-compose.cloud.yml(1 hunks)docs/src/content/self-hosting-advanced.mdx(1 hunks)server/drizzle.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{client,server}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{client,server}/**/*.{ts,tsx}: Use TypeScript with strict typing throughout both client and server
Use try/catch blocks with specific error types for error handling
Use camelCase for variables and functions, PascalCase for components and types
Group imports by external, then internal, and sort alphabetically within groups
Files:
server/drizzle.config.ts
server/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Backend: Use Fastify, Drizzle ORM (Postgres), ClickHouse, and Zod
Files:
server/drizzle.config.ts
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 26-26: [UnorderedKey] The POSTGRES_SSL key should go before the POSTGRES_USER key
(UnorderedKey)
🔇 Additional comments (2)
docs/src/content/self-hosting-advanced.mdx (1)
70-70: LGTM!The documentation correctly reflects the new
POSTGRES_SSLenvironment variable with the appropriate default value for backwards compatibility.docker-compose.cloud.yml (1)
89-89: LGTM!The environment variable is correctly configured with a sensible default, maintaining backwards compatibility while allowing SSL to be enabled when needed.
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
🧹 Nitpick comments (1)
server/src/db/postgres/postgres.ts (1)
15-15: Simplify the redundant ternary expression.The expression
process.env.POSTGRES_SSL === "true" ? true : falsecan be simplified.Apply this diff to simplify:
- ssl: process.env.POSTGRES_SSL === "true" ? true : false, + ssl: process.env.POSTGRES_SSL === "true",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/db/postgres/postgres.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{client,server}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{client,server}/**/*.{ts,tsx}: Use TypeScript with strict typing throughout both client and server
Use try/catch blocks with specific error types for error handling
Use camelCase for variables and functions, PascalCase for components and types
Group imports by external, then internal, and sort alphabetically within groups
Files:
server/src/db/postgres/postgres.ts
server/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Backend: Use Fastify, Drizzle ORM (Postgres), ClickHouse, and Zod
Files:
server/src/db/postgres/postgres.ts
server/src/db/postgres/postgres.ts
Outdated
| database: process.env.POSTGRES_DB, | ||
| username: process.env.POSTGRES_USER, | ||
| password: process.env.POSTGRES_PASSWORD, | ||
| ssl: process.env.POSTGRES_SSL === "true" ? true : false, |
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.
Consider SSL certificate validation for production environments.
While enabling SSL with ssl: true encrypts the connection, it doesn't validate the server's certificate by default. For production environments like RDS, you should verify the server certificate to prevent man-in-the-middle attacks.
Consider using an object configuration for SSL to enable certificate validation:
- ssl: process.env.POSTGRES_SSL === "true" ? true : false,
+ ssl: process.env.POSTGRES_SSL === "true" ? { rejectUnauthorized: true } : false,Note: RDS instances typically require certificate validation. If you need to provide a custom CA certificate, the postgres library also supports:
ssl: {
rejectUnauthorized: true,
ca: fs.readFileSync('/path/to/ca-certificate.crt').toString(),
}🤖 Prompt for AI Agents
In server/src/db/postgres/postgres.ts around line 15, the SSL option is
currently set as a boolean which enables encryption but does not enforce server
certificate validation; change the ssl option to an object that sets
rejectUnauthorized: true for production (e.g., when NODE_ENV === "production" or
POSTGRES_SSL === "true") and, if required by your RDS setup, load a CA bundle
from a configured path (read the CA file with fs.readFileSync and pass it as
ca); ensure you conditionally construct this object (or leave ssl: false for
non-SSL envs), and add the needed fs import and error handling for missing CA
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: 0
🧹 Nitpick comments (2)
.env.example (2)
26-26: ReorderPOSTGRES_SSLto maintain alphabetical ordering within the PostgreSQL section.The
POSTGRES_SSLvariable should appear beforePOSTGRES_USERto maintain alphabetical consistency within the PostgreSQL Database Configuration section.Apply this diff to fix the ordering:
# PostgreSQL Database Configuration POSTGRES_DB=analytics POSTGRES_USER=frog POSTGRES_PASSWORD=frog -POSTGRES_SSL=false +POSTGRES_SSL=falseReorganized section:
# PostgreSQL Database Configuration POSTGRES_DB=analytics POSTGRES_PASSWORD=frog POSTGRES_SSL=false POSTGRES_USER=frog
22-26: Add a clarifying comment for the newPOSTGRES_SSLvariable.Developers reading the
.env.examplemay benefit from understanding what this variable controls and why it defaults tofalse. A brief inline comment would improve clarity.Apply this diff to add a descriptive comment:
# PostgreSQL Database Configuration POSTGRES_DB=analytics +POSTGRES_PASSWORD=frog +POSTGRES_SSL=false +# Set to true to enable SSL (required for some environments like RDS; disabled by default for backwards compatibility) POSTGRES_USER=frog -POSTGRES_PASSWORD=frog -POSTGRES_SSL=false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.env.example(1 hunks)
🧰 Additional context used
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 26-26: [UnorderedKey] The POSTGRES_SSL key should go before the POSTGRES_USER key
(UnorderedKey)
We use RDS and require an ssl connection in our environment. This PR still defaults the ssl mode to false for backwards compatibility, but also introduced an environment variable to overwrite that and enable SSL mode on the postgres connection.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.