Move to login with token and not session cookie #138
Conversation
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Fix pylint style issues: C0301, C0303, C0304
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| redirect = RedirectResponse(url="/", status_code=302) | ||
| redirect.set_cookie( | ||
| key="api_token", | ||
| value=api_token, |
Check failure
Code scanning / CodeQL
Clear-text storage of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix this issue, refactor the code so that the user's browser never receives the raw API token. Instead, store in the cookie a randomly generated session ID (not the token itself). On the server, maintain a mapping from session ID to the user's API token (and, if necessary, metadata).
Specifically:
- On login, generate a secure session ID (use
secrets.token_urlsafe()for the ID as well). - Store the mapping
{session_id: api_token}server-side. (Since you can’t see the server-side store here, insert a TODO or a placeholder, e.g., a call to a notional functionstore_session_token(session_id, api_token)). - Set only the
session_id(not theapi_token) in the cookie. - On subsequent requests, retrieve the session ID from the cookie and look up the real API token using this mapping.
- Make sure to update any subsequent code that relies on
api_tokenfrom the cookie (here, just update setting the cookie).
Necessary changes:
- Replace setting
api_tokenin theredirect.set_cookie()call with setting a server-managed session ID. - Add a placeholder for storing the session ID and API token mapping (since you do not have access to the actual server-side storage logic).
- Add import if adding any new library (not needed here, since you already import
secrets). - Leave a TODO/comment for the session mapping where persistent storage would be handled.
| @@ -162,14 +162,19 @@ | ||
| handler = getattr(request.app.state, "callback_handler", None) | ||
| if handler: | ||
| api_token = secrets.token_urlsafe(32) # ~43 chars, hard to guess | ||
| session_id = secrets.token_urlsafe(32) # Session cookie value, also random | ||
|
|
||
| # Store session_id --> api_token mapping in a secure server-side store. | ||
| # TODO: Implement persistent session management. E.g.: | ||
| # await store_session_token(session_id, api_token) | ||
|
|
||
| # call the registered handler (await if async) | ||
| await handler('google', user_data, api_token) | ||
|
|
||
| redirect = RedirectResponse(url="/", status_code=302) | ||
| redirect.set_cookie( | ||
| key="api_token", | ||
| value=api_token, | ||
| key="session_id", | ||
| value=session_id, | ||
| httponly=True, | ||
| secure=True | ||
| ) |
There was a problem hiding this comment.
which data sensitive data? we don't store the password
| redirect = RedirectResponse(url="/", status_code=302) | ||
| redirect.set_cookie( | ||
| key="api_token", | ||
| value=api_token, |
Check failure
Code scanning / CodeQL
Clear-text storage of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
General approach:
Always avoid storing sensitive tokens directly in client cookies. Instead, generate a random session identifier on the server, map it (in a database or in-memory cache) to the sensitive token server-side, and set only the opaque session identifier as a cookie. When the client returns with this session ID, the server fetches the sensitive token as needed.
Detailed fix:
In api/routes/auth.py, after generating the api_token, also generate a random session ID (using a secure random generator). Store a mapping from this session ID to the api_token server-side (in-memory dict, Redis cache, database, etc.—must be persistent if the server can be restarted). Set only the session ID in the cookie. Later, code retrieving the token (not shown in this snippet) should look up the token by the session ID.
What needs to be changed:
- After generating
api_token, generate asession_id. - Store
{session_id: api_token}in an in-memory dictionary or preferred storage. - Set
session_id(notapi_token) as the cookie value.
Additional needs:
- A storage for the mapping, e.g., a dictionary attached to
app.state(for demo/fix purposes), or a simple process-wide global dict. - Update the cookie setter to store the session ID instead.
- (If code to look up the token from the session ID exists elsewhere, it must be updated to perform the lookup; for the scope of this fix, we limit ourselves only to changing what's shown.)
| @@ -245,14 +245,24 @@ | ||
| if handler: | ||
|
|
||
| api_token = secrets.token_urlsafe(32) # ~43 chars, hard to guess | ||
| session_id = secrets.token_urlsafe(32) # New session id | ||
|
|
||
| # Store the api_token server-side, associated with session_id | ||
| # Use app.state to store session mappings if available, else global | ||
| session_store = getattr(request.app.state, "session_store", None) | ||
| if session_store is None: | ||
| # Attach a dict to app.state for session mappings | ||
| session_store = {} | ||
| setattr(request.app.state, "session_store", session_store) | ||
| session_store[session_id] = api_token | ||
|
|
||
| # call the registered handler (await if async) | ||
| await handler('github', user_data, api_token) | ||
|
|
||
| redirect = RedirectResponse(url="/", status_code=302) | ||
| redirect.set_cookie( | ||
| key="api_token", | ||
| value=api_token, | ||
| key="session_id", | ||
| value=session_id, | ||
| httponly=True, | ||
| secure=True | ||
| ) |
There was a problem hiding this comment.
we don't store password
There was a problem hiding this comment.
Pull Request Overview
This PR transitions the authentication system from session-based cookies to token-based authentication. The key change involves storing user authentication tokens in the database and using HTTP-only cookies to manage session state instead of server-side sessions.
Key changes:
- Replaces session-based authentication with API tokens stored in the database
- Changes user graph namespacing from underscore (
_) to pipe (|) separator - Migrates JavaScript functionality from inline scripts to TypeScript modules
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| api/routes/auth.py | Major authentication system overhaul - token generation, cookie management, and simplified OAuth flows |
| api/auth/user_management.py | Database-based token validation replacing session validation with new token management functions |
| api/auth/oauth_handlers.py | Simplified OAuth callback handlers to use unified token-based approach |
| api/routes/graphs.py | Updated graph namespacing from underscore to pipe separator for user isolation |
| api/loaders/*.py | Updated database loaders to use new pipe separator for graph naming |
| app/ts/modules/left_toolbar.ts | New TypeScript module extracting left toolbar functionality from inline scripts |
| app/ts/app.ts | Integration of new left toolbar module into main application |
| app/templates/components/left_toolbar.j2 | Removed inline JavaScript in favor of TypeScript module |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.