-
Notifications
You must be signed in to change notification settings - Fork 0
Require Clerk JWKS outside DEV_MODE so auth cannot fall back to dev tokens #47
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
Open
MrTig-afk
wants to merge
3
commits into
main
Choose a base branch
from
fix/require-clerk-in-production
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| """Production must never fall back to the dev HMAC token. | ||
|
|
||
| api/auth.py has two auth paths: real Clerk JWTs (CLERK_JWKS_URL) and dev-login | ||
| HMAC tokens. Previously, an unconfigured production silently used the second | ||
| one, making SESSION_SECRET the only thing protecting a venue dashboard. | ||
|
|
||
| The fix is a refusal inside _verify_token, not a refusal to start: this module | ||
| is imported by every router, so a hard failure here would also take down the | ||
| patron game routes, which never use Clerk at all. A missing JWKS URL therefore | ||
| warns loudly and breaks owner and admin login only. | ||
|
|
||
| The import checks run in a subprocess so import-time behaviour is observed | ||
| cleanly, without touching the suite's already-imported api.auth. | ||
| """ | ||
| import os | ||
| import subprocess | ||
| import sys | ||
|
|
||
| import pytest | ||
| from fastapi import HTTPException | ||
|
|
||
| import api.auth | ||
|
|
||
|
|
||
| REPO_ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) | ||
|
|
||
|
|
||
| def _import_auth_with(env_overrides): | ||
| """Import api.auth in a clean subprocess. Returns (returncode, stderr).""" | ||
| env = {**os.environ, **env_overrides} | ||
| for key, value in env_overrides.items(): | ||
| if value is None: | ||
| env.pop(key, None) | ||
| proc = subprocess.run( | ||
| [sys.executable, "-c", "import api.auth"], | ||
| cwd=REPO_ROOT, env=env, capture_output=True, text=True, | ||
| ) | ||
| return proc.returncode, proc.stderr | ||
|
|
||
|
|
||
| def test_production_without_clerk_jwks_warns_but_still_boots(): | ||
| """A missing JWKS URL must be loud, but must not take the app down. | ||
|
|
||
| api.auth is imported by every router, so raising here would kill the patron | ||
| game routes, which never touch Clerk. The security guarantee is enforced in | ||
| _verify_token instead (see the HMAC test below), not by refusing to start. | ||
| """ | ||
| code, stderr = _import_auth_with({"DEV_MODE": "false", "CLERK_JWKS_URL": None}) | ||
| assert code == 0, f"api.auth refused to import, taking patron routes with it:\n{stderr}" | ||
| assert "CLERK_JWKS_URL is not set" in stderr | ||
|
|
||
|
|
||
| def test_production_with_clerk_jwks_boots(): | ||
| code, stderr = _import_auth_with({ | ||
| "DEV_MODE": "false", | ||
| "CLERK_JWKS_URL": "https://example.test/.well-known/jwks.json", | ||
| }) | ||
| assert code == 0, f"api.auth failed to import with CLERK_JWKS_URL set:\n{stderr}" | ||
|
|
||
|
|
||
| def test_dev_mode_without_clerk_jwks_still_boots(): | ||
| code, stderr = _import_auth_with({"DEV_MODE": "true", "CLERK_JWKS_URL": None}) | ||
| assert code == 0, f"api.auth failed to import in DEV_MODE:\n{stderr}" | ||
|
|
||
|
|
||
| def test_hmac_token_is_rejected_outside_dev_mode(monkeypatch): | ||
| """A validly-signed dev token must not authenticate when DEV_MODE is off.""" | ||
| token = api.auth.issue_dev_token("user_whatever") | ||
| assert api.auth._verify_token(token) == "user_whatever" # DEV_MODE on: accepted | ||
|
|
||
| monkeypatch.setenv("DEV_MODE", "false") | ||
| with pytest.raises(HTTPException) as exc: | ||
| api.auth._verify_token(token) | ||
| assert exc.value.status_code == 401 | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Restore the backend CI contract.
subprocess.runat Line 29 triggers the code-exec guard. The backend CI fails before the test suite can pass. Replace this isolation method with an allowed mechanism, or add a path-specific exception forapi/tests/test_auth_production_guard.py. Do not exclude the fullapi/test tree from the guard.🧰 Tools
🪛 ast-grep (0.45.2)
[error] 28-31: Command coming from incoming request
Context: subprocess.run(
[sys.executable, "-c", "import api.auth"],
cwd=REPO_ROOT, env=env, capture_output=True, text=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 GitHub Actions: CI / 2_backend.txt
[error] 24-29: Security guard test detected subprocess usage in the scanned api/ directory: subprocess.run(). Exclude this test file or adjust the scan to avoid flagging its intentional subprocess checks.
🪛 GitHub Actions: CI / backend
[error] 24-29: Code-exec guard failed: forbidden subprocess usage detected in the test file (subprocess documentation and subprocess.run call).
🪛 Ruff (0.16.3)
[warning] 29-29:
subprocess.runwithout explicitcheckargumentAdd explicit
check=False(PLW1510)
🤖 Prompt for AI Agents
Source: Pipeline failures