fix(security): startup config validation + fail-closed unsigned-session fallback (#174)#228
Merged
Jose-Gael-Cruz-Lopez merged 1 commit intoJun 14, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR hardens application startup by adding environment validation and fail-closed OAuth security. It introduces ChangesStartup Configuration Validation and Auth Security Hardening
Sequence Diagram: Startup Validation FlowsequenceDiagram
participant FastAPI
participant validate_config
participant ConfigVars as CONFIG VARS
FastAPI->>validate_config: Call at lifespan start
validate_config->>ConfigVars: Read SUPABASE_URL
validate_config->>ConfigVars: Read SUPABASE_SERVICE_KEY
validate_config->>ConfigVars: Read GEMINI_API_KEY
validate_config->>ConfigVars: Read SESSION_SECRET (if not IS_LOCAL)
alt All required vars present
validate_config-->>FastAPI: Success
FastAPI->>FastAPI: Continue initialization
else Missing required vars
validate_config-->>FastAPI: RuntimeError (list all missing)
FastAPI->>FastAPI: Abort startup
end
Sequence Diagram: OAuth Cookie Encoding and Decoding GuardsequenceDiagram
participant OAuth as OAuth Handler
participant encode as _encode_oauth_cookie
participant decode as _decode_oauth_cookie
participant check as Secret Check
OAuth->>encode: Encode state cookie
encode->>check: SESSION_SECRET and IS_LOCAL?
alt SESSION_SECRET present
check-->>encode: Use HMAC signing
encode-->>OAuth: Signed cookie (contains .)
else IS_LOCAL = True
check-->>encode: Use in-memory fallback
encode-->>OAuth: Unsigned cookie
else Production mode, no secret
check-->>encode: Raise RuntimeError
encode-->>OAuth: Fail-closed
end
OAuth->>decode: Decode received cookie
decode->>check: Verify cookie signature
alt Cookie is signed or IS_LOCAL
check-->>decode: Accept cookie
decode-->>OAuth: State nonce
else Unsigned in production
check-->>decode: Reject
decode-->>OAuth: Return None
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
frontend | 9a456e5 | Commit Preview URL Branch Preview URL |
Jun 14 2026, 02:16 AM |
Jose-Gael-Cruz-Lopez
force-pushed
the
security/174-config-validation-failclosed
branch
from
7d02a68 to
3a2b66d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/tests/test_config_validation_failclosed.py (1)
16-27: ⚡ Quick winExercise
APP_ENVin this suite instead of patchingIS_LOCALdirectly.The new contract here is
APP_ENV -> IS_LOCAL, but_set_required()bypasses it by monkeypatchingconfig.IS_LOCALitself. A regression in the mode parsing would still leave these tests green. I'd add at least onemonkeypatch.setenv("APP_ENV", ...)+importlib.reload(config)path forlocalandproduction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_config_validation_failclosed.py` around lines 16 - 27, The helper _set_required currently monkeypatches config.IS_LOCAL directly which bypasses the APP_ENV -> IS_LOCAL conversion; change it to exercise APP_ENV instead by calling monkeypatch.setenv("APP_ENV", overrides.pop("APP_ENV", "production" if not overrides.get("IS_LOCAL") else "local")) (or set explicit values in tests), then call importlib.reload(config) to pick up the env change before monkeypatching the remaining config attributes; remove direct monkeypatch.setattr(config, "IS_LOCAL", ...) usage and instead assert config.IS_LOCAL is derived from APP_ENV in the tests (add explicit test cases that set monkeypatch.setenv("APP_ENV","local") and monkeypatch.setenv("APP_ENV","production") followed by importlib.reload(config) to verify behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/config.py`:
- Around line 22-23: The module-level reads of APP_ENV and IS_LOCAL happen
before the project's .env is loaded, so move the dotenv load into config.py and
call load_dotenv(Path(__file__).with_name(".env")) (or equivalent dotenv load)
at the top of the file before evaluating APP_ENV, IS_LOCAL and any secret reads;
this ensures the globals (APP_ENV, IS_LOCAL) used by validate_config() and
routes.auth reflect values from .env. Alternatively, replace the module-level
snapshots with accessor functions (e.g., get_app_env(), is_local()) that read
os.getenv at call time, but the simplest fix is to load the same .env source at
the top of config.py before assigning APP_ENV and IS_LOCAL.
- Around line 57-65: The environment-var presence checks currently allow
whitespace-only values; update the checks that build the missing list to reject
strings that are blank after trimming: for SUPABASE_URL, SUPABASE_SERVICE_KEY,
GEMINI_API_KEY, and SESSION_SECRET (when not IS_LOCAL) use a test like "if not
(VAR and VAR.strip()): missing.append(...)" (or call .strip() before checking
truthiness) so purely-whitespace values are treated as absent; keep the existing
IS_LOCAL exemption for SESSION_SECRET.
---
Nitpick comments:
In `@backend/tests/test_config_validation_failclosed.py`:
- Around line 16-27: The helper _set_required currently monkeypatches
config.IS_LOCAL directly which bypasses the APP_ENV -> IS_LOCAL conversion;
change it to exercise APP_ENV instead by calling monkeypatch.setenv("APP_ENV",
overrides.pop("APP_ENV", "production" if not overrides.get("IS_LOCAL") else
"local")) (or set explicit values in tests), then call importlib.reload(config)
to pick up the env change before monkeypatching the remaining config attributes;
remove direct monkeypatch.setattr(config, "IS_LOCAL", ...) usage and instead
assert config.IS_LOCAL is derived from APP_ENV in the tests (add explicit test
cases that set monkeypatch.setenv("APP_ENV","local") and
monkeypatch.setenv("APP_ENV","production") followed by importlib.reload(config)
to verify behavior).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 64554247-dd9f-47be-839e-93bb62adfa32
📒 Files selected for processing (5)
backend/config.pybackend/main.pybackend/routes/auth.pybackend/tests/test_auth_state.pybackend/tests/test_config_validation_failclosed.py
Comment on lines
+57
to
+65
| missing = [] | ||
| if not SUPABASE_URL: | ||
| missing.append("SUPABASE_URL") | ||
| if not SUPABASE_SERVICE_KEY: | ||
| missing.append("SUPABASE_SERVICE_KEY") | ||
| if not GEMINI_API_KEY: | ||
| missing.append("GEMINI_API_KEY") | ||
| if not SESSION_SECRET and not IS_LOCAL: | ||
| missing.append("SESSION_SECRET") |
There was a problem hiding this comment.
Reject whitespace-only config values here.
These checks only fail on empty strings. Values like " " still pass for SUPABASE_URL, SUPABASE_SERVICE_KEY, GEMINI_API_KEY, and SESSION_SECRET, so production can boot with unusable config and a trivially weak signing key.
Suggested fix
+def _is_blank(value: str) -> bool:
+ return not value or not value.strip()
+
def validate_config() -> None:
@@
- if not SUPABASE_URL:
+ if _is_blank(SUPABASE_URL):
missing.append("SUPABASE_URL")
- if not SUPABASE_SERVICE_KEY:
+ if _is_blank(SUPABASE_SERVICE_KEY):
missing.append("SUPABASE_SERVICE_KEY")
- if not GEMINI_API_KEY:
+ if _is_blank(GEMINI_API_KEY):
missing.append("GEMINI_API_KEY")
- if not SESSION_SECRET and not IS_LOCAL:
+ if _is_blank(SESSION_SECRET) and not IS_LOCAL:
missing.append("SESSION_SECRET")📝 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.
Suggested change
| missing = [] | |
| if not SUPABASE_URL: | |
| missing.append("SUPABASE_URL") | |
| if not SUPABASE_SERVICE_KEY: | |
| missing.append("SUPABASE_SERVICE_KEY") | |
| if not GEMINI_API_KEY: | |
| missing.append("GEMINI_API_KEY") | |
| if not SESSION_SECRET and not IS_LOCAL: | |
| missing.append("SESSION_SECRET") | |
| def _is_blank(value: str) -> bool: | |
| return not value or not value.strip() | |
| def validate_config() -> None: | |
| missing = [] | |
| if _is_blank(SUPABASE_URL): | |
| missing.append("SUPABASE_URL") | |
| if _is_blank(SUPABASE_SERVICE_KEY): | |
| missing.append("SUPABASE_SERVICE_KEY") | |
| if _is_blank(GEMINI_API_KEY): | |
| missing.append("GEMINI_API_KEY") | |
| if _is_blank(SESSION_SECRET) and not IS_LOCAL: | |
| missing.append("SESSION_SECRET") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/config.py` around lines 57 - 65, The environment-var presence checks
currently allow whitespace-only values; update the checks that build the missing
list to reject strings that are blank after trimming: for SUPABASE_URL,
SUPABASE_SERVICE_KEY, GEMINI_API_KEY, and SESSION_SECRET (when not IS_LOCAL) use
a test like "if not (VAR and VAR.strip()): missing.append(...)" (or call
.strip() before checking truthiness) so purely-whitespace values are treated as
absent; keep the existing IS_LOCAL exemption for SESSION_SECRET.
…ned-session fallback (#174) config.py read every required secret with an empty-string default and validated none, so the app booted with all secrets unset and failed opaquely later — malformed REST URL on first DB call, mid-request Gemini errors, and worst, an empty SESSION_SECRET silently switched OAuth state to an unsigned in-memory fallback (insecure degradation). - Add validate_config(), called from the FastAPI lifespan: raises one clear error naming every missing key (SUPABASE_URL, SUPABASE_SERVICE_KEY, GEMINI_API_KEY always; SESSION_SECRET outside local). - Add APP_ENV (defaults to production → fail-closed) / IS_LOCAL. The unsigned OAuth-state fallback now raises outside local dev (defense in depth at the use site), so it is unreachable in production. Tests: validate_config names all missing keys and relaxes SESSION_SECRET only in local; the unsigned fallback raises in production (fails pre-fix, which silently used it). Updated the two pre-existing fallback round-trip tests to declare local mode.
Jose-Gael-Cruz-Lopez
force-pushed
the
security/174-config-validation-failclosed
branch
from
3a2b66d to
9a456e5
Compare
Jose-Gael-Cruz-Lopez
deleted the
security/174-config-validation-failclosed
branch
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
1 participant
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.
Closes #174. Labeled P2 but load-bearing: the whole security model rests on session-validated route code, so a forgeable/unsigned session undermines all of it (underscored by the realtime RLS finding).
Problem
config.pyread every required secret with an empty-string default and validated none. The app booted with everything unset and failed opaquely later:""SUPABASE_URL → malformed REST URL on the first DB call;routes/auth.pysilently switched OAuth state to an unsigned in-memory fallback — an insecure degradation rather than a hard failure.Fix
validate_config(), called from the FastAPI lifespan: raises one clear error naming every missing key.SUPABASE_URL/SUPABASE_SERVICE_KEY/GEMINI_API_KEYalways required;SESSION_SECRETrequired outside local.APP_ENV(defaults toproduction) /IS_LOCAL— fail-closed by default: a deployment that sets nothing gets the strict checks. SetAPP_ENV=localto relaxSESSION_SECRETfor local dev.Tests
tests/test_config_validation_failclosed.py:validate_confignames all missing keys, requires SESSION_SECRET outside local, relaxes it in local; the unsigned fallback raises in production (fails pre-fix, which silently used it). Updated the two pre-existing fallback round-trip tests intest_auth_state.pyto declare local mode (the fallback is now local-only). Verified the lifespan storage-bucket test still passes (test env supplies all secrets).Summary by CodeRabbit
New Features
Tests