-
Notifications
You must be signed in to change notification settings - Fork 5
fix: auth hardening -- domain exceptions + null user_id safety (OPE-76, OPE-77) #259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
DevanshuNEU
merged 4 commits into
OpenCodeIntel:main
from
DevanshuNEU:fix/auth-hardening
Feb 24, 2026
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
25edad1
fix: auth hardening -- domain exceptions + null user_id safety (OPE-7…
DevanshuNEU 3c8003a
fix: review findings -- get_current_user catches domain exceptions, e…
DevanshuNEU 9e6d2c1
fix: review findings -- jwt alias, explicit None check, test cleanup
DevanshuNEU 38e0e40
fix: add missing exception chaining in _verify_via_api
DevanshuNEU File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| """ | ||
| Domain exceptions for authentication. | ||
|
|
||
| Services raise these; the middleware/route layer translates them to HTTP responses. | ||
| This decouples business logic from the HTTP framework. | ||
| """ | ||
|
|
||
|
|
||
| class AuthenticationError(Exception): | ||
| """Base auth error. All auth exceptions inherit from this.""" | ||
| pass | ||
|
|
||
|
|
||
| class TokenExpiredError(AuthenticationError): | ||
| """JWT token has expired.""" | ||
| pass | ||
|
|
||
|
|
||
| class InvalidTokenError(AuthenticationError): | ||
| """JWT token is malformed, wrong signature, or invalid audience.""" | ||
| pass | ||
|
|
||
|
|
||
| class TokenMissingClaimError(AuthenticationError): | ||
| """JWT token is missing a required claim (e.g., sub).""" | ||
|
|
||
| def __init__(self, claim: str = "sub"): | ||
| super().__init__(f"Token missing required claim: {claim}") | ||
| self.claim = claim | ||
|
|
||
|
|
||
| class InvalidCredentialsError(AuthenticationError): | ||
| """Login failed due to wrong email/password.""" | ||
| pass | ||
|
|
||
|
|
||
| class SignupError(AuthenticationError): | ||
| """User registration failed.""" | ||
| pass | ||
|
|
||
|
|
||
| class SessionError(AuthenticationError): | ||
| """Token refresh or logout failed.""" | ||
| pass | ||
|
|
||
|
|
||
| class UserIdRequiredError(AuthenticationError): | ||
| """Operation requires a user_id but auth context has None (e.g., API key without user).""" | ||
| pass |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| """Tests for auth hardening -- domain exceptions + null safety (OPE-76, OPE-77).""" | ||
| import pytest | ||
| from unittest.mock import patch, MagicMock | ||
| from fastapi import HTTPException | ||
|
|
||
|
|
||
| class TestNullUserIdSafety: | ||
| """API key users (no user_id) get 401, not confusing 404.""" | ||
|
|
||
| def test_search_with_null_user_id_returns_401(self, client, valid_headers): | ||
| """Search should reject None user_id via the real verify_repo_access null guard.""" | ||
| from fastapi.testclient import TestClient | ||
| from main import app | ||
| from middleware.auth import require_auth, AuthContext | ||
|
|
||
| # Mock auth to return a context with no user_id (API key user) | ||
| async def mock_auth_no_user(): | ||
| return AuthContext(api_key_name="test-key", user_id=None) | ||
|
|
||
| app.dependency_overrides[require_auth] = mock_auth_no_user | ||
| try: | ||
| resp = client.post( | ||
| "/api/v1/search", | ||
| json={"query": "auth", "repo_id": "test"}, | ||
| headers=valid_headers, | ||
| ) | ||
| # verify_repo_access in dependencies.py should catch None user_id | ||
| assert resp.status_code == 401 | ||
| assert "User ID required" in resp.json()["detail"] | ||
| finally: | ||
| app.dependency_overrides.pop(require_auth, None) | ||
|
|
||
| def test_get_repo_or_404_rejects_none_user_id(self): | ||
| """get_repo_or_404 should raise 401 when user_id is None.""" | ||
| from dependencies import get_repo_or_404 | ||
| from fastapi import HTTPException | ||
|
|
||
| with pytest.raises(HTTPException) as exc: | ||
| get_repo_or_404("some-repo", None) | ||
| assert exc.value.status_code == 401 | ||
|
|
||
| def test_verify_repo_access_rejects_none_user_id(self): | ||
| """verify_repo_access should raise 401 when user_id is None.""" | ||
| from dependencies import verify_repo_access | ||
| from fastapi import HTTPException | ||
|
|
||
| with pytest.raises(HTTPException) as exc: | ||
| verify_repo_access("some-repo", None) | ||
| assert exc.value.status_code == 401 | ||
|
|
||
|
|
||
| class TestDomainExceptions: | ||
| """Auth service raises domain exceptions, not HTTPException.""" | ||
|
|
||
| def test_expired_token_raises_domain_exception(self): | ||
| """Auth service should raise TokenExpiredError, not HTTPException.""" | ||
| from services.exceptions import TokenExpiredError | ||
| assert issubclass(TokenExpiredError, Exception) | ||
| assert not issubclass(TokenExpiredError, HTTPException) | ||
|
|
||
| def test_exception_hierarchy(self): | ||
| """All auth exceptions inherit from AuthenticationError.""" | ||
| from services.exceptions import ( | ||
| AuthenticationError, | ||
| TokenExpiredError, | ||
| InvalidTokenError, | ||
| TokenMissingClaimError, | ||
| InvalidCredentialsError, | ||
| SignupError, | ||
| SessionError, | ||
| UserIdRequiredError, | ||
| ) | ||
| for exc_class in [ | ||
| TokenExpiredError, InvalidTokenError, TokenMissingClaimError, | ||
| InvalidCredentialsError, SignupError, SessionError, UserIdRequiredError, | ||
| ]: | ||
| assert issubclass(exc_class, AuthenticationError) |
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
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.
Uh oh!
There was an error while loading. Please reload this page.