Exclude secrets from configuration revision history#225
Conversation
… are excluded from revision history
…ining validation logic
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes implement a secrets masking and restoration mechanism for application configuration. Sensitive fields are now excluded from database revisions via masking, restored during validation, and handled separately in serialization and persistence workflows across models, views, and utilities. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend UI
participant Backend as Views/API
participant DB as Database
participant FSys as Filesystem
participant Config as Config Manager
User->>Frontend: Save Configuration (with secrets)
Frontend->>Backend: POST configuration
Backend->>Backend: Validate & Restore Masked Secrets
Note over Backend: replace MASK_STRING with actual values
Backend->>DB: Save sanitized config (secrets removed)
Backend->>FSys: Write full config (with secrets)
Backend->>Frontend: 200 - Configuration saved.<br/>Secrets excluded from revision.
Frontend->>User: Snack notification
User->>Frontend: Revert to Previous Revision
Frontend->>Backend: GET revision config
Backend->>Frontend: Return revision (with masked secrets)
Frontend->>Frontend: Apply masked config to form
Frontend->>User: Direct snack notification
sequenceDiagram
participant Export as Serialize
participant Runtime as Runtime Config
Note over Export: Configuration Export Flow
Export->>Runtime: Check for API keys/cookies
alt Value exists
Export->>Export: Replace with MASK_STRING
else No value
Export->>Export: Keep as-is
end
Export->>Export: Return masked export
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
Apply old revisions directly to the frontend form instead of backend processing. Secret fields keep their current masked values, eliminating validation errors. Previous flow: User clicks restore → Backend validates partial config → Error New flow: Config populates form → User reviews → Saves with complete config
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
enferno/admin/models/AppConfig.py (1)
16-24: Annotate SECRET_FIELDS as ClassVar and consider making it immutableRuff is right about the mutable class attribute here. Also, since this set is a security‑critical allowlist, making it immutable and clearly class‑scoped helps avoid accidental mutation and improves type checking.
You can address both by annotating it as a
ClassVarand switching to afrozenset:-from typing import Any +from typing import Any, ClassVar, FrozenSet ... - # Secret fields that should not be stored in revision history - SECRET_FIELDS = { - "RECAPTCHA_PRIVATE_KEY", - "GOOGLE_CLIENT_SECRET", - "AWS_SECRET_ACCESS_KEY", - "MAIL_PASSWORD", - "YTDLP_COOKIES", - "GOOGLE_MAPS_API_KEY", - } + # Secret fields that should not be stored in revision history + SECRET_FIELDS: ClassVar[FrozenSet[str]] = frozenset( + { + "RECAPTCHA_PRIVATE_KEY", + "GOOGLE_CLIENT_SECRET", + "AWS_SECRET_ACCESS_KEY", + "MAIL_PASSWORD", + "YTDLP_COOKIES", + "GOOGLE_MAPS_API_KEY", + } + )Also, keep an eye on consistency: if more secret keys are introduced later, they’ll need to be added here and in
ConfigManager.serialize()masking logic to stay in sync.enferno/utils/config_utils.py (1)
306-307: Good expansion of masking; consider deriving this list from SECRET_FIELDSMasking
GOOGLE_MAPS_API_KEYandYTDLP_COOKIESinserialize()closes two clear leaks of sensitive data to the frontend/API and aligns them with how other secrets are treated.To reduce maintenance drift between this function and
AppConfig.SECRET_FIELDS, consider post‑processingconfwith a single loop using the shared secret list instead of hard‑coding each key here, e.g.:from enferno.admin.models import AppConfig for field in AppConfig.SECRET_FIELDS: if field in conf: conf[field] = ConfigManager.MASK_STRING if getattr(cfg, field, "") else ""This way, adding a new secret key in one place (the model) automatically updates both masking and DB sanitization.
Also applies to: 338-339
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
enferno/admin/models/AppConfig.py(2 hunks)enferno/admin/templates/admin/system-administration.html(1 hunks)enferno/admin/validation/models.py(6 hunks)enferno/admin/views.py(1 hunks)enferno/utils/config_utils.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.14.5)
enferno/admin/models/AppConfig.py
17-24: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
enferno/admin/validation/models.py
616-616: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: semgrep/ci
🔇 Additional comments (10)
enferno/admin/models/AppConfig.py (2)
29-31: User linkage for AppConfig looks consistentThe
user_idFK anduserrelationship/backref give you clear attribution of who saved each configuration snapshot and align with how other models track user ownership. No issues from this snippet.
33-43: The sanitization is correct and properly isolated; no changes neededThe implementation properly handles the semantics change to partial/sanitized configs. Verification confirms:
write_config()correctly persists onlysanitized_conf(secrets filtered) to the database at line 365- Full config with secrets is written separately to
config.jsonat line 377- The only backend flow querying
AppConfigrecords returnsto_dict()which excludes secrets (line 6341)ConfigManagerprovides the mask-restore logic (MASK_STRING,restore_masked_secretsvalidator) for runtime restoration- No backend flows attempt to restore full configuration directly from
AppConfig.configThe architecture correctly delegates runtime config to the filesystem plus mask-restore logic, leaving the database to store only sanitized history.
enferno/admin/views.py (1)
6391-6393: Updated success message matches new secrets handlingThe new message correctly informs admins that secrets are excluded from revision history and is consistent with the new
AppConfig/ConfigManagerbehavior. No further changes needed here.enferno/utils/config_utils.py (1)
352-362: Verification confirms both follow-up concerns are unfounded—AppConfig.config is write-only for audit trail; runtime config sourced from config.json never rebuilds from DB snapshotsVerification findings:
AppConfig snapshots concern: No code path rebuilds full config from
AppConfig.config. Runtime configuration is sourced fromconfig.jsonviaConfigManager.get_config()at application startup (referenced throughoutenferno/settings.py). The sanitizedAppConfig.configserves only as an audit trail; it is never read back for runtime use.Error handling concern: Already acknowledged as acceptable for this PR's scope. Both callers of
write_config(setup/views.py:210, admin/views.py:6384) properly handle the return value and propagate errors appropriately.The code meets the stated security goal of keeping secrets out of revision history without introducing runtime risks.
enferno/admin/templates/admin/system-administration.html (1)
1240-1251: LGTM! Client-side revision restore correctly implemented.The client-side approach ensures that secrets (which are excluded from revision history) maintain their current masked values when reverting to an old configuration. The try/catch with
structuredClonefallback toJSON.parse/stringifyprovides good browser compatibility.enferno/admin/validation/models.py (5)
17-17: LGTM! Necessary imports for secrets restoration.The imports of
AppConfigandConfigManagersupport the newrestore_masked_secretsvalidator functionality.Also applies to: 19-19
510-512: Good refactor: More Pythonic membership check.Changing from
if not v.lower() in [...]toif v.lower() not in [...]improves readability and follows Python style guidelines.Also applies to: 615-617
1496-1497: LGTM! Removed unused exception variable.The exception variable
ewas not being used, so removing it is a good cleanup.
1210-1211: LGTM! Improved error message clarity.The error message update improves user experience by making the incompatibility more explicit.
1797-1806: Edge case concerns are valid design limitations, not bugs—consider if they're acceptable.Verification confirms your concerns are valid:
Empty string default handles missing attributes silently: If a field is added to
AppConfig.SECRET_FIELDSbut not initialized inConfig,getattr()returns empty string without error. This could mask configuration oversights. Consider raising an exception or logging a warning.Intentional design limitation—secrets cannot be cleared through UI: The validator always restores masked secrets from current config, preventing accidental deletion but also preventing intentional clearing. This appears intentional and is the same pattern used in
enferno/utils/config_utils.py:353–355. Verify this behavior aligns with requirements.Literal mask collision is theoretically possible: If an actual secret value equals
"**********", it would be incorrectly treated as masked. Though unlikely, consider a more unique sentinel value (e.g., a UUID or null byte prefix).No bugs found, but review whether these design choices match your security and UX requirements.
Allow ConfigManager.MASK_STRING in secret field validators so users can save config without changing secrets. Remove duplicate secret restoration from validation model.
|
|
||
| from enferno.admin.constants import Constants | ||
| from enferno.admin.models import Activity | ||
| from enferno.admin.models import Activity, AppConfig |
Problem
Secrets (passwords, API keys, cookies) were being stored in database revision history, creating a security vulnerability.
Solution
**********before validationKey Changes
AppConfig.to_dict()strips secrets from revision snapshotsConfigManager.serialize()masks secrets when displaying current configSecret Fields Protected
RECAPTCHA_PRIVATE_KEY,GOOGLE_CLIENT_SECRET,AWS_SECRET_ACCESS_KEY,MAIL_PASSWORD,YTDLP_COOKIES,GOOGLE_MAPS_API_KEYSummary by CodeRabbit
New Features
UI/UX Improvements
Messages
✏️ Tip: You can customize this high-level summary in your review settings.