Skip to content

Commit 7b132e4

Browse files
code-ashercode-asher
committed
Move redirect check into query parser
It will now be checked on the token side as well, although we do not actually use it there anyway. But this seems more right. The URL parser was added for this OAuth2 code so we can just rename it.
1 parent 6e3940d commit 7b132e4

File tree

3 files changed

+14
-27
lines changed

3 files changed

+14
-27
lines changed

‎coderd/httpapi/queryparams.go

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,14 +123,24 @@ func (p *QueryParamParser) UUIDs(vals url.Values, def []uuid.UUID, queryParam st
123123
})
124124
}
125125

126-
func (p *QueryParamParser) URL(vals url.Values, def *url.URL, queryParam string) *url.URL {
127-
v, err := parseQueryParam(p, vals, url.Parse, def, queryParam)
126+
func (p *QueryParamParser) RedirectURL(vals url.Values, base *url.URL, queryParam string) *url.URL {
127+
v, err := parseQueryParam(p, vals, url.Parse, base, queryParam)
128128
if err != nil {
129129
p.Errors = append(p.Errors, codersdk.ValidationError{
130130
Field: queryParam,
131131
Detail: fmt.Sprintf("Query param %q must be a valid url: %s", queryParam, err.Error()),
132132
})
133133
}
134+
135+
// It can be a sub-directory but not a sub-domain, as we have apps on
136+
// sub-domains and that seems too dangerous.
137+
if v.Host != base.Host || !strings.HasPrefix(v.Path, base.Path) {
138+
p.Errors = append(p.Errors, codersdk.ValidationError{
139+
Field: queryParam,
140+
Detail: fmt.Sprintf("Query param %q is invalid", queryParam),
141+
})
142+
}
143+
134144
return v
135145
}
136146

‎enterprise/coderd/identityprovider/authorize.go

Lines changed: 1 addition & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,8 @@ package identityprovider
33
import (
44
"database/sql"
55
"errors"
6-
"fmt"
76
"net/http"
87
"net/url"
9-
"strings"
108
"time"
119

1210
"github.com/google/uuid"
@@ -40,7 +38,7 @@ func extractAuthorizeParams(r *http.Request, callbackURL string) (authorizeParam
4038
}
4139
params := authorizeParams{
4240
clientID: p.String(vals, "", "client_id"),
43-
redirectURL: p.URL(vals, cb, "redirect_uri"),
41+
redirectURL: p.RedirectURL(vals, cb, "redirect_uri"),
4442
responseType: httpapi.ParseCustom(p, vals, "", "response_type", httpapi.ParseEnum[codersdk.OAuth2ProviderResponseType]),
4543
scope: p.Strings(vals, []string{}, "scope"),
4644
state: p.String(vals, "", "state"),
@@ -49,13 +47,6 @@ func extractAuthorizeParams(r *http.Request, callbackURL string) (authorizeParam
4947
// We add "redirected" when coming from the authorize page.
5048
_ = p.String(vals, "", "redirected")
5149

52-
if err := validateRedirectURL(cb, params.redirectURL.String()); err != nil {
53-
p.Errors = append(p.Errors, codersdk.ValidationError{
54-
Field: "redirect_uri",
55-
Detail: fmt.Sprintf("Query param %q is invalid", "redirect_uri"),
56-
})
57-
}
58-
5950
p.ErrorExcessParams(vals)
6051
return params, p.Errors, nil
6152
}
@@ -143,17 +134,3 @@ func Authorize(db database.Store, accessURL *url.URL) http.HandlerFunc {
143134
// Always wrap with its custom mw.
144135
return authorizeMW(accessURL)(http.HandlerFunc(handler)).ServeHTTP
145136
}
146-
147-
// validateRedirectURL validates that the redirectURL is contained in baseURL.
148-
func validateRedirectURL(baseURL *url.URL, redirectURL string) error {
149-
redirect, err := url.Parse(redirectURL)
150-
if err != nil {
151-
return err
152-
}
153-
// It can be a sub-directory but not a sub-domain, as we have apps on
154-
// sub-domains so it seems too dangerous.
155-
if redirect.Host != baseURL.Host || !strings.HasPrefix(redirect.Path, baseURL.Path) {
156-
return xerrors.New("invalid redirect URL")
157-
}
158-
return nil
159-
}

‎enterprise/coderd/identityprovider/tokens.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ func extractTokenParams(r *http.Request, callbackURL string) (tokenParams, []cod
5252
clientID: p.String(vals, "", "client_id"),
5353
clientSecret: p.String(vals, "", "client_secret"),
5454
code: p.String(vals, "", "code"),
55-
redirectURL: p.URL(vals, cb, "redirect_uri"),
55+
redirectURL: p.RedirectURL(vals, cb, "redirect_uri"),
5656
grantType: httpapi.ParseCustom(p, vals, "", "grant_type", httpapi.ParseEnum[codersdk.OAuth2ProviderGrantType]),
5757
}
5858

0 commit comments

Comments
 (0)