8000 Apparently browsers do not always set origin · coder/coder@d854f4d · GitHub
[go: up one dir, main page]
Skip to content

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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)
0