Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request from GHSA-5m6c-jp6f-2vcv
* Add more Open Redirect test cases

* Add whitelisted domain to test

* Add more test cases

* Improve invalid redirect regex
  • Loading branch information
JoelSpeed committed Jun 27, 2020
1 parent 1b6c54c commit ee5662e
Show file tree
Hide file tree
Showing 3 changed files with 641 additions and 1 deletion.
2 changes: 1 addition & 1 deletion oauthproxy.go
Expand Up @@ -63,7 +63,7 @@ var (

// Used to check final redirects are not susceptible to open redirects.
// Matches //, /\ and both of these with whitespace in between (eg / / or / \).
invalidRedirectRegex = regexp.MustCompile(`^/(\s|\v)?(/|\\)`)
invalidRedirectRegex = regexp.MustCompile(`[/\\](?:[\s\v]*|\.{1,2})[/\\]`)
)

// OAuthProxy is the main authentication proxy
Expand Down
81 changes: 81 additions & 0 deletions oauthproxy_test.go
@@ -1,6 +1,7 @@
package main

import (
"bufio"
"context"
"crypto"
"encoding/base64"
Expand All @@ -11,6 +12,7 @@ import (
"net/http"
"net/http/httptest"
"net/url"
"os"
"regexp"
"strings"
"testing"
Expand Down Expand Up @@ -386,6 +388,41 @@ func TestIsValidRedirect(t *testing.T) {
Redirect: "/\r\\evil.com",
ExpectedResult: false,
},
{
Desc: "openRedirectTripleTab",
Redirect: "/\t\t/\t/evil.com",
ExpectedResult: false,
},
{
Desc: "openRedirectTripleTab2",
Redirect: "/\t\t\\\t/evil.com",
ExpectedResult: false,
},
{
Desc: "openRedirectQuadTab1",
Redirect: "/\t\t/\t\t\\evil.com",
ExpectedResult: false,
},
{
Desc: "openRedirectQuadTab2",
Redirect: "/\t\t\\\t\t/evil.com",
ExpectedResult: false,
},
{
Desc: "openRedirectPeriod1",
Redirect: "/./\\evil.com",
ExpectedResult: false,
},
{
Desc: "openRedirectPeriod2",
Redirect: "/./../../\\evil.com",
ExpectedResult: false,
},
{
Desc: "openRedirectDoubleTab",
Redirect: "/\t/\t\\evil.com",
ExpectedResult: false,
},
}

for _, tc := range testCases {
Expand All @@ -399,6 +436,50 @@ func TestIsValidRedirect(t *testing.T) {
}
}

func TestOpenRedirects(t *testing.T) {
opts := NewOptions()
opts.ClientID = "skdlfj"
opts.ClientSecret = "fgkdsgj"
opts.Cookie.Secret = "ljgiogbj"
// Should match domains that are exactly foo.bar and any subdomain of bar.foo
opts.WhitelistDomains = []string{
"foo.bar",
".bar.foo",
"port.bar:8080",
".sub.port.bar:8080",
"anyport.bar:*",
".sub.anyport.bar:*",
"www.whitelisteddomain.tld",
}
opts.Validate()

proxy := NewOAuthProxy(opts, func(string) bool { return true })

file, err := os.Open("./test/openredirects.txt")
if err != nil {
t.Fatal(err)
}
defer file.Close()

scanner := bufio.NewScanner(file)
for scanner.Scan() {
rd := scanner.Text()
t.Run(rd, func(t *testing.T) {
rdUnescaped, err := url.QueryUnescape(rd)
if err != nil {
t.Fatal(err)
}
if proxy.IsValidRedirect(rdUnescaped) {
t.Errorf("Expected %q to not be valid (unescaped: %q)", rd, rdUnescaped)
}
})
}

if err := scanner.Err(); err != nil {
t.Fatal(err)
}
}

type TestProvider struct {
*providers.ProviderData
EmailAddress string
Expand Down

0 comments on commit ee5662e

Please sign in to comment.