Remove the undocumented login-with-token page #8336
Conversation
WalkthroughThe changes involve the removal of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant AuthAPI
User->>App: Request login
App->>AuthAPI: Send credentials
AuthAPI-->>App: Return authentication token
App-->>User: Successful login
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There are several problems with this feature: 1. To use it, you have to put the user's token in the URL. This token lasts forever (unless the user explicitly logs out), so it is nearly as sensitive as the user's password. Embedding such sensitive information in the URL is problematic, because URLs are saved in the browser history, dumped to server logs and displayed on the screen, none of which are secure locations. A user could also accidentally share a URL with an embedded token. 2. If an attacker can get a user to follow a malicious link, they could forcibly log that user into the attacker's account (AKA "login CSRF"). This by itself is just a nuisance, but the attacker could potentially use this to trick the victim into, for example, uploading confidential data to the attacker's account. 3. By design, it requires the use of token authentication, whose drawbacks I have explained in cvat-ai#8289. In fairness, when originally implemented, this feature set the session cookie rather than the token, but this cannot work if the user is already logged in, as the `sessionid` cookie is marked `HTTPOnly` and cannot be overridden by JavaScript. So the only way for this feature to work in all circumstances is to set the token. Generally, the use cases of this feature are better served by single sign-on protocols, which don't suffer from these drawbacks.
SpecLad
force-pushed
the
rm-login-with-token
branch
from
e59729a to
f3ebea1
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- changelog.d/20240822_134319_roman_rm_login_with_token.md (1 hunks)
- cvat-ui/package.json (1 hunks)
- cvat-ui/src/components/cvat-app.tsx (3 hunks)
- tests/cypress/e2e/actions_users/issue_1810_login_logout.js (1 hunks)
Files skipped from review due to trivial changes (3)
- cvat-ui/package.json
- cvat-ui/src/components/cvat-app.tsx
- tests/cypress/e2e/actions_users/issue_1810_login_logout.js
Additional comments not posted (1)
changelog.d/20240822_134319_roman_rm_login_with_token.md (1)
1-4: Changelog entry looks good.The changelog entry is clear and follows the expected format. It accurately describes the removal of the
/auth/login-with-tokenpage and provides a link to the pull request for reference.
Motivation and context
There are several problems with this feature:
To use it, you have to put the user's token in the URL. This token lasts forever (unless the user explicitly logs out), so it is nearly as sensitive as the user's password. Embedding such sensitive information in the URL is problematic, because URLs are saved in the browser history, dumped to server logs and displayed on the screen, none of which are secure locations. A user could also accidentally share a URL with an embedded token.
If an attacker can get a user to follow a malicious link, they could forcibly log that user into the attacker's account (AKA "login CSRF"). This by itself is just a nuisance, but the attacker could potentially use this to trick the victim into, for example, uploading confidential data to the attacker's account.
By design, it requires the use of token authentication, whose drawbacks I have explained in Stop using token authentication in the UI #8289.
In fairness, when originally implemented, this feature set the session cookie rather than the token, but this cannot work if the user is already logged in, as the
sessionidcookie is markedHTTPOnlyand cannot be overridden by JavaScript. So the only way for this feature to work in all circumstances is to set the token.Generally, the use cases of this feature are better served by single sign-on protocols, which don't suffer from these drawbacks.
How has this been tested?
Checklist
developbranch[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
cvat-uipackage to indicate a new release.