feat(auth): implement comprehensive JWT and Basic authentication#23
Merged
feat(auth): implement comprehensive JWT and Basic authentication#23
Conversation
- Add JWT algorithm configuration with validation (HS256, HS384, HS512, RS256, RS384, RS512, ES256, ES384, ES512) - Implement BasicAuthentication class with proper 401 error responses - Add rate limiting for auth endpoints (10/min, 100/hour, 1000/day default) - Fix JWT expiration configuration not being passed to authentication instances - Add JWT claim validation to prevent overwriting reserved 'exp' claim - Consolidate Basic auth parsing in _login.py module - Add comprehensive test suite covering all auth scenarios - Update YAML configuration support for JWT algorithm and expiration - Fix typing issues in auth modules - Add proper error response differentiation (401 for auth failures, 403 for permissions) BREAKING CHANGE: JWTAuthentication constructor now accepts optional parameters: - expiration: Override default 3600s token lifetime - algorithm: Override default HS256 algorithm - secret_key: Custom secret (defaults to LIGHTAPI_JWT_SECRET) Security enhancements: - Rate limiting prevents brute force attacks on login endpoints - Basic auth returns proper WWW-Authenticate header semantics - JWT algorithm validation prevents insecure configurations
📝 WalkthroughWalkthroughAdds login/token endpoints, BasicAuthentication, login_validator registration, JWT configuration (expiration/extra claims/algorithm), per‑IP auth rate limiting, YAML config support for auth settings and callable resolution, state-backed registry/cache, and automatic /auth route registration when relevant backends are present. (33 words) Changes
Sequence DiagramssequenceDiagram
participant Client
participant LightApi as LightApi Server
participant LoginHandler as Login Handler
participant Validator as Login Validator
participant JWT as JWT Generator
Client->>LightApi: POST /auth/login (Basic header or JSON body)
LightApi->>LoginHandler: route -> login_handler
LoginHandler->>LoginHandler: extract credentials (header or body)
LoginHandler->>Validator: login_validator(username, password)
alt credentials valid
Validator-->>LoginHandler: user dict
alt JWT enabled
LoginHandler->>JWT: generate_token(user, extra_claims, exp, alg)
JWT-->>LoginHandler: token string
LoginHandler-->>Client: 200 {token, user}
else Basic-only
LoginHandler-->>Client: 200 {user}
end
else invalid
LoginHandler-->>Client: 401 {error: "authentication failed"}
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lightapi/auth.py (1)
20-42:⚠️ Potential issue | 🟠 MajorPlease revert changes inside the frozen auth classes.
This PR edits both
BaseAuthenticationandJWTAuthentication, but those class bodies are explicitly frozen in this repository. Add the new behavior via new helpers/types rather than changing these two implementations.As per coding guidelines, "lightapi/auth.py: Do not modify lightapi/auth.py: JWTAuthentication and BaseAuthentication class bodies are FROZEN"
Also applies to: 59-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/auth.py` around lines 20 - 42, Revert any edits to the frozen class bodies for BaseAuthentication and JWTAuthentication in lightapi/auth.py so their original implementations of methods like authenticate and get_auth_error_response are restored; instead move the new behavior into a new helper (e.g., a separate class or function such as CustomAuthHelper or jwt_auth_helper) and update callers to use that helper where custom logic is required, ensuring you do not change the BaseAuthentication or JWTAuthentication class definitions themselves and that new helpers are defined in a new module or below the frozen classes in the file.lightapi/lightapi.py (2)
549-561:⚠️ Potential issue | 🟠 MajorThe auth pipeline still bypasses backend-specific 401 responses.
_check_auth()returns its own generic 401 instead of delegating toauthenticator.get_auth_error_response(request), so the new backends never get to emitWWW-Authenticateor scheme-specific bodies.Possible fix
- if not authenticator.authenticate(request): - return JSONResponse( - {"detail": "Authentication credentials invalid."}, status_code=401 - ) + if not authenticator.authenticate(request): + return authenticator.get_auth_error_response(request)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/lightapi.py` around lines 549 - 561, The current _check_auth() always returns a generic 401 JSONResponse when authenticator.authenticate(request) fails, which prevents backends from returning scheme-specific responses; change the failure path so that after creating the authenticator (including the JWTAuthentication instantiation logic) you check the result of authenticator.authenticate(request) and if it is falsy call and return authenticator.get_auth_error_response(request) when that method exists, otherwise fall back to the existing JSONResponse({"detail": "Authentication credentials invalid."}, status_code=401); keep using backend, JWTAuthentication and authenticate/get_auth_error_response to locate the relevant code.
47-81:⚠️ Potential issue | 🟠 MajorKeep these auth changes out of
lightapi/lightapi.py.This file is explicitly preserved as legacy, but the PR adds new constructor state, route registration, and auth behavior here. Please move this wiring into the non-legacy entrypoint instead of extending
lightapi/lightapi.py.As per coding guidelines, "lightapi/lightapi.py: Do not modify lightapi/lightapi.py; it is preserved as legacy until init.py rewrite absorbs it"
Also applies to: 155-230, 549-556
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/lightapi.py` around lines 47 - 81, The patch added auth and route-related state into the legacy LightAPI constructor (symbols: login_validator parameter, auth_path, self._login_validator, self._auth_path, and the set_login_validator call) and also touched route/registration areas (see other edits around lines noted). Revert these changes in lightapi/lightapi.py so the legacy file remains unchanged, then move the wiring: accept login_validator/auth_path and call set_login_validator from the non-legacy entrypoint (the new package entry module), and initialize route/endpoint/middleware/cors state there instead (use the same symbols: self._routes, self._endpoint_map, self._middlewares, self._cors_origins). Ensure the legacy file no longer modifies auth behavior or registers routes and that all new behavior lives in the modern entrypoint module.
🧹 Nitpick comments (1)
tests/test_login_auth.py (1)
530-548: Pin a single expected outcome for extra whitespace afterBasic.
assert resp.status_code in (200, 401)makes this case non-diagnostic: the suite will pass whether the parser starts accepting or rejecting the same header. Once the contract is decided, assert that one behavior directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_login_auth.py` around lines 530 - 548, The test test_login_basic_header_multiple_spaces_after_basic is nondeterministic because it allows either 200 or 401; change it to assert a single expected outcome (pick one behavior and enforce it). Replace the line "assert resp.status_code in (200, 401)" with a strict assertion, e.g. "assert resp.status_code == 401" (or "== 200" if you decide to accept extra spaces), and remove the conditional block that checks resp.status_code == 200 (or update it to a direct assertion of the JSON when expecting 200); update the test docstring to reflect the chosen contract so test_login_basic_header_multiple_spaces_after_basic documents the expected behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/examples/yaml-configuration.md`:
- Around line 25-26: The YAML docs omit the jwt_algorithm setting; add a
jwt_algorithm entry alongside jwt_expiration and jwt_extra_claims in the example
(e.g., jwt_algorithm: HS256) and update the schema/documentation table to
include defaults.authentication.jwt_algorithm with its description and allowed
values (e.g., HS256, RS256) and default; this ensures the jwt_algorithm key is
discoverable for YAML users and matches the configuration schema.
In `@lightapi/_login.py`:
- Around line 35-36: The auth-scheme checks currently use startswith("Basic ")
which is case-sensitive; update the checks that inspect auth_header (the one at
lines checking startswith("Basic ") and the similar block at 57-59) to perform a
case-insensitive comparison (e.g., compare auth_header.lower().startswith("basic
") or use a case-insensitive regex like re.match(r"^basic\s+", auth_header,
re.I)) so requests with "basic" or other case variants are correctly recognized
as Basic auth before parsing the body. Ensure you update both occurrences that
reference auth_header to the same case-insensitive approach.
- Around line 77-78: The module-level rate limit on login_handler (decorated
with rate_limit_auth_endpoint) uses a global auth limiter and causes
cross-app/test interference; change the limiter to be app-scoped or
test-configurable by removing or modifying the module-level decorator: either
(a) remove the `@rate_limit_auth_endpoint` from login_handler and apply rate
limiting when registering the route so each FastAPI/ASGI app can attach its own
limiter instance, or (b) change rate_limit_auth_endpoint to obtain the limiter
from the request/app state (e.g., request.app.state.auth_limiter) so tests can
inject/disable a limiter per app; update route registration or app setup to
create and attach a per-app limiter or a no-op limiter for tests.
In `@lightapi/_registry.py`:
- Around line 18-23: The global _state["login_validator"] leaks validators
across LightApi instances; change the design to store the validator per-app and
read it from the app/registry context instead of the singleton. Specifically:
remove usage of the global login_validator entry in _state, have
LightApi.__init__ attach the validator to the instance-specific
registry/metadata object (or keyed by registry id inside _state["registry"]) and
update BasicAuthentication.authenticate to fetch the validator from the incoming
request's associated registry/metadata (or the registry-id keyed entry) rather
than reading _state["login_validator"]; update any helper code that reads/writes
_state to use the per-registry slot so multiple apps/tests don’t overwrite each
other.
In `@lightapi/config.py`:
- Around line 12-22: VALID_JWT_ALGORITHMS is defined as a mutable set which
triggers RUF012 and risks accidental mutation; change it to an immutable
collection (e.g., frozenset or tuple) and annotate it as a constant using
typing.Final to signal immutability. Update the declaration of
VALID_JWT_ALGORITHMS (the existing set literal) to use frozenset({...}) or a
tuple of strings and add "from typing import Final" then declare
VALID_JWT_ALGORITHMS: Final = frozenset(...) so linters and readers treat it as
an immutable constant. Ensure any places that iterate over it still work with
the chosen immutable type (frozenset supports membership and iteration).
- Around line 61-73: The per-endpoint jwt_algorithm value must be validated at
config parse time: in the initializer that sets self.jwt_algorithm (the function
shown), check that the provided jwt_algorithm is either None or a member of the
allowed algorithm list used by JWTAuthentication; if it is invalid, raise
ConfigurationError immediately. Reference Authentication.jwt_algorithm and
JWTAuthentication.__init__ behavior when implementing this check so invalid
YAML/per-endpoint values are rejected during config parsing rather than failing
later at runtime.
In `@lightapi/lightapi.py`:
- Around line 176-205: The register() logic inserts auth routes every time
auth_backends contains JWTAuthentication or BasicAuthentication, causing
duplicates; modify register() (which uses auth_backends, _make_login_endpoint,
_auth_path, _routes, and _endpoint_map) to only add the login/token routes once
by either: (a) introducing a one-time flag (e.g. self._auth_routes_registered)
set after first insertion and skip subsequent inserts, or (b) checking
_endpoint_map or existing entries in self._routes for
f"{self._auth_path.rstrip('/')}/login" and
f"{self._auth_path.rstrip('/')}/token" and replacing/skipping insertion if
found. Ensure the chosen approach prevents duplicate insertions while preserving
current behavior of _make_login_endpoint and route order.
- Around line 76-81: The constructor is registering login_validator globally
which makes it shared across all apps; remove the call to set_login_validator
from LightApi.__init__ (keep self._login_validator and self._auth_path) and
instead make authentication use the per-instance validator: update
BasicAuthentication.authenticate (or the auth middleware) to read the validator
from the app instance (e.g., request.app._login_validator) or accept the
validator when the authentication object/middleware is created so each LightApi
instance uses its own login_validator rather than the global set_login_validator
registry.
In `@lightapi/rate_limiter.py`:
- Around line 46-47: The cleanup routine is passing full redis keys (e.g.,
"auth:hour" / "auth:day") into _get_window_seconds(), which only recognizes bare
window names and thus returns 60 for unknown inputs; update _cleanup_old_entries
(and the other occurrences referenced around lines 64-73 and 112) to extract the
window suffix from the key (e.g., split the key on ":" and use the last segment
like "minute"/"hour"/"day") before calling _get_window_seconds(), so
window_seconds is computed correctly for hourly and daily buckets used by
is_rate_limited().
- Around line 75-85: The _get_client_ip function currently trusts
X-Forwarded-For/X-Real-IP/X-Client-IP headers which can be forged; change it to
default to request.client.host and only parse forwarded headers when the app is
configured to trust proxies (e.g., a trusted_proxies set or a boolean like
trust_proxy on the RateLimiter). Implement logic in _get_client_ip to: 1) obtain
remote_addr = request.client.host (or "0.0.0.0" fallback), 2) if trust_proxy is
enabled AND remote_addr is in trusted_proxies (or matches configured proxy
CIDRs), then parse the forwarded headers and return the first non-empty IP;
otherwise always return remote_addr. Update any constructor/initializer for the
RateLimiter to accept the trust_proxy/trusted_proxies configuration and
reference _get_client_ip accordingly.
- Around line 130-144: get_rate_limit_response always returns the per-minute
limit and a fixed 60s retry even when the hourly or daily quota caused the
block; update get_rate_limit_response to detect which quota/window actually
overflowed (minute/hour/day) using the rate limiter's counters or reset
timestamps (e.g., attributes like requests_per_minute, requests_per_hour,
requests_per_day and corresponding reset times or remaining counters on self)
and return headers matching that window: set Retry-After to the actual seconds
until that window resets, X-RateLimit-Limit to the correct window limit,
X-RateLimit-Remaining to the remaining for that window (usually 0), and
X-RateLimit-Reset to the epoch second when that window will reset; modify
get_rate_limit_response to choose the longest blocking window when multiple
quotas are exceeded.
In `@lightapi/yaml_loader.py`:
- Around line 404-419: The endpoint-level dict auth is currently overriding
defaults so jwt_* fields are lost; fix by building a merged authentication
source before reading jwt values (e.g. use the existing merge/helper used
elsewhere such as _make_authentication or a per-field fallback) and read
backend, jwt_expiration, jwt_extra_claims and jwt_algorithm from that merged
object instead of from meta.authentication alone; update the block around
attrs["authentication"] (referencing meta.authentication,
defaults.authentication, jwt_exp, jwt_claims, jwt_algo and Authentication) to
use the merged auth source so default JWT settings persist when only permissions
are overridden.
- Around line 86-103: The _resolve_callable function currently only checks
callable(), which allows async functions and wrong-arity callables; update
_resolve_callable to (1) reject coroutine functions by using
asyncio.iscoroutinefunction(fn) (or inspect.iscoroutinefunction) and raise
ConfigurationError mentioning the dotted path, and (2) inspect the function
signature (via inspect.signature(fn)) to ensure it accepts exactly two required
positional parameters (username, password) — i.e., count parameters that are
POSITIONAL_ONLY or POSITIONAL_OR_KEYWORD without defaults — and raise
ConfigurationError if the arity is not exactly 2; keep the existing import
resolution and only return fn after these validations so login_handler(username,
password) can safely call it synchronously.
In `@tests/test_auth.py`:
- Around line 62-64: The helper _login_validator declares unused parameters and
lacks a return annotation which trips Ruff ARG001 and mypy; update the signature
of _login_validator to prefix the parameters with underscores (e.g., _username,
_password) to mark them intentionally unused and add an explicit return type ->
None so the function becomes def _login_validator(_username: str, _password:
str) -> None: returning None.
In `@tests/test_yaml_config.py`:
- Around line 33-42: The helper _from_str is always passing a login_validator to
LightApi.from_config (injecting _dummy_login_validator when JWT/Basic appears),
which prevents YAML-specified auth.login_validator from being used; change
_from_str to stop injecting a dummy: detect needs_validator but do not set
login_validator when the caller passed None, and call LightApi.from_config
without the login_validator kwarg unless login_validator was explicitly
provided; apply the same conditional-kwarg pattern to the other occurrence noted
(lines 347-367) so YAML dotted-path validators can be exercised.
---
Outside diff comments:
In `@lightapi/auth.py`:
- Around line 20-42: Revert any edits to the frozen class bodies for
BaseAuthentication and JWTAuthentication in lightapi/auth.py so their original
implementations of methods like authenticate and get_auth_error_response are
restored; instead move the new behavior into a new helper (e.g., a separate
class or function such as CustomAuthHelper or jwt_auth_helper) and update
callers to use that helper where custom logic is required, ensuring you do not
change the BaseAuthentication or JWTAuthentication class definitions themselves
and that new helpers are defined in a new module or below the frozen classes in
the file.
In `@lightapi/lightapi.py`:
- Around line 549-561: The current _check_auth() always returns a generic 401
JSONResponse when authenticator.authenticate(request) fails, which prevents
backends from returning scheme-specific responses; change the failure path so
that after creating the authenticator (including the JWTAuthentication
instantiation logic) you check the result of authenticator.authenticate(request)
and if it is falsy call and return
authenticator.get_auth_error_response(request) when that method exists,
otherwise fall back to the existing JSONResponse({"detail": "Authentication
credentials invalid."}, status_code=401); keep using backend, JWTAuthentication
and authenticate/get_auth_error_response to locate the relevant code.
- Around line 47-81: The patch added auth and route-related state into the
legacy LightAPI constructor (symbols: login_validator parameter, auth_path,
self._login_validator, self._auth_path, and the set_login_validator call) and
also touched route/registration areas (see other edits around lines noted).
Revert these changes in lightapi/lightapi.py so the legacy file remains
unchanged, then move the wiring: accept login_validator/auth_path and call
set_login_validator from the non-legacy entrypoint (the new package entry
module), and initialize route/endpoint/middleware/cors state there instead (use
the same symbols: self._routes, self._endpoint_map, self._middlewares,
self._cors_origins). Ensure the legacy file no longer modifies auth behavior or
registers routes and that all new behavior lives in the modern entrypoint
module.
---
Nitpick comments:
In `@tests/test_login_auth.py`:
- Around line 530-548: The test
test_login_basic_header_multiple_spaces_after_basic is nondeterministic because
it allows either 200 or 401; change it to assert a single expected outcome (pick
one behavior and enforce it). Replace the line "assert resp.status_code in (200,
401)" with a strict assertion, e.g. "assert resp.status_code == 401" (or "==
200" if you decide to accept extra spaces), and remove the conditional block
that checks resp.status_code == 200 (or update it to a direct assertion of the
JSON when expecting 200); update the test docstring to reflect the chosen
contract so test_login_basic_header_multiple_spaces_after_basic documents the
expected behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc4f1af1-c0e2-4c29-89f0-24a895db4abf
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
README.mddocs/examples/yaml-configuration.mdlightapi/__init__.pylightapi/_login.pylightapi/_registry.pylightapi/auth.pylightapi/cache.pylightapi/config.pylightapi/lightapi.pylightapi/rate_limiter.pylightapi/yaml_loader.pytests/test_auth.pytests/test_login_auth.pytests/test_yaml_config.py
Comment on lines
+18
to
+23
| _state: dict[str, object | None] = { | ||
| "registry": None, | ||
| "metadata": None, | ||
| "engine": None, | ||
| "login_validator": None, | ||
| } |
There was a problem hiding this comment.
A single global login_validator breaks multi-app isolation.
LightApi.__init__ stores the validator in _state, while BasicAuthentication.authenticate() reads it back from the same singleton at request time. If two LightApi instances in one process configure different validators, the later one overwrites the earlier one for both apps; the same leakage can happen between tests.
Also applies to: 51-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightapi/_registry.py` around lines 18 - 23, The global
_state["login_validator"] leaks validators across LightApi instances; change the
design to store the validator per-app and read it from the app/registry context
instead of the singleton. Specifically: remove usage of the global
login_validator entry in _state, have LightApi.__init__ attach the validator to
the instance-specific registry/metadata object (or keyed by registry id inside
_state["registry"]) and update BasicAuthentication.authenticate to fetch the
validator from the incoming request's associated registry/metadata (or the
registry-id keyed entry) rather than reading _state["login_validator"]; update
any helper code that reads/writes _state to use the per-registry slot so
multiple apps/tests don’t overwrite each other.
lightapi/rate_limiter.py
Outdated
Comment on lines
+130
to
+144
| def get_rate_limit_response(self, request: Request) -> JSONResponse: | ||
| """Get standard rate limit exceeded response.""" | ||
| return JSONResponse( | ||
| { | ||
| "error": "rate_limit_exceeded", | ||
| "detail": "Too many requests. Please try again later.", | ||
| }, | ||
| status_code=429, | ||
| headers={ | ||
| "Retry-After": "60", # Retry after 60 seconds | ||
| "X-RateLimit-Limit": str(self.requests_per_minute), | ||
| "X-RateLimit-Remaining": "0", | ||
| "X-RateLimit-Reset": str(int(time.time() + 60)), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Return 429 headers for the window that actually overflowed.
get_rate_limit_response() always advertises the per-minute limit and a 60-second retry, even when the hourly or daily quota is what blocked the request. That will tell well-behaved clients to retry far too early.
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 130-130: Unused method argument: request
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightapi/rate_limiter.py` around lines 130 - 144, get_rate_limit_response
always returns the per-minute limit and a fixed 60s retry even when the hourly
or daily quota caused the block; update get_rate_limit_response to detect which
quota/window actually overflowed (minute/hour/day) using the rate limiter's
counters or reset timestamps (e.g., attributes like requests_per_minute,
requests_per_hour, requests_per_day and corresponding reset times or remaining
counters on self) and return headers matching that window: set Retry-After to
the actual seconds until that window resets, X-RateLimit-Limit to the correct
window limit, X-RateLimit-Remaining to the remaining for that window (usually
0), and X-RateLimit-Reset to the epoch second when that window will reset;
modify get_rate_limit_response to choose the longest blocking window when
multiple quotas are exceeded.
tests/test_auth.py
Outdated
Comment on lines
+62
to
+64
| def _login_validator(username: str, password: str): | ||
| """Test validator; always returns None (tests use _make_token for tokens).""" | ||
| return None |
There was a problem hiding this comment.
Prefix the unused params and annotate the return type.
This helper will trip Ruff (ARG001) as written, and leaving the return type implicit is unnecessary friction for mypy --strict.
Suggested cleanup
-def _login_validator(username: str, password: str):
+def _login_validator(
+ _username: str, _password: str
+) -> dict[str, object] | None:
"""Test validator; always returns None (tests use _make_token for tokens)."""
return NoneAs per coding guidelines, "Pass all ruff check linting before merge" and "Pass mypy --strict type checking before merge".
📝 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.
Suggested change
| def _login_validator(username: str, password: str): | |
| """Test validator; always returns None (tests use _make_token for tokens).""" | |
| return None | |
| def _login_validator( | |
| _username: str, _password: str | |
| ) -> dict[str, object] | None: | |
| """Test validator; always returns None (tests use _make_token for tokens).""" | |
| return None |
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 62-62: Unused function argument: username
(ARG001)
[warning] 62-62: Unused function argument: password
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_auth.py` around lines 62 - 64, The helper _login_validator
declares unused parameters and lacks a return annotation which trips Ruff ARG001
and mypy; update the signature of _login_validator to prefix the parameters with
underscores (e.g., _username, _password) to mark them intentionally unused and
add an explicit return type -> None so the function becomes def
_login_validator(_username: str, _password: str) -> None: returning None.
- Fix case-insensitive Basic auth header check - Make rate limiter app-scoped and configurable - Fix global login_validator breaking multi-app isolation - Make VALID_JWT_ALGORITHMS immutable - Validate per-endpoint jwt_algorithm before storing - Fix duplicate auth routes in register() - Fix rate limiter cleanup for hourly/daily buckets - Improve IP header trust for rate limiting - Fix rate limit response headers for correct window - Validate login validator signature at load time - Fix dict-form method auth dropping default JWT settings - Fix test helper signatures and type annotations - Update YAML documentation with jwt_algorithm - Fix circular imports and linting issues
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lightapi/auth.py (1)
22-44:⚠️ Potential issue | 🟠 Major
BaseAuthenticationandJWTAuthenticationare marked frozen in repo rules.This PR edits both class bodies directly. Please move these extensions behind a new module or adapter instead of changing the frozen implementations.
As per coding guidelines, "Do not modify lightapi/auth.py: JWTAuthentication and BaseAuthentication class bodies are FROZEN".
Also applies to: 61-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/auth.py` around lines 22 - 44, You modified frozen class bodies BaseAuthentication and JWTAuthentication in lightapi/auth.py; revert those changes and implement your behavior in a new adapter module (e.g., auth_adapter) that subclasses or wraps BaseAuthentication/JWTAuthentication instead of editing them. Create a new class (e.g., AdapterJWTAuthentication or AuthenticationAdapter) that implements authenticate(request: Request) -> bool and get_auth_error_response(request: Request) -> JSONResponse, delegate to the original JWTAuthentication/BaseAuthentication for shared logic, and update call sites to import and use the adapter class rather than the frozen classes. Ensure you reference the original symbols BaseAuthentication, JWTAuthentication, authenticate, and get_auth_error_response when locating where to delegate or replace usage.lightapi/lightapi.py (2)
593-596:⚠️ Potential issue | 🟠 MajorUse the backend's auth-error response here.
This hard-coded 401 drops backend-specific behavior. In particular, Basic auth can never emit its
WWW-Authenticatechallenge while_check_auth()returns this generic JSON body instead.🛠️ Possible fix
- if not authenticator.authenticate(request): - return JSONResponse( - {"detail": "Authentication credentials invalid."}, status_code=401 - ) + if not authenticator.authenticate(request): + return authenticator.get_auth_error_response(request)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/lightapi.py` around lines 593 - 596, Replace the hard-coded JSONResponse used when authentication fails so backend-specific behavior (e.g., WWW-Authenticate header for Basic auth) is preserved: instead of returning JSONResponse({"detail": "Authentication credentials invalid."}, status_code=401) in the block that checks authenticator.authenticate(request), call the authenticator's auth-error response helper (e.g., authenticator.auth_error_response(request) or authenticator.get_auth_error_response(request)) and return that response; if the authenticator has no such method provide a fallback that preserves headers/status from the backend (including WWW-Authenticate) rather than emitting the static JSONResponse. Ensure this change is applied where authenticator.authenticate(request) is tested (and in any related _check_auth() flow).
56-90:⚠️ Potential issue | 🟠 MajorPlease move these auth changes out of the frozen legacy entrypoint.
This file is preserved for the
__init__.pyrewrite, so adding auth-route registration, limiter wiring, and_check_auth()changes here breaks the repo contract and makes the planned migration harder.As per coding guidelines, "Do not modify lightapi/lightapi.py; it is preserved as legacy until init.py rewrite absorbs it".
Also applies to: 163-258, 553-596
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/lightapi.py` around lines 56 - 90, Revert the auth-related wiring added inside the LightAPI constructor: remove any registration of auth routes, rate-limiter setup/assignment to _auth_rate_limiter, and any modifications to or calls of _check_auth in lightapi.py; keep only storing login_validator and calling set_login_validator(login_validator) as before. Specifically undo additions that introduce or use attributes _auth_path, _auth_rate_limiter, auth-route registration code, and any new _check_auth logic in the LightAPI __init__ (constructor) so this legacy entrypoint remains unchanged; move the auth-route registration and limiter wiring into the new/init entrypoint where auth is intended to be implemented. Ensure set_login_validator and login_validator handling remain intact and nothing else related to routing or rate-limiting is wired here.
♻️ Duplicate comments (2)
lightapi/_login.py (1)
57-59:⚠️ Potential issue | 🟠 MajorMake this Basic-scheme guard case-insensitive too.
_parse_basic_header()already supportsbasic, but this pre-check still requires the exactBasicprefix. Lowercase variants fall through to body parsing and hit the wrong auth/error path.🛠️ Possible fix
auth_header = request.headers.get("Authorization") - if auth_header and auth_header.startswith("Basic "): - return _parse_basic_header(auth_header) + if auth_header: + parts = auth_header.split(None, 1) + if len(parts) == 2 and parts[0].lower() == "basic": + return _parse_basic_header(auth_header)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/_login.py` around lines 57 - 59, The Authorization header guard currently checks for auth_header.startswith("Basic ") which is case-sensitive and lets lowercase "basic " fall through; update the guard in the handler that reads request.headers.get("Authorization") to perform a case-insensitive scheme check (e.g., compare auth_header[:6].lower() == "basic " or lower()/startswith on a lowercase copy) and still call _parse_basic_header(auth_header) when matched so _parse_basic_header (which already accepts "basic") is used for any case variant.lightapi/lightapi.py (1)
183-188:⚠️ Potential issue | 🟠 MajorOnly auto-register
/authroutes when login support is actually configured.Right now any JWT/Basic backend triggers built-in login routes, a fixed limiter, and a startup failure when
login_validatoris absent. Basic auth really does need the validator, but JWT-only apps can validate externally issued tokens without it; this is also where the currentConfigurationError/429 test failures are coming from.Also applies to: 190-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/lightapi.py` around lines 183 - 188, The current check using auth_backends & {JWTAuthentication, BasicAuthentication} wrongly triggers auto-registration of /auth routes, creation of self._auth_rate_limiter, and a startup ConfigurationError even for JWT-only apps without a login_validator; change the conditional logic so /auth routes and the RateLimiter (self._auth_rate_limiter) are only created when login support is actually configured: require either a provided login_validator or the presence of BasicAuthentication (which must still raise ConfigurationError if login_validator is missing), while JWT-only backends without login_validator should skip route registration and not instantiate the limiter; update the checks around auth_backends, login_validator, and the /auth registration code paths to reflect this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightapi/auth.py`:
- Around line 202-212: The 401 response from get_auth_error_response currently
lacks the WWW-Authenticate header required for HTTP Basic challenge; update
get_auth_error_response to return the JSONResponse with a WWW-Authenticate
header (e.g. 'Basic realm="Authentication Required"') so clients/browsers
trigger the Basic auth flow—use the JSONResponse headers parameter (or
equivalent on JSONResponse) to set 'WWW-Authenticate' and keep the existing body
and 401 status code.
In `@lightapi/config.py`:
- Around line 73-75: Validate and reject reserved JWT claim names in the
configuration so invalid settings fail at startup: in the config initialization
where jwt_extra_claims and jwt_expiration are set (the attribute
jwt_extra_claims), check the provided jwt_extra_claims against the reserved
claim set used by JWTAuthentication.generate_token (e.g., "exp", "iat", "nbf",
"iss", "aud", "sub", "jti"); if any reserved names are present, raise
ConfigurationError with a clear message identifying the offending claim(s) so
the error surfaces at config time rather than causing a 500 during
JWTAuthentication.generate_token().
In `@lightapi/rate_limiter.py`:
- Around line 192-194: The wrapper coroutine currently treats the tuple returned
by _auth_rate_limiter.is_rate_limited(...) as a boolean; unpack the return into
(is_limited, window) and branch on is_limited, e.g. call
_auth_rate_limiter.get_rate_limit_response(request, window) only when is_limited
is True so the wrapped endpoint can run when not rate limited; update the
wrapper signature where it calls is_rate_limited and pass the unpacked window
into get_rate_limit_response.
In `@lightapi/yaml_loader.py`:
- Around line 436-462: The merge loop currently carries backend and jwt_* fields
forward but never merges defaults.authentication.permission, so when
meta.methods is a dict and the default permission only exists in defaults the
permission_map remains empty and _check_auth() falls back to AllowAny; fix by
adding a merged_permission variable (e.g., merged_permission = None), populate
it from the same loop by checking source.permission, and then incorporate it
into the permission_map logic before creating the Authentication object (either
by using merged_permission as a fallback when permission_map is empty or by
merging keys from merged_permission into permission_map), then pass the
resulting permission_map into Authentication(permission=...) so
defaults.authentication.permission is honored.
---
Outside diff comments:
In `@lightapi/auth.py`:
- Around line 22-44: You modified frozen class bodies BaseAuthentication and
JWTAuthentication in lightapi/auth.py; revert those changes and implement your
behavior in a new adapter module (e.g., auth_adapter) that subclasses or wraps
BaseAuthentication/JWTAuthentication instead of editing them. Create a new class
(e.g., AdapterJWTAuthentication or AuthenticationAdapter) that implements
authenticate(request: Request) -> bool and get_auth_error_response(request:
Request) -> JSONResponse, delegate to the original
JWTAuthentication/BaseAuthentication for shared logic, and update call sites to
import and use the adapter class rather than the frozen classes. Ensure you
reference the original symbols BaseAuthentication, JWTAuthentication,
authenticate, and get_auth_error_response when locating where to delegate or
replace usage.
In `@lightapi/lightapi.py`:
- Around line 593-596: Replace the hard-coded JSONResponse used when
authentication fails so backend-specific behavior (e.g., WWW-Authenticate header
for Basic auth) is preserved: instead of returning JSONResponse({"detail":
"Authentication credentials invalid."}, status_code=401) in the block that
checks authenticator.authenticate(request), call the authenticator's auth-error
response helper (e.g., authenticator.auth_error_response(request) or
authenticator.get_auth_error_response(request)) and return that response; if the
authenticator has no such method provide a fallback that preserves
headers/status from the backend (including WWW-Authenticate) rather than
emitting the static JSONResponse. Ensure this change is applied where
authenticator.authenticate(request) is tested (and in any related _check_auth()
flow).
- Around line 56-90: Revert the auth-related wiring added inside the LightAPI
constructor: remove any registration of auth routes, rate-limiter
setup/assignment to _auth_rate_limiter, and any modifications to or calls of
_check_auth in lightapi.py; keep only storing login_validator and calling
set_login_validator(login_validator) as before. Specifically undo additions that
introduce or use attributes _auth_path, _auth_rate_limiter, auth-route
registration code, and any new _check_auth logic in the LightAPI __init__
(constructor) so this legacy entrypoint remains unchanged; move the auth-route
registration and limiter wiring into the new/init entrypoint where auth is
intended to be implemented. Ensure set_login_validator and login_validator
handling remain intact and nothing else related to routing or rate-limiting is
wired here.
---
Duplicate comments:
In `@lightapi/_login.py`:
- Around line 57-59: The Authorization header guard currently checks for
auth_header.startswith("Basic ") which is case-sensitive and lets lowercase
"basic " fall through; update the guard in the handler that reads
request.headers.get("Authorization") to perform a case-insensitive scheme check
(e.g., compare auth_header[:6].lower() == "basic " or lower()/startswith on a
lowercase copy) and still call _parse_basic_header(auth_header) when matched so
_parse_basic_header (which already accepts "basic") is used for any case
variant.
In `@lightapi/lightapi.py`:
- Around line 183-188: The current check using auth_backends &
{JWTAuthentication, BasicAuthentication} wrongly triggers auto-registration of
/auth routes, creation of self._auth_rate_limiter, and a startup
ConfigurationError even for JWT-only apps without a login_validator; change the
conditional logic so /auth routes and the RateLimiter (self._auth_rate_limiter)
are only created when login support is actually configured: require either a
provided login_validator or the presence of BasicAuthentication (which must
still raise ConfigurationError if login_validator is missing), while JWT-only
backends without login_validator should skip route registration and not
instantiate the limiter; update the checks around auth_backends,
login_validator, and the /auth registration code paths to reflect this behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e212585-5b21-4714-ae79-59b3274dbf66
📒 Files selected for processing (9)
docs/examples/yaml-configuration.mdlightapi/_login.pylightapi/auth.pylightapi/config.pylightapi/lightapi.pylightapi/rate_limiter.pylightapi/yaml_loader.pytests/test_auth.pytests/test_yaml_config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/examples/yaml-configuration.md
lightapi/rate_limiter.py
Outdated
Comment on lines
+192
to
+194
| async def wrapper(request: Request, *args: Any, **kwargs: Any) -> Any: | ||
| if _auth_rate_limiter.is_rate_limited(request, endpoint="auth"): | ||
| return _auth_rate_limiter.get_rate_limit_response(request) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
result = (False, None)
print(f"bool({result!r}) = {bool(result)}")
PYRepository: iklobato/lightapi
Length of output: 87
🏁 Script executed:
# First, find the file and check its size
fd -t f "rate_limiter.py" lightapi
# Get line count to determine how to read it
wc -l lightapi/rate_limiter.py 2>/dev/null || echo "File not found"Repository: iklobato/lightapi
Length of output: 114
🏁 Script executed:
# Search for is_rate_limited method definition and the decorator usage around lines 192-194
rg "def is_rate_limited|rate_limit_auth_endpoint" lightapi/rate_limiter.py -B 2 -A 10Repository: iklobato/lightapi
Length of output: 662
🏁 Script executed:
# Get the exact lines 192-194 and surrounding context
sed -n '185,205p' lightapi/rate_limiter.pyRepository: iklobato/lightapi
Length of output: 480
🏁 Script executed:
# Search for all usages of is_rate_limited to understand the return type
rg "is_rate_limited\(" lightapi/rate_limiter.py -B 1 -A 1Repository: iklobato/lightapi
Length of output: 359
🏁 Script executed:
# Check if there are other usages of is_rate_limited that might show the correct pattern
rg "is_rate_limited" lightapi/rate_limiter.py -B 2 -A 2
# Also check if there are type hints or tests that might show the intended usage
fd -t f "test" lightapi/ --name "*rate*"Repository: iklobato/lightapi
Length of output: 739
🏁 Script executed:
# Let's also see the full method to understand the return value structure
sed -n '67,95p' lightapi/rate_limiter.pyRepository: iklobato/lightapi
Length of output: 1038
🏁 Script executed:
# Let's see the full is_rate_limited method implementation and its return statement
sed -n '68,110p' lightapi/rate_limiter.pyRepository: iklobato/lightapi
Length of output: 1411
🏁 Script executed:
# Also check what get_rate_limit_response returns
rg "def get_rate_limit_response" lightapi/rate_limiter.py -A 10Repository: iklobato/lightapi
Length of output: 479
🏁 Script executed:
# Let's check if is_rate_limited is used elsewhere and how
rg "is_rate_limited" . -A 2 -B 2Repository: iklobato/lightapi
Length of output: 1517
🏁 Script executed:
# Also verify the exact line numbers around 192-194 once more to be absolutely sure
sed -n '190,196p' lightapi/rate_limiter.pyRepository: iklobato/lightapi
Length of output: 341
Unpack is_rate_limited() before branching.
is_rate_limited() returns tuple[bool, str | None], and a non-empty tuple is always truthy in Python—even (False, None) evaluates to True. As written, the condition is always true, so the decorated endpoint always returns 429 and the function never executes.
Unpack the tuple and branch on the first element, following the pattern used in lightapi/_login.py:
is_limited, window = _auth_rate_limiter.is_rate_limited(request, endpoint="auth")
if is_limited:
return _auth_rate_limiter.get_rate_limit_response(request, window)Note: The current call to get_rate_limit_response() also omits the window parameter that should be passed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightapi/rate_limiter.py` around lines 192 - 194, The wrapper coroutine
currently treats the tuple returned by _auth_rate_limiter.is_rate_limited(...)
as a boolean; unpack the return into (is_limited, window) and branch on
is_limited, e.g. call _auth_rate_limiter.get_rate_limit_response(request,
window) only when is_limited is True so the wrapped endpoint can run when not
rate limited; update the wrapper signature where it calls is_rate_limited and
pass the unpacked window into get_rate_limit_response.
- Add WWW-Authenticate header to Basic auth 401 response - Add login_validator parameter to BasicAuthentication constructor - Fix case-insensitive Basic auth header check - Reject reserved claims in jwt_extra_claims at config time - Fix rate limiter by removing unused global decorator - Fix YAML dict-method permission merging - Update imports and fix linting issues
iklobato
force-pushed
the
feat/improve-authentication-jwt-and-basic
branch
from
4b9e6d4 to
563976d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
lightapi/rate_limiter.py (2)
83-90:⚠️ Potential issue | 🟠 MajorDon't fall back to forwarded headers without explicit proxy trust.
If
request.clientis unavailable, these headers become the bucket key even though they are client-controlled unless the app has trusted-proxy handling. That still leaves an auth-rate-limit bypass by rotating header values. Safer default is to keep a fixed fallback here and only parse forwarded headers behind an explicittrust_proxy/trusted_proxiessetting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/rate_limiter.py` around lines 83 - 90, The code falls back to client-controlled forwarded headers when request.client is missing; change get-client-ip logic to only parse X-Forwarded-For/X-Real-IP/X-Client-IP when an explicit trust setting is enabled. Add/consume a trust_proxy (bool) or trusted_proxies (list) flag on the rate limiter or calling context, check request.client first, and if missing only inspect forwarded headers when trust_proxy is true (or the parsed IP is validated against trusted_proxies); otherwise keep the fixed fallback (e.g., "0.0.0.0"). Update the logic around request.client and request.headers to gate header parsing on trust_proxy/trusted_proxies.
118-130:⚠️ Potential issue | 🟡 MinorReturn the window that actually keeps the client blocked.
is_rate_limited()stops at the first exceeded window, andget_rate_limit_response()replies with a full 60/3600/86400-second delay from “now”. If minute and hour/day are exhausted together, the response can tell clients to retry at the wrong time. Pick the window with the latest next-available timestamp and deriveRetry-After/X-RateLimit-Resetfrom that bucket’s stored timestamps.Also applies to: 139-167
🧹 Nitpick comments (2)
lightapi/auth.py (1)
187-190: Avoid catching bareException.Catching bare
Exceptionmasks all errors including programming bugs. Consider catching specific exceptions that the validator might raise, or at minimum re-raise unexpected exceptions after logging.Suggested approach
try: payload = validator(username, password) - except Exception: + except (ValueError, TypeError, LookupError) as exc: + # Log the error for debugging if needed return FalseAlternatively, if the validator contract is well-defined, document what exceptions it may raise and catch only those.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/auth.py` around lines 187 - 190, The try/except around calling validator(username, password) is catching bare Exception which masks programming errors; update the block in which validator is invoked (the call that assigns payload) to catch only the specific exceptions the validator can raise (e.g., ValidationError or AuthError) or catch Exception as e, log the unexpected error, and re-raise it—ensure you reference the validator function and the variables username, password and payload so the handler only swallows known validation errors and does not hide other exceptions.lightapi/yaml_loader.py (1)
113-117: Silently returning uninspectable callables may cause runtime failures.When
inspect.signature(fn)raisesValueErrorfor builtins or C extensions, the function returnsfnwithout validating arity. If such a callable has wrong arity, the first login attempt will fail with a confusing error instead of failing at config time.Consider either documenting this limitation or raising a warning/error when signature inspection fails.
Suggested approach
try: sig = inspect.signature(fn) except ValueError: # Some callables (e.g., builtins) don't have inspectable signatures - return fn + import warnings + warnings.warn( + f"Cannot validate signature of login_validator '{dotted_path}'. " + "Ensure it accepts (username, password) arguments.", + stacklevel=2, + ) + return fn🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/yaml_loader.py` around lines 113 - 117, The code currently swallows inspect.signature(fn) ValueError and returns fn, which defers arity errors to runtime; change this to fail-fast by replacing the bare return fn in the except ValueError block with a clear failure or warning: either raise a descriptive TypeError/ValueError (including the callable name and expected arity) so config loading fails immediately, or emit warnings.warn(...) with the same descriptive message so the user is alerted at load time; reference inspect.signature(fn), the except ValueError handler, and the current return fn to locate and update the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightapi/auth.py`:
- Line 2: Remove the unused symbol Any from the typing import in
lightapi/auth.py to resolve the F401 lint error; update the import line that
currently reads "from typing import Any, Dict, Optional" to only import the
actually used names (Dict and Optional) so the file no longer imports unused
Any.
In `@lightapi/rate_limiter.py`:
- Around line 139-140: The get_rate_limit_response function currently has an
unused parameter named request which triggers Ruff ARG002; rename the parameter
to _request (i.e., change the signature in get_rate_limit_response to use
_request: Request) to mark it as intentionally unused, and update any internal
references (there should be none) and call sites if they rely on named
args—otherwise no further changes are needed.
- Around line 29-34: The in-memory per-process counters in RateLimiter (the
_store attribute in rate_limiter.py) are unsuitable for auth throttling because
each worker gets its own RateLimiter instance, multiplying attacker budgets;
replace the process-local _store with a shared backend (e.g., Redis) and use
atomic/increment and expiry operations for windowed counters, or explicitly
document and enforce single-process use. Concretely: modify RateLimiter to
accept a pluggable storage client (e.g., redis_client) instead of _store,
implement increment/get/cleanup via Redis commands (INCR, EXPIRE, HINCRBY/HGET,
or Lua for multi-key atomicity) and update the code paths that instantiate
RateLimiter so they pass a shared redis client; alternatively, if you cannot add
shared storage now, add clear docs and runtime guard in RateLimiter to refuse
use unless explicitly enabled for single-process mode.
- Around line 118-135: The loop over windows currently updates per-window
counters as it iterates, so a request that is later blocked still increments
earlier window buckets (see the loop using for window_name, limit in windows and
the writes to self._store[client_ip][window_key][current_time]); change the
logic to first check all windows for violations without mutating state
(calculate counts for each window using self._store reads), and only if no
window would block, perform the writes for all relevant window_keys (i.e., move
the self._store[...] assignment out of the initial loop and execute it once
after all checks pass); this prevents rejected requests from consuming earlier
buckets and preserves correct reporting of the blocking window.
---
Duplicate comments:
In `@lightapi/rate_limiter.py`:
- Around line 83-90: The code falls back to client-controlled forwarded headers
when request.client is missing; change get-client-ip logic to only parse
X-Forwarded-For/X-Real-IP/X-Client-IP when an explicit trust setting is enabled.
Add/consume a trust_proxy (bool) or trusted_proxies (list) flag on the rate
limiter or calling context, check request.client first, and if missing only
inspect forwarded headers when trust_proxy is true (or the parsed IP is
validated against trusted_proxies); otherwise keep the fixed fallback (e.g.,
"0.0.0.0"). Update the logic around request.client and request.headers to gate
header parsing on trust_proxy/trusted_proxies.
---
Nitpick comments:
In `@lightapi/auth.py`:
- Around line 187-190: The try/except around calling validator(username,
password) is catching bare Exception which masks programming errors; update the
block in which validator is invoked (the call that assigns payload) to catch
only the specific exceptions the validator can raise (e.g., ValidationError or
AuthError) or catch Exception as e, log the unexpected error, and re-raise
it—ensure you reference the validator function and the variables username,
password and payload so the handler only swallows known validation errors and
does not hide other exceptions.
In `@lightapi/yaml_loader.py`:
- Around line 113-117: The code currently swallows inspect.signature(fn)
ValueError and returns fn, which defers arity errors to runtime; change this to
fail-fast by replacing the bare return fn in the except ValueError block with a
clear failure or warning: either raise a descriptive TypeError/ValueError
(including the callable name and expected arity) so config loading fails
immediately, or emit warnings.warn(...) with the same descriptive message so the
user is alerted at load time; reference inspect.signature(fn), the except
ValueError handler, and the current return fn to locate and update the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bdab05a-3741-468f-889c-d4ba1cddf54a
📒 Files selected for processing (4)
lightapi/auth.pylightapi/config.pylightapi/rate_limiter.pylightapi/yaml_loader.py
| @@ -1,10 +1,11 @@ | |||
| from datetime import datetime, timedelta | |||
| from typing import Dict, Optional | |||
| from typing import Any, Dict, Optional | |||
There was a problem hiding this comment.
Remove unused import to fix pipeline failure.
Any is imported but not used anywhere in the file, causing the F401 linting error in the pipeline.
Proposed fix
-from typing import Any, Dict, Optional
+from typing import Dict, Optional📝 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.
Suggested change
| from typing import Any, Dict, Optional | |
| from typing import Dict, Optional |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightapi/auth.py` at line 2, Remove the unused symbol Any from the typing
import in lightapi/auth.py to resolve the F401 lint error; update the import
line that currently reads "from typing import Any, Dict, Optional" to only
import the actually used names (Dict and Optional) so the file no longer imports
unused Any.
Comment on lines
+118
to
+135
| for window_name, limit in windows: | ||
| window_seconds = self._get_window_seconds(window_name) | ||
| window_key = f"{endpoint}:{window_name}" if endpoint else window_name | ||
|
|
||
| # Count requests in this window | ||
| count = 0 | ||
| for timestamp, request_count in self._store[client_ip][window_key].items(): | ||
| if current_time - timestamp < window_seconds: | ||
| count += request_count | ||
|
|
||
| if count >= limit: | ||
| # Don't count this request since it's being blocked | ||
| return (True, window_name) | ||
|
|
||
| # Add current request | ||
| self._store[client_ip][window_key][current_time] = ( | ||
| self._store[client_ip][window_key].get(current_time, 0) + 1 | ||
| ) |
There was a problem hiding this comment.
Blocked requests are still consuming earlier buckets.
The method increments minute before checking hour, and increments minute/hour before checking day. So a request rejected by a later window has already been recorded in earlier ones, which burns quota for traffic that never ran and can change which window gets reported next.
Suggested fix
- for window_name, limit in windows:
+ window_keys: list[tuple[str, str]] = []
+ blocked_window: str | None = None
+
+ for window_name, limit in windows:
window_seconds = self._get_window_seconds(window_name)
window_key = f"{endpoint}:{window_name}" if endpoint else window_name
+ window_keys.append((window_name, window_key))
# Count requests in this window
count = 0
for timestamp, request_count in self._store[client_ip][window_key].items():
if current_time - timestamp < window_seconds:
count += request_count
if count >= limit:
- # Don't count this request since it's being blocked
- return (True, window_name)
-
- # Add current request
- self._store[client_ip][window_key][current_time] = (
- self._store[client_ip][window_key].get(current_time, 0) + 1
- )
+ blocked_window = window_name
+ break
+
+ if blocked_window is not None:
+ return (True, blocked_window)
+
+ for _, window_key in window_keys:
+ self._store[client_ip][window_key][current_time] = (
+ self._store[client_ip][window_key].get(current_time, 0) + 1
+ )
return (False, None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightapi/rate_limiter.py` around lines 118 - 135, The loop over windows
currently updates per-window counters as it iterates, so a request that is later
blocked still increments earlier window buckets (see the loop using for
window_name, limit in windows and the writes to
self._store[client_ip][window_key][current_time]); change the logic to first
check all windows for violations without mutating state (calculate counts for
each window using self._store reads), and only if no window would block, perform
the writes for all relevant window_keys (i.e., move the self._store[...]
assignment out of the initial loop and execute it once after all checks pass);
this prevents rejected requests from consuming earlier buckets and preserves
correct reporting of the blocking window.
Comment on lines
+139
to
+140
| def get_rate_limit_response( | ||
| self, request: Request, window: str = "minute" |
There was a problem hiding this comment.
Rename the unused request argument to satisfy Ruff.
request is never read here, so ARG002 will keep this file from meeting the repo’s lint gate. Rename it to _request or drop it if you can change the call sites. As per coding guidelines "Pass all ruff check linting before merge".
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 140-140: Unused method argument: request
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightapi/rate_limiter.py` around lines 139 - 140, The get_rate_limit_response
function currently has an unused parameter named request which triggers Ruff
ARG002; rename the parameter to _request (i.e., change the signature in
get_rate_limit_response to use _request: Request) to mark it as intentionally
unused, and update any internal references (there should be none) and call sites
if they rely on named args—otherwise no further changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lightapi/auth.py (1)
1-163:⚠️ Potential issue | 🔴 CriticalCritical:
JWTAuthenticationclass is missing, breaking imports in dependent modules.The class is imported by:
lightapi/lightapi.pylightapi/_login.py- Multiple test files
Per the coding guidelines,
JWTAuthenticationclass is FROZEN and must not be removed. It must be restored to fix the build failure.Additionally, the following issues must be resolved before merge:
Dead code: Lines 109–141 contain unreachable code after the return statement on line 107. Remove the old basic auth parsing logic that duplicates the
authenticatemethod.Ruff linting failures (8 errors, all must pass):
- ARG001/ARG002: Remove unused
self/requestparameters inBaseAuthentication.authenticate()(line 20), module-levelget_auth_error_response()(line 33), andAllowAny.has_permission()(line 147).- F811: Remove duplicate
from lightapi._registry import get_login_validatorat lines 80 and 126; use the module-level import (line 8).- BLE001: Replace blind
except Exceptionat lines 88 and 134 with specific exception types.Structural issue: The module-level
get_auth_error_response()function (lines 33–47) is defined outside any class but has aselfparameter. Either move it into a class or remove theselfparameter.
♻️ Duplicate comments (1)
lightapi/rate_limiter.py (1)
118-135:⚠️ Potential issue | 🟠 MajorBlocked requests still consume earlier buckets.
The loop increments counters for earlier windows (e.g.,
minute) before checking later windows (e.g.,hour,day). If a request is blocked by thehourlimit, it has already been counted in theminutebucket, burning quota for traffic that never executed.Suggested fix: check all windows first, then increment
+ # First pass: check all windows without incrementing + window_keys: list[tuple[str, str, int]] = [] for window_name, limit in windows: window_seconds = self._get_window_seconds(window_name) window_key = f"{endpoint}:{window_name}" if endpoint else window_name + window_keys.append((window_name, window_key, window_seconds)) # Count requests in this window count = 0 - for timestamp, request_count in self._store[client_ip][window_key].items(): + for ts, req_count in self._store[client_ip][window_key].items(): - if current_time - timestamp < window_seconds: - count += request_count + if current_time - ts < window_seconds: + count += req_count if count >= limit: - # Don't count this request since it's being blocked return (True, window_name) - # Add current request - self._store[client_ip][window_key][current_time] = ( - self._store[client_ip][window_key].get(current_time, 0) + 1 - ) + # Second pass: increment all windows (only reached if not blocked) + for _, window_key, _ in window_keys: + self._store[client_ip][window_key][current_time] = ( + self._store[client_ip][window_key].get(current_time, 0) + 1 + ) return (False, None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/rate_limiter.py` around lines 118 - 135, The code currently counts and increments each window inside the for loop (using windows, _get_window_seconds, window_key, self._store[client_ip][window_key]) which can burn quota in earlier buckets even if a later window blocks the request; change the logic in the method to first iterate over all windows to compute counts and determine if any window would block (collecting counts per window but not mutating self._store), returning (True, window_name) immediately if any limit is exceeded, and only if all windows pass perform a second pass to increment self._store[client_ip][window_key][current_time] for each window (preserve use of current_time, client_ip, endpoint and existing keys).
🧹 Nitpick comments (2)
lightapi/config.py (1)
77-77: Consider extractingRESERVED_CLAIMSas a class constant.The
RESERVED_CLAIMSset is defined inline within__init__. If this set is needed elsewhere (e.g., for documentation or other validation), consider promoting it to a class-level constant onAuthenticationor_Config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/config.py` at line 77, Move the inline RESERVED_CLAIMS set out of the Authentication.__init__ and promote it to a class-level constant (e.g., Authentication.RESERVED_CLAIMS or _Config.RESERVED_CLAIMS) so it’s reusable; update any references in __init__ and other methods to use the new class constant name (RESERVED_CLAIMS) instead of the local variable, and ensure the constant is documented with a brief docstring/comment near the class definition.lightapi/auth.py (1)
20-20: Consider renaming unusedrequestparameter inBaseAuthentication.authenticate.The base implementation doesn't use
request, which triggers Ruff ARG002. Rename to_requestto indicate intentional non-use.- def authenticate(self, request: Request) -> bool: + def authenticate(self, _request: Request) -> bool:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lightapi/auth.py` at line 20, Rename the unused parameter in BaseAuthentication.authenticate from request to _request to signal intentional non-use and satisfy Ruff ARG002; update the method signature in the BaseAuthentication class (authenticate) accordingly and ensure any internal references are adjusted (there should be none), leaving subclass interfaces unchanged if they override authenticate with a used request parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lightapi/auth.py`:
- Line 80: Remove the redundant local import of get_login_validator in
lightapi/auth.py (the duplicate import shown in the diff) — rely on the
module-level import already present (get_login_validator) instead; delete the
local "from lightapi._registry import get_login_validator" and verify no local
variable or function redefines get_login_validator to avoid the Ruff F811
redefinition error.
- Around line 33-47: Remove the incorrect module-level function
get_auth_error_response that accepts a self parameter (it's
misplaced/duplicated), and ensure the class method
BasicAuthentication.get_auth_error_response is the single implementation; update
BasicAuthentication.get_auth_error_response to add the header
{"WWW-Authenticate": 'Basic realm="Restricted Area"'} to its JSONResponse so
behavior is consistent, and delete the standalone/duplicate module-level
get_auth_error_response definition.
- Around line 109-141: Remove the duplicated Basic auth block (the second copy
starting with auth_header = request.headers.get("Authorization") through the
final return True) so only the original authentication logic remains; locate and
delete the redundant code in the auth function where the block repeats the same
decode/validator/payload steps (search for the duplicate sequence of auth_header
extraction, base64 decoding, username/password split, get_login_validator and
request.state.user assignment) and leave the first, correct implementation
intact.
---
Duplicate comments:
In `@lightapi/rate_limiter.py`:
- Around line 118-135: The code currently counts and increments each window
inside the for loop (using windows, _get_window_seconds, window_key,
self._store[client_ip][window_key]) which can burn quota in earlier buckets even
if a later window blocks the request; change the logic in the method to first
iterate over all windows to compute counts and determine if any window would
block (collecting counts per window but not mutating self._store), returning
(True, window_name) immediately if any limit is exceeded, and only if all
windows pass perform a second pass to increment
self._store[client_ip][window_key][current_time] for each window (preserve use
of current_time, client_ip, endpoint and existing keys).
---
Nitpick comments:
In `@lightapi/auth.py`:
- Line 20: Rename the unused parameter in BaseAuthentication.authenticate from
request to _request to signal intentional non-use and satisfy Ruff ARG002;
update the method signature in the BaseAuthentication class (authenticate)
accordingly and ensure any internal references are adjusted (there should be
none), leaving subclass interfaces unchanged if they override authenticate with
a used request parameter.
In `@lightapi/config.py`:
- Line 77: Move the inline RESERVED_CLAIMS set out of the
Authentication.__init__ and promote it to a class-level constant (e.g.,
Authentication.RESERVED_CLAIMS or _Config.RESERVED_CLAIMS) so it’s reusable;
update any references in __init__ and other methods to use the new class
constant name (RESERVED_CLAIMS) instead of the local variable, and ensure the
constant is documented with a brief docstring/comment near the class definition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5178939c-6682-469a-bd85-c867c0a2748d
📒 Files selected for processing (4)
lightapi/auth.pylightapi/config.pylightapi/rate_limiter.pylightapi/yaml_loader.py
Comment on lines
+33
to
+47
| def get_auth_error_response(self, request: Request) -> JSONResponse: | ||
| """ | ||
| Get the response to return when authentication fails. | ||
|
|
||
| Args: | ||
| request: The HTTP request object. | ||
|
|
||
| class JWTAuthentication(BaseAuthentication): | ||
| Returns: | ||
| Response object for authentication error. | ||
| """ | ||
| JWT (JSON Web Token) based authentication. | ||
| return JSONResponse( | ||
| {"error": "authentication failed"}, | ||
| status_code=401, | ||
| headers={"WWW-Authenticate": 'Basic realm="Restricted Area"'}, | ||
| ) |
There was a problem hiding this comment.
Function get_auth_error_response has incorrect signature and placement.
This is defined as a module-level function but has a self parameter, which makes no sense. It should either be:
- A method inside
BasicAuthentication(removing the duplicate at lines 97-107), or - A standalone function without
self
Additionally, this function includes WWW-Authenticate: Basic header, but there's another get_auth_error_response method in BasicAuthentication (lines 97-107) that does NOT include this header. This inconsistency needs to be resolved.
Suggested fix: Remove the module-level function
-def get_auth_error_response(self, request: Request) -> JSONResponse:
- """
- Get the response to return when authentication fails.
-
- Args:
- request: The HTTP request object.
-
- Returns:
- Response object for authentication error.
- """
- return JSONResponse(
- {"error": "authentication failed"},
- status_code=401,
- headers={"WWW-Authenticate": 'Basic realm="Restricted Area"'},
- )Then update BasicAuthentication.get_auth_error_response to include the header:
def get_auth_error_response(self, request: Request) -> JSONResponse:
- return JSONResponse({"error": "authentication failed"}, status_code=401)
+ return JSONResponse(
+ {"error": "authentication failed"},
+ status_code=401,
+ headers={"WWW-Authenticate": 'Basic realm="lightapi"'},
+ )🧰 Tools
🪛 Ruff (0.15.5)
[warning] 33-33: Unused function argument: self
(ARG001)
[warning] 33-33: Unused function argument: request
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightapi/auth.py` around lines 33 - 47, Remove the incorrect module-level
function get_auth_error_response that accepts a self parameter (it's
misplaced/duplicated), and ensure the class method
BasicAuthentication.get_auth_error_response is the single implementation; update
BasicAuthentication.get_auth_error_response to add the header
{"WWW-Authenticate": 'Basic realm="Restricted Area"'} to its JSONResponse so
behavior is consistent, and delete the standalone/duplicate module-level
get_auth_error_response definition.
| return False | ||
|
|
||
| username, password = credentials | ||
| from lightapi._registry import get_login_validator |
There was a problem hiding this comment.
Remove redundant import of get_login_validator.
get_login_validator is already imported at the module level (line 8). The local import at line 80 triggers Ruff F811 (redefinition).
username, password = credentials
- from lightapi._registry import get_login_validator
-
validator = self.login_validator or get_login_validator()🧰 Tools
🪛 Ruff (0.15.5)
[error] 80-80: Redefinition of unused get_login_validator from line 8: get_login_validator redefined here
(F811)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightapi/auth.py` at line 80, Remove the redundant local import of
get_login_validator in lightapi/auth.py (the duplicate import shown in the diff)
— rely on the module-level import already present (get_login_validator) instead;
delete the local "from lightapi._registry import get_login_validator" and verify
no local variable or function redefines get_login_validator to avoid the Ruff
F811 redefinition error.
Comment on lines
+109
to
+141
| auth_header = request.headers.get("Authorization") | ||
| if not auth_header or not auth_header.lower().startswith("basic "): | ||
| return False | ||
|
|
||
| Returns: | ||
| dict: The decoded token payload. | ||
| try: | ||
| import base64 | ||
|
|
||
| Raises: | ||
| jwt.InvalidTokenError: If the token is invalid or expired. | ||
| """ | ||
| return jwt.decode(token, self.secret_key, algorithms=[self.algorithm]) | ||
| token = auth_header.split(" ", 1)[1] | ||
| decoded = base64.b64decode(token).decode("utf-8") | ||
| except (ValueError, IndexError, UnicodeDecodeError): | ||
| return False | ||
|
|
||
| parts = decoded.split(":", 1) | ||
| if len(parts) != 2: | ||
| return False | ||
|
|
||
| username, password = parts[0], parts[1] | ||
| from lightapi._registry import get_login_validator | ||
|
|
||
| validator = get_login_validator() | ||
| if validator is None: | ||
| return False | ||
|
|
||
| try: | ||
| payload = validator(username, password) | ||
| except Exception: | ||
| return False | ||
|
|
||
| if payload is None: | ||
| return False | ||
|
|
||
| request.state.user = payload | ||
| return True |
There was a problem hiding this comment.
Dead code after return statement.
Lines 109-141 contain unreachable code that duplicates the authentication logic already present in lines 64-95. This appears to be leftover from a merge conflict or copy-paste error.
Remove the dead code
return JSONResponse({"error": "authentication failed"}, status_code=401)
-
- auth_header = request.headers.get("Authorization")
- if not auth_header or not auth_header.lower().startswith("basic "):
- return False
-
- try:
- import base64
-
- token = auth_header.split(" ", 1)[1]
- decoded = base64.b64decode(token).decode("utf-8")
- except (ValueError, IndexError, UnicodeDecodeError):
- return False
-
- parts = decoded.split(":", 1)
- if len(parts) != 2:
- return False
-
- username, password = parts[0], parts[1]
- from lightapi._registry import get_login_validator
-
- validator = get_login_validator()
- if validator is None:
- return False
-
- try:
- payload = validator(username, password)
- except Exception:
- return False
-
- if payload is None:
- return False
-
- request.state.user = payload
- return True🧰 Tools
🪛 Ruff (0.15.5)
[error] 126-126: Redefinition of unused get_login_validator from line 8: get_login_validator redefined here
(F811)
[warning] 134-134: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lightapi/auth.py` around lines 109 - 141, Remove the duplicated Basic auth
block (the second copy starting with auth_header =
request.headers.get("Authorization") through the final return True) so only the
original authentication logic remains; locate and delete the redundant code in
the auth function where the block repeats the same decode/validator/payload
steps (search for the duplicate sequence of auth_header extraction, base64
decoding, username/password split, get_login_validator and request.state.user
assignment) and leave the first, correct implementation intact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE: JWTAuthentication constructor now accepts optional parameters:
Security enhancements:
Summary by CodeRabbit
New Features
Documentation
Tests