Skip to content

Commit d854f4d

Browse files
code-ashercode-asher
committed
Apparently browsers do not always set origin
The referer check was not quite right either.
1 parent cae4e61 commit d854f4d

File tree

2 files changed

+84
-79
lines changed

2 files changed

+84
-79
lines changed

‎enterprise/coderd/identityprovider/middleware.go

Lines changed: 81 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -12,17 +12,16 @@ import (
1212

1313
// authorizeMW serves to remove some code from the primary authorize handler.
1414
// It decides when to show the html allow page, and when to just continue.
15+
// TODO: For now only browser-based auth flow is officially supported but in a
16+
// future PR we should support a cURL-based flow.
1517
func authorizeMW(accessURL *url.URL) func(next http.Handler) http.Handler {
1618
return func(next http.Handler) http.Handler {
1719
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
1820
origin := r.Header.Get(httpmw.OriginHeader)
19-
// TODO: The origin can be blank from some clients, like cURL. For now
20-
// only browser-based auth flow is officially supported but in a future PR
21-
// we should support a cURL-based and blank origin flows.
2221
originU, err := url.Parse(origin)
23-
if err != nil || origin == "" {
22+
if err != nil {
2423
httpapi.Write(r.Context(), rw, http.StatusBadRequest, codersdk.Response{
25-
Message: "Invalid or missing origin header.",
24+
Message: "Invalid origin header.",
2625
Detail: err.Error(),
2726
})
2827
return
@@ -32,7 +31,7 @@ func authorizeMW(accessURL *url.URL) func(next http.Handler) http.Handler {
3231
refererU, err := url.Parse(referer)
3332
if err != nil {
3433
httpapi.Write(r.Context(), rw, http.StatusBadRequest, codersdk.Response{
35-
Message: "Invalid or missing referer header.",
34+
Message: "Invalid referer header.",
3635
Detail: err.Error(),
3736
})
3837
return
@@ -41,80 +40,90 @@ func authorizeMW(accessURL *url.URL) func(next http.Handler) http.Handler {
4140
app := httpmw.OAuth2ProviderApp(r)
4241
ua := httpmw.UserAuthorization(r)
4342

44-
// If the request comes from outside, then we show the html allow page.
45-
// TODO: Skip this step if the user has already clicked allow before, and
46-
// we can just reuse the token.
47-
if originU.Hostname() != accessURL.Hostname() && refererU.Path != "/login/oauth2/authorize" {
48-
if r.URL.Query().Get("redirected") != "" {
49-
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
50-
Status: http.StatusInternalServerError,
51-
HideStatus: false,
52-
Title: "Oauth Redirect Loop",
53-
Description: "Oauth redirect loop detected.",
54-
RetryEnabled: false,
55-
DashboardURL: accessURL.String(),
56-
Warnings: nil,
57-
})
58-
return
59-
}
43+
// url.Parse() allows empty URLs, which is fine because the origin is not
44+
// always set by browsers (or other tools like cURL). If the origin does
45+
// exist, we will make sure it matches. We require `referer` to be set at
46+
// a minimum, however.
47+
cameFromSelf := (origin == "" || originU.Hostname() == accessURL.Hostname()) &&
48+
refererU.Hostname() == accessURL.Hostname() &&
49+
refererU.Path == "/login/oauth2/authorize"
6050

61-
callbackURL, err := url.Parse(app.CallbackURL)
62-
if err != nil {
63-
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
64-
Status: http.StatusInternalServerError,
65-
HideStatus: false,
66-
Title: "Internal Server Error",
67-
Description: err.Error(),
68-
RetryEnabled: false,
69-
DashboardURL: accessURL.String(),
70-
Warnings: nil,
71-
})
72-
return
73-
}
51+
// If we were redirected here from this same page it means the user
52+
// pressed the allow button so defer to the authorize handler which
53+
// generates the code, otherwise show the HTML allow page.
54+
// TODO: Skip this step if the user has already clicked allow before, and
55+
// we can just reuse the token.
56+
if cameFromSelf {
57+
next.ServeHTTP(rw, r)
58+
return
59+
}
7460

75-
// Extract the form parameters for two reasons:
76-
// 1. We need the redirect URI to build the cancel URI.
77-
// 2. Since validation will run once the user clicks "allow", it is
78-
// better to validate now to avoid wasting the user's time clicking a
79-
// button that will just error anyway.
80-
params, validationErrs, err := extractAuthorizeParams(r, callbackURL)
81-
if err != nil {
82-
errStr := make([]string, len(validationErrs))
83-
for i, err := range validationErrs {
84-
errStr[i] = err.Detail
85-
}
86-
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
87-
Status: http.StatusBadRequest,
88-
HideStatus: false,
89-
Title: "Invalid Query Parameters",
90-
Description: "One or more query parameters are missing or invalid.",
91-
RetryEnabled: false,
92-
DashboardURL: accessURL.String(),
93-
Warnings: errStr,
94-
})
95-
return
96-
}
61+
if r.URL.Query().Get("redirected") != "" {
62+
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
63+
Status: http.StatusInternalServerError,
64+
HideStatus: false,
65+
Title: "Oauth Redirect Loop",
66+
Description: "Oauth redirect loop detected.",
67+
RetryEnabled: false,
68+
DashboardURL: accessURL.String(),
69+
Warnings: nil,
70+
})
71+
return
72+
}
9773

98-
cancel := params.redirectURL
99-
cancelQuery := params.redirectURL.Query()
100-
cancelQuery.Add("error", "access_denied")
101-
cancel.RawQuery = cancelQuery.Encode()
74+
callbackURL, err := url.Parse(app.CallbackURL)
75+
if err != nil {
76+
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
77+
Status: http.StatusInternalServerError,
78+
HideStatus: false,
79+
Title: "Internal Server Error",
80+
Description: err.Error(),
81+
RetryEnabled: false,
82+
DashboardURL: accessURL.String(),
83+
Warnings: nil,
84+
})
85+
return
86+
}
10287

