From 6a967051bada88cf66cf2df4edbabc215d41b588 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 7 Mar 2026 13:18:33 +0000 Subject: [PATCH] fix: address code review feedback - Use astimezone() instead of replace() for timezone conversion in is_token_expired - Log cleanup exceptions with logger.exception() in signup - Add security warning when STRIPE_WEBHOOK_SECRET is not configured - Increase Stripe price ID column length from 64 to 128 characters - Replace alert() with aria-live assertive region in pricing.html - Convert auth() login tests to use pytest.mark.asyncio and await Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- app/api/billing.py | 5 ++++ app/api/local_auth.py | 1 + app/models.py | 4 ++-- app/utils/local_auth.py | 2 +- frontend/templates/pricing.html | 15 ++++++++++-- .../018_add_local_users_and_billing.py | 4 ++-- tests/test_local_auth.py | 24 +++++++++---------- 7 files changed, 36 insertions(+), 19 deletions(-) diff --git a/app/api/billing.py b/app/api/billing.py index c44dd074..6fa2f6b1 100644 --- a/app/api/billing.py +++ b/app/api/billing.py @@ -238,6 +238,11 @@ async def stripe_webhook(request: Request, db: Session = Depends(get_db)) -> dic if settings.stripe_webhook_secret: event = stripe.Webhook.construct_event(payload, sig_header, settings.stripe_webhook_secret) else: + logger.warning( + "[SECURITY] STRIPE_WEBHOOK_SECRET is not configured. " + "Webhook events are accepted without signature verification. " + "Set STRIPE_WEBHOOK_SECRET in production to prevent spoofed events." + ) event = stripe.Event.construct_from(json.loads(payload), stripe.api_key) except stripe.SignatureVerificationError: logger.warning("[SECURITY] Stripe webhook signature verification failed") diff --git a/app/api/local_auth.py b/app/api/local_auth.py index a74d0a33..162c07d1 100644 --- a/app/api/local_auth.py +++ b/app/api/local_auth.py @@ -187,6 +187,7 @@ async def signup(request: Request, body: SignupBody, db: DbSession) -> dict[str, db.commit() except Exception: db.rollback() + logger.exception("Failed to clean up orphan records for %s after email send failure", body.email) logger.warning("Signup email failed for %s: %s", body.email, exc) raise HTTPException( status_code=status.HTTP_503_SERVICE_UNAVAILABLE, diff --git a/app/models.py b/app/models.py index a81d93ba..9f0c9cec 100644 --- a/app/models.py +++ b/app/models.py @@ -288,8 +288,8 @@ class SubscriptionPlan(Base): sort_order = Column(Integer, nullable=False, default=0) features = Column(Text, nullable=True) # JSON-encoded list[str] api_access = Column(Boolean, nullable=False, default=False) - stripe_price_id_monthly = Column(String(64), nullable=True) - stripe_price_id_yearly = Column(String(64), nullable=True) + stripe_price_id_monthly = Column(String(128), nullable=True) + stripe_price_id_yearly = Column(String(128), nullable=True) created_at = Column(DateTime(timezone=True), server_default=func.now()) updated_at = Column(DateTime(timezone=True), server_default=func.now(), onupdate=func.now()) diff --git a/app/utils/local_auth.py b/app/utils/local_auth.py index ce52de2f..669d6819 100644 --- a/app/utils/local_auth.py +++ b/app/utils/local_auth.py @@ -46,7 +46,7 @@ def is_token_expired(sent_at: datetime | None) -> bool: """Return True when *sent_at* is None or older than TOKEN_EXPIRY_HOURS.""" if sent_at is None: return True - return datetime.now(tz=timezone.utc) > sent_at.replace(tzinfo=timezone.utc) + timedelta(hours=TOKEN_EXPIRY_HOURS) + return datetime.now(tz=timezone.utc) > sent_at.astimezone(timezone.utc) + timedelta(hours=TOKEN_EXPIRY_HOURS) def _smtp_send(subject: str, html_body: str, plain_body: str, recipient: str) -> None: diff --git a/frontend/templates/pricing.html b/frontend/templates/pricing.html index 4764e64f..17ff6e06 100644 --- a/frontend/templates/pricing.html +++ b/frontend/templates/pricing.html @@ -397,6 +397,10 @@ async function startCheckout(planId) { const cycleEl = document.querySelector('[data-billing-cycle]'); const billingCycle = (cycleEl && cycleEl.dataset.billingCycle) ? cycleEl.dataset.billingCycle : 'monthly'; + // Clear previous error + const errEl = document.getElementById('checkout-error'); + if (errEl) { errEl.textContent = ''; errEl.hidden = true; } + try { const resp = await fetch('/api/billing/create-checkout-session', { method: 'POST', @@ -415,10 +419,17 @@ async function startCheckout(planId) { } } const data = await resp.json().catch(() => ({})); - alert(data.detail || 'Unable to start checkout. Please try again.'); + const msg = data.detail || 'Unable to start checkout. Please try again.'; + if (errEl) { errEl.textContent = msg; errEl.hidden = false; } } catch (e) { - alert('Network error. Please try again.'); + if (errEl) { errEl.textContent = 'Network error. Please try again.'; errEl.hidden = false; } } } + {% endblock %} diff --git a/migrations/versions/018_add_local_users_and_billing.py b/migrations/versions/018_add_local_users_and_billing.py index b226a0d3..91e2dafb 100644 --- a/migrations/versions/018_add_local_users_and_billing.py +++ b/migrations/versions/018_add_local_users_and_billing.py @@ -37,8 +37,8 @@ def upgrade() -> None: sa.UniqueConstraint("username"), ) op.add_column("user_profiles", sa.Column("stripe_customer_id", sa.String(64), nullable=True)) - op.add_column("subscription_plans", sa.Column("stripe_price_id_monthly", sa.String(64), nullable=True)) - op.add_column("subscription_plans", sa.Column("stripe_price_id_yearly", sa.String(64), nullable=True)) + op.add_column("subscription_plans", sa.Column("stripe_price_id_monthly", sa.String(128), nullable=True)) + op.add_column("subscription_plans", sa.Column("stripe_price_id_yearly", sa.String(128), nullable=True)) def downgrade() -> None: diff --git a/tests/test_local_auth.py b/tests/test_local_auth.py index e2bcf518..502e445e 100644 --- a/tests/test_local_auth.py +++ b/tests/test_local_auth.py @@ -550,9 +550,9 @@ def test_reset_password_page(la_client): @pytest.mark.unit -def test_local_login_success(la_session, active_user): +@pytest.mark.asyncio +async def test_local_login_success(la_session, active_user): """auth() with valid LocalUser credentials sets session and redirects.""" - import asyncio from unittest.mock import AsyncMock, MagicMock from fastapi import Request @@ -563,16 +563,16 @@ def test_local_login_success(la_session, active_user): mock_request.form = AsyncMock(return_value={"username": "activeuser", "password": "password123"}) mock_request.session = {} - result = asyncio.get_event_loop().run_until_complete(auth(mock_request, db=la_session)) + result = await auth(mock_request, db=la_session) assert result.status_code == 302 assert "user" in mock_request.session assert mock_request.session["user"]["email"] == "active@example.com" @pytest.mark.unit -def test_local_login_by_email(la_session, active_user): +@pytest.mark.asyncio +async def test_local_login_by_email(la_session, active_user): """auth() accepts email as username for LocalUser lookup.""" - import asyncio from unittest.mock import AsyncMock, MagicMock from fastapi import Request @@ -583,15 +583,15 @@ def test_local_login_by_email(la_session, active_user): mock_request.form = AsyncMock(return_value={"username": "active@example.com", "password": "password123"}) mock_request.session = {} - result = asyncio.get_event_loop().run_until_complete(auth(mock_request, db=la_session)) + result = await auth(mock_request, db=la_session) assert result.status_code == 302 assert "user" in mock_request.session @pytest.mark.unit -def test_local_login_wrong_password(la_session, active_user): +@pytest.mark.asyncio +async def test_local_login_wrong_password(la_session, active_user): """auth() with wrong password redirects to login with error.""" - import asyncio from unittest.mock import AsyncMock, MagicMock from fastapi import Request @@ -602,16 +602,16 @@ def test_local_login_wrong_password(la_session, active_user): mock_request.form = AsyncMock(return_value={"username": "activeuser", "password": "wrongpassword"}) mock_request.session = {} - result = asyncio.get_event_loop().run_until_complete(auth(mock_request, db=la_session)) + result = await auth(mock_request, db=la_session) assert result.status_code == 302 assert "/login" in result.headers["location"] assert "user" not in mock_request.session @pytest.mark.unit -def test_local_login_unverified(la_session, pending_user): +@pytest.mark.asyncio +async def test_local_login_unverified(la_session, pending_user): """auth() for unverified user redirects with verification message.""" - import asyncio from unittest.mock import AsyncMock, MagicMock from fastapi import Request @@ -622,6 +622,6 @@ def test_local_login_unverified(la_session, pending_user): mock_request.form = AsyncMock(return_value={"username": "pendinguser", "password": "password123"}) mock_request.session = {} - result = asyncio.get_event_loop().run_until_complete(auth(mock_request, db=la_session)) + result = await auth(mock_request, db=la_session) assert result.status_code == 302 assert "verify" in result.headers["location"].lower()