Skip to content

Commit 5993f85

Browse files
deansheatherEmyrk
deansheather
and
Emyrk
authored
fix: avoid redirect loop on workspace proxies (coder#9389)
* fix: avoid redirect loop on workspace proxies --------- Co-authored-by: Steven Masley <stevenmasley@coder.com>
1 parent eb68684 commit 5993f85

12 files changed

Lines changed: 265 additions & 99 deletions

File tree

‎coderd/httpapi/cookie.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,9 @@ func StripCoderCookies(header string) string {
2323
if name == codersdk.SessionTokenCookie ||
2424
name == codersdk.OAuth2StateCookie ||
2525
name == codersdk.OAuth2RedirectCookie ||
26-
name == codersdk.DevURLSessionTokenCookie ||
27-
name == codersdk.DevURLSignedAppTokenCookie {
26+
name == codersdk.PathAppSessionTokenCookie ||
27+
name == codersdk.SubdomainAppSessionTokenCookie ||
28+
name == codersdk.SignedAppTokenCookie {
2829
continue
2930
}
3031
cookies = append(cookies, part)

‎coderd/httpmw/apikey.go‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -447,10 +447,10 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
447447
// APITokenFromRequest returns the api token from the request.
448448
// Find the session token from:
449449
// 1: The cookie
450-
// 1: The devurl cookie
451-
// 3: The old cookie
452-
// 4. The coder_session_token query parameter
453-
// 5. The custom auth header
450+
// 2. The coder_session_token query parameter
451+
// 3. The custom auth header
452+
//
453+
// API tokens for apps are read from workspaceapps/cookies.go.
454454
func APITokenFromRequest(r *http.Request) string {
455455
cookie, err := r.Cookie(codersdk.SessionTokenCookie)
456456
if err == nil && cookie.Value != "" {
@@ -467,11 +467,6 @@ func APITokenFromRequest(r *http.Request) string {
467467
return headerValue
468468
}
469469

470-
cookie, err = r.Cookie(codersdk.DevURLSessionTokenCookie)
471-
if err == nil && cookie.Value != "" {
472-
return cookie.Value
473-
}
474-
475470
return ""
476471
}
477472

‎coderd/workspaceapps/apptest/apptest.go‎

Lines changed: 16 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
257257

258258
var appTokenCookie *http.Cookie
259259
for _, c := range resp.Cookies() {
260-
if c.Name == codersdk.DevURLSignedAppTokenCookie {
260+
if c.Name == codersdk.SignedAppTokenCookie {
261261
appTokenCookie = c
262262
break
263263
}
@@ -302,7 +302,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
302302

303303
var appTokenCookie *http.Cookie
304304
for _, c := range resp.Cookies() {
305-
if c.Name == codersdk.DevURLSignedAppTokenCookie {
305+
if c.Name == codersdk.SignedAppTokenCookie {
306306
appTokenCookie = c
307307
break
308308
}
@@ -400,30 +400,19 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
400400
appDetails := setupProxyTest(t, nil)
401401

402402
cases := []struct {
403-
name string
404-
appURL *url.URL
405-
verifyCookie func(t *testing.T, c *http.Cookie)
403+
name string
404+
appURL *url.URL
405+
sessionTokenCookieName string
406406
}{
407407
{
408-
name: "Subdomain",
409-
appURL: appDetails.SubdomainAppURL(appDetails.Apps.Owner),
410-
verifyCookie: func(t *testing.T, c *http.Cookie) {
411-
// TODO(@dean): fix these asserts, they don't seem to
412-
// work. I wonder if Go strips the domain from the
413-
// cookie object if it's invalid or something.
414-
// domain := strings.SplitN(appDetails.Options.AppHost, ".", 2)
415-
// require.Equal(t, "."+domain[1], c.Domain, "incorrect domain on app token cookie")
416-
},
408+
name: "Subdomain",
409+
appURL: appDetails.SubdomainAppURL(appDetails.Apps.Owner),
410+
sessionTokenCookieName: codersdk.SubdomainAppSessionTokenCookie,
417411
},
418412
{
419-
name: "Path",
420-
appURL: appDetails.PathAppURL(appDetails.Apps.Owner),
421-
verifyCookie: func(t *testing.T, c *http.Cookie) {
422-
// TODO(@dean): fix these asserts, they don't seem to
423-
// work. I wonder if Go strips the domain from the
424-
// cookie object if it's invalid or something.
425-
// require.Equal(t, "", c.Domain, "incorrect domain on app token cookie")
426-
},
413+
name: "Path",
414+
appURL: appDetails.PathAppURL(appDetails.Apps.Owner),
415+
sessionTokenCookieName: codersdk.PathAppSessionTokenCookie,
427416
},
428417
}
429418

@@ -508,14 +497,13 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
508497

509498
cookies := resp.Cookies()
510499
var cookie *http.Cookie
511-
for _, c := range cookies {
512-
if c.Name == codersdk.DevURLSessionTokenCookie {
513-
cookie = c
500+
for _, co := range cookies {
501+
if co.Name == c.sessionTokenCookieName {
502+
cookie = co
514503
break
515504
}
516505
}
517506
require.NotNil(t, cookie, "no app session token cookie was set")
518-
c.verifyCookie(t, cookie)
519507
apiKey := cookie.Value
520508

521509
// Fetch the API key from the API.
@@ -715,7 +703,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
715703

716704
var appTokenCookie *http.Cookie
717705
for _, c := range resp.Cookies() {
718-
if c.Name == codersdk.DevURLSignedAppTokenCookie {
706+
if c.Name == codersdk.SignedAppTokenCookie {
719707
appTokenCookie = c
720708
break
721709
}
@@ -759,7 +747,7 @@ func Run(t *testing.T, appHostIsPrimary bool, factory DeploymentFactory) {
759747

760748
var appTokenCookie *http.Cookie
761749
for _, c := range resp.Cookies() {
762-
if c.Name == codersdk.DevURLSignedAppTokenCookie {
750+
if c.Name == codersdk.SignedAppTokenCookie {
763751
appTokenCookie = c
764752
break
765753
}

‎coderd/workspaceapps/cookies.go‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
package workspaceapps
2+
3+
import (
4+
"net/http"
5+
6+
"github.com/coder/coder/v2/coderd/httpmw"
7+
"github.com/coder/coder/v2/codersdk"
8+
)
9+
10+
// AppConnectSessionTokenCookieName returns the cookie name for the session
11+
// token for the given access method.
12+
func AppConnectSessionTokenCookieName(accessMethod AccessMethod) string {
13+
if accessMethod == AccessMethodSubdomain {
14+
return codersdk.SubdomainAppSessionTokenCookie
15+
}
16+
return codersdk.PathAppSessionTokenCookie
17+
}
18+
19+
// AppConnectSessionTokenFromRequest returns the session token from the request
20+
// if it exists. The access method is used to determine which cookie name to
21+
// use.
22+
//
23+
// We use different cookie names for path apps and for subdomain apps to avoid
24+
// both being set and sent to the server at the same time and the server using
25+
// the wrong value.
26+
//
27+
// We use different cookie names for:
28+
// - path apps on primary access URL: coder_session_token
29+
// - path apps on proxies: coder_path_app_session_token
30+
// - subdomain apps: coder_subdomain_app_session_token
31+
//
32+
// First we try the default function to get a token from request, which supports
33+
// query parameters, the Coder-Session-Token header and the coder_session_token
34+
// cookie.
35+
//
36+
// Then we try the specific cookie name for the access method.
37+
func AppConnectSessionTokenFromRequest(r *http.Request, accessMethod AccessMethod) string {
38+
// Try the default function first.
39+
token := httpmw.APITokenFromRequest(r)
40+
if token != "" {
41+
return token
42+
}
43+
44+
// Then try the specific cookie name for the access method.
45+
cookie, err := r.Cookie(AppConnectSessionTokenCookieName(accessMethod))
46+
if err == nil && cookie.Value != "" {
47+
return cookie.Value
48+
}
49+
50+
return ""
51+
}

0 commit comments

Comments
 (0)