Skip to content

🛡️ Sentinel: [CRITICAL] Fix missing auth on migrate-formats #14

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

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

🛡️ Sentinel: [CRITICAL] Fix missing auth on migrate-formats #14

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

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions CricketGame/backend/api/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
from fastapi import APIRouter, Depends, HTTPException
from fastapi import APIRouter, Depends, HTTPException, Header, status
from sqlalchemy.orm import Session
from pydantic import BaseModel
import secrets
import json

from ..data.database import get_db
from ..core.config import ADMIN_SECRET
from ..core.auth import register_player, login_player, get_player_stats, decode_token, create_token
from ..data.models import Player, MatchHistory, TournamentHistory, FormatStats

Expand Down Expand Up @@ -62,7 +64,15 @@ def _merge_format_stats(keeper: FormatStats, src: FormatStats) -> None:
keeper.best_bowling_runs = src.best_bowling_runs

@router.get("/migrate-formats")
def migrate_formats(db: Session = Depends(get_db)):
def migrate_formats(
x_admin_secret: str = Header(..., alias="X-Admin-Secret"),
db: Session = Depends(get_db),
):
Comment on lines 66 to +70
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

This endpoint performs a data migration (writes + commit) but is exposed as a GET. GET requests are commonly cached/retried/prefetched and are expected to be side-effect free, which increases the risk of accidental or repeated migrations. Consider switching this route to POST (or another non-GET verb) to better match HTTP semantics and reduce operational risk.

Copilot uses AI. Check for mistakes.
if not secrets.compare_digest(x_admin_secret, ADMIN_SECRET):
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN, detail="Invalid admin secret"
)

stats_merged_2v2 = 0
stats_deduped = 0
legacy_rows = db.query(FormatStats).filter(FormatStats.format == "2v2").all()
Expand Down
6 changes: 6 additions & 0 deletions CricketGame/backend/core/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
raise ValueError("SECRET_KEY is required in production")
SECRET_KEY = "cricket-dev-key-2026"

ADMIN_SECRET = os.getenv("ADMIN_SECRET")
if not ADMIN_SECRET:
if APP_ENV in ("production", "prod"):
raise ValueError("ADMIN_SECRET is required in production")
ADMIN_SECRET = "super-secret-admin-key"

Comment on lines +14 to +19
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

ADMIN_SECRET falls back to a hardcoded default value when APP_ENV is not production. If a deployment forgets to set APP_ENV=production (or uses another value like staging), the admin protection becomes a known, guessable secret. Prefer failing fast when ADMIN_SECRET is unset (in all environments), or at least generate a random per-run dev secret and log it explicitly to avoid a stable default.

Copilot uses AI. Check for mistakes.
ALGORITHM = "HS256"
ACCESS_TOKEN_EXPIRE_MINUTES = int(os.getenv("ACCESS_TOKEN_EXPIRE_MINUTES", "1440"))

Expand Down
29 changes: 29 additions & 0 deletions CricketGame/backend/test_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
from fastapi.testclient import TestClient
from backend.main import app
from backend.core.config import ADMIN_SECRET
from backend.data.database import init_db

# Initialize DB for tests
init_db()

client = TestClient(app)

def test_migrate_formats_no_header():
# Expect 422 because the header is required (Header(...))
response = client.get("/auth/migrate-formats")
assert response.status_code == 422

def test_migrate_formats_wrong_header():
# Expect 403 because the secret is invalid
response = client.get("/auth/migrate-formats", headers={"X-Admin-Secret": "wrong-secret"})
assert response.status_code == 403
assert response.json()["detail"] == "Invalid admin secret"

def test_migrate_formats_correct_header():
Comment on lines +1 to +22
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

init_db() is executed at import time and TestClient(app) is created as a module-level global. With this app, constructing the TestClient will run the FastAPI lifespan (which starts the background learning processor) and init_db() will write to the real sqlite:///./cricket.db by default. This can create/modify local files during test import and make tests flaky. Prefer creating the client inside each test (or a fixture), and configure an isolated test database (e.g., set DATABASE_URL to an in-memory/temporary SQLite URL before importing the app).

Suggested change
from fastapi.testclient import TestClient
from backend.main import app
from backend.core.config import ADMIN_SECRET
from backend.data.database import init_db
# Initialize DB for tests
init_db()
client = TestClient(app)
def test_migrate_formats_no_header():
# Expect 422 because the header is required (Header(...))
response = client.get("/auth/migrate-formats")
assert response.status_code == 422
def test_migrate_formats_wrong_header():
# Expect 403 because the secret is invalid
response = client.get("/auth/migrate-formats", headers={"X-Admin-Secret": "wrong-secret"})
assert response.status_code == 403
assert response.json()["detail"] == "Invalid admin secret"
def test_migrate_formats_correct_header():
import os
import pytest
from fastapi.testclient import TestClient
# Configure an isolated test database before importing the app
os.environ.setdefault("DATABASE_URL", "sqlite:///:memory:")
from backend.main import app
from backend.core.config import ADMIN_SECRET
from backend.data.database import init_db
@pytest.fixture
def client():
# Initialize DB for each test using the test database
init_db()
with TestClient(app) as test_client:
yield test_client
def test_migrate_formats_no_header(client):
# Expect 422 because the header is required (Header(...))
response = client.get("/auth/migrate-formats")
assert response.status_code == 422
def test_migrate_formats_wrong_header(client):
# Expect 403 because the secret is invalid
response = client.get("/auth/migrate-formats", headers={"X-Admin-Secret": "wrong-secret"})
assert response.status_code == 403
assert response.json()["detail"] == "Invalid admin secret"
def test_migrate_formats_correct_header(client):

Copilot uses AI. Check for mistakes.
# Expect 200 because the secret matches
response = client.get("/auth/migrate-formats", headers={"X-Admin-Secret": ADMIN_SECRET})
assert response.status_code == 200
data = response.json()
assert "format_stats_2v2_merged" in data
assert "duplicate_rows_removed" in data
assert "match_history_fixed" in data
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

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

This test module is written in pytest style, but the existing backend tests in this repo are standalone scripts (with run_all_tests() + __main__) and requirements.txt does not include pytest. Unless the project is switching to pytest (and adding it to dependencies/CI), these tests likely won't run in the current test workflow. Either adapt this test to the existing script pattern, or add pytest as a dependency and update the repo’s test instructions accordingly.

Suggested change
assert "match_history_fixed" in data
assert "match_history_fixed" in data
def run_all_tests():
"""
Run all tests in this module.
This mirrors the standalone script pattern used by other backend tests.
"""
test_migrate_formats_no_header()
test_migrate_formats_wrong_header()
test_migrate_formats_correct_header()
if __name__ == "__main__":
run_all_tests()

Copilot uses AI. Check for mistakes.
Binary file added cricket.db
View file
Open in desktop
Binary file not shown.