103-
redirect := r.URL
104-
vals := redirect.Query()
105-
vals.Add("redirected", "true")
106-
r.URL.RawQuery = vals.Encode()
107-
site.RenderOAuthAllowPage(rw, r, site.RenderOAuthAllowData{
108-
AppIcon: app.Icon,
109-
AppName: app.Name,
110-
CancelURI: cancel.String(),
111-
RedirectURI: r.URL.String(),
112-
Username: ua.ActorName,
88+
// Extract the form parameters for two reasons:
89+
// 1. We need the redirect URI to build the cancel URI.
90+
// 2. Since validation will run once the user clicks "allow", it is
91+
// better to validate now to avoid wasting the user's time clicking a
92+
// button that will just error anyway.
93+
params, validationErrs, err := extractAuthorizeParams(r, callbackURL)
94+
if err != nil {
95+
errStr := make([]string, len(validationErrs))
96+
for i, err := range validationErrs {
97+
errStr[i] = err.Detail
98+
}
99+
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
100+
Status: http.StatusBadRequest,
101+
HideStatus: false,
102+
Title: "Invalid Query Parameters",
103+
Description: "One or more query parameters are missing or invalid.",
104+
RetryEnabled: false,
105+
DashboardURL: accessURL.String(),
106+
Warnings: errStr,
113107
})
114108
return
115109
}
116110

117-
next.ServeHTTP(rw, r)
111+
cancel := params.redirectURL
112+
cancelQuery := params.redirectURL.Query()
113+
cancelQuery.Add("error", "access_denied")
114+
cancel.RawQuery = cancelQuery.Encode()
115+
116+
redirect := r.URL
117+
vals := redirect.Query()
118+
vals.Add("redirected", "true") // For loop detection.
119+
r.URL.RawQuery = vals.Encode()
120+
site.RenderOAuthAllowPage(rw, r, site.RenderOAuthAllowData{
121+
AppIcon: app.Icon,
122+
AppName: app.Name,
123+
CancelURI: cancel.String(),
124+
RedirectURI: r.URL.String(),
125+
Username: ua.ActorName,
126+
})
118127
})
119128
}
120129
}

‎enterprise/coderd/oauth2_test.go

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import (
1919
"github.com/coder/coder/v2/coderd/database"
2020
"github.com/coder/coder/v2/coderd/database/dbtestutil"
2121
"github.com/coder/coder/v2/coderd/database/dbtime"
22-
"github.com/coder/coder/v2/coderd/httpmw"
2322
"github.com/coder/coder/v2/coderd/userpassword"
2423
"github.com/coder/coder/v2/coderd/util/ptr"
2524
"github.com/coder/coder/v2/codersdk"
@@ -1131,13 +1130,10 @@ func authorizationFlow(ctx context.Context, client *codersdk.Client, cfg *oauth2
11311130
return http.ErrUseLastResponse
11321131
}
11331132
return client.Request(ctx, req.Method, req.URL.String(), nil, func(req *http.Request) {
1134-
// Set some headers so the request bypasses the HTML page (normally you
1133+
// Set the referer so the request bypasses the HTML page (normally you
11351134
// have to click "allow" first, and the way we detect that is using the
1136-
// origin and referer headers).
1137-
// Normally origin does not include the path, but it is not relevant to
1138-
// the check we make.
1139-
req.Header.Set(httpmw.OriginHeader, client.URL.String())
1140-
req.Header.Set("Referer", client.URL.String())
1135+
// referer header).
1136+
req.Header.Set("Referer", req.URL.String())
11411137
})
11421138
},
11431139
)

0 commit comments

Comments
 (0)