Security: Authentication is fail-open when API key is unset#1131
Security: Authentication is fail-open when API key is unset#1131tuanaiseo wants to merge 1 commit intoace-step:mainfrom
Conversation
Both `verify_token_from_request` and `verify_api_key` return success when `_api_key` is `None`. If deployment/configuration accidentally omits setting the key, all protected endpoints become publicly accessible without authentication. Affected files: auth.py Signed-off-by: tuanaiseo <221258316+tuanaiseo@users.noreply.github.com>
📝 WalkthroughWalkthroughToken and API key verification functions in the auth module now raise an HTTP 503 error when the API key is not configured, rather than treating an unset key as disabled authentication. Docstrings were updated to reflect this behavior change. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acestep/api/http/auth.py`:
- Around line 39-40: The current HTTP module unconditionally raises
HTTPException when _api_key is None; change this to respect an opt-out flag by
adding and reading a boolean ALLOW_UNAUTHENTICATED (set via set_api_key), emit a
startup logger.warning/error in set_api_key when auth is unset, and only raise
HTTPException(503) inside verify_token_from_request and verify_api_key when
_api_key is None AND ALLOW_UNAUTHENTICATED is False; update tests to set
ALLOW_UNAUTHENTICATED=True for the "auth disabled" path and ensure behavior
matches the gradio path (verify_token_from_request) so both fail-open or
fail-closed are aligned.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6a78085-6a7d-45f5-afbb-5a033897d43d
📒 Files selected for processing (1)
acestep/api/http/auth.py
| if _api_key is None: | ||
| return None | ||
| raise HTTPException(status_code=503, detail="API key is not configured") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the broken test and any other call sites relying on the old "None == disabled" contract.
rg -nP -C2 '\bverify_(token_from_request|api_key)\b'
rg -nP -C2 '\bset_api_key\b'
rg -nP 'ALLOW_UNAUTHENTICATED'Repository: ace-step/ACE-Step-1.5
Length of output: 50377
🏁 Script executed:
#!/bin/bash
# Check the actual auth.py implementation
cat acestep/api/http/auth.pyRepository: ace-step/ACE-Step-1.5
Length of output: 2376
🏁 Script executed:
#!/bin/bash
# Check if auth_test.py exists and what tests it contains
find . -name "*auth*test*.py" -type f | head -10Repository: ace-step/ACE-Step-1.5
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Search for ALLOW_UNAUTHENTICATED in entire codebase with extended context
find . -type f -name "*.py" -exec grep -l "ALLOW_UNAUTHENTICATED" {} \;Repository: ace-step/ACE-Step-1.5
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Read the auth_test.py file
cat acestep/api/http/auth_test.pyRepository: ace-step/ACE-Step-1.5
Length of output: 3347
🏁 Script executed:
#!/bin/bash
# Verify api_server.py calls set_api_key to understand deployment impact
rg -A5 "set_api_key" acestep/api_server.pyRepository: ace-step/ACE-Step-1.5
Length of output: 507
🏁 Script executed:
#!/bin/bash
# Check gradio api_routes for the duplicate verification logic mentioned
rg -B2 -A10 "def verify_token_from_request" acestep/ui/gradio/api/api_routes.pyRepository: ace-step/ACE-Step-1.5
Length of output: 485
Implementation diverges from PR objectives: missing opt-out mechanism and startup warning; breaks existing tests.
The PR description specifies fail-closed by default with an explicit ALLOW_UNAUTHENTICATED escape hatch plus a high-severity startup log when auth is disabled. The current implementation instead unconditionally raises HTTP 503 when _api_key is None, which breaks the codebase in three ways:
-
Existing test fails.
acestep/api/http/auth_test.py::test_verify_token_returns_none_when_auth_is_disabledcallsverify_token_from_request(body={}, authorization=None)and expectsNone, but lines 39–40 now raiseHTTPException(503). The test also callsverify_api_key(authorization=None)expecting success, but line 68 raises 503. -
Breaks unauthenticated deployments.
acestep/api_server.pyreadsACESTEP_API_KEYviaos.getenv("ACESTEP_API_KEY", None)and passes it toset_api_key. Any deployment without the env var (local dev, the gradio path inacestep/ui/gradio/api/api_routes.py) now 503s on every protected route with no migration path. -
Missing startup logging. There is no
logger.warningorlogger.erroremitted at configuration time (inset_api_key) to surface the insecure state.
Also note: acestep/ui/gradio/api/api_routes.py::verify_token_from_request still silently returns None on unset key (fail-open), while the HTTP module version raises 503 (fail-closed). Either align them or clarify intent (is gradio intentionally left fail-open, or is that a bug?).
Suggested fix: Gate the 503 response on an ALLOW_UNAUTHENTICATED flag read in set_api_key, emit a startup warning/error log if auth is unset, and update tests to set the flag when testing the "disabled auth" path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@acestep/api/http/auth.py` around lines 39 - 40, The current HTTP module
unconditionally raises HTTPException when _api_key is None; change this to
respect an opt-out flag by adding and reading a boolean ALLOW_UNAUTHENTICATED
(set via set_api_key), emit a startup logger.warning/error in set_api_key when
auth is unset, and only raise HTTPException(503) inside
verify_token_from_request and verify_api_key when _api_key is None AND
ALLOW_UNAUTHENTICATED is False; update tests to set ALLOW_UNAUTHENTICATED=True
for the "auth disabled" path and ensure behavior matches the gradio path
(verify_token_from_request) so both fail-open or fail-closed are aligned.
1 participant
Problem
Both
verify_token_from_requestandverify_api_keyreturn success when_api_keyisNone. If deployment/configuration accidentally omits setting the key, all protected endpoints become publicly accessible without authentication.Severity:
mediumFile:
acestep/api/http/auth.pySolution
Adopt fail-closed behavior by default: require explicit configuration to disable auth (e.g.,
ALLOW_UNAUTHENTICATED=falseby default), and log a high-severity startup warning/error when auth is disabled.Changes
acestep/api/http/auth.py(modified)Testing
Summary by CodeRabbit