Feat(auth): Implement Signup Authentication Flow - #4
Conversation
Feat/database models
… and update response model
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a signup flow and JWT-based auth: new refresh token DB table and model, Alembic migrations, AuthService (password hashing, JWT create/verify, refresh token storage), signup API endpoint, Pydantic schemas and exceptions, config/env entries, tests, and CI/dev docs tweaks. ChangesUser Signup and Authentication
Sequence DiagramsequenceDiagram
actor User
participant Client
participant API as "API (/auth/signup)"
participant AuthSvc as "AuthService"
participant DB as Database
participant Resp as "Response Handler"
User->>Client: Submit signup (name, email, password)
Client->>API: POST /auth/signup
API->>AuthSvc: check_email_exists(email)
AuthSvc->>DB: SELECT user WHERE email=...
DB-->>AuthSvc: user or none
alt email exists
AuthSvc-->>API: raise UserAlreadyExistsException
API-->>Resp: HTTP 400 ErrorResponse
Resp-->>Client: 400
else email unique
API->>AuthSvc: create_user(name,email,password)
AuthSvc->>AuthSvc: hash_password(password)
AuthSvc->>DB: INSERT user
DB-->>AuthSvc: saved user
API->>AuthSvc: create_access_token(user)
AuthSvc-->>API: access_token (JWT)
API->>AuthSvc: create_refresh_token(user)
AuthSvc->>AuthSvc: generate raw token, hash
AuthSvc->>DB: INSERT refresh_token (hash, expires_at)
DB-->>AuthSvc: saved refresh token
API->>Resp: set HttpOnly cookies (access, refresh)
API-->>Client: 201 SignupResponse (user + tokens)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a user signup flow, introducing a new authentication service, database migrations for refresh tokens, and a dedicated signup endpoint. The implementation includes password hashing with bcrypt, JWT-based token generation, and custom exception handling for API robustness. Feedback focuses on improving the reliability and security of the new code. Key recommendations include logging exceptions for better observability, hardening exception handlers against potential indexing or type errors, and replacing brittle regex-based XSS prevention with standard sanitization libraries. Additionally, the reviewer suggests using db.flush() instead of db.commit() within service methods to ensure transactional atomicity and improving type hints for better static analysis.
| except UserAlreadyExistsException as e: | ||
| raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) | ||
|
|
||
| except Exception as e: |
There was a problem hiding this comment.
Catching all exceptions and returning a generic 500 error is appropriate for production security to avoid leaking internal details. However, the actual exception should be logged here for observability. Without logging, it will be nearly impossible to diagnose unexpected failures in the signup flow in a production environment.
| status_code=exc.status_code, | ||
| content=ErrorResponse( | ||
| status_code=exc.status_code, | ||
| message=exc.detail |
There was a problem hiding this comment.
The exc.detail attribute in FastAPI's HTTPException can be a string, a list, or a dictionary. Since the ErrorResponse schema explicitly expects a str for the message field, passing a non-string detail will cause a validation error within this exception handler, resulting in an unhandled 500 error. Explicitly casting to str ensures robustness.
| message=exc.detail | |
| message=str(exc.detail) |
| @app.exception_handler(RequestValidationError) | ||
| async def validation_exception_handler(request: Request, exc: RequestValidationError): | ||
| errors = exc.errors() | ||
| first_error = errors[0] |
There was a problem hiding this comment.
Accessing errors[0] without verifying that the errors list is non-empty could lead to an IndexError. While Pydantic validation errors usually contain details, defensive programming here prevents the exception handler from crashing if an unexpected validation state occurs.
| first_error = errors[0] | |
| first_error = errors[0] if errors else {"loc": ["body"], "msg": "Unknown validation error"} |
| if re.search(r'<[^>]+>', v): | ||
| raise ValueError('Name contains unsafe characters') |
There was a problem hiding this comment.
The regex r'<[^>]+>' is a brittle and incomplete method for preventing XSS. It can be easily bypassed (e.g., using javascript: URIs or event handlers like onmouseover) and may also block legitimate user input. Per security best practices, XSS should be mitigated by proper output escaping or by using a dedicated sanitization library like bleach if HTML input is required.
| @@ -0,0 +1,89 @@ | |||
| import hashlib | |||
There was a problem hiding this comment.
The uuid module needs to be imported to support the suggested type hint improvement for user_id in the create_refresh_token method.
| import hashlib | |
| import uuid | |
| import hashlib |
| await db.commit() | ||
| await db.refresh(user) |
There was a problem hiding this comment.
Performing db.commit() inside service methods breaks transactional atomicity. If the signup flow fails after create_user succeeds (e.g., during token generation), the user will remain in the database in an inconsistent state. It is better to use db.flush() to persist changes to the session and let the endpoint handler perform a single db.commit() for the entire unit of work.
| await db.commit() | |
| await db.refresh(user) | |
| await db.flush() |
| return jwt.decode(token, settings.JWT_SECRET, algorithms=[settings.JWT_ALGORITHM]) | ||
|
|
||
| @staticmethod | ||
| async def create_refresh_token(db: AsyncSession, user_id: str) -> str: |
There was a problem hiding this comment.
The user_id parameter should be typed as uuid.UUID to match the database model and the actual type being passed from the endpoint. This improves static analysis and code clarity.
| async def create_refresh_token(db: AsyncSession, user_id: str) -> str: | |
| async def create_refresh_token(db: AsyncSession, user_id: uuid.UUID) -> str: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Around line 1-2: Update the .env.example so TEST_DATABASE_URL does not point
to the same production DSN as DATABASE_URL: change TEST_DATABASE_URL to
reference a separate test database (e.g., change the database name suffix to
postgres_test) and add a note in README/CONTRIBUTING calling out that
TEST_DATABASE_URL must point to a dedicated test DB to avoid destructive
operations on dev/staging; ensure the variables DATABASE_URL and
TEST_DATABASE_URL are clearly named and documented so developers won’t
accidentally reuse the production DSN.
- Around line 5-6: The .env.example uses non-integer placeholder strings for
integer settings which breaks Pydantic validation in app/core/config.py; update
ACCESS_TOKEN_EXPIRE_MINUTES and REFRESH_TOKEN_EXPIRE_MINUTES to sensible integer
placeholders (e.g., 30, 1440) or add commented examples showing integers,
ensuring the variables match the int type expected by the Config parsing so cp
.env.example .env won't fail on startup.
In `@alembic/versions/d44e91e81013_add_refresh_token_table.py`:
- Around line 26-27: The `revoked` column is currently created as nullable with
no default, leading to ambiguous NULL states; update the column definition for
`revoked` in the migration (the sa.Column call that defines 'revoked') to be
non-nullable and supply a server-side default (e.g., replace nullable=True with
nullable=False and add server_default=sa.text('false') or equivalent) so new
rows default to false and revocation filters never see NULL.
In `@app/api/v1/endpoints/auth.py`:
- Around line 21-23: Wrap the signup flow in a single DB transaction so user
creation and refresh-token insertion are atomic: call AuthService.create_user
and AuthService.create_refresh_token inside one transaction boundary (e.g.,
using the db session/transaction context) and commit only once after both
succeed, rolling back on any exception; refactor/remove internal commits from
AuthService.create_user and AuthService.create_refresh_token so they do not
commit independently and ensure AuthService.create_access_token remains pure (no
DB commit) while the endpoint controls the transaction lifecycle.
- Around line 44-53: The SignupResponse currently includes access_token and
refresh_token in SignupResponseData which exposes tokens in JSON; remove
access_token and refresh_token from the SignupResponse/SignupResponseData
payload returned by the signup endpoint (leave id, email, name only) and ensure
the tokens are set as HttpOnly secure cookies in the request handler (e.g.,
where you call create access/refresh tokens) instead of being included in the
response body so cookies remain cookie-only and XSS-resistant.
In `@app/core/config.py`:
- Around line 19-24: Update the Pydantic settings: change TEST_DATABASE_URL from
str to PostgresDsn to get DSN validation like DATABASE_URL; constrain
JWT_ALGORITHM to a fixed set of supported algorithms (use Literal[...] or an
Enum and apply it to the JWT_ALGORITHM field) so typos fail validation early;
add Field(gt=0) to ACCESS_TOKEN_EXPIRE_MINUTES and REFRESH_TOKEN_EXPIRE_MINUTES
to prevent zero/negative values; and enforce a minimum length on JWT_SECRET via
Field(min_length=32) (or constr/min_length) to ensure sufficient secret
strength; adjust imports to include PostgresDsn, Field and Literal/Enum as
needed.
In `@app/models/user.py`:
- Around line 26-28: Add a non-unique index on refresh_tokens.user_id so queries
that filter by user_id are fast: update the model where user_id is declared (the
mapped_column UUID(as_uuid=True) ForeignKey on refresh_tokens.user_id /
Mapped[uuid.UUID]) to include an index=True (or add an Index for that column),
and create a new Alembic revision that calls
op.create_index("ix_refresh_tokens_user_id", "refresh_tokens", ["user_id"]) (and
the matching op.drop_index on downgrade) rather than editing existing
migrations; leave the existing unique index on token_hash untouched.
- Around line 31-34: The schema allows NULLs for revoked and mismatches the
created_at typing: update the model so revoked is non-optional and has a
DB-level default and NOT NULL (change revoked: Mapped[bool] =
mapped_column(Boolean, nullable=False, server_default=text('false')) or
equivalent) and change created_at typing to non-optional (created_at:
Mapped[datetime] not Mapped[datetime | None]) while keeping
mapped_column(DateTime(timezone=True), server_default=func.now(),
nullable=False); also create an Alembic migration to backfill any existing NULL
revoked values, set a server_default for revoked, and alter the column to NOT
NULL in the database before deploying the model change.
In `@app/schemas/auth.py`:
- Line 8: The schema's password Field currently allows up to 255 chars which
conflicts with bcrypt's 72-byte limit; either reduce the Field max_length to 72
and update the description to reflect the bcrypt limit, or keep the longer
schema but modify AuthService.hash_password to pre-hash the plaintext with
SHA-256 (or similar fixed-length digest) and feed that digest to bcrypt so
bcrypt only ever sees a safe fixed-size input; update validation and comments
accordingly and reference the Field named password in the schema and the
AuthService.hash_password method when making the change.
In `@app/services/auth.py`:
- Around line 16-17: The _now function in app/services/auth.py is indented with
a tab which is inconsistent with the file's 4-space indentation; update the
indentation for the _now definition and its return line to use 4 spaces (fix the
leading tab before "return datetime.now(timezone.utc)") so the function
signature and body use spaces consistently.
- Around line 76-82: The create_refresh_token signature incorrectly types
user_id as str even though RefreshToken.user_id and User.id are uuid.UUID;
change the parameter annotation in create_refresh_token to user_id: uuid.UUID
(and add import uuid at the top) so callers see the correct type and static
checks align with the RefreshToken model.
- Around line 70-73: The decode_access_token function currently returns the
decoded payload without asserting the token purpose; modify
auth.decode_access_token to decode the token as it does now and then verify
payload.get("type") == "access", raising a JWTError (or appropriate
authentication exception) if the check fails so only tokens stamped with "type":
"access" are accepted by access-protected endpoints.
- Around line 38-55: In create_user, keep the fast-path check_email_exists but
wrap the db.add/commit/refresh sequence in a try/except that catches
sqlalchemy.exc.IntegrityError (or the Async DB's IntegrityError), calls await
db.rollback() and raises UserAlreadyExistsException(email=request.email); ensure
other exceptions still rollback and re-raise appropriately so the AsyncSession
is not left poisoned; reference the functions/classes: create_user,
AuthService.check_email_exists, User, and UserAlreadyExistsException.
- Around line 24-30: The bcrypt calls in hash_password and verify_password are
synchronous/CPU-bound and currently defined as async functions which will block
the event loop; change them to run the blocking work in a thread (use
asyncio.to_thread) or convert them to regular synchronous functions.
Specifically, update hash_password(password: str) and the `@staticmethod`
verify_password(password: str, hashed: str) so that bcrypt.hashpw and
bcrypt.checkpw are executed via asyncio.to_thread(...) (and import asyncio) OR
make both functions plain def and call them from sync context; ensure return
types remain str and bool respectively.
- Around line 24-26: Update hash_password and verify_password to pre-hash the
incoming password with SHA-256 before calling bcrypt so passwords of any length
are safe; specifically, import base64, compute
hashlib.sha256(password.encode('utf-8')).digest(), base64-encode that digest (or
use the raw digest) and pass the resulting bytes into
bcrypt.hashpw/bcrypt.checkpw (instead of the raw password), and ensure
verify_password mirrors the same pre-hash process before calling bcrypt.checkpw;
no schema change required.
In `@docs/architecture/system-overview.md`:
- Line 115: The Mermaid diagram currently has two merged classDef declarations
(classDef db and classDef decision) joined with a literal "\n" which prevents
proper parsing; update the diagram so each classDef is on its own line by
removing the embedded "\n" and placing "classDef db
fill:`#f0f9ff`,stroke:`#38bdf8`,color:`#0c4a6e`,stroke-dasharray: 3 2" and "classDef
decision fill:`#fff7ed`,stroke:`#fb923c`,color:`#431407`,stroke-width:1px" as separate
lines so Mermaid can apply both styles correctly.
In `@pyproject.toml`:
- Line 15: The dependency "python-jose>=3.5.0" in pyproject.toml should be
migrated to a maintained JWT library; replace that entry with an appropriate
PyJWT pin (e.g., "PyJWT>=2.8") and update any imports/usages that reference
python-jose (search for imports like "from jose import jwt" or "jose.jwt") to
the PyJWT API (standard "import jwt" and jwt.encode/decode call sites), run
tests and adjust call signatures/algorithms where needed; if full JOSE feature
parity is required, consider instead switching to "joserfc" and follow its
migration path.
- Line 14: Remove the unused synchronous Postgres driver entry
"psycopg2-binary>=2.9.12" from pyproject.toml; locate the dependency string in
the project's dependency list (the exact token "psycopg2-binary>=2.9.12") and
delete it, then run a quick search for any references to
psycopg2/psycopg2-binary in the codebase (imports or config) and remove or
replace them if present so only async drivers (asyncpg and sqlalchemy[asyncio])
remain.
In `@tests/conftest.py`:
- Around line 25-29: The fixture currently clears all dependency overrides with
app.dependency_overrides.clear() which can break other tests; instead remove
only the override you set by deleting or popping the get_session entry (the one
assigned to mock_get_session) from app.dependency_overrides in the teardown of
the fixture so other overrides remain intact—locate the assignment
app.dependency_overrides[get_session] = mock_get_session and replace the final
clear() call with a targeted removal of that key.
- Line 4: Replace the use of os.environ.setdefault for DATABASE_URL to ensure
tests always use the test DB: locate the os.environ.setdefault("DATABASE_URL",
settings.TEST_DATABASE_URL) call in tests/conftest.py and change it to
explicitly override DATABASE_URL so it always equals settings.TEST_DATABASE_URL
(i.e., assign/replace os.environ["DATABASE_URL"] with
settings.TEST_DATABASE_URL) to avoid accidentally using a non-test database
during test runs.
In `@tests/test_auth.py`:
- Around line 94-101: Update the test_cookies_set_on_success test to assert the
security attributes of the cookies, not just their presence: after the POST to
SIGNUP_URL with VALID_PAYLOAD (within the same patched
CREATE_USER/CREATE_ACCESS/CREATE_REFRESH context), inspect
response.cookies["access_token"] and response.cookies["refresh_token"] and
assert their HttpOnly flag is set, Secure is set, and SameSite equals the
expected value (e.g. "Lax" or "Strict" per app policy); keep the existing
presence assertions and add these attribute checks so regressions in cookie
flags are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bfecfed5-5526-4bf5-9823-b029030e3b9c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.env.exampleCONTRIBUTING.mdalembic/versions/8d114ef61fcc_made_datetime_fields_in_refresh_token_.pyalembic/versions/d44e91e81013_add_refresh_token_table.pyapp/api/v1/endpoints/auth.pyapp/api/v1/router.pyapp/core/config.pyapp/core/exceptions.pyapp/main.pyapp/models/user.pyapp/schemas/auth.pyapp/services/auth.pydocs/architecture/system-overview.mdpyproject.tomltests/conftest.pytests/test_auth.pytests/test_models.py
| sa.Column('revoked', sa.Boolean(), nullable=True), | ||
| sa.Column('created_at', sa.DateTime(), server_default=sa.text('now()'), nullable=False), |
There was a problem hiding this comment.
Make revocation state non-null with a default.
Line [26] defines revoked as nullable with no default, which creates ambiguous token state (NULL) and can break revocation filters.
Suggested change
- sa.Column('revoked', sa.Boolean(), nullable=True),
+ sa.Column('revoked', sa.Boolean(), server_default=sa.text('false'), nullable=False),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sa.Column('revoked', sa.Boolean(), nullable=True), | |
| sa.Column('created_at', sa.DateTime(), server_default=sa.text('now()'), nullable=False), | |
| sa.Column('revoked', sa.Boolean(), server_default=sa.text('false'), nullable=False), | |
| sa.Column('created_at', sa.DateTime(), server_default=sa.text('now()'), nullable=False), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@alembic/versions/d44e91e81013_add_refresh_token_table.py` around lines 26 -
27, The `revoked` column is currently created as nullable with no default,
leading to ambiguous NULL states; update the column definition for `revoked` in
the migration (the sa.Column call that defines 'revoked') to be non-nullable and
supply a server-side default (e.g., replace nullable=True with nullable=False
and add server_default=sa.text('false') or equivalent) so new rows default to
false and revocation filters never see NULL.
| user = await AuthService.create_user(request, db) | ||
| access_token = await AuthService.create_access_token(user) | ||
| refresh_token = await AuthService.create_refresh_token(db, user.id) |
There was a problem hiding this comment.
Signup flow is non-atomic across user and refresh-token persistence.
Line [21] and Line [23] rely on service methods that commit separately, so a refresh-token failure can leave a created user while returning 500. This creates inconsistent state and confusing retries.
Suggested direction
# Wrap user creation + refresh token insert in one DB transaction boundary,
# commit once after both succeed, rollback on any exception.
# Move commit control out of individual service methods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/v1/endpoints/auth.py` around lines 21 - 23, Wrap the signup flow in a
single DB transaction so user creation and refresh-token insertion are atomic:
call AuthService.create_user and AuthService.create_refresh_token inside one
transaction boundary (e.g., using the db session/transaction context) and commit
only once after both succeed, rolling back on any exception; refactor/remove
internal commits from AuthService.create_user and
AuthService.create_refresh_token so they do not commit independently and ensure
AuthService.create_access_token remains pure (no DB commit) while the endpoint
controls the transaction lifecycle.
| return SignupResponse( | ||
| status_code=201, | ||
| message="Account created successfully", | ||
| data=SignupResponseData( | ||
| id=str(user.id), | ||
| email=user.email, | ||
| name=user.name, | ||
| access_token=access_token, | ||
| refresh_token=refresh_token | ||
| ) |
There was a problem hiding this comment.
Do not return auth tokens in JSON when using HttpOnly cookies.
At Line [51] and Line [52], exposing access_token/refresh_token in the response body undermines the XSS-resistance benefit of HttpOnly cookies. Keep tokens cookie-only and return user data metadata only.
Suggested change
return SignupResponse(
status_code=201,
message="Account created successfully",
data=SignupResponseData(
id=str(user.id),
email=user.email,
name=user.name,
- access_token=access_token,
- refresh_token=refresh_token
)
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return SignupResponse( | |
| status_code=201, | |
| message="Account created successfully", | |
| data=SignupResponseData( | |
| id=str(user.id), | |
| email=user.email, | |
| name=user.name, | |
| access_token=access_token, | |
| refresh_token=refresh_token | |
| ) | |
| return SignupResponse( | |
| status_code=201, | |
| message="Account created successfully", | |
| data=SignupResponseData( | |
| id=str(user.id), | |
| email=user.email, | |
| name=user.name | |
| ) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/v1/endpoints/auth.py` around lines 44 - 53, The SignupResponse
currently includes access_token and refresh_token in SignupResponseData which
exposes tokens in JSON; remove access_token and refresh_token from the
SignupResponse/SignupResponseData payload returned by the signup endpoint (leave
id, email, name only) and ensure the tokens are set as HttpOnly secure cookies
in the request handler (e.g., where you call create access/refresh tokens)
instead of being included in the response body so cookies remain cookie-only and
XSS-resistant.
| "alembic>=1.14.0", | ||
| "pydantic-settings>=2.7.0", | ||
| "bcrypt>=4.0.0", | ||
| "psycopg2-binary>=2.9.12", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether psycopg2 is actually imported anywhere or only used by alembic env.py
rg -nP '\b(psycopg2|psycopg)\b' --type=py -C2
fd -i 'env.py' alembicRepository: hngprojects/meetmind-be
Length of output: 82
🏁 Script executed:
cat -n alembic/env.pyRepository: hngprojects/meetmind-be
Length of output: 2088
🏁 Script executed:
head -50 pyproject.tomlRepository: hngprojects/meetmind-be
Length of output: 655
🏁 Script executed:
rg 'DATABASE_URL' --type=py -A2 -B2Repository: hngprojects/meetmind-be
Length of output: 961
🏁 Script executed:
rg '(create_engine|Engine|connect\(\))' --type=py | head -20Repository: hngprojects/meetmind-be
Length of output: 134
Remove psycopg2-binary from dependencies.
The entire runtime stack is async-only (sqlalchemy[asyncio] + asyncpg), and Alembic also runs migrations asynchronously. psycopg2-binary is unused and adds unnecessary bloat to the production image. No synchronous PostgreSQL driver is required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyproject.toml` at line 14, Remove the unused synchronous Postgres driver
entry "psycopg2-binary>=2.9.12" from pyproject.toml; locate the dependency
string in the project's dependency list (the exact token
"psycopg2-binary>=2.9.12") and delete it, then run a quick search for any
references to psycopg2/psycopg2-binary in the codebase (imports or config) and
remove or replace them if present so only async drivers (asyncpg and
sqlalchemy[asyncio]) remain.
| "pydantic-settings>=2.7.0", | ||
| "bcrypt>=4.0.0", | ||
| "psycopg2-binary>=2.9.12", | ||
| "python-jose>=3.5.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
python-jose unmaintained CVE 2024 PyJWT replacement
💡 Result:
python-jose has faced maintenance concerns and multiple CVEs in 2024, including CVE-2024-33663 (algorithm confusion, CVSS 6.5)[1][2], CVE-2024-33664 (DoS via JWE compression, CVSS 5.3)[3][4], and indirect exposure via ecdsa dependency CVE-2024-23342[5]. These affected versions up to 3.3.0[4]. A 3.4.0 release in Feb 2025[6] and latest 3.5.0 in May 2025[7][8][9] address earlier issues, with security scanners showing no known vulnerabilities in 3.5.0[9]. However, ongoing open issues question maintenance[10][5], and projects like Okta recommend switching to PyJWT due to unmaintained status and CVEs[11][12][13][14]. Recommended replacement: PyJWT, actively maintained with migration guides available[13][15]. It's a drop-in replacement for basic JWT needs[15], used in production migrations[12][14]. For fuller JOSE support (JWE, etc.), consider joserfc (latest 1.6.3 Feb 2026[16][17]), which has migration guides from python-jose[18][19] and comprehensive RFC coverage[19]. To mitigate immediately: - Upgrade to python-jose>=3.5.0 with cryptography backend: pip install "python-jose[cryptography]" to avoid ecdsa issues[5][8]. - But prefer migrating to PyJWT: pip install pyjwt[12][13]. Example PyJWT migration (from guides[13]): # python-jose from jose import jwt payload = jwt.decode(token, key, algorithms=['RS256']) # PyJWT import jwt payload = jwt.decode(token, key, algorithms=['RS256']) PyJWT is suitable for most JWT use cases and avoids python-jose risks[11][20][15].
Citations:
- 1: https://nvd.nist.gov/vuln/detail/cve-2024-33663
- 2: https://www.vicarius.io/vsociety/posts/algorithm-confusion-in-python-jose-cve-2024-33663
- 3: https://nvd.nist.gov/vuln/detail/CVE-2024-33664
- 4: https://getsafety.com/vulnerabilities/70716
- 5: CVE-2024-23342, High level vulnerability mpdavis/python-jose#390
- 6: New version?🤔 mpdavis/python-jose#332
- 7: https://pypi.org/project/python-jose/
- 8: https://github.com/mpdavis/python-jose/tree/master/
- 9: https://security.snyk.io/package/pip/python-jose
- 10: Is python-jose still supported? mpdavis/python-jose#340
- 11: Dependency python-Jose appears to be unmaintained okta/okta-jwt-verifier-python#54
- 12: Replace
python-josewithpyjwtokta/okta-jwt-verifier-python#59 - 13: Migration guide for python-jose users jpadilla/pyjwt#942
- 14: Update to use pyjwt instead of python-jose okta/okta-jwt-verifier-python#60
- 15: https://python.plainenglish.io/python-jose-encoding-and-decoding-jwt-tokens-for-rs256-e17fd8137e37
- 16: https://pypi.org/project/joserfc/
- 17: https://github.com/authlib/joserfc/
- 18: Migrating from python-jose authlib/joserfc#25
- 19: https://jose.authlib.org/en/migrations
- 20: Fix CVE-2024-33663 mpdavis/python-jose#349
Consider migrating to PyJWT for long-term maintainability.
While python-jose 3.5.0 addresses the 2024 CVEs (CVE-2024-33663, CVE-2024-33664), the project shows ongoing maintenance concerns with open issues questioning active development. PyJWT is actively maintained, provides a drop-in replacement for basic JWT operations (jwt.encode/decode signatures are nearly identical), and eliminates the supply-chain risk. If fuller JOSE support is needed, joserfc is also maintained and offers migration guides.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyproject.toml` at line 15, The dependency "python-jose>=3.5.0" in
pyproject.toml should be migrated to a maintained JWT library; replace that
entry with an appropriate PyJWT pin (e.g., "PyJWT>=2.8") and update any
imports/usages that reference python-jose (search for imports like "from jose
import jwt" or "jose.jwt") to the PyJWT API (standard "import jwt" and
jwt.encode/decode call sites), run tests and adjust call signatures/algorithms
where needed; if full JOSE feature parity is required, consider instead
switching to "joserfc" and follow its migration path.
| "DATABASE_URL", | ||
| "postgresql+asyncpg://postgres:postgres@localhost:5432/test", | ||
| ) | ||
| os.environ.setdefault("DATABASE_URL", settings.TEST_DATABASE_URL) |
There was a problem hiding this comment.
Force test DB URL instead of using setdefault.
Line [4] can retain an existing DATABASE_URL, which risks tests running against a non-test database. In tests, this should be explicitly overridden.
Suggested change
-os.environ.setdefault("DATABASE_URL", settings.TEST_DATABASE_URL)
+os.environ["DATABASE_URL"] = settings.TEST_DATABASE_URL📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| os.environ.setdefault("DATABASE_URL", settings.TEST_DATABASE_URL) | |
| os.environ["DATABASE_URL"] = settings.TEST_DATABASE_URL |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/conftest.py` at line 4, Replace the use of os.environ.setdefault for
DATABASE_URL to ensure tests always use the test DB: locate the
os.environ.setdefault("DATABASE_URL", settings.TEST_DATABASE_URL) call in
tests/conftest.py and change it to explicitly override DATABASE_URL so it always
equals settings.TEST_DATABASE_URL (i.e., assign/replace
os.environ["DATABASE_URL"] with settings.TEST_DATABASE_URL) to avoid
accidentally using a non-test database during test runs.
| app.dependency_overrides[get_session] = mock_get_session | ||
| transport = ASGITransport(app=app) | ||
| async with AsyncClient(transport=transport, base_url="http://test") as ac: | ||
| yield ac | ||
| app.dependency_overrides.clear() No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Avoid clearing all dependency overrides in fixture teardown.
Line [29] removes every override, which can interfere with other fixtures/tests. Remove only the override set by this fixture.
Suggested change
`@pytest.fixture`
async def client():
app.dependency_overrides[get_session] = mock_get_session
transport = ASGITransport(app=app)
async with AsyncClient(transport=transport, base_url="http://test") as ac:
yield ac
- app.dependency_overrides.clear()
+ app.dependency_overrides.pop(get_session, None)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| app.dependency_overrides[get_session] = mock_get_session | |
| transport = ASGITransport(app=app) | |
| async with AsyncClient(transport=transport, base_url="http://test") as ac: | |
| yield ac | |
| app.dependency_overrides.clear() | |
| app.dependency_overrides[get_session] = mock_get_session | |
| transport = ASGITransport(app=app) | |
| async with AsyncClient(transport=transport, base_url="http://test") as ac: | |
| yield ac | |
| app.dependency_overrides.pop(get_session, None) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/conftest.py` around lines 25 - 29, The fixture currently clears all
dependency overrides with app.dependency_overrides.clear() which can break other
tests; instead remove only the override you set by deleting or popping the
get_session entry (the one assigned to mock_get_session) from
app.dependency_overrides in the teardown of the fixture so other overrides
remain intact—locate the assignment app.dependency_overrides[get_session] =
mock_get_session and replace the final clear() call with a targeted removal of
that key.
| async def test_cookies_set_on_success(self, client): | ||
| user = make_user() | ||
| with patch(CREATE_USER, new_callable=AsyncMock, return_value=user), \ | ||
| patch(CREATE_ACCESS, new_callable=AsyncMock, return_value=FAKE_ACCESS), \ | ||
| patch(CREATE_REFRESH, new_callable=AsyncMock, return_value=FAKE_REFRESH): | ||
| response = await client.post(SIGNUP_URL, json=VALID_PAYLOAD) | ||
| assert "access_token" in response.cookies | ||
| assert "refresh_token" in response.cookies |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Assert cookie security attributes, not just cookie presence.
Line [100]-Line [101] validates existence only; this misses regressions in HttpOnly, Secure, and SameSite flags that are core to this auth design.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_auth.py` around lines 94 - 101, Update the
test_cookies_set_on_success test to assert the security attributes of the
cookies, not just their presence: after the POST to SIGNUP_URL with
VALID_PAYLOAD (within the same patched CREATE_USER/CREATE_ACCESS/CREATE_REFRESH
context), inspect response.cookies["access_token"] and
response.cookies["refresh_token"] and assert their HttpOnly flag is set, Secure
is set, and SameSite equals the expected value (e.g. "Lax" or "Strict" per app
policy); keep the existing presence assertions and add these attribute checks so
regressions in cookie flags are caught.
…correct UUID type annotation
…nutes in .env.example
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Around line 5-6: The .env.example uses identical lifetimes for both tokens;
change ACCESS_TOKEN_EXPIRE_MINUTES to a short-lived value (e.g., 15) and
REFRESH_TOKEN_EXPIRE_MINUTES to a long-lived value (e.g., 10080 for 7 days) so
the access/refresh separation and rotation model works; update the .env.example
values for ACCESS_TOKEN_EXPIRE_MINUTES and REFRESH_TOKEN_EXPIRE_MINUTES
accordingly and add a brief inline comment explaining which is short-lived vs
long-lived to prevent future copy-paste mistakes.
In `@app/api/v1/endpoints/auth.py`:
- Around line 58-63: Change the two HTTPException raises inside the signup
exception handlers to use explicit exception chaining: in the
UserAlreadyExistsException except block (catching UserAlreadyExistsException as
e) re-raise the HTTPException with "from e" so the domain exception is preserved
as the cause; in the generic except Exception as e block (which logs via
logger.exception) re-raise the HTTPException with "from None" to suppress
chaining of the internal traceback. Target the raise statements in the except
blocks handling UserAlreadyExistsException and Exception in auth.py (around the
signup handler).
In `@app/main.py`:
- Line 36: Replace the named Starlette constant usage with the integer literal
422 where the validation response status is set (the code referencing
status_code=status.HTTP_422_UNPROCESSABLE_CONTENT); change it to status_code=422
to avoid relying on Starlette 0.48+ symbols and preserve compatibility with
older Starlette/FastAPI versions while keeping the intent of signaling an
Unprocessable Entity response.
In `@tests/test_auth.py`:
- Around line 41-56: The test_response_body_shape currently asserts
body["status_code"] == 201 (a JSON field) but not the actual HTTP status; add an
assertion checking response.status_code == 201 immediately after the POST to
SIGNUP_URL (using the existing response variable) to verify the real HTTP
response code, keeping the existing JSON field assertions intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8244586-6612-4be1-a6ef-6f3f73111118
📒 Files selected for processing (6)
.env.exampleapp/api/v1/endpoints/auth.pyapp/main.pyapp/schemas/auth.pyapp/services/auth.pytests/test_auth.py
| ACCESS_TOKEN_EXPIRE_MINUTES=1000 | ||
| REFRESH_TOKEN_EXPIRE_MINUTES=1000 No newline at end of file |
There was a problem hiding this comment.
Both token expiry values are identical, contradicting the short-lived access / long-lived refresh model.
ACCESS_TOKEN_EXPIRE_MINUTES=1000 and REFRESH_TOKEN_EXPIRE_MINUTES=1000 set the same ~16.7-hour lifetime for both tokens. The PR explicitly describes access tokens as "short-lived" and refresh tokens as "long-lived"; developers who copy this example verbatim will deploy with no expiry separation, nullifying the refresh-token rotation security model.
♻️ Proposed fix
-ACCESS_TOKEN_EXPIRE_MINUTES=1000
-REFRESH_TOKEN_EXPIRE_MINUTES=1000
+ACCESS_TOKEN_EXPIRE_MINUTES=30
+REFRESH_TOKEN_EXPIRE_MINUTES=10080📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ACCESS_TOKEN_EXPIRE_MINUTES=1000 | |
| REFRESH_TOKEN_EXPIRE_MINUTES=1000 | |
| ACCESS_TOKEN_EXPIRE_MINUTES=30 | |
| REFRESH_TOKEN_EXPIRE_MINUTES=10080 |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 5-5: [UnorderedKey] The ACCESS_TOKEN_EXPIRE_MINUTES key should go before the DATABASE_URL key
(UnorderedKey)
[warning] 6-6: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 6-6: [UnorderedKey] The REFRESH_TOKEN_EXPIRE_MINUTES key should go before the TEST_DATABASE_URL key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.env.example around lines 5 - 6, The .env.example uses identical lifetimes
for both tokens; change ACCESS_TOKEN_EXPIRE_MINUTES to a short-lived value
(e.g., 15) and REFRESH_TOKEN_EXPIRE_MINUTES to a long-lived value (e.g., 10080
for 7 days) so the access/refresh separation and rotation model works; update
the .env.example values for ACCESS_TOKEN_EXPIRE_MINUTES and
REFRESH_TOKEN_EXPIRE_MINUTES accordingly and add a brief inline comment
explaining which is short-lived vs long-lived to prevent future copy-paste
mistakes.
| except UserAlreadyExistsException as e: | ||
| raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) | ||
|
|
||
| except Exception as e: | ||
| logger.exception(f"An unexpected error occurred during signup: {e}") | ||
| raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error") No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use explicit exception chaining in both raise statements.
Both raise HTTPException(...) calls inside except clauses are missing exception chaining (Ruff B904). Without it, the implicit __context__ chain is ambiguous and debuggers may misattribute the cause.
- Line 59: use
raise ... from eto preserve the domain exception as cause. - Line 63: use
raise ... from Noneto explicitly suppress the internal traceback from leaking into the exception chain, since the detail is already logged.
♻️ Proposed fix
except UserAlreadyExistsException as e:
- raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e))
+ raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) from e
except Exception as e:
logger.exception("An unexpected error occurred during signup")
- raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error")
+ raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="Internal server error") from None🧰 Tools
🪛 Ruff (0.15.12)
[warning] 59-59: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 62-62: Logging statement uses f-string
(G004)
[warning] 62-62: Redundant exception object included in logging.exception call
(TRY401)
[warning] 63-63: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/v1/endpoints/auth.py` around lines 58 - 63, Change the two
HTTPException raises inside the signup exception handlers to use explicit
exception chaining: in the UserAlreadyExistsException except block (catching
UserAlreadyExistsException as e) re-raise the HTTPException with "from e" so the
domain exception is preserved as the cause; in the generic except Exception as e
block (which logs via logger.exception) re-raise the HTTPException with "from
None" to suppress chaining of the internal traceback. Target the raise
statements in the except blocks handling UserAlreadyExistsException and
Exception in auth.py (around the signup handler).
| custom_message = f"Invalid input for {field_name}: {first_error['msg']}" | ||
|
|
||
| return JSONResponse( | ||
| status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What version of Starlette is required by FastAPI 0.136.1?
💡 Result:
FastAPI 0.136.1 requires Starlette >=0.46.0.[1][2]
Citations:
- 1: https://pypi.org/project/fastapi/
- 2: https://github.com/fastapi/fastapi/blob/master/pyproject.toml
🌐 Web query:
When was HTTP_422_UNPROCESSABLE_CONTENT introduced in Starlette changelog
💡 Result:
HTTP_422_UNPROCESSABLE_CONTENT was introduced in Starlette version 0.48.0, released on September 13, 2025. It was added as part of implementing RFC 9110 HTTP status names via Pull Request #2939, which renamed HTTP_422_UNPROCESSABLE_ENTITY to HTTP_422_UNPROCESSABLE_CONTENT while keeping the old name for backwards compatibility with a deprecation warning.[1][2][3][4]
Citations:
- 1: https://starlette.dev/release-notes/
- 2: Add
*argstoMiddlewareand improve its type hints Kludex/starlette#2381 - 3: Kludex/starlette@0.47.3...0.48.0
- 4: https://github.com/Kludex/starlette/releases/tag/0.48.0
🏁 Script executed:
# Check FastAPI 0.136.1 source code to see if it uses literal 422 or named constants
# First, let's search for how status codes are used in FastAPI's validation error handling
rg "HTTP_422" --type py -A 2 -B 2Repository: hngprojects/meetmind-be
Length of output: 272
🏁 Script executed:
# Let's also check the Starlette version history and when this constant was introduced
# by looking at recent Starlette releases
rg "422_UNPROCESSABLE" --type pyRepository: hngprojects/meetmind-be
Length of output: 138
🌐 Web query:
FastAPI 0.136.1 validation error handler status code implementation
💡 Result:
FastAPI uses HTTP 422 (Unprocessable Entity) for request validation errors produced from invalid client input via RequestValidationError, unless you override that exception handler. 1) What handler to override FastAPI raises an internal RequestValidationError when the request body/query/path contains invalid data, and FastAPI ships a default exception handler for it. You can override it by registering an exception handler for RequestValidationError using @app.exception_handler(RequestValidationError) [1]. 2) Default status code behavior The official FastAPI documentation examples show the custom RequestValidationError handler returning a response with status_code=422 (Unprocessable Entity) [1]. This corresponds to the default behavior FastAPI uses for request validation errors. 3) How the override changes the status code If you override the handler, the status code is whatever you return. For example, the docs show a handler that returns PlainTextResponse(..., status_code=400) [1]. So “implementation” is simply: FastAPI’s default handler for RequestValidationError returns 422, but your registered handler fully determines the status code. 4) Where to change it (application-wide) You can also supply exception_handlers when creating the FastAPI app to override how RequestValidationError is handled globally [2]. Regarding FastAPI 0.136.1 specifically: The 0.136.1 release notes primarily mention upgrades like Pydantic v2 deprecation fixes and documentation/tooling changes, not a change to the RequestValidationError status-code behavior [3][4].
Citations:
- 1: https://fastapi.tiangolo.com/tutorial/handling-errors/?h=validation
- 2: Allow customization of validation error fastapi/fastapi#1376
- 3: https://github.com/fastapi/fastapi/releases/tag/0.136.1
- 4: https://github.com/fastapi/fastapi/releases/latest
🏁 Script executed:
# Search for how FastAPI handles validation errors and 422 status codes
# in the FastAPI codebase or documentation
rg "422" app/main.py -B 5 -A 5Repository: hngprojects/meetmind-be
Length of output: 518
🏁 Script executed:
# Check the broader context of this status code usage in app/main.py
cat -n app/main.py | sed -n '30,45p'Repository: hngprojects/meetmind-be
Length of output: 627
Replace named constant with literal 422 for Starlette version compatibility.
HTTP_422_UNPROCESSABLE_CONTENT was introduced in Starlette 0.48.0 (September 2025), but FastAPI 0.136.1 requires only Starlette ≥0.46.0. Using this named constant creates a silent version dependency: downgrading Starlette to 0.46.x or 0.47.x will produce an AttributeError at runtime when the validation handler fires. FastAPI's own documentation uses the literal integer 422 for validation error responses, which remains compatible across all supported Starlette versions.
♻️ Proposed fix
- return JSONResponse(
- status_code=status.HTTP_422_UNPROCESSABLE_CONTENT,
+ return JSONResponse(
+ status_code=422,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/main.py` at line 36, Replace the named Starlette constant usage with the
integer literal 422 where the validation response status is set (the code
referencing status_code=status.HTTP_422_UNPROCESSABLE_CONTENT); change it to
status_code=422 to avoid relying on Starlette 0.48+ symbols and preserve
compatibility with older Starlette/FastAPI versions while keeping the intent of
signaling an Unprocessable Entity response.
| async def test_response_body_shape(self, client): | ||
| user = make_user() | ||
| with patch(CREATE_USER, new_callable=AsyncMock, return_value=user), \ | ||
| patch(CREATE_ACCESS, new_callable=AsyncMock, return_value=FAKE_ACCESS), \ | ||
| patch(CREATE_REFRESH, new_callable=AsyncMock, return_value=FAKE_REFRESH): | ||
| response = await client.post(SIGNUP_URL, json=VALID_PAYLOAD) | ||
| body = response.json() | ||
| data = response.json()["data"] | ||
| assert body["status_code"] == 201 | ||
| assert body["message"] == "Account created successfully" | ||
| assert "data" in body | ||
| assert data["access_token"] == FAKE_ACCESS | ||
| assert data["refresh_token"] == FAKE_REFRESH | ||
| assert data["email"] == "john@example.com" | ||
| assert data["name"] == "John Doe" | ||
| assert "id" in data |
There was a problem hiding this comment.
test_response_body_shape asserts the JSON status_code field but not the HTTP response status.
body["status_code"] == 201 checks a field inside the JSON payload, not the actual HTTP status code. A regression that sends HTTP 200 with the right body would pass silently. Add a direct assertion on response.status_code.
💚 Proposed fix
body = response.json()
data = response.json()["data"]
+ assert response.status_code == 201
assert body["status_code"] == 201🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_auth.py` around lines 41 - 56, The test_response_body_shape
currently asserts body["status_code"] == 201 (a JSON field) but not the actual
HTTP status; add an assertion checking response.status_code == 201 immediately
after the POST to SIGNUP_URL (using the existing response variable) to verify
the real HTTP response code, keeping the existing JSON field assertions intact.
2 participants
Description
Implemented a secure, production-ready user registration (/signup) flow. This PR also establishes modern security best practices for token management using httpOnly cookies and transitions the codebase toward explicit domain-driven exception handling.
Type of Change
feat— New featurefix— Bug fixrefactor— Code refactoring (no functional change)docs— Documentation updatetest— Adding or updating testschore— Maintenance (dependencies, CI, tooling)Related Issue
Closes AUTH-SU-01-BE
Changes Made
How to Verify
Proof of Work
API Response / Screenshots
Test Cases
This PR introduces a comprehensive suite of unit tests for the
/api/v1/auth/signupendpoint. These tests use mocking to isolate the endpoint layer, ensuring that request validation, response formatting, and error handling are correct.Success Scenarios
test_signup_returns_201_on_valid_payload and correct responseDuplicate Email Scenarios
test_signup_returns_400_when_email_is_duplicateInput Validation:
nametest_signup_returns_422_when_name_is_missingInput Validation:
emailtest_signup_returns_422_when_email_is_missingtest_signup_returns_422_when_email_format_is_invalidInput Validation:
passwordtest_signup_returns_422_when_password_is_missingtest_signup_returns_422_when_password_is_too_shorttest_signup_returns_422_when_password_lacks_uppercasetest_signup_returns_422_when_password_lacks_lowercasetest_signup_returns_422_when_password_lacks_digittest_signup_succeeds_when_password_is_at_min_lengthTest output
Checklist
<type>/<short-description>)uv run pytest)Summary by CodeRabbit
New Features
Documentation
Tests