devurls working + tests

coder · deansheather · Sep 13, 2022 · Aug 26, 2022 · Aug 26, 2022 · Aug 29, 2022
commit f3c664576e7b14ec0b173fed14a3e9f919e668bf
diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -63,6 +63,7 @@ import (
 )
 
 type Options struct {
+	AccessURL            *url.URL
 	AWSCertificates      awsidentity.Certificates
 	Authorizer           rbac.Authorizer
 	AzureCertificates    x509.VerifyOptions
@@ -197,13 +198,19 @@ func newWithAPI(t *testing.T, options *Options) (*codersdk.Client, io.Closer, *c
 		_ = turnServer.Close()
 	})
 
+	accessURL := options.AccessURL
+	if accessURL == nil {
+		accessURL, err = url.Parse(srv.URL)
+		require.NoError(t, err)
+	}
+
 	// We set the handler after server creation for the access URL.
 	coderAPI := options.APIBuilder(&coderd.Options{
 		AgentConnectionUpdateFrequency: 150 * time.Millisecond,
 		// Force a long disconnection timeout to ensure
 		// agents are not marked as disconnected during slow tests.
 		AgentInactiveDisconnectTimeout: testutil.WaitShort,
-		AccessURL:                      serverURL,
+		AccessURL:                      accessURL,
 		Logger:                         slogtest.Make(t, nil).Leveled(slog.LevelDebug),
 		CacheDir:                       t.TempDir(),
 		Database:                       db,

diff --git a/coderd/httpmw/logger.go b/coderd/httpmw/logger.go
@@ -42,15 +42,13 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler {
 				)
 			}
 
+			// We should not log at level ERROR for 5xx status codes because 5xx
+			// includes proxy errors etc. It also causes slogtest to fail
+			// instantly without an error message by default.
 			logLevelFn := httplog.Debug
 			if sw.Status >= 400 {
 				logLevelFn = httplog.Warn
 			}
-			if sw.Status >= 500 {
-				// Server errors should be treated as an ERROR
-				// log level.
-				logLevelFn = httplog.Error
-			}
 
 			logLevelFn(r.Context(), r.Method)
 		})

diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -162,8 +162,7 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	http.SetCookie(rw, cookie)
-	http.SetCookie(rw, api.applicationCookie(cookie))
+	api.setAuthCookie(rw, cookie)
 
 	redirect := state.Redirect
 	if redirect == "" {
@@ -297,8 +296,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	http.SetCookie(rw, cookie)
-	http.SetCookie(rw, api.applicationCookie(cookie))
+	api.setAuthCookie(rw, cookie)
 
 	redirect := state.Redirect
 	if redirect == "" {

diff --git a/coderd/users.go b/coderd/users.go
@@ -809,8 +809,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	http.SetCookie(rw, cookie)
-	http.SetCookie(rw, api.applicationCookie(cookie))
+	api.setAuthCookie(rw, cookie)
 
 	httpapi.Write(rw, http.StatusCreated, codersdk.LoginWithPasswordResponse{
 		SessionToken: cookie.Value,
@@ -887,9 +886,12 @@ func (api *API) postLogout(rw http.ResponseWriter, r *http.Request) {
 		Name:   codersdk.SessionTokenKey,
 		Path:   "/",
 	}
-
 	http.SetCookie(rw, cookie)
-	http.SetCookie(rw, api.applicationCookie(cookie))
+
+	devurlCookie := api.applicationCookie(cookie)
+	if devurlCookie != nil {
+		http.SetCookie(rw, devurlCookie)
+	}
 
 	// Delete the session token from database.
 	apiKey := httpmw.APIKey(r)
@@ -1069,6 +1071,15 @@ func (api *API) createUser(ctx context.Context, store database.Store, req create
 	})
 }
 
+func (api *API) setAuthCookie(rw http.ResponseWriter, cookie *http.Cookie) {
+	http.SetCookie(rw, cookie)
+
+	devurlCookie := api.applicationCookie(cookie)
+	if devurlCookie != nil {
+		http.SetCookie(rw, devurlCookie)
+	}
+}
+
 func convertUser(user database.User, organizationIDs []uuid.UUID) codersdk.User {
 	convertedUser := codersdk.User{
 		ID:              user.ID,

diff --git a/coderd/users_test.go b/coderd/users_test.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"net/url"
 	"sort"
 	"strings"
 	"testing"
@@ -261,7 +262,12 @@ func TestPostLogout(t *testing.T) {
 	t.Run("Logout", func(t *testing.T) {
 		t.Parallel()
 
-		client := coderdtest.New(t, nil)
+		accessURL, err := url.Parse("http://somedomain.com")
+		require.NoError(t, err)
+
+		client := coderdtest.New(t, &coderdtest.Options{
+			AccessURL: accessURL,
+		})
 		admin := coderdtest.CreateFirstUser(t, client)
 
 		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)

diff --git a/coderd/workspaceapps.go b/coderd/workspaceapps.go
@@ -2,6 +2,7 @@ package coderd
 
 import (
 	"fmt"
+	"net"
 	"net/http"
 	"net/http/httputil"
 	"net/url"
@@ -42,11 +43,12 @@ func (api *API) workspaceAppsProxyPath(rw http.ResponseWriter, r *http.Request)
 
 	appName, port := AppNameOrPort(chi.URLParam(r, "workspaceapp"))
 	api.proxyWorkspaceApplication(proxyApplication{
-		Workspace: workspace,
-		Agent:     agent,
-		AppName:   appName,
-		Port:      port,
-		Path:      chiPath,
+		Workspace:        workspace,
+		Agent:            agent,
+		AppName:          appName,
+		Port:             port,
+		Path:             chiPath,
+		DashboardOnError: true,
 	}, rw, r)
 }
 
@@ -87,11 +89,12 @@ func (api *API) handleSubdomainApplications(middlewares ...func(http.Handler) ht
 				agent := httpmw.WorkspaceAgentParam(r)
 
 				api.proxyWorkspaceApplication(proxyApplication{
-					Workspace: workspace,
-					Agent:     agent,
-					AppName:   app.AppName,
-					Port:      app.Port,
-					Path:      r.URL.Path,
+					Workspace:        workspace,
+					Agent:            agent,
+					AppName:          app.AppName,
+					Port:             app.Port,
+					Path:             r.URL.Path,
+					DashboardOnError: false,
 				}, rw, r)
 			})).ServeHTTP(rw, r.WithContext(ctx))
 		})
@@ -108,6 +111,11 @@ type proxyApplication struct {
 	Port    uint16
 	// Path must either be empty or have a leading slash.
 	Path string
+
+	// DashboardOnError determines whether or not the dashboard should be
+	// rendered on error. This should be set for proxy path URLs but not
+	// hostname based URLs.
+	DashboardOnError bool
 }
 
 func (api *API) proxyWorkspaceApplication(proxyApp proxyApplication, rw http.ResponseWriter, r *http.Request) {
@@ -178,14 +186,21 @@ func (api *API) proxyWorkspaceApplication(proxyApp proxyApplication, rw http.Res
 
 	proxy := httputil.NewSingleHostReverseProxy(appURL)
 	proxy.ErrorHandler = func(w http.ResponseWriter, r *http.Request, err error) {
-		// This is a browser-facing route so JSON responses are not viable here.
-		// To pass friendly errors to the frontend, special meta tags are overridden
-		// in the index.html with the content passed here.
-		r = r.WithContext(site.WithAPIResponse(r.Context(), site.APIResponse{
-			StatusCode: http.StatusBadGateway,
-			Message:    err.Error(),
-		}))
-		api.siteHandler.ServeHTTP(w, r)
+		if proxyApp.DashboardOnError {
+			// To pass friendly errors to the frontend, special meta tags are
+			// overridden in the index.html with the content passed here.
+			r = r.WithContext(site.WithAPIResponse(r.Context(), site.APIResponse{
+				StatusCode: http.StatusBadGateway,
+				Message:    err.Error(),
+			}))
+			api.siteHandler.ServeHTTP(w, r)
+			return
+		}
+
+		httpapi.Write(w, http.StatusBadGateway, codersdk.Response{
+			Message: "Failed to proxy request to application.",
+			Detail:  err.Error(),
+		})
 	}
 
 	conn, release, err := api.workspaceAgentCache.Acquire(r, proxyApp.Agent.ID)
@@ -234,6 +249,16 @@ type ApplicationURL struct {
 	BaseHostname string
 }
 
+// String returns the application URL hostname without scheme.
+func (a ApplicationURL) String() string {
+	appNameOrPort := a.AppName
+	if a.Port != 0 {
+		appNameOrPort = strconv.Itoa(int(a.Port))
+	}
+
+	return fmt.Sprintf("%s--%s--%s--%s.%s", appNameOrPort, a.AgentName, a.WorkspaceName, a.Username, a.BaseHostname)
+}
+
 // ParseSubdomainAppURL parses an application from the subdomain of r's Host
 // header. If the subdomain is not a valid application URL hostname, returns a
 // non-nil error.
@@ -297,11 +322,17 @@ func AppNameOrPort(val string) (string, uint16) {
 // only do application authentication, we will just reuse the original token.
 // This code should be temporary and be replaced with something that creates
 // a unique session_token.
+//
+// Returns nil if the access URL isn't a hostname.
 func (api *API) applicationCookie(authCookie *http.Cookie) *http.Cookie {
+	if net.ParseIP(api.AccessURL.Hostname()) != nil {
+		return nil
+	}
+
 	appCookie := *authCookie
-	// We only support setting this cookie on the access url subdomains.
-	// This is to ensure we don't accidentally leak the auth cookie to subdomains
-	// on another hostname.
+	// We only support setting this cookie on the access URL subdomains. This is
+	// to ensure we don't accidentally leak the auth cookie to subdomains on
+	// another hostname.
 	appCookie.Domain = "." + api.AccessURL.Hostname()
 	return &appCookie
 }