feat: loading secrets not from toml but from env files#261
Merged
Conversation
📝 WalkthroughWalkthroughThis PR switches secrets provisioning from a file-based approach (backend/secrets.toml) to environment-variable-based secrets (SECRET_KEY, MONGO_USER, MONGO_PASSWORD, REDIS_PASSWORD), removing secrets file usage across CI workflows, docker-compose, backend settings, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev/CI
participant GitHub_Actions
participant Docker_Compose
participant Backend_App
participant MongoDB
Dev/CI->>GitHub_Actions: Provide env vars (SECRET_KEY, MONGO_USER, MONGO_PASSWORD, REDIS_PASSWORD)
GitHub_Actions->>Docker_Compose: Start services with merged env (x-backend-secrets)
Docker_Compose->>Backend_App: Process environment on start
Backend_App->>Backend_App: Settings.load() reads env vars (no secrets.toml)
Backend_App->>MongoDB: Connect using MONGO_USER/MONGO_PASSWORD -> authenticate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="AGENTS.md">
<violation number="1" location="AGENTS.md:451">
P2: This added env-var guidance conflicts with adjacent section text that still says environment variables must never be used, leaving contradictory setup instructions.</violation>
</file>
<file name="backend/app/settings.py">
<violation number="1" location="backend/app/settings.py:42">
P0: Do not use a hardcoded fallback for `SECRET_KEY`; require it from environment/config so JWT signing cannot default to a known value.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
AGENTS.md (1)
438-452:⚠️ Potential issue | 🟡 MinorFix the section title to match the new guidance.
Line 438 says “No Env Vars”, but Line 451 documents env vars for secrets. This contradiction will confuse contributors.
✏️ Proposed doc fix
-## Backend: Settings (TOML — No Env Vars) +## Backend: Settings (TOML + Env Vars for Secrets)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 438 - 452, The section title "Backend: Settings (TOML — No Env Vars)" is contradictory with the later lines that allow environment variables for secrets; update the heading to reflect that env vars are allowed for secrets (for example "Backend: Settings (TOML — env vars allowed for secrets)") so it aligns with the examples referencing DATABASE_URL/DATABASE_URL usage and the documented environment variables SECRET_KEY, MONGO_USER, MONGO_PASSWORD, REDIS_PASSWORD and the DI example `settings: FromDishka[Settings]`.backend/app/settings.py (1)
31-47:⚠️ Potential issue | 🟠 MajorReintroduce TOML-based secret loading in
Settingsinitialization.This constructor now reads secrets directly from environment variables and defaults, which breaks the current settings contract and makes behavior depend on ambient process env.
Proposed direction
def __init__( self, config_path: str = "config.toml", override_path: str | None = None, + secrets_path: str = "secrets.toml", ) -> None: with open(config_path, "rb") as f: data = tomllib.load(f) if override_path: with open(override_path, "rb") as f: data |= tomllib.load(f) - for key, default in ( - ("SECRET_KEY", "CHANGE_ME_min_32_chars_long_!!!!"), - ("MONGO_USER", "root"), - ("MONGO_PASSWORD", "rootpassword"), - ("REDIS_PASSWORD", "redispassword"), - ): - data[key] = os.environ.get(key) or data.get(key) or default + with open(secrets_path, "rb") as f: + data |= tomllib.load(f)Based on learnings: "Settings must load from TOML files (config.toml, secrets.toml, config..toml) via Settings(config_path=...) or Settings(override_path=...); never use environment variables directly; use model_config = ConfigDict(extra='forbid') to catch unknown keys as errors".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/settings.py` around lines 31 - 47, The Settings.__init__ currently pulls values from os.environ and defaults, violating the requirement to load configuration strictly from TOML files; change Settings.__init__ to only read config_path and override_path TOML data (including loading secrets.toml and config.<worker>.toml variants when appropriate) and merge them deterministically without consulting os.environ or runtime env vars, and remove the final os.environ.get(...) logic that populates SECRET_KEY, MONGO_USER, MONGO_PASSWORD, REDIS_PASSWORD; also enforce strict schema validation by setting model_config = ConfigDict(extra='forbid') on the Settings model so unknown keys in the TOML cause errors.
🧹 Nitpick comments (1)
backend/app/settings.py (1)
14-27: Use Google-style sections in theSettingsdocstring.Please convert this class docstring to include explicit
Args,Returns, andRaisessections.As per coding guidelines: "Use Google-style docstrings with Args/Returns/Raises sections".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/settings.py` around lines 14 - 27, Update the Settings class docstring to Google-style sections: replace the current narrative block with a short summary line followed by "Args:" documenting config_path (str, path to base TOML), override_path (str, optional per-worker overrides), and noting env var secrets (SECRET_KEY, MONGO_USER, MONGO_PASSWORD, REDIS_PASSWORD); add a "Returns:" section describing that it constructs and returns a Settings instance (or the loaded configuration object); and add a "Raises:" section listing possible exceptions (e.g., FileNotFoundError for missing config files, TOMLDecodeError for parse errors, or ValueError for invalid/missing required env vars). Ensure the docstring mentions the load order (config_path, override_path, then env vars) and keep it inside the Settings class docstring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yaml`:
- Around line 2-5: The compose file uses fallback defaults for sensitive env
vars (SECRET_KEY, MONGO_USER, MONGO_PASSWORD, REDIS_PASSWORD) which allows
fail‑open predictable credentials; change the parameter expansion from the
permissive ":-" form to the strict ":?" form so Docker Compose will fail fast
when any of these environment variables are unset or empty, enforcing explicit
secret configuration instead of using the provided defaults.
---
Outside diff comments:
In `@AGENTS.md`:
- Around line 438-452: The section title "Backend: Settings (TOML — No Env
Vars)" is contradictory with the later lines that allow environment variables
for secrets; update the heading to reflect that env vars are allowed for secrets
(for example "Backend: Settings (TOML — env vars allowed for secrets)") so it
aligns with the examples referencing DATABASE_URL/DATABASE_URL usage and the
documented environment variables SECRET_KEY, MONGO_USER, MONGO_PASSWORD,
REDIS_PASSWORD and the DI example `settings: FromDishka[Settings]`.
In `@backend/app/settings.py`:
- Around line 31-47: The Settings.__init__ currently pulls values from
os.environ and defaults, violating the requirement to load configuration
strictly from TOML files; change Settings.__init__ to only read config_path and
override_path TOML data (including loading secrets.toml and config.<worker>.toml
variants when appropriate) and merge them deterministically without consulting
os.environ or runtime env vars, and remove the final os.environ.get(...) logic
that populates SECRET_KEY, MONGO_USER, MONGO_PASSWORD, REDIS_PASSWORD; also
enforce strict schema validation by setting model_config =
ConfigDict(extra='forbid') on the Settings model so unknown keys in the TOML
cause errors.
---
Nitpick comments:
In `@backend/app/settings.py`:
- Around line 14-27: Update the Settings class docstring to Google-style
sections: replace the current narrative block with a short summary line followed
by "Args:" documenting config_path (str, path to base TOML), override_path (str,
optional per-worker overrides), and noting env var secrets (SECRET_KEY,
MONGO_USER, MONGO_PASSWORD, REDIS_PASSWORD); add a "Returns:" section describing
that it constructs and returns a Settings instance (or the loaded configuration
object); and add a "Raises:" section listing possible exceptions (e.g.,
FileNotFoundError for missing config files, TOMLDecodeError for parse errors, or
ValueError for invalid/missing required env vars). Ensure the docstring mentions
the load order (config_path, override_path, then env vars) and keep it inside
the Settings class docstring.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/actions/e2e-ready/action.yml.github/workflows/docs.yml.github/workflows/release-deploy.yml.github/workflows/stack-tests.ymlAGENTS.mdREADME.mdbackend/app/settings.pybackend/config.test.tomlbackend/config.tomlbackend/secrets.example.tomldocker-compose.yamldocs/getting-started.mddocs/operations/cicd.mddocs/operations/deployment.mddocs/reference/configuration.md
💤 Files with no reviewable changes (3)
- .github/workflows/docs.yml
- .github/workflows/stack-tests.yml
- backend/secrets.example.toml
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/app/settings.py">
<violation number="1" location="backend/app/settings.py:47">
P1: `SECRET_KEY` is only set when the environment variable is present. Unlike `MONGO_USER`, `MONGO_PASSWORD`, and `REDIS_PASSWORD` which retain dev defaults via the `or default` fallback, `SECRET_KEY` has no fallback at all. If the env var is unset and not present in TOML config, `data["SECRET_KEY"]` will be missing, likely causing a `KeyError` or broken auth at runtime. Consider adding a dev default or raising an explicit error when the key is missing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
AGENTS.md (1)
438-452: Rename this section header to match the new behavior.The title says “No Env Vars,” but the content now correctly uses env vars for secrets via
Settings. Please rename the heading (for example, “TOML + Env Secrets via Settings”) to avoid policy confusion for contributors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 438 - 452, The section header "Backend: Settings (TOML — No Env Vars)" is now misleading because the content documents using environment variables for secrets via the Settings object (see Settings and the listed SECRET_KEY, MONGO_USER, MONGO_PASSWORD, REDIS_PASSWORD); update the header to reflect the new behavior (e.g., "Backend: Settings (TOML + Env Secrets via Settings)" or similar) so it aligns with the text referencing Settings and env var secrets handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@AGENTS.md`:
- Around line 438-452: The section header "Backend: Settings (TOML — No Env
Vars)" is now misleading because the content documents using environment
variables for secrets via the Settings object (see Settings and the listed
SECRET_KEY, MONGO_USER, MONGO_PASSWORD, REDIS_PASSWORD); update the header to
reflect the new behavior (e.g., "Backend: Settings (TOML + Env Secrets via
Settings)" or similar) so it aligns with the text referencing Settings and env
var secrets handling.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
AGENTS.mdbackend/app/settings.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/settings.py
|
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/app/settings.py">
<violation number="1" location="backend/app/settings.py:42">
P0: Security: hardcoded default for `SECRET_KEY` means production can silently start with a well-known JWT secret if the env var is missing. The previous code would fail to start (fail-fast), which is far safer. Consider keeping a fail-fast behavior for `SECRET_KEY` in non-dev environments, or at minimum emitting a loud warning at startup when the default is used.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Switched secrets from secrets.toml to environment variables with dev defaults (including a safe fallback for SECRET_KEY). This streamlines local dev, CI, and production by passing secrets via env vars and Docker Compose.
Refactors
Migration
Written for commit ded545c. Summary will update on new commits.
Summary by CodeRabbit
Documentation
Chores