fix(auth): require JWT_SECRET_KEY in prod + run config validation (C1/C2)#56
fix(auth): require JWT_SECRET_KEY in prod + run config validation (C1/C2)#56PenguinzTech wants to merge 1 commit into
Conversation
…startup Fixes two critical findings: - C1: JWT_SECRET_KEY no longer falls back to the dev SECRET_KEY default in production. ProductionConfig now requires it via env var and validate() rejects the dev default, closing an admin-token forgery vector. - C2: create_app() now calls ProductionConfig.validate() at startup (guarded by hasattr so Dev/Testing configs are unaffected), so the dead secret-guard actually runs and the app boot-fails on default/missing secrets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR hardens production authentication by requiring a dedicated JWT secret key and ensuring production config validation is executed at app startup, causing startup to fail if required secrets are missing or insecure. Sequence diagram for production config validation during app startupsequenceDiagram
participant Caller
participant create_app
participant ProductionConfig
participant QuartApp
Caller->>create_app: create_app(config_class)
create_app->>QuartApp: Quart(__name__)
alt config_class is None
create_app->>create_app: get_config()
end
create_app->>QuartApp: config.from_object(config_class)
alt hasattr(config_class, validate)
create_app->>ProductionConfig: validate()
alt [missing or insecure secrets]
ProductionConfig-->>Caller: ValueError
else [valid secrets]
ProductionConfig-->>create_app: None
create_app->>create_app: _setup_logging(app)
end
else no validate attribute
create_app->>create_app: _setup_logging(app)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
create_appdocstring and comment say validation is “production only”, but thehasattr(config_class, "validate")check will run it for any config that definesvalidate; consider tightening this to an explicitProductionConfig(or subclass) check or updating the comment/docstring to match the behavior. - Instead of only checking
hasattr(config_class, "validate"), you might also confirm it’s callable (e.g.callable(getattr(config_class, "validate", None))) to avoid surprising behavior if a non-callable attribute namedvalidateis introduced on a config class.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `create_app` docstring and comment say validation is “production only”, but the `hasattr(config_class, "validate")` check will run it for any config that defines `validate`; consider tightening this to an explicit `ProductionConfig` (or subclass) check or updating the comment/docstring to match the behavior.
- Instead of only checking `hasattr(config_class, "validate")`, you might also confirm it’s callable (e.g. `callable(getattr(config_class, "validate", None))`) to avoid surprising behavior if a non-callable attribute named `validate` is introduced on a config class.
## Individual Comments
### Comment 1
<location path="services/flask-backend/app/config.py" line_range="180-183" />
<code_context>
# Strict security in production
SECURITY_PASSWORD_SALT = os.getenv("SECURITY_PASSWORD_SALT") # Required
SECRET_KEY = os.getenv("SECRET_KEY") # Required
+ JWT_SECRET_KEY = os.getenv("JWT_SECRET_KEY") # Required — must not fall back to dev default
@classmethod
</code_context>
<issue_to_address>
**suggestion:** The hardcoded dev default string in validation is brittle and may drift from the actual dev default.
This check couples production validation to the hardcoded string `"dev-secret-key-change-in-production"`, which can drift if the dev default changes elsewhere. Prefer referencing a single canonical dev/default value (e.g., `Config.DEFAULT_JWT_SECRET_KEY` or `DevelopmentConfig.JWT_SECRET_KEY`) rather than inlining the literal here.
Suggested implementation:
```python
if (
not cls.JWT_SECRET_KEY
or cls.JWT_SECRET_KEY == DevelopmentConfig.JWT_SECRET_KEY
):
```
If `DevelopmentConfig.JWT_SECRET_KEY` is not yet defined or the development default is stored elsewhere, you should:
1. Define the canonical dev default in one place (e.g., `DevelopmentConfig.JWT_SECRET_KEY = "dev-secret-key-change-in-production"` or `Config.DEFAULT_JWT_SECRET_KEY = "dev-secret-key-change-in-production"`).
2. Update this production validation and any other dev default usages to reference that single canonical location.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| JWT_SECRET_KEY = os.getenv("JWT_SECRET_KEY") # Required — must not fall back to dev default | ||
|
|
||
| @classmethod | ||
| def validate(cls) -> None: |
There was a problem hiding this comment.
suggestion: The hardcoded dev default string in validation is brittle and may drift from the actual dev default.
This check couples production validation to the hardcoded string "dev-secret-key-change-in-production", which can drift if the dev default changes elsewhere. Prefer referencing a single canonical dev/default value (e.g., Config.DEFAULT_JWT_SECRET_KEY or DevelopmentConfig.JWT_SECRET_KEY) rather than inlining the literal here.
Suggested implementation:
if (
not cls.JWT_SECRET_KEY
or cls.JWT_SECRET_KEY == DevelopmentConfig.JWT_SECRET_KEY
):If DevelopmentConfig.JWT_SECRET_KEY is not yet defined or the development default is stored elsewhere, you should:
- Define the canonical dev default in one place (e.g.,
DevelopmentConfig.JWT_SECRET_KEY = "dev-secret-key-change-in-production"orConfig.DEFAULT_JWT_SECRET_KEY = "dev-secret-key-change-in-production"). - Update this production validation and any other dev default usages to reference that single canonical location.
Security fixes (Track A1)
Targets
release/v0.1.x(branched offv1.x— the active branch;mainis stale).C1 — JWT signing key no longer falls back to the dev default in production
ProductionConfignow requiresJWT_SECRET_KEYvia env var (no fallback to the basedev-secret-key-change-in-production), andvalidate()rejects the dev default. Closes an admin-token forgery vector where prod silently signed tokens with a public key.C2 — Production config validation actually runs
create_app()now callsProductionConfig.validate()at startup (guarded byhasattrso Dev/Testing configs are unaffected). Previously the guard was dead code (only called from tests), so prod could boot with default secrets.Tests
19 config tests pass; mypy --strict compatible; Dev/Testing boot unaffected.
Part of the security-hardening series on release/v0.1.x.
🤖 Generated with Claude Code
Summary by Sourcery
Enforce production-time configuration validation and require an explicit JWT secret in production to prevent insecure defaults.
Bug Fixes: