Skip to content

Security: Authentication is fail-open when API key is unset #1131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tuanaiseo wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Security: Authentication is fail-open when API key is unset #1131

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions acestep/api/http/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def set_api_key(key: Optional[str]) -> None:
"""Set the process-level API key used by auth verification helpers.

Args:
key: API key string, or ``None`` to disable auth enforcement.
key: API key string, or ``None`` when auth is unconfigured.
"""

global _api_key
Expand All @@ -30,14 +30,14 @@ def verify_token_from_request(
authorization: Optional ``Authorization`` header value.

Returns:
Validated token value, or ``None`` when auth is disabled.
Validated token value.

Raises:
HTTPException: If auth is required and token is missing/invalid.
"""

if _api_key is None:
return None
raise HTTPException(status_code=503, detail="API key is not configured")
Comment on lines 39 to +40
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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.py

Repository: 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 -10

Repository: 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.py

Repository: 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.py

Repository: 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.py

Repository: 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:

  1. Existing test fails. acestep/api/http/auth_test.py::test_verify_token_returns_none_when_auth_is_disabled calls verify_token_from_request(body={}, authorization=None) and expects None, but lines 39–40 now raise HTTPException(503). The test also calls verify_api_key(authorization=None) expecting success, but line 68 raises 503.

  2. Breaks unauthenticated deployments. acestep/api_server.py reads ACESTEP_API_KEY via os.getenv("ACESTEP_API_KEY", None) and passes it to set_api_key. Any deployment without the env var (local dev, the gradio path in acestep/ui/gradio/api/api_routes.py) now 503s on every protected route with no migration path.

  3. Missing startup logging. There is no logger.warning or logger.error emitted at configuration time (in set_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.


ai_token = body.get("ai_token") if body else None
if ai_token:
Expand Down Expand Up @@ -65,7 +65,7 @@ async def verify_api_key(authorization: Optional[str] = Header(None)) -> None:
"""

if _api_key is None:
return
raise HTTPException(status_code=503, detail="API key is not configured")

if not authorization:
raise HTTPException(status_code=401, detail="Missing Authorization header")
Expand Down