fix(config): refuse to start with an empty SECRET_KEY#41299
Conversation
check_secret_key already refused to start (in non-debug/non-testing mode) when SECRET_KEY equalled the well-known placeholder. An explicitly empty SECRET_KEY (e.g. SECRET_KEY = "" set in superset_config.py) was not covered: the env fallback only substitutes the placeholder, so an empty value could reach the app unguarded. Treat a missing/empty SECRET_KEY the same as the placeholder: warn in debug/testing, and refuse to start otherwise. Adds unit tests covering empty/None/placeholder keys (fail closed in production, warn in debug) and a strong key (no warning). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dosubot
Bot
added
authentication
Code Review Agent Run #10f5fcActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| from superset.initialization import SupersetAppInitializer | ||
|
|
||
|
|
||
| def _make_initializer(secret_key, *, debug=False, testing=False): |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations for all parameters and the return type on this helper function to satisfy the fully-typed new Python code rule. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added Python helper function, and it omits type annotations for its parameters and return value. The custom rule requires new Python code to be fully typed, so this is a real violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/test_check_secret_key.py
**Line:** 27:27
**Comment:**
*Custom Rule: Add explicit type annotations for all parameters and the return type on this helper function to satisfy the fully-typed new Python code rule.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
|
||
|
|
||
| @pytest.mark.parametrize("secret_key", ["", None, CHANGE_ME_SECRET_KEY]) | ||
| def test_check_secret_key_refuses_to_start_when_insecure(secret_key): |
There was a problem hiding this comment.
Suggestion: Add a type annotation for secret_key and an explicit -> None return annotation for this new test function. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This newly added test function has an untyped parameter and no return annotation. The rule explicitly requires new Python functions to be fully typed, so the violation is present.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/test_check_secret_key.py
**Line:** 39:39
**Comment:**
*Custom Rule: Add a type annotation for `secret_key` and an explicit `-> None` return annotation for this new test function.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
|
||
|
|
||
| @pytest.mark.parametrize("secret_key", ["", None, CHANGE_ME_SECRET_KEY]) | ||
| def test_check_secret_key_warns_but_starts_in_debug(secret_key): |
There was a problem hiding this comment.
Suggestion: Add a type annotation for secret_key and an explicit -> None return annotation for this new test function. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a new Python test function without type annotations on its parameter or its return type. That directly violates the fully-typed new Python code rule.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/test_check_secret_key.py
**Line:** 48:48
**Comment:**
*Custom Rule: Add a type annotation for `secret_key` and an explicit `-> None` return annotation for this new test function.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
|
||
|
|
||
| def test_check_secret_key_accepts_strong_key(): | ||
| """A non-empty, non-placeholder key starts without warning or exit.""" |
There was a problem hiding this comment.
Suggestion: Add an explicit -> None return type annotation to this new test function to keep new Python code fully typed. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This newly added function has no return type annotation. Since the custom rule requires new Python functions to be fully typed, the suggestion identifies a real violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/test_check_secret_key.py
**Line:** 57:57
**Comment:**
*Custom Rule: Add an explicit `-> None` return type annotation to this new test function to keep new Python code fully typed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41299 +/- ##
==========================================
- Coverage 64.34% 64.34% -0.01%
==========================================
Files 2653 2653
Lines 144952 144956 +4
Branches 33433 33434 +1
==========================================
- Hits 93273 93269 -4
- Misses 49995 50001 +6
- Partials 1684 1686 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
SUMMARY
check_secret_key()already refuses to start (in non-debug/non-testing mode) whenSECRET_KEYequals the well-known placeholder constant, logging guidance and exiting. However, an explicitly emptySECRET_KEYwas not covered: the environment fallback only substitutes the placeholder when the env var is unset, so a deployment that setsSECRET_KEY = ""(e.g. insuperset_config.py) could reach the app with an empty key.This treats a missing/empty
SECRET_KEYthe same as the placeholder — warn in debug/testing, refuse to start otherwise — closing the remaining gap with the existing guard.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — startup validation behavior.
TESTING INSTRUCTIONS
Unit tests added in
tests/unit_tests/test_check_secret_key.py:None/ placeholder keys fail closed (SystemExit) in non-debug mode.Run:
pytest tests/unit_tests/test_check_secret_key.pyADDITIONAL INFORMATION