Merge pull request #969 from ericchiang/cherry-pick-941

[cherry pick] server: fix localhost redirect validation for public clients

connector/ldap/ldap.go

@@ -345,6 +345,11 @@ func (c *ldapConnector) userEntry(conn *ldap.Conn, username string) (user ldap.E
 }
 
 func (c *ldapConnector) Login(ctx context.Context, s connector.Scopes, username, password string) (ident connector.Identity, validPass bool, err error) {
+	// make this check to avoid anonymous bind to the LDAP server.
+	if password == "" {
+		return connector.Identity{}, false, nil
+	}
+
 	var (
 		// We want to return a different error if the user's password is incorrect vs
 		// if there was an error.

server/api.go

@@ -215,13 +215,20 @@ func (d dexAPI) ListRefresh(ctx context.Context, req *api.ListRefreshReq) (*api.
 		return nil, err
 	}
 
+	var refreshTokenRefs []*api.RefreshTokenRef
 	offlineSessions, err := d.s.GetOfflineSessions(id.UserId, id.ConnId)
 	if err != nil {
-		d.logger.Errorf("api: failed to list refresh tokens: %v", err)
+		if err == storage.ErrNotFound {
+			// This means that this user-client pair does not have a refresh token yet.
+			// An empty list should be returned instead of an error.
+			return &api.ListRefreshResp{
+				RefreshTokens: refreshTokenRefs,
+			}, nil
+		}
+		d.logger.Errorf("api: failed to list refresh tokens %t here : %v", err == storage.ErrNotFound, err)
 		return nil, err
 	}
 
-	var refreshTokenRefs []*api.RefreshTokenRef
 	for _, session := range offlineSessions.Refresh {
 		r := api.RefreshTokenRef{
 			Id:        session.ID,

server/oauth2.go

@@ -12,6 +12,7 @@ import (
 	"fmt"
 	"hash"
 	"io"
+	"net"
 	"net/http"
 	"net/url"
 	"strconv"
@@ -518,9 +519,18 @@ func validateRedirectURI(client storage.Client, redirectURI string) bool {
 	if redirectURI == redirectURIOOB {
 		return true
 	}
-	if !strings.HasPrefix(redirectURI, "http://localhost:") {
+
+	// verify that the host is of form "http://localhost:(port)(path)" or "http://localhost(path)"
+	u, err := url.Parse(redirectURI)
+	if err != nil {
 		return false
 	}
-	n, err := strconv.Atoi(strings.TrimPrefix(redirectURI, "https://localhost:"))
-	return err == nil && n <= 0
+	if u.Scheme != "http" {
+		return false
+	}
+	if u.Host == "localhost" {
+		return true
+	}
+	host, _, err := net.SplitHostPort(u.Host)
+	return err == nil && host == "localhost"
 }

server/oauth2_test.go

@@ -195,3 +195,67 @@ func TestAccessTokenHash(t *testing.T) {
 		t.Errorf("expected %q got %q", googleAccessTokenHash, atHash)
 	}
 }
+
+func TestValidRedirectURI(t *testing.T) {
+	tests := []struct {
+		client      storage.Client
+		redirectURI string
+		wantValid   bool
+	}{
+		{
+			client: storage.Client{
+				RedirectURIs: []string{"http://foo.com/bar"},
+			},
+			redirectURI: "http://foo.com/bar",
+			wantValid:   true,
+		},
+		{
+			client: storage.Client{
+				RedirectURIs: []string{"http://foo.com/bar"},
+			},
+			redirectURI: "http://foo.com/bar/baz",
+		},
+		{
+			client: storage.Client{
+				Public: true,
+			},
+			redirectURI: "urn:ietf:wg:oauth:2.0:oob",
+			wantValid:   true,
+		},
+		{
+			client: storage.Client{
+				Public: true,
+			},
+			redirectURI: "http://localhost:8080/",
+			wantValid:   true,
+		},
+		{
+			client: storage.Client{
+				Public: true,
+			},
+			redirectURI: "http://localhost:991/bar",
+			wantValid:   true,
+		},
+		{
+			client: storage.Client{
+				Public: true,
+			},
+			redirectURI: "http://localhost",
+			wantValid:   true,
+		},
+		{
+			client: storage.Client{
+				Public: true,
+			},
+			redirectURI: "http://localhost.localhost:8080/",
+			wantValid:   false,
+		},
+	}
+	for _, test := range tests {
+		got := validateRedirectURI(test.client, test.redirectURI)
+		if got != test.wantValid {
+			t.Errorf("client=%#v, redirectURI=%q, wanted valid=%t, got=%t",
+				test.client, test.redirectURI, test.wantValid, got)
+		}
+	}
+}