Skip to content

refactor: simplify database connection options and maintain SSL support#5

Merged
BeforeLights merged 1 commit into
mainfrom
feat/ssl-support
May 6, 2026
Merged

refactor: simplify database connection options and maintain SSL support#5
BeforeLights merged 1 commit into
mainfrom
feat/ssl-support

Conversation

@BeforeLights
Copy link
Copy Markdown
Contributor

@BeforeLights BeforeLights commented May 6, 2026

  • Consolidated database connection options into a single object for better readability.
  • Preserved SSL configuration handling while ensuring clarity in the connection setup.

Summary by CodeRabbit

  • Refactor
    • Centralized database configuration options to reduce code duplication and improve maintainability of the database initialization logic.

- Consolidated database connection options into a single object for better readability.
- Preserved SSL configuration handling while ensuring clarity in the connection setup.
@BeforeLights BeforeLights merged commit 30280b2 into main May 6, 2026
1 check was pending
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 6, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fc00735b-3560-40bd-9bec-34d593221f04

📥 Commits

Reviewing files that changed from the base of the PR and between 5b69410 and 28f59d0.

📒 Files selected for processing (1)
  • apps/api/src/config/database.ts

📝 Walkthrough

Walkthrough

The PR centralizes Sequelize configuration into a reusable options object and refactors SSL configuration to be conditional based on the DB_SSL environment variable, replacing inline duplicated SSL dialectOptions with a centralized approach.

Changes

Sequelize Configuration Refactoring

Layer / File(s) Summary
Configuration Object
apps/api/src/config/database.ts (lines 8–14)
New sequelizeOptions object centralizes host, port, dialect, and logging settings for Sequelize.
Conditional SSL Wiring
apps/api/src/config/database.ts (lines 15–29)
Sequelize initialization refactored to conditionally merge dialectOptions.ssl when DB_SSL is true; otherwise passes sequelizeOptions directly.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • Nail-Addison/nail-star#4: Both PRs modify apps/api/src/config/database.ts to add conditional SSL support for Sequelize with centralized options.

Poem

🐰 Config once scattered, now it's neat,
Options centralized, can't be beat!
SSL conditional, truth be told,
Sequelize configuration, refactored with care, bold!

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/ssl-support

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Refactor database connection with extracted options object

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Extracted database connection options into reusable object
• Simplified SSL configuration with conditional spread operator
• Improved code readability and maintainability
Diagram
flowchart LR
  A["Database Config"] --> B["sequelizeOptions Object"]
  B --> C["SSL Enabled"]
  B --> D["SSL Disabled"]
  C --> E["Spread with dialectOptions"]
  D --> F["Use as-is"]
  E --> G["Sequelize Instance"]
  F --> G
Loading

Grey Divider

File Changes

1. apps/api/src/config/database.ts ✨ Enhancement +14/-10

Extract and simplify database connection options

• Extracted database connection options into a separate sequelizeOptions object
• Refactored SSL configuration to use conditional ternary with spread operator
• Improved code organization by separating base options from SSL-specific configuration
• Maintained existing SSL support with require: true and rejectUnauthorized: false

apps/api/src/config/database.ts


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 6, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0)

Grey Divider


Action required

1. Insecure SSL verification 🐞 Bug ⛨ Security
Description
When DB_SSL is enabled, the DB connection sets ssl.rejectUnauthorized: false, which disables
certificate verification and makes the encrypted connection susceptible to MITM attacks. This should
not be used in production DB connections.
Code

apps/api/src/config/database.ts[R23-26]

                  ssl: {
                      require: true,
                      rejectUnauthorized: false,
                  },
-              }
-            : undefined,
-    }
Evidence
The Sequelize options explicitly disable TLS certificate verification when SSL is on, by setting
rejectUnauthorized: false under dialectOptions.ssl.

apps/api/src/config/database.ts[19-27]
Best Practice: Node TLS / PostgreSQL SSL best practices

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`rejectUnauthorized: false` disables TLS certificate verification for DB SSL connections, allowing MITM.

### Issue Context
This configuration is applied whenever `process.env.DB_SSL === "true"`.

### Fix Focus Areas
- apps/api/src/config/database.ts[19-29]

### Suggested change
- Default to `rejectUnauthorized: true`.
- Support providing a CA bundle via env (e.g., `DB_SSL_CA`, `DB_SSL_CA_PATH`) and pass it to `ssl.ca`.
- If you must support self-signed certs, gate `rejectUnauthorized: false` behind a non-production condition (e.g., `NODE_ENV !== "production"`) and/or an explicit separate env flag (e.g., `DB_SSL_INSECURE=true`) so it cannot be accidentally enabled in production.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Missing DB env validation 🐞 Bug ☼ Reliability
Description
sequelizeOptions coerces potentially-missing DB env vars with String()/Number(), which turns
missing values into the literal host "undefined" and port NaN, causing immediate connection
failures with unclear root cause. This is avoidable by validating required env vars and parsing the
port explicitly before constructing Sequelize.
Code

apps/api/src/config/database.ts[R8-13]

+const sequelizeOptions = {
+    host: String(process.env.DB_HOST),
+    port: Number(process.env.DB_PORT),
+    dialect: "postgres" as const,
+    logging: false,
+};
Evidence
The DB config is built from process.env using coercions that produce invalid-but-well-typed
values; the server then calls sequelize.authenticate() at startup, so these invalid values surface
as runtime connection errors instead of a clear configuration error.

apps/api/src/config/database.ts[8-13]
apps/api/src/server.ts[62-76]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Missing DB_* env vars currently become runtime values like host "undefined" and port NaN, producing confusing connection errors.

### Issue Context
`database.ts` constructs Sequelize config at module load; `server.ts` calls `sequelize.authenticate()` during startup.

### Fix Focus Areas
- apps/api/src/config/database.ts[8-30]
- apps/api/src/server.ts[62-76]

### Suggested change
- Add a small helper (e.g., `requireEnv(name): string`) that throws with a clear message when an env var is missing/empty.
- Parse/validate `DB_PORT` (e.g., `const port = Number.parseInt(requireEnv("DB_PORT"), 10); if (!Number.isFinite(port)) throw ...`).
- Use the validated values when constructing `sequelizeOptions` (so misconfiguration fails fast with a clear error message before attempting a network connection).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

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