fix: require authentication for Vercel API endpoints#4
fix: require authentication for Vercel API endpoints#4semgrep-code-lahiruramesh[bot] wants to merge 1 commit into
Conversation
Replace user_id query parameter with authenticated user from Bearer token to prevent IDOR vulnerability. ## Changes - Add `dependencies.py` with `get_current_user` authentication dependency that validates Bearer tokens - Add `session_token` column to users table for secure session management - Add `get_user_by_session_token()` and `update_user_session_token()` methods to database service - Update auth.py to generate and return secure session tokens on OAuth login - Update all Vercel endpoints to use `Depends(get_current_user)` instead of accepting `user_id` as a parameter ## Why The endpoints accepted `user_id` as an untrusted query parameter, allowing any caller to impersonate any user. By extracting the user identity from a validated session token in the Authorization header, users can only perform actions as themselves. ## Semgrep Finding Details The endpoint accepts user_id as a query parameter with no authentication verification. Any caller can delete another user's Vercel deployments by supplying that user's ID. Semgrep generated this pull request to fix [a finding](https://semgrep.dev/orgs/sylonik/ai-findings/768608297).
There was a problem hiding this comment.
Pull request overview
This PR addresses an IDOR risk by removing the caller-supplied user_id from /vercel/* endpoints and instead deriving the user from a Bearer session token.
Changes:
- Added a
get_current_userFastAPI dependency that authenticates via Bearer session tokens. - Extended the users table and DB service with
session_tokensupport (lookup + update). - Updated
/vercel/*endpoints to useDepends(get_current_user)and the authenticated user context.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| api/app/database/service.py | Adds session_token column + DB methods for session-token auth. |
| api/app/api/dependencies.py | Introduces Bearer-token authentication dependencies. |
| api/app/api/auth.py | Generates and persists a session token on Google OAuth callback. |
| api/app/api/vercel.py | Replaces user_id parameters with authenticated current_user. |
Comments suppressed due to low confidence (1)
api/app/api/vercel.py:215
redeploy_vercel_deployment()anddelete_vercel_deployment()fetch the deployment record bydeployment_idbut never verifydeployment.user_id == current_user.idbefore redeploying/deleting and before deleting the DB record. Add an ownership check (or query by bothdeployment_idandcurrent_user.id) to prevent cross-user actions if deployment IDs are guessable or if multiple users share a Vercel team.
# Get deployment info from database
deployment = await db_service.get_vercel_deployment_by_deployment_id(deployment_id)
if not deployment:
raise HTTPException(status_code=404, detail="Deployment not found")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @router.post("/deployments") | ||
| async def create_vercel_deployment( | ||
| user_id: str, | ||
| project_id: str, | ||
| deployment_data: VercelDeployment | ||
| deployment_data: VercelDeployment, | ||
| current_user: User = Depends(get_current_user) | ||
| ): |
There was a problem hiding this comment.
New authentication behavior (get_current_user + session-token lookup) and the updated /vercel/* endpoints aren’t covered by tests. Since this repo already has pytest coverage for other API modules, add tests that assert: (1) requests without/with invalid Bearer tokens are rejected, and (2) authenticated requests succeed and are scoped to the authenticated user.
| async def get_user_by_session_token(self, session_token: str) -> Optional[User]: | ||
| """Look up a user by their session token for authentication""" | ||
| query = "SELECT * FROM users WHERE session_token = ?" | ||
| result = self.conn.execute(query, [session_token]).fetchone() | ||
| if result: | ||
| return User( | ||
| id=result[0], email=result[1], name=result[2], avatar_url=result[3], | ||
| google_id=result[4], github_username=result[5], github_token=result[6], | ||
| vercel_token=result[7], vercel_team_id=result[8], | ||
| created_at=result[10], updated_at=result[11] | ||
| ) | ||
| return None | ||
|
|
||
| async def update_user_session_token(self, user_id: str, session_token: str): | ||
| """Update user's session token""" | ||
| query = """ | ||
| UPDATE users | ||
| SET session_token = ?, updated_at = CURRENT_TIMESTAMP | ||
| WHERE id = ? | ||
| """ | ||
| self.conn.execute(query, [session_token, user_id]) | ||
| self.conn.commit() |
There was a problem hiding this comment.
The new get_user_by_session_token() / update_user_session_token() methods are security-critical but currently have no unit tests in api/tests/test_database_service.py (which already tests other DatabaseService behaviors). Add tests covering: updating the session token, looking up by token, and ensuring invalid tokens return None.
| return User( | ||
| id=result[0], email=result[1], name=result[2], avatar_url=result[3], | ||
| google_id=result[4], github_username=result[5], github_token=result[6], | ||
| vercel_token=result[7], vercel_team_id=result[8], | ||
| created_at=result[9], updated_at=result[10] | ||
| created_at=result[10], updated_at=result[11] | ||
| ) |
There was a problem hiding this comment.
After adding the session_token column, SELECT * FROM users shifts created_at/updated_at to indexes 10 and 11. get_user_by_email() was updated accordingly, but get_user_by_id() still maps created_at=result[9], updated_at=result[10], which will now return a string session token as created_at and shift timestamps. Update get_user_by_id() (and ideally avoid index-based SELECT * mappings by selecting explicit columns).
| # Add session_token column if it doesn't exist (migration) | ||
| try: | ||
| cursor.execute("ALTER TABLE users ADD COLUMN session_token TEXT") | ||
| except: | ||
| pass # Column might already exist |
There was a problem hiding this comment.
The migration logic for session_token uses a bare except, which can silently swallow unrelated DuckDB errors (e.g., permission issues, corrupted schema) and leave the app running with a partially-migrated database. Catch the specific exception type and only ignore the "column already exists" case (or query information_schema.columns before altering).
| vercel_token TEXT, | ||
| vercel_team_id TEXT, | ||
| session_token TEXT, | ||
| created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP |
There was a problem hiding this comment.
session_token is used as a bearer credential but is stored as plain text and has no uniqueness constraint. Consider (1) storing only a hash (e.g., SHA-256) of the token and comparing hashes on lookup, and (2) adding a UNIQUE constraint/index on session_token to prevent duplicates and speed up authentication lookups.
| # Generate and store a session token for authenticated API access | ||
| session_token = generate_session_token() | ||
| await db_service.update_user_session_token(user.id, session_token) | ||
|
|
||
| return { | ||
| "user": { | ||
| "id": user.id, | ||
| "email": user.email, | ||
| "name": user.name, | ||
| "avatar_url": user.avatar_url | ||
| }, | ||
| "access_token": access_token | ||
| "session_token": session_token | ||
| } |
There was a problem hiding this comment.
connect_vercel(), get_user(), and logout() still accept a caller-supplied user_id and perform user-specific actions/data access without verifying it matches the authenticated user. With the new session-token auth flow, these endpoints should switch to Depends(get_current_user) (or otherwise validate ownership) to avoid reintroducing the same IDOR class of issue outside /vercel/*.
| if not user: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| detail="Invalid or expired authentication token", |
There was a problem hiding this comment.
The error message says "Invalid or expired authentication token", but there is no expiry/TTL check for session tokens in the current implementation. Either implement token expiration (recommended) or adjust the message to avoid implying expiration behavior that doesn’t exist.
| detail="Invalid or expired authentication token", | |
| detail="Invalid authentication token", |
Replace user_id query parameter with authenticated user from Bearer token to prevent IDOR vulnerability.
Changes
dependencies.pywithget_current_userauthentication dependency that validates Bearer tokenssession_tokencolumn to users table for secure session managementget_user_by_session_token()andupdate_user_session_token()methods to database serviceDepends(get_current_user)instead of acceptinguser_idas a parameterWhy
The endpoints accepted
user_idas an untrusted query parameter, allowing any caller to impersonate any user. By extracting the user identity from a validated session token in the Authorization header, users can only perform actions as themselves.Semgrep Finding Details
The endpoint accepts user_id as a query parameter with no authentication verification. Any caller can delete another user's Vercel deployments by supplying that user's ID.
Semgrep generated this pull request to fix a finding.