diff --git a/modules/httplib/url.go b/modules/httplib/url.go index 2a1376b8d4..5f35e66964 100644 --- a/modules/httplib/url.go +++ b/modules/httplib/url.go @@ -24,7 +24,18 @@ func urlIsRelative(s string, u *url.URL) bool { if len(s) > 1 && (s[0] == '/' || s[0] == '\\') && (s[1] == '/' || s[1] == '\\') { return false } - return u != nil && u.Scheme == "" && u.Host == "" + if u == nil { + return false // invalid URL + } + if u.Scheme != "" || u.Host != "" { + return false // absolute URL with scheme or host + } + // Now, the URL is likely a relative URL + // HINT: GOLANG-HTTP-REDIRECT-BUG: Golang security vulnerability: "http.Redirect" calls "path.Clean" and changes the meaning of a path + // For example, `/a/../\b` will be changed to `/\b`, then it hits the first checked pattern and becomes an open redirect to "{current-scheme}://b" + // For a valid relative URL, its "path" shouldn't contain `\` because such char must be escaped. + // So if the "path" contains `\`, it is not a valid relative URL, then we can prevent open redirect. + return !strings.Contains(u.Path, "\\") } // IsRelativeURL detects if a URL is relative (no scheme or host) diff --git a/modules/httplib/url_test.go b/modules/httplib/url_test.go index 0ffb0cac05..4270d9fa89 100644 --- a/modules/httplib/url_test.go +++ b/modules/httplib/url_test.go @@ -23,6 +23,7 @@ func TestIsRelativeURL(t *testing.T) { "foo", "/", "/foo?k=%20#abc", + "/foo?k=\\", } for _, s := range rel { assert.True(t, IsRelativeURL(s), "rel = %q", s) @@ -32,6 +33,8 @@ func TestIsRelativeURL(t *testing.T) { "\\\\", "/\\", "\\/", + "/a/../\\b", + "/any\\thing", "mailto:a@b.com", "https://test.com", }