fix(api): enforce configured inference bearer token - #15
Conversation
📝 WalkthroughWalkthroughThe inference API now requires a configured bearer token, returns 503 when authentication is unset, and validates tokens with constant-time comparison. Tests and configuration documentation use ChangesInference API authentication
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant _require_bearer
participant Environment
participant InferenceAPI
Client->>_require_bearer: Send Authorization header
_require_bearer->>Environment: Read API_BEARER_TOKEN
alt Token is unset
_require_bearer-->>Client: Return 503
else Header or token is invalid
_require_bearer-->>Client: Return 401
else Token is valid
_require_bearer->>InferenceAPI: Allow POST /infer
InferenceAPI-->>Client: Return inference response
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b55e4f0351
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return authorization.removeprefix("Bearer ").strip() | ||
|
|
||
| token = authorization.removeprefix("Bearer ").strip() | ||
| if not hmac.compare_digest(token, expected_token): |
There was a problem hiding this comment.
When either the configured secret or a received credential contains a non-ASCII character, Python's hmac.compare_digest() raises TypeError for str operands rather than returning False. Consequently, an operator choosing a Unicode secret makes every authenticated request fail with HTTP 500, and an ASGI server that accepts a non-ASCII header lets malformed credentials trigger the same error instead of a 401. Encode both values to bytes before comparison or explicitly reject non-ASCII credentials.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/main.py`:
- Around line 37-41: Update both 401 HTTPException branches in _require_bearer()
to include headers={"WWW-Authenticate": "Bearer"}, covering missing/invalid
authorization and token mismatch cases. Extend tests/test_auth.py to assert this
WWW-Authenticate header on both responses.
- Around line 36-39: Update the authorization parsing around the Bearer
validation and token extraction to accept case-insensitive scheme values while
preserving the existing token validation behavior. Use the proposed parser for
separating scheme and token fields, and add coverage in tests/test_auth.py for a
mixed- or lowercase header such as “bearer test-token”.
In `@README.md`:
- Line 154: Update the API_BEARER_TOKEN table description in README.md to begin
with “Shared bearer token required for” while preserving the existing endpoint
and 503 behavior details.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ca35ab6-c89b-4888-bf31-aed85ad3b944
📒 Files selected for processing (6)
.env.exampleREADME.mdapi/main.pytests/test_auth.pytests/test_inference.pytests/test_rate_limit.py
| if not authorization or not authorization.startswith("Bearer "): | ||
| raise HTTPException(status_code=401, detail="Missing or invalid Authorization header") | ||
| return authorization.removeprefix("Bearer ").strip() | ||
|
|
||
| token = authorization.removeprefix("Bearer ").strip() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)(main\.py|test_auth\.py)$|api/|tests/' | sed -n '1,120p'
echo "== api/main.py around auth =="
if [ -f api/main.py ]; then
nl -ba api/main.py | sed -n '1,80p'
fi
echo "== test_auth.py =="
if [ -f tests/test_auth.py ]; then
nl -ba tests/test_auth.py | sed -n '1,220p'
fi
echo "== search authorization usage =="
rg -n "Authorization|removeprefix|bearer|Bearer|HTTPException|status_code=401" api tests -S || trueRepository: CoreyLeath-code/SentinelAI
Length of output: 589
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== api/main.py =="
awk '{printf "%6d\t%s\n", NR, $0}' api/main.py | sed -n '1,90p'
echo "== api/auth.py =="
awk '{printf "%6d\t%s\n", NR, $0}' api/auth.py | sed -n '1,160p'
echo "== tests/test_auth.py =="
awk '{printf "%6d\t%s\n", NR, $0}' tests/test_auth.py | sed -n '1,220p'
echo "== deterministic header parser behavior =="
python3 - <<'PY'
def current(authorization):
if not authorization or not authorization.startswith("Bearer "):
return "401"
token = authorization.removeprefix("Bearer ").strip()
return ("ok", token)
def proposed(authorization):
parts = authorization.split(maxsplit=1) if authorization else []
if len(parts) != 2 or parts[0].lower() != "bearer":
return "401"
token = parts[1].strip()
return ("ok", token)
cases = [
"Bearer test-token",
"bearer test-token",
"BeArEr test-token",
"bearer test-token",
"Bearer",
"",
"Basic abc",
]
for header in cases:
print(repr(header), "current=", current(header), "proposed=", proposed(header))
PYRepository: CoreyLeath-code/SentinelAI
Length of output: 4641
Accept case-insensitive Bearer token schemes.
A lowercase or mixed-case scheme should not fail authentication when the token is valid. Use the proposed parser for scheme/token fields, and add a cover test such as bearer test-token in tests/test_auth.py.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/main.py` around lines 36 - 39, Update the authorization parsing around
the Bearer validation and token extraction to accept case-insensitive scheme
values while preserving the existing token validation behavior. Use the proposed
parser for separating scheme and token fields, and add coverage in
tests/test_auth.py for a mixed- or lowercase header such as “bearer test-token”.
| raise HTTPException(status_code=401, detail="Missing or invalid Authorization header") | ||
| return authorization.removeprefix("Bearer ").strip() | ||
|
|
||
| token = authorization.removeprefix("Bearer ").strip() | ||
| if not hmac.compare_digest(token, expected_token): | ||
| raise HTTPException(status_code=401, detail="Missing or invalid Authorization header") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)api/main\.py$|(^|/)tests/test_auth\.py$|pyproject\.toml$|requirements.*\.txt$' || true
echo "== api/main.py relevant =="
if [ -f api/main.py ]; then
cat -n api/main.py | sed -n '1,120p'
fi
echo "== tests/test_auth.py relevant =="
if [ -f tests/test_auth.py ]; then
cat -n tests/test_auth.py | sed -n '1,220p'
fi
echo "== search auth challenge usages =="
rg -n "WWW-Authenticate|Bearer|Authorization|HTTPException|401" .Repository: CoreyLeath-code/SentinelAI
Length of output: 5519
🌐 Web query:
FastAPI HTTPException headers WWW-Authenticate Bearer authentication challenge
💡 Result:
To include a WWW-Authenticate Bearer challenge in a FastAPI response, use the HTTPException class with the headers parameter [1][2]. When an authentication error occurs (typically a 401 Unauthorized), the HTTP specification requires that the server includes a WWW-Authenticate header to inform the client about the authentication scheme [1][3][4]. For Bearer token authentication, you should provide the following: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, detail="Not authenticated", headers={"WWW-Authenticate": "Bearer"},) Key points regarding this implementation: 1. Status Code: Use status.HTTP_401_UNAUTHORIZED [1][2]. Starting with FastAPI version 0.122.0, built-in security utilities (like OAuth2PasswordBearer or HTTPBearer) automatically return this 401 status code along with the appropriate WWW-Authenticate header when authentication fails [5][4]. 2. Headers Parameter: The HTTPException class accepts a headers dictionary, which allows you to pass the WWW-Authenticate instruction directly to the client [6][7]. 3. Standard Compliance: Including this header ensures your API follows HTTP specifications (such as RFC 9110), signaling to clients that they should attempt authentication using the Bearer scheme [1][3][4]. For advanced use cases or when overriding default security behavior, you can extend classes like HTTPBearer and override the make_not_authenticated_error method to customize how these 401 responses are generated [5][8].
Citations:
- 1: https://fastapi.tiangolo.com/tutorial/security/simple-oauth2/
- 2: https://fastapi.tiangolo.com/tutorial/security/oauth2-jwt/
- 3: https://fastapi.tiangolo.com/reference/security/
- 4: HTTPBearer security scheme is returning 403 instead or 401 fastapi/fastapi#10177
- 5: https://fastapi.tiangolo.com/how-to/authentication-error-status-code/
- 6: https://fastapi.tiangolo.com/reference/exceptions/
- 7: https://fastapi.tiangolo.com/tutorial/handling-errors/?h=error+handling
- 8: https://github.com/fastapi/fastapi/blob/e94028ab/fastapi/security/http.py
Include the Bearer challenge on both 401 responses.
Both _require_bearer() branches return HTTPException(status_code=401, ...) without WWW-Authenticate: Bearer. Add headers={"WWW-Authenticate": "Bearer"} to each branch, and assert the header in tests/test_auth.py.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/main.py` around lines 37 - 41, Update both 401 HTTPException branches in
_require_bearer() to include headers={"WWW-Authenticate": "Bearer"}, covering
missing/invalid authorization and token mismatch cases. Extend
tests/test_auth.py to assert this WWW-Authenticate header on both responses.
| | `POSTGRES_DB` | `sentinel` | Postgres database | | ||
| | `OLLAMA_HOST` | `http://ollama:11434` | Ollama endpoint (optional) | | ||
| | `LLM_MODEL` | `llama2` | LLM model name | | ||
| | `API_BEARER_TOKEN` | *(unset)* | Required shared bearer token for `POST /infer`; the endpoint returns 503 until configured | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rewrite the bearer-token description.
Use Shared bearer token required for ... instead of Required shared bearer token .... This removes the awkward wording without changing the meaning.
The provided LanguageTool hint identifies this wording issue.
Proposed wording
-| `API_BEARER_TOKEN` | *(unset)* | Required shared bearer token for `POST /infer`; the endpoint returns 503 until configured |
+| `API_BEARER_TOKEN` | *(unset)* | Shared bearer token required for `POST /infer`; the endpoint returns 503 until configured |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `API_BEARER_TOKEN` | *(unset)* | Required shared bearer token for `POST /infer`; the endpoint returns 503 until configured | | |
| | `API_BEARER_TOKEN` | *(unset)* | Shared bearer token required for `POST /infer`; the endpoint returns 503 until configured | |
🧰 Tools
🪛 LanguageTool
[style] ~154-~154: The double modal “Required shared” is nonstandard (only accepted in certain dialects). Consider “to be shared”.
Context: ...PI_BEARER_TOKEN| *(unset)* | Required shared bearer token forPOST /infer`; the end...
(NEEDS_FIXED)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 154, Update the API_BEARER_TOKEN table description in
README.md to begin with “Shared bearer token required for” while preserving the
existing endpoint and 503 behavior details.
Source: Linters/SAST tools
1 participant
Summary
Makes the public inference-stub route verify a configured bearer token instead of accepting any non-empty Authorization: Bearer value.
Addresses part of #13.
Problem
POST /infer previously treated bearer authentication as header presence only. Any non-empty token could invoke the endpoint, and an unset server-side token was not detected.
Root cause
The request dependency stripped the bearer prefix but did not compare the supplied token against configured server state.
Changes
Claim hardening
No production-security claim is added. This change only enforces a shared-token boundary for the demo inference route; token rotation, identity management, and authorization scopes remain out of scope.
Reproducibility and validation
Executed locally against the branch source:
GitHub Actions remains the final clean-runner verification.
Security
The route now fails closed when no server-side bearer token is configured and does not accept arbitrary bearer values.
Risk
Low for correctly configured callers. Deployments using /infer must set API_BEARER_TOKEN; otherwise the endpoint deliberately returns 503 instead of allowing unauthenticated access.
Rollback
Revert the six commits in this PR. No database or API-schema migration is involved.
Remaining work
Issue #13 retains broader clean-clone, coverage, security-evidence, container, and evaluation work.
Summary by CodeRabbit
Security
Documentation
Tests