feat: implement X OAuth 2.0 PKCE backend and env var support#60
feat: implement X OAuth 2.0 PKCE backend and env var support#60shaal wants to merge 1 commit intoviperrcrypto:mainfrom
Conversation
- Add complete OAuth 2.0 PKCE flow: authorize, callback, status, disconnect, and bookmark fetch endpoints - Support X_OAUTH_CLIENT_ID and X_OAUTH_CLIENT_SECRET env vars as fallback when credentials aren't stored in the database - Auto-refresh expired tokens using offline.access scope - Paginate through X API v2 bookmarks with media expansion - Document new env vars in .env.example Closes viperrcrypto#59
xkonjin
left a comment
There was a problem hiding this comment.
Code Review: X OAuth 2.0 PKCE Integration
Overall: Comprehensive OAuth implementation with proper PKCE flow, token refresh, and secure storage. The code follows X's OAuth 2.0 guidelines and handles the edge cases well.
What looks good
- Proper PKCE implementation with SHA256 code challenge
- State parameter for CSRF protection
- Token refresh logic with automatic retry on expiry
- Graceful handling of missing user info during callback
- Token revocation on disconnect (optional but good practice)
- Support for both env vars and DB-stored credentials
Security concerns
1. Token storage (medium risk)
OAuth tokens are stored in the database unencrypted. While better than localStorage, consider:
- Documenting that this is acceptable for the threat model (single-user app)
- Or adding encryption at rest using an encryption key from env
- The refresh token is particularly sensitive — if leaked, it grants persistent access
2. Error message information disclosure
Some error responses include raw error details that might leak internal state. Consider sanitizing error messages in production.
Issues to address
1. Missing rate limiting on OAuth endpoints
The authorize/fetch endpoints don't appear to have rate limiting. Consider adding per-IP and per-user rate limiting.
2. No token expiration notification
If the refresh token expires or is revoked, the user just sees an error. Consider storing a needs-reconnect flag and showing a UI banner.
3. Pagination token not persisted
If the fetch is interrupted mid-pagination, the next_token is lost. Consider storing it in DB to resume large imports.
Testing gap
OAuth flows are hard to test, but at minimum:
- Unit tests for refreshAccessToken and getAccessToken with mocked Prisma
- Test for state mismatch rejection
- Test for token exchange failure handling
Summary
X_OAUTH_CLIENT_ID,X_OAUTH_CLIENT_SECRET) as fallback for X OAuth credentials, consistent with howANTHROPIC_API_KEYworks.env.exampleNew API routes
/api/import/x-oauth/status/api/import/x-oauth/authorize/api/import/x-oauth/callback/api/import/x-oauth/disconnect/api/import/x-oauth/fetchNote
The X API v2
GET /2/users/:id/bookmarksendpoint requires a paid developer plan (Basic at $200/month). Free-tier accounts will see a credits error after connecting. This is an X platform limitation, not a Siftly bug — but it may be worth adding a note in the UI.Test plan
X_OAUTH_CLIENT_IDandX_OAUTH_CLIENT_SECRETin.env.local/importpage shows "Connect X Account" button (not "not configured" warning)/import?x_connected=trueconnected: truewith user infoCloses #59