fix: address final review findings for foundation plan
- C1: drop module-level app singleton in app/main.py so importing the package no longer validates Settings or creates DATA_DIR; run uvicorn with --factory in the Dockerfile. pytest now collects and passes with no ambient env vars. - I2: add missing app/auth, app/security, app/web __init__.py so setuptools discovers all five packages. - I3: resolve the Jinja2 template directory relative to __file__ instead of the process CWD. - I4: add .gitignore covering .env, data/, .venv/, caches and build artifacts so example deployment secrets cannot be committed. - I5: assert UserRepository.list_enabled() excludes disabled users. - M6: encode both operands before hmac.compare_digest in validate_csrf so a non-ASCII token yields 403 instead of an unhandled 500. - M9: remove unused relationship / HealthState imports. - M11: make session cookie https_only configurable via SESSION_HTTPS_ONLY (default unchanged: false). - M13: dispose SQLAlchemy engines in the db_session and client fixtures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -5,3 +5,5 @@ CREDENTIAL_ENCRYPTION_KEY=replace-with-a-valid-fernet-key
|
|||||||
SYNC_INTERVAL_MINUTES=5
|
SYNC_INTERVAL_MINUTES=5
|
||||||
DATA_DIR=/data
|
DATA_DIR=/data
|
||||||
DATABASE_URL=sqlite:////data/app.db
|
DATABASE_URL=sqlite:////data/app.db
|
||||||
|
# Set to true only when TLS terminates in front of this service.
|
||||||
|
SESSION_HTTPS_ONLY=false
|
||||||
|
|||||||
18
.gitignore
vendored
Normal file
18
.gitignore
vendored
Normal file
@@ -0,0 +1,18 @@
|
|||||||
|
# Local secrets and runtime data
|
||||||
|
.env
|
||||||
|
data/
|
||||||
|
|
||||||
|
# Virtual environments
|
||||||
|
.venv/
|
||||||
|
venv/
|
||||||
|
|
||||||
|
# Python bytecode and build artifacts
|
||||||
|
__pycache__/
|
||||||
|
*.pyc
|
||||||
|
*.pyo
|
||||||
|
*.egg-info/
|
||||||
|
build/
|
||||||
|
dist/
|
||||||
|
|
||||||
|
# Tooling caches
|
||||||
|
.pytest_cache/
|
||||||
@@ -7,4 +7,4 @@ COPY app /app/app
|
|||||||
RUN mkdir -p /data && chmod 700 /data
|
RUN mkdir -p /data && chmod 700 /data
|
||||||
ENV DATA_DIR=/data
|
ENV DATA_DIR=/data
|
||||||
EXPOSE 8080
|
EXPOSE 8080
|
||||||
CMD ["uvicorn", "app.main:app", "--host", "0.0.0.0", "--port", "8080"]
|
CMD ["uvicorn", "app.main:create_app", "--factory", "--host", "0.0.0.0", "--port", "8080"]
|
||||||
|
|||||||
0
app/auth/__init__.py
Normal file
0
app/auth/__init__.py
Normal file
@@ -14,5 +14,7 @@ def ensure_csrf_token(request: Request) -> str:
|
|||||||
|
|
||||||
def validate_csrf(request: Request, submitted_token: str) -> None:
|
def validate_csrf(request: Request, submitted_token: str) -> None:
|
||||||
expected = request.session.get("csrf_token")
|
expected = request.session.get("csrf_token")
|
||||||
if not isinstance(expected, str) or not hmac.compare_digest(expected, submitted_token):
|
if not isinstance(expected, str) or not hmac.compare_digest(
|
||||||
|
expected.encode("utf-8"), submitted_token.encode("utf-8")
|
||||||
|
):
|
||||||
raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Invalid CSRF token")
|
raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Invalid CSRF token")
|
||||||
|
|||||||
@@ -14,6 +14,7 @@ class Settings(BaseSettings):
|
|||||||
data_dir: Path = Field(default=Path("/data"), alias="DATA_DIR")
|
data_dir: Path = Field(default=Path("/data"), alias="DATA_DIR")
|
||||||
database_url: str | None = Field(default=None, alias="DATABASE_URL")
|
database_url: str | None = Field(default=None, alias="DATABASE_URL")
|
||||||
sync_interval_minutes: PositiveInt = Field(default=5, alias="SYNC_INTERVAL_MINUTES")
|
sync_interval_minutes: PositiveInt = Field(default=5, alias="SYNC_INTERVAL_MINUTES")
|
||||||
|
session_https_only: bool = Field(default=False, alias="SESSION_HTTPS_ONLY")
|
||||||
|
|
||||||
@model_validator(mode="after")
|
@model_validator(mode="after")
|
||||||
def derive_paths(self) -> "Settings":
|
def derive_paths(self) -> "Settings":
|
||||||
|
|||||||
@@ -4,7 +4,7 @@ import enum
|
|||||||
from datetime import datetime, timezone
|
from datetime import datetime, timezone
|
||||||
|
|
||||||
from sqlalchemy import Boolean, DateTime, Enum, ForeignKey, Integer, String, Text, UniqueConstraint
|
from sqlalchemy import Boolean, DateTime, Enum, ForeignKey, Integer, String, Text, UniqueConstraint
|
||||||
from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column, relationship
|
from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column
|
||||||
|
|
||||||
|
|
||||||
def utcnow() -> datetime:
|
def utcnow() -> datetime:
|
||||||
|
|||||||
@@ -4,7 +4,7 @@ from sqlalchemy import select
|
|||||||
from sqlalchemy.exc import IntegrityError
|
from sqlalchemy.exc import IntegrityError
|
||||||
from sqlalchemy.orm import Session
|
from sqlalchemy.orm import Session
|
||||||
|
|
||||||
from app.db.models import Activity, ActivityStatus, HealthState, SyncUser
|
from app.db.models import Activity, ActivityStatus, SyncUser
|
||||||
|
|
||||||
|
|
||||||
class UserRepository:
|
class UserRepository:
|
||||||
|
|||||||
@@ -24,7 +24,7 @@ def create_app(settings: Settings | None = None) -> FastAPI:
|
|||||||
SessionMiddleware,
|
SessionMiddleware,
|
||||||
secret_key=resolved.secret_key,
|
secret_key=resolved.secret_key,
|
||||||
same_site="lax",
|
same_site="lax",
|
||||||
https_only=False,
|
https_only=resolved.session_https_only,
|
||||||
)
|
)
|
||||||
app.include_router(web_router)
|
app.include_router(web_router)
|
||||||
|
|
||||||
@@ -33,6 +33,3 @@ def create_app(settings: Settings | None = None) -> FastAPI:
|
|||||||
return {"status": "ok"}
|
return {"status": "ok"}
|
||||||
|
|
||||||
return app
|
return app
|
||||||
|
|
||||||
|
|
||||||
app = create_app()
|
|
||||||
|
|||||||
0
app/security/__init__.py
Normal file
0
app/security/__init__.py
Normal file
0
app/web/__init__.py
Normal file
0
app/web/__init__.py
Normal file
@@ -1,3 +1,5 @@
|
|||||||
|
from pathlib import Path
|
||||||
|
|
||||||
from fastapi import APIRouter, Form, HTTPException, Request, status
|
from fastapi import APIRouter, Form, HTTPException, Request, status
|
||||||
from fastapi.responses import HTMLResponse, RedirectResponse
|
from fastapi.responses import HTMLResponse, RedirectResponse
|
||||||
from fastapi.templating import Jinja2Templates
|
from fastapi.templating import Jinja2Templates
|
||||||
@@ -10,7 +12,7 @@ from app.security.credentials import CredentialCipher
|
|||||||
from app.web.forms import UserFormData
|
from app.web.forms import UserFormData
|
||||||
|
|
||||||
router = APIRouter()
|
router = APIRouter()
|
||||||
templates = Jinja2Templates(directory="app/web/templates")
|
templates = Jinja2Templates(directory=str(Path(__file__).resolve().parent / "templates"))
|
||||||
|
|
||||||
|
|
||||||
def _get_user_or_404(repository: UserRepository, user_id: int) -> SyncUser:
|
def _get_user_or_404(repository: UserRepository, user_id: int) -> SyncUser:
|
||||||
|
|||||||
@@ -22,8 +22,11 @@ def db_session() -> Session:
|
|||||||
)
|
)
|
||||||
Base.metadata.create_all(engine)
|
Base.metadata.create_all(engine)
|
||||||
factory = sessionmaker(bind=engine, expire_on_commit=False)
|
factory = sessionmaker(bind=engine, expire_on_commit=False)
|
||||||
with factory() as session:
|
try:
|
||||||
yield session
|
with factory() as session:
|
||||||
|
yield session
|
||||||
|
finally:
|
||||||
|
engine.dispose()
|
||||||
|
|
||||||
|
|
||||||
@pytest.fixture
|
@pytest.fixture
|
||||||
@@ -46,4 +49,8 @@ def client(tmp_path: Path) -> TestClient:
|
|||||||
DATABASE_URL=f"sqlite:///{tmp_path / 'app.db'}",
|
DATABASE_URL=f"sqlite:///{tmp_path / 'app.db'}",
|
||||||
SYNC_INTERVAL_MINUTES=5,
|
SYNC_INTERVAL_MINUTES=5,
|
||||||
)
|
)
|
||||||
return TestClient(create_app(settings))
|
app = create_app(settings)
|
||||||
|
try:
|
||||||
|
yield TestClient(app)
|
||||||
|
finally:
|
||||||
|
app.state.db_engine.dispose()
|
||||||
|
|||||||
@@ -21,8 +21,20 @@ def test_create_two_independent_users(db_session, user_repository) -> None:
|
|||||||
garmin_password_enc="g-pw-2",
|
garmin_password_enc="g-pw-2",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
disabled = user_repository.create(
|
||||||
|
name="Paused",
|
||||||
|
enabled=False,
|
||||||
|
health_state=HealthState.DISABLED,
|
||||||
|
mywhoosh_email_enc="mw-3",
|
||||||
|
mywhoosh_password_enc="mw-pw-3",
|
||||||
|
garmin_email_enc="g-3",
|
||||||
|
garmin_password_enc="g-pw-3",
|
||||||
|
)
|
||||||
|
|
||||||
assert first.id != second.id
|
assert first.id != second.id
|
||||||
assert {u.name for u in user_repository.list_enabled()} == {"Max", "Anna"}
|
assert {u.name for u in user_repository.list_enabled()} == {"Max", "Anna"}
|
||||||
|
assert disabled.id not in {u.id for u in user_repository.list_enabled()}
|
||||||
|
assert {u.name for u in user_repository.list_all()} == {"Max", "Anna", "Paused"}
|
||||||
|
|
||||||
|
|
||||||
def test_activity_external_id_is_unique_per_user(user_repository, activity_repository) -> None:
|
def test_activity_external_id_is_unique_per_user(user_repository, activity_repository) -> None:
|
||||||
|
|||||||
@@ -225,6 +225,23 @@ def test_create_user_rejects_invalid_csrf(client: TestClient) -> None:
|
|||||||
assert response.status_code == 403
|
assert response.status_code == 403
|
||||||
|
|
||||||
|
|
||||||
|
def test_create_user_rejects_non_ascii_csrf_token(client: TestClient) -> None:
|
||||||
|
login(client)
|
||||||
|
response = client.post(
|
||||||
|
"/users",
|
||||||
|
data={
|
||||||
|
"csrf_token": "invalid-tokeü",
|
||||||
|
"name": "Max",
|
||||||
|
"mywhoosh_email": "max@example.com",
|
||||||
|
"mywhoosh_password": "mw-secret",
|
||||||
|
"garmin_email": "max-garmin@example.com",
|
||||||
|
"garmin_password": "garmin-secret",
|
||||||
|
"enabled": "on",
|
||||||
|
},
|
||||||
|
)
|
||||||
|
assert response.status_code == 403
|
||||||
|
|
||||||
|
|
||||||
def test_dashboard_lists_created_user_without_secrets(client: TestClient) -> None:
|
def test_dashboard_lists_created_user_without_secrets(client: TestClient) -> None:
|
||||||
login(client)
|
login(client)
|
||||||
create_user_via_http(client)
|
create_user_via_http(client)
|
||||||
|
|||||||
Reference in New Issue
Block a user