Upgrade auth to RS256 signing, age-verification field, refresh-token sessions #30

2026-08-19T14:44:54Z

agent-bot commented

2026-08-19 14:44:54 +00:00

Auto-opened by Claude for task a755410a1c91 (unblocking manually after verification).

Dashboard: http://cloudbison:7070/task/a755410a1c91

Auto-opened by Claude for task `a755410a1c91` (unblocking manually after verification). Dashboard: http://cloudbison:7070/task/a755410a1c91

agent-bot added 4 commits 2026-08-19 14:44:55 +00:00

WIP checkpoint: Upgrade auth to RS256 signing, age-verification field, refresh-token sessions 98bbe67406

WIP checkpoint: Upgrade auth to RS256 signing, age-verification field, refresh-token sessions a9137f6505

WIP checkpoint: Upgrade auth to RS256 signing, age-verification field, refresh-token sessions a5cdabf6de

WIP checkpoint: Upgrade auth to RS256 signing, age-verification field, refresh-token sessions dc0cf80000

reviewer-bot commented

2026-08-19 16:01:43 +00:00

Review Summary

The PR correctly upgrades auth from HS256 to RS256, adds the date_of_birth field (via migration 0002), and introduces refresh token persistence (migration 0003 + model + router changes). The RSA key generation/loading logic is clean and the keys default to ~/.agp/keys (outside the repo).

Issues Found

1. Critical — Duplicate refresh token collision (IntegrityError risk)

_create_refresh_token only encodes {"sub": str(user_id)} with no iat, jti, or any other varying claim. Two tokens issued within the same second for the same user produce byte-identical JWTs. The unique=True index on refresh_tokens.token will cause an IntegrityError on concurrent register/login calls.

Empirically confirmed: two calls 1ms apart produce identical tokens (418 chars, a == b: True).

Fix: Add jti (JWT ID) via uuid.uuid4() to the refresh token payload:

def _create_refresh_token(data: dict) -> str:
    to_encode = data.copy()
    to_encode["jti"] = str(uuid.uuid4())  # unique per token
    ...

This makes each refresh token globally unique and enables token revocation lookups.

2. Medium — Migration/model column length mismatch

Migration 0003 uses sa.String(512) for the token column, but the model uses unbounded String. On PostgreSQL this means the migration creates VARCHAR(512) but the ORM metadata says unbounded. RS256 JWTs are ~418 chars so 512 is sufficient, but alembic check would flag the mismatch.

Fix: Either add sa.String(512) to the model's RefreshToken.token column, or change the migration to use sa.Text() to match the model's unbounded intent.

3. Low — calculate_age lost type annotations

Changed from def calculate_age(birth_date: date, today: date | None = None) -> int: to def calculate_age(birth_date, today=None) -> int:. Minor but loses type safety.

4. Low — Refresh token commit ordering

In both register and login, the user is committed first, then the refresh token is added and committed. If the refresh token insert fails (e.g., duplicate key before fix #1), the user is already committed and the access token is already issued — the client receives a refresh token that doesn't exist in the DB. This is a minor edge case but worth noting for the follow-up /auth/refresh task.

Positive Notes

Clean RSA key generation with file-based persistence outside the repo
Proper Alembic migration chain (0001 → 0002 → 0003)
cryptography dependency correctly included via python-jose[cryptography]
calculate_age validation correctly rejects users under 18
TokenResponse now includes access_token_expires_minutes for client-side expiry tracking
No lingering SECRET_KEY references — clean HS256→RS256 migration

## Review Summary The PR correctly upgrades auth from HS256 to RS256, adds the `date_of_birth` field (via migration 0002), and introduces refresh token persistence (migration 0003 + model + router changes). The RSA key generation/loading logic is clean and the keys default to `~/.agp/keys` (outside the repo). ### Issues Found **1. Critical — Duplicate refresh token collision (IntegrityError risk)** `_create_refresh_token` only encodes `{"sub": str(user_id)}` with no `iat`, `jti`, or any other varying claim. Two tokens issued within the same second for the same user produce byte-identical JWTs. The `unique=True` index on `refresh_tokens.token` will cause an `IntegrityError` on concurrent register/login calls. Empirically confirmed: two calls 1ms apart produce identical tokens (418 chars, `a == b: True`). **Fix**: Add `jti` (JWT ID) via `uuid.uuid4()` to the refresh token payload: ```python def _create_refresh_token(data: dict) -> str: to_encode = data.copy() to_encode["jti"] = str(uuid.uuid4()) # unique per token ... ``` This makes each refresh token globally unique and enables token revocation lookups. **2. Medium — Migration/model column length mismatch** Migration 0003 uses `sa.String(512)` for the `token` column, but the model uses unbounded `String`. On PostgreSQL this means the migration creates `VARCHAR(512)` but the ORM metadata says unbounded. RS256 JWTs are ~418 chars so 512 is sufficient, but `alembic check` would flag the mismatch. **Fix**: Either add `sa.String(512)` to the model's `RefreshToken.token` column, or change the migration to use `sa.Text()` to match the model's unbounded intent. **3. Low — `calculate_age` lost type annotations** Changed from `def calculate_age(birth_date: date, today: date | None = None) -> int:` to `def calculate_age(birth_date, today=None) -> int:`. Minor but loses type safety. **4. Low — Refresh token commit ordering** In both `register` and `login`, the user is committed first, then the refresh token is added and committed. If the refresh token insert fails (e.g., duplicate key before fix #1), the user is already committed and the access token is already issued — the client receives a refresh token that doesn't exist in the DB. This is a minor edge case but worth noting for the follow-up `/auth/refresh` task. ### Positive Notes - Clean RSA key generation with file-based persistence outside the repo - Proper Alembic migration chain (0001 → 0002 → 0003) - `cryptography` dependency correctly included via `python-jose[cryptography]` - `calculate_age` validation correctly rejects users under 18 - `TokenResponse` now includes `access_token_expires_minutes` for client-side expiry tracking - No lingering `SECRET_KEY` references — clean HS256→RS256 migration

agent-bot referenced this pull request

2026-08-19 20:16:32 +00:00

Address review feedback: Upgrade auth to RS256 signing, age-verification field, refresh-token sessions #33

reviewer-bot commented

2026-08-19 21:12:42 +00:00

Superseded by #33, which addressed the requested changes and merged. Closing.

reviewer-bot closed this pull request

2026-08-19 21:12:42 +00:00