-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Add integration tests for signin #2363
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
Conversation
Signed-off-by: David Schneiderbauer <[email protected]>
LGTM |
LGTM |
"user_name": username, | ||
"password": password, | ||
}) | ||
resp := session.MakeRequest(t, req, http.StatusOK) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daviian For future reference, you can just use the MakeRequest(..)
function, you don't need an empty session 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ethantkoenig it is for reusing GetCSRF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lafriks You don't need a CSRF if you're unauthenticated 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ethantkoenig you need otherwise login post will fail, at least it should fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daviian No, you don't need a CSRF for a sign-in post, since you aren't authenticated when you are making the request.
You can try it out for yourself! Comment out line 21, and run the integration tests locally; they should still pass (they passed for me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ethantkoenig first of all to prevent CSRF attack ;)
http://pico.ninja/gitea-login.html - try entering your try.gitea.io username/password :P (don't worry it's not logged just to demonstrate idea)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the CSRF attack will not succeed because noone's logged in yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lafriks @ethantkoenig found this one: https://stackoverflow.com/a/15350123
So @lafriks may be right ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daviian but you will get logged in and if this is used in combination with other vulnerability it can be dangerous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- When a event is caused by `Ctrl+Enter` jQuery might not wrap the event and in that case `originalEvent` is not defined. Check for this case. - Log the error along with showing an toast. - Resolves go-gitea#2363
- Backport of go-gitea#2370 - When a event is caused by `Ctrl+Enter` jQuery might not wrap the event and in that case `originalEvent` is not defined. Check for this case. - Log the error along with showing an toast. - Resolves go-gitea#2363 (cherry picked from commit f04589d)
As title.