-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Fix open redirect vulnerability on login screen #4312
Conversation
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Codecov Report
@@ Coverage Diff @@
## master #4312 +/- ##
=========================================
+ Coverage 20.09% 20.1% +0.01%
=========================================
Files 153 153
Lines 30696 30705 +9
=========================================
+ Hits 6168 6174 +6
- Misses 23586 23588 +2
- Partials 942 943 +1
Continue to review full report at Codecov.
|
newTest(true, | ||
"http://example.com"), | ||
newTest(false, | ||
"a/"), |
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.
Why is this false?
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.
Because it is not external
"a/"), | ||
newTest(false, | ||
"https://try.gitea.io/test?param=false"), | ||
newTest(false, |
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.
Same here
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.
Because it is not external
"test?param=false"), | ||
newTest(false, | ||
"//try.gitea.io/test?param=false"), | ||
newTest(false, |
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.
And here
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.
Because it is not external
LGTM |
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.
Looks good, thanks!
@JonasFranzDEV could you backport this to 1.4 as well? |
I'm working on a backport to 1.4 |
if err != nil { | ||
return true | ||
} | ||
if len(parsed.Host) != 0 && strings.Replace(parsed.Host, "www.", "", 1) != strings.Replace(setting.Domain, "www.", "", 1) { |
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.
Fix #4307 by checking if URL is external before redirecting.
Affected: