Skip to content

Commit 08abf9a

Browse files
bnfinetbnfinet
committed
Merge branch 'pr/airbornemint/296'
2 parents dc39f88 + 52e0158 commit 08abf9a

2 files changed

Lines changed: 51 additions & 45 deletions

File tree

‎handlers/login.go‎

Lines changed: 24 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -95,13 +95,12 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) {
9595
}
9696

9797
var (
98-
errNoURL = errors.New("no destination URL requested")
99-
errInvalidURL = errors.New("requested destination URL appears to be invalid")
100-
errURLNotHTTP = errors.New("requested destination URL is not a valid URL (does not begin with 'http://' or 'https://')")
101-
errLoginStrayParams = errors.New("login request included unrecognized parameters")
102-
errDangerQS = errors.New("requested destination URL has a dangerous query string")
103-
badStrings = []string{"http://", "https://", "data:", "ftp://", "ftps://", "//", "javascript:"}
104-
reAmpSemi = regexp.MustCompile("[&;]")
98+
errNoURL = errors.New("no destination URL requested")
99+
errInvalidURL = errors.New("requested destination URL appears to be invalid")
100+
errURLNotHTTP = errors.New("requested destination URL is not a valid URL (does not begin with 'http://' or 'https://')")
101+
errDangerQS = errors.New("requested destination URL has a dangerous query string")
102+
badStrings = []string{"http://", "https://", "data:", "ftp://", "ftps://", "//", "javascript:"}
103+
reAmpSemi = regexp.MustCompile("[&;]")
105104
)
106105

107106
// inspect login query params to located the url param, while taking into account that the login URL may be
@@ -117,39 +116,41 @@ var (
117116
// * All other login params are treated as non-login params
118117
// * All non-login params between the url param and the first true login param are folded into the url param
119118
// * All remaining non-login params are considered stray non-login params
120-
// * Error is returned if the url cannot be parsed *or* it contains stray non-login params
121-
// * For the benefit of unit-testing, if the url contains stray non-login params, it's
122-
// returned anyway (in addition to error return)
123-
func normalizeLoginURLParam(loginURL *url.URL) (*url.URL, error) {
119+
//
120+
// Returns
121+
// * _, _, err: if an error occurred while parsing the URL
122+
// * URL, empty array, nil: if URL is valid and contains no stray non-login params
123+
// * URL, array of stray params, nil: if URL is valid and contains stray non-login params
124+
func normalizeLoginURLParam(loginURL *url.URL) (*url.URL, []string, error) {
124125
// url.URL.Query return a map and therefore makes no guarantees about param order
125126
// Therefore we have to ascertain the param order by inspecting the raw query
126127
var urlParam *url.URL = nil // Will be url.URL for the url param
127-
strayParams := false // Will be true if stray params are found
128128
urlParamDone := false // Will be true when we're done building urlParam (but we're still checking for stray params)
129+
strays := []string{} // List of stray params
129130

130131
for _, param := range reAmpSemi.Split(loginURL.RawQuery, -1) {
131132
paramKeyVal := strings.Split(param, "=")
132133
paramKey := paramKeyVal[0]
133134
lcParamKey := strings.ToLower(paramKey)
134135
isVouchParam := strings.HasPrefix(lcParamKey, cfg.Branding.LCName) ||
135136
strings.HasPrefix(lcParamKey, "x-"+cfg.Branding.LCName) ||
136-
paramKey == "error" ||
137-
paramKey == "rd"
137+
paramKey == "error" || // Used by VouchProxy login
138+
paramKey == "rd" // Passed to VouchProxy by nginx-ingress and then ignored (see #289)
138139

139140
if urlParam == nil {
140141
// Still looking for url param
141142
if paramKey == "url" {
142143
// Found it
143144
parsed, e := url.Parse(loginURL.Query().Get("url"))
144145
if e != nil {
145-
return nil, e // failure to parse url param
146+
return nil, []string{}, e // failure to parse url param
146147
}
147148

148149
urlParam = parsed
149150
} else if !isVouchParam {
150151
// Non-vouch param before url param is a stray param
151152
log.Infof("Stray param in login request (%s)", paramKey)
152-
strayParams = true
153+
strays = append(strays, paramKey)
153154
} // else vouch param before url param, doesn't change outcome
154155
} else {
155156
// Looking at params after url param
@@ -167,22 +168,22 @@ func normalizeLoginURLParam(loginURL *url.URL) (*url.URL, error) {
167168
} else if !isVouchParam {
168169
// Non-vouch param after vouch param is a stray param
169170
log.Infof("Stray param in login request (%s)", paramKey)
170-
strayParams = true
171+
strays = append(strays, paramKey)
171172
} // else vouch param after vouch param, doesn't change outcome
172173
}
173174
}
174175

175-
// if there were stray parameters return an error
176176
log.Debugf("Login url param normalized to '%s'", urlParam)
177-
if strayParams {
178-
return urlParam, errLoginStrayParams
179-
}
180-
return urlParam, nil
177+
return urlParam, strays, nil
181178

182179
}
183180

184181
func getValidRequestedURL(r *http.Request) (string, error) {
185-
u, err := normalizeLoginURLParam(r.URL)
182+
u, strays, err := normalizeLoginURLParam(r.URL)
183+
184+
if len(strays) > 0 {
185+
log.Debugf("Stray params in login url (%+q) will be ignored", strays)
186+
}
186187

187188
if err != nil {
188189
return "", fmt.Errorf("Not a valid login URL: %w %s", errInvalidURL, err)

‎handlers/login_test.go‎

Lines changed: 27 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,67 +16,72 @@ import (
1616
"net/url"
1717
"testing"
1818

19+
"github.com/google/go-cmp/cmp"
1920
"github.com/stretchr/testify/assert"
2021
"github.com/vouch/vouch-proxy/pkg/cfg"
2122
)
2223

2324
func Test_normalizeLoginURL(t *testing.T) {
2425
setUp("/config/testing/handler_login_url.yml")
2526
tests := []struct {
26-
name string
27-
url string
28-
want string
29-
wantErr bool
27+
name string
28+
url string
29+
want string
30+
wantStray []string
31+
wantErr bool
3032
}{
3133
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
32-
{"extra params", "http://host/login?url=http://host/path?p2=2", "http://host/path?p2=2", false},
33-
{"extra params (blank)", "http://host/login?url=http://host/path?p2=", "http://host/path?p2=", false},
34+
{"extra params", "http://host/login?url=http://host/path?p2=2", "http://host/path?p2=2", []string{}, false},
35+
{"extra params (blank)", "http://host/login?url=http://host/path?p2=", "http://host/path?p2=", []string{}, false},
3436
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
3537
// Even though the p1 param is not a login param, we do not interpret is as part of the url param because it precedes it
36-
{"prior params", "http://host/login?p1=1&url=http://host/path", "http://host/path", true},
38+
{"prior params", "http://host/login?p1=1&url=http://host/path", "http://host/path", []string{"p1"}, false},
3739
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
3840
// We assume vouch-* is a login param and do not fold it into url
39-
{"vouch-* params after", "http://host/login?url=http://host/path&vouch-xxx=2", "http://host/path", false},
41+
{"vouch-* params after", "http://host/login?url=http://host/path&vouch-xxx=2", "http://host/path", []string{}, false},
4042
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
4143
// We assume vouch-* is a login param and do not fold it into url
42-
{"vouch-* params before", "http://host/login?vouch-xxx=1&url=http://host/path", "http://host/path", false},
44+
{"vouch-* params before", "http://host/login?vouch-xxx=1&url=http://host/path", "http://host/path", []string{}, false},
4345
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
4446
// We assume x-vouch-* is a login param and do not fold it into url
45-
{"x-vouch-* params after", "http://host/login?url=http://host/path&vouch-xxx=2", "http://host/path", false},
47+
{"x-vouch-* params after", "http://host/login?url=http://host/path&vouch-xxx=2", "http://host/path", []string{}, false},
4648
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
4749
// We assume x-vouch-* is a login param and do not fold it into url
48-
{"x-vouch-* params before", "http://host/login?x-vouch-xxx=1&url=http://host/path", "http://host/path", false},
50+
{"x-vouch-* params before", "http://host/login?x-vouch-xxx=1&url=http://host/path", "http://host/path", []string{}, false},
4951
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
5052
// Even though p1 is not a login param, we do not interpret is as part of url because it follows a login param (vouch-*)
51-
{"params after vouch-* params", "http://host/login?url=http://host/path&vouch-xxx=2&p3=3", "http://host/path", true},
53+
{"params after vouch-* params", "http://host/login?url=http://host/path&vouch-xxx=2&p3=3", "http://host/path", []string{"p3"}, false},
5254
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
5355
// Even though p1 is not a login param, we do not interpret is as part of url because it follows a login param (x-vouch-*)
54-
{"params after x-vouch-* params", "http://host/login?url=http://host/path&x-vouch-xxx=2&p3=3", "http://host/path", true},
56+
{"params after x-vouch-* params", "http://host/login?url=http://host/path&x-vouch-xxx=2&p3=3", "http://host/path", []string{"p3"}, false},
5557
// This is not an RFC-compliant URL; it combines all the aspects above
56-
{"all params", "http://host/login?p1=1&url=http://host/path?p2=2&p3=3&x-vouch-xxx=4&vouch=5&error=6&p7=7", "http://host/path?p2=2&p3=3", true},
58+
{"all params", "http://host/login?p1=1&url=http://host/path?p2=2&p3=3&x-vouch-xxx=4&vouch=5&error=6&p7=7", "http://host/path?p2=2&p3=3", []string{"p1", "p7"}, false},
5759
// This is an RFC-compliant URL
58-
{"all params (encoded)", "http://host/login?p1=1&url=http%3a%2f%2fhost/path%3fp2=2%26p3=3&x-vouch-xxx=4&vouch=5&error=6&p7=7", "http://host/path?p2=2&p3=3", true},
60+
{"all params (encoded)", "http://host/login?p1=1&url=http%3a%2f%2fhost/path%3fp2=2%26p3=3&x-vouch-xxx=4&vouch=5&error=6&p7=7", "http://host/path?p2=2&p3=3", []string{"p1", "p7"}, false},
5961
// This is not an RFC-compliant URL; it combines all the aspects above, and it uses semicolons as parameter separators
6062
// Note that when we fold a stray param into the url param, we always do so with &s
61-
{"all params (semicolons)", "http://host/login?p1=1;url=http://host/path?p2=2;p3=3;x-vouch-xxx=4;p5=5", "http://host/path?p2=2&p3=3", true},
63+
{"all params (semicolons)", "http://host/login?p1=1;url=http://host/path?p2=2;p3=3;x-vouch-xxx=4;p5=5", "http://host/path?p2=2&p3=3", []string{"p1", "p5"}, false},
6264
// This is an RFC-compliant URL that uses semicolons as parameter separators
63-
{"all params (encoded, semicolons)", "http://host/login?p1=1;url=http%3a%2f%2fhost/path%3fp2=2%3bp3=3;x-vouch-xxx=4;p5=5", "http://host/path?p2=2;p3=3", true},
65+
{"all params (encoded, semicolons)", "http://host/login?p1=1;url=http%3a%2f%2fhost/path%3fp2=2%3bp3=3;x-vouch-xxx=4;p5=5", "http://host/path?p2=2;p3=3", []string{"p1", "p5"}, false},
6466
// Real world tests
6567
// since v0.4.0 the vouch README has specified an Nginx config including a 302 redirect in the following format...
66-
{"Vouch Proxy README (with error)", "http://host/login?url=http://host/path?p2=2&vouch-failcount=3&X-Vouch-Token=TOKEN&error=anerror", "http://host/path?p2=2", false},
67-
{"Vouch Proxy README (blank error)", "http://host/login?url=http://host/path?p2=2&vouch-failcount=&X-Vouch-Token=&error=", "http://host/path?p2=2", false},
68-
{"Vouch Proxy README (semicolons, blank error)", "http://host/login?url=http://host/path?p2=2;p3=3&vouch-failcount=&X-Vouch-Token=&error=", "http://host/path?p2=2&p3=3", false},
68+
{"Vouch Proxy README (with error)", "http://host/login?url=http://host/path?p2=2&vouch-failcount=3&X-Vouch-Token=TOKEN&error=anerror", "http://host/path?p2=2", []string{}, false},
69+
{"Vouch Proxy README (blank error)", "http://host/login?url=http://host/path?p2=2&vouch-failcount=&X-Vouch-Token=&error=", "http://host/path?p2=2", []string{}, false},
70+
{"Vouch Proxy README (semicolons, blank error)", "http://host/login?url=http://host/path?p2=2;p3=3&vouch-failcount=&X-Vouch-Token=&error=", "http://host/path?p2=2&p3=3", []string{}, false},
6971
// Nginx Ingress controler for Kubernetes adds the parameter `rd` to these calls
7072
// https://github.com/vouch/vouch-proxy/issues/289
71-
{"rd param appended by Nginx Ingress", "http://host/login?url=http://host/path?p2=2&p3=3&vouch-failcount=&X-Vouch-Token=&error=&rd=http%3a%2f%2fhost/path%3fp2=2%3bp3=3", "http://host/path?p2=2&p3=3", false},
73+
{"rd param appended by Nginx Ingress", "http://host/login?url=http://host/path?p2=2&p3=3&vouch-failcount=&X-Vouch-Token=&error=&rd=http%3a%2f%2fhost/path%3fp2=2%3bp3=3", "http://host/path?p2=2&p3=3", []string{}, false},
7274
}
7375
for _, tt := range tests {
7476
t.Run(tt.name, func(t *testing.T) {
7577
u, _ := url.Parse(tt.url)
76-
got, err := normalizeLoginURLParam(u)
78+
got, stray, err := normalizeLoginURLParam(u)
7779
if got.String() != tt.want {
7880
t.Errorf("normalizeLoginURLParam() = %v, want %v", got, tt.want)
7981
}
82+
if !cmp.Equal(stray, tt.wantStray) {
83+
t.Errorf("normalizeLoginURLParam() stray params incorrectly parsed, got %+q, expected %+q", stray, tt.wantStray)
84+
}
8085
if (err != nil) != tt.wantErr {
8186
t.Errorf("normalizeLoginURLParam() err = %v", err)
8287
}

0 commit comments

Comments
 (0)