-
Notifications
You must be signed in to change notification settings - Fork 69
security: inject session credentials into API requests #408
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for opening this PR! Before a maintainer takes a look, it would be really helpful if you could walk through your changes using GitHub's review tools. Please take a moment to:
More information on how to conduct a self review: This helps make the review process smoother and gives us a clearer understanding of your thought process. Once you've added your self-review, we'll continue from our side. Thank you! |
Add authentication middleware that injects credentials from session into request body, preventing credential manipulation attacks. This approach: - Validates user has an authenticated session - Overwrites any provided credentials with session credentials - Prevents users from manipulating credentials to access other data - Makes frontend credential storage optional (still shown in setup guide) Note: Encryption secret is still shown in SetupGuide for users to configure their local taskwarrior CLI, but is no longer trusted from API requests. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
f3f1a83 to
1f5871a
Compare
Probably needs to update the frontend, in order to completely test this. The backward compatibility could be a grey area, making it seem to work. Though instead of injecting this directly via auth, one better way could be to just validate the session upon a login or request, and re construct the Secrets on its own, using utils, if not provided by frontend (remove them from frontend calls as well). So one may or may not provide those creds, if not, generate on your own using the authenticated session else just use the provided values |
|
Thanks for the feedback! I want to make sure I understand your concern correctly. The current implementation overwrites any credentials in the request body with session values (not just injects when missing): // middleware/auth.go lines 66-70
bodyMap["email"] = sessionEmail
bodyMap["UUID"] = sessionUUID
bodyMap["encryptionSecret"] = sessionSecretThis means it's already backward compatible:
Example attack scenario the PR prevents:
If we used your suggested approach ("if credentials provided, use them; if not, generate"), the vulnerability would still exist because attackers could just keep sending manipulated credentials. Unless you meant: validate that provided credentials match the session, and reject if they don't? That would also be secure, but the overwrite approach is simpler and has the same security properties. The frontend can be updated to stop sending credentials at any time - the backend will work either way. No coordination needed. Am I missing something in your suggestion? Happy to discuss further if you see an issue with this approach. |
|
Makes sense. Can merge this one, but it has some conflicts, they need to be resolved. |
The auth middleware added in PR #408 requires session cookies on all task API endpoints. Frontend fetch calls were missing credentials: 'include', causing 401 errors when fetching/modifying tasks. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Security Issues Addressed
Encryption Secret Exposed to Frontend (High) and Missing Authorization on API Endpoints (High)
Previously, the frontend stored and sent credentials (email, UUID, encryption_secret) with every API request. Users could potentially manipulate these values to access other users' tasks. This PR addresses both issues by:
Changes
backend/middleware/auth.go: New authentication middleware that:backend/main.go: Apply auth middleware to task endpointsTest plan
🤖 Generated with Claude Code