refactor: rework scheme validation in oauth controller and frontend (#1026)

frontend/src/lib/hooks/redirect-uri.ts

@@ -75,19 +75,6 @@ export const useRedirectUri = (
   };
 };
 
-// ported from internal/controller/oauth_controller.go
-const getEffectivePort = (url: URL): string => {
-  if (url.port) {
-    return url.port;
-  }
-
-  if (url.protocol == "https:") {
-    return "443";
-  }
-
-  return "80";
-};
-
 // https://www.geeksforgeeks.org/javascript/how-to-check-if-a-string-is-a-valid-ip-address-format-in-javascript
 const isIP = (str: string): boolean => {
   const ipv4 =
@@ -114,7 +101,7 @@ export const isTrustedDomain = (
     return false;
   }
 
-  if (getEffectivePort(url) != getEffectivePort(appUrl)) {
+  if (url.port != appUrl.port) {
     return false;
   }
 

internal/controller/oauth_controller.go

@@ -331,8 +331,7 @@ func (controller *OAuthController) isRedirectSafe(redirectURI string) bool {
 
 	controller.log.App.Debug().Err(err).Msg("Failed to validate redirect URI")
 
-	if errors.Is(err, validators.ErrInvalidURL) ||
+	if !errors.Is(err, validators.ErrHostnameMismatch) {
-		errors.Is(err, validators.ErrPortMismatch) {
 		return false
 	}
 

internal/controller/oauth_controller_test.go

@@ -81,14 +81,6 @@ func TestOAuthController_isRedirectSafe(t *testing.T) {
 			redirectURI:       "https://sub.example.com",
 			expected:          false,
 		},
-		{
-			description:       "Different scheme returns false",
-			appURL:            "https://tinyauth.example.com",
-			cookieDomain:      "example.com",
-			subdomainsEnabled: true,
-			redirectURI:       "http://tinyauth.example.com",
-			expected:          false,
-		},
 		{
 			description:       "Different port returns false",
 			appURL:            "https://tinyauth.example.com",

internal/service/access_controls_service.go

@@ -50,9 +50,7 @@ func (service *AccessControlsService) lookupStaticACLs(domain string) *model.App
 				service.log.App.Debug().Str("name", app).Msg("Found matching container by domain")
 				return &config
 			}
-			if !errors.Is(err, validators.ErrHostnameMismatch) &&
+			if !errors.Is(err, validators.ErrHostnameMismatch) {
-				!errors.Is(err, validators.ErrPortMismatch) &&
-				!errors.Is(err, validators.ErrSchemeMismatch) {
 				service.log.App.Debug().Str("name", app).Err(err).Msg("Domain validation failed")
 			}
 		}

pkg/README.md

@@ -0,0 +1,10 @@
+# Public packages
+
+This directory contains packages that can be used by
+other projects.
+
+While we try to maintain a consistent API, no promises
+can be made for non-breaking changes throughout updates
+as we constantly need to make changes to comply with the
+needs of Tinyauth. We advise pinning the version of the
+package you wish to use.

pkg/validators/domain_validator.go

@@ -10,14 +10,13 @@ import (
 	"fmt"
 	"net"
 	"net/url"
-	"slices"
 	"strings"
 
 	"golang.org/x/net/idna"
 )
 
+// Errors
 var (
-	ErrInvalidURL       = fmt.Errorf("invalid url")
 	ErrSchemeMismatch   = fmt.Errorf("scheme mismatch")
 	ErrPortMismatch     = fmt.Errorf("port mismatch")
 	ErrHostnameMismatch = fmt.Errorf("hostname mismatch")
@@ -29,8 +28,7 @@ type DomainValidatorOptions struct {
 	WithScheme bool
 	// Ensure domains have the same port.
 	WithPort bool
-	// Specify a list of allowed schemes IF WithScheme is set to true.
+	// Specify a list of allowed schemes if WithScheme is set to true.
-	// Leave empty to allow any scheme.
 	AllowedSchemes []string
 }
 
@@ -48,53 +46,74 @@ func NewDomainValidator(opts DomainValidatorOptions) *DomainValidator {
 	}
 }
 
+func (v *DomainValidator) checkScheme(rawURL string) error {
+	if !v.opts.WithScheme {
+		return nil
+	}
+
+	if len(v.opts.AllowedSchemes) == 0 {
+		return fmt.Errorf("allowed schemes must be specified")
+	}
+
+	for _, scheme := range v.opts.AllowedSchemes {
+		if strings.HasPrefix(strings.ToLower(rawURL), strings.ToLower(scheme)+"://") {
+			return nil
+		}
+	}
+
+	return fmt.Errorf("invalid scheme")
+
+}
+
 func (v *DomainValidator) getURL(i string) (*url.URL, error) {
-	u, err := url.Parse(i)
+	if i == "" {
-
+		return nil, fmt.Errorf("url cannot be empty")
-	if !v.opts.WithScheme && (err != nil || u.Host == "") {
-		u, err = url.Parse("tinyauth://" + i)
-	}
-
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse input url: %w", err)
-	}
-
-	if u.Host == "" {
-		return nil, ErrInvalidURL
-	}
-
-	if v.opts.WithPort && u.Port() == "" && (u.Scheme != "http" && u.Scheme != "https") {
-		return nil, fmt.Errorf("port validation is enabled but port is missing in input url and schemes are not enabled")
 	}
 
 	if v.opts.WithScheme {
-		// Empty scheme means that we parsed the url with the tinyauth:// placeholder
+		err := v.checkScheme(i)
-		if u.Scheme == "tinyauth" {
+
-			return nil, fmt.Errorf("input url is missing scheme")
+		if err != nil {
+			return nil, fmt.Errorf("invalid scheme: %w", err)
 		}
-		if len(v.opts.AllowedSchemes) > 0 && !slices.Contains(v.opts.AllowedSchemes, u.Scheme) {
+
-			return nil, fmt.Errorf("scheme %s not allowed", u.Scheme)
+		u, err := url.Parse(i)
+
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse input url: %w", err)
 		}
+
+		if u.Host == "" || u.Scheme == "" {
+			return nil, fmt.Errorf("missing host or scheme in url: %s", i)
+		}
+
+		return u, nil
+	}
+
+	rawURL := i
+
+	if !strings.Contains(i, "://") {
+		// From godoc: [scheme:][//[userinfo@]host][/]path[?query][#fragment]
+		// So, we can omit the colon and tell the Go URL lib that we want
+		// to parse the URL without the scheme. If we don't do this,
+		// the URL lib will parse our entire domain as the path.
+		rawURL = "//" + i
+	}
+
+	u, err := url.Parse(rawURL)
+
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse host: %w", err)
+	}
+
+	if u.Host == "" {
+		return nil, fmt.Errorf("missing host in url: %s", i)
 	}
 
 	return u, nil
 }
 
-func (v *DomainValidator) getEffectivePort(u *url.URL) (string, bool) {
+func (v *DomainValidator) getHostname(hostname string) (string, error) {
-	if u.Port() != "" {
-		return u.Port(), true
-	}
-	switch u.Scheme {
-	case "http":
-		return "80", true
-	case "https":
-		return "443", true
-	default:
-		return "", false
-	}
-}
-
-func (v *DomainValidator) formatHostname(hostname string) (string, error) {
 	hostname = strings.ToLower(hostname)
 	hostname = strings.TrimSuffix(hostname, ".")
 	if net.ParseIP(hostname) != nil {
@@ -133,26 +152,18 @@ func (v *DomainValidator) Validate(expected, actual string) error {
 	}
 
 	if v.opts.WithPort {
-		eup, ok := v.getEffectivePort(eu)
+		if eu.Port() != au.Port() {
-		if !ok {
-			return fmt.Errorf("failed to get effective port for url: %s", eu.String())
-		}
-		aup, ok := v.getEffectivePort(au)
-		if !ok {
-			return fmt.Errorf("failed to get effective port for url: %s", au.String())
-		}
-		if eup != aup {
 			return ErrPortMismatch
 		}
 	}
 
-	euf, err := v.formatHostname(eu.Hostname())
+	euf, err := v.getHostname(eu.Hostname())
 
 	if err != nil {
 		return err
 	}
 
-	auf, err := v.formatHostname(au.Hostname())
+	auf, err := v.getHostname(au.Hostname())
 
 	if err != nil {
 		return err
@@ -165,7 +176,7 @@ func (v *DomainValidator) Validate(expected, actual string) error {
 	return nil
 }
 
-// SafeHostname uses the internal validation for domains that Validator uses
+// SafeHostname uses the internal validation for domains that the validator uses
 // to parse a hostname. It ensures the input URL is a valid URL, that a host
 // is present and that the hostname is lowercased and without a trailing dot.
 func (v *DomainValidator) SafeHostname(input string) (string, error) {
@@ -175,5 +186,5 @@ func (v *DomainValidator) SafeHostname(input string) (string, error) {
 		return "", err
 	}
 
-	return v.formatHostname(u.Hostname())
+	return v.getHostname(u.Hostname())
 }

pkg/validators/domain_validator_test.go

@@ -20,16 +20,36 @@ func TestDomainValidator_SafeHostname(t *testing.T) {
 		{
 			description: "Empty url fails",
 			errorFunc: func(t *testing.T, e error) {
-				assert.ErrorIs(t, e, ErrInvalidURL)
+				assert.ErrorContains(t, e, "url cannot be empty")
+			},
+		},
+		{
+			description: "URL without host fails",
+			input:       "/foo",
+			errorFunc: func(t *testing.T, e error) {
+				assert.ErrorContains(t, e, "missing host in url")
 			},
 		},
 		{
 			description: "Invalid url fails",
 			input:       "foo:foo",
+			errorFunc: func(t *testing.T, e error) {
+				assert.ErrorContains(t, e, "failed to parse host")
+			},
+		},
+		{
+			description: "With scheme and invalid url should fail",
+			options:     DomainValidatorOptions{WithScheme: true, AllowedSchemes: []string{"https"}},
+			input:       "https://foo:foo",
 			errorFunc: func(t *testing.T, e error) {
 				assert.ErrorContains(t, e, "failed to parse input url")
 			},
 		},
+		{
+			description: "Scheme disabled with scheme should pass",
+			input:       "https://example.com",
+			expected:    "example.com",
+		},
 		{
 			description: "Domain without scheme should parse if scheme is disabled",
 			input:       "example.com",
@@ -40,7 +60,7 @@ func TestDomainValidator_SafeHostname(t *testing.T) {
 			options:     DomainValidatorOptions{WithScheme: true},
 			input:       "example.com",
 			errorFunc: func(t *testing.T, e error) {
-				assert.ErrorIs(t, e, ErrInvalidURL)
+				assert.ErrorContains(t, e, "invalid scheme")
 			},
 		},
 		{
@@ -48,7 +68,7 @@ func TestDomainValidator_SafeHostname(t *testing.T) {
 			options:     DomainValidatorOptions{WithScheme: true, AllowedSchemes: []string{"https"}},
 			input:       "foo://example.com",
 			errorFunc: func(t *testing.T, e error) {
-				assert.ErrorContains(t, e, "foo not allowed")
+				assert.ErrorContains(t, e, "invalid scheme")
 			},
 		},
 		{
@@ -94,32 +114,9 @@ func TestDomainValidator_SafeHostname(t *testing.T) {
 			},
 		},
 		{
-			// Placeholder should not be used by users and is reserved for the validator.
+			description: "With port enabled without any port should work",
-			// Using it is like not using any scheme for the validator, and thus it will fail
-			// with schemes enabled.
-			description: "Placeholder scheme supplied directly should fail",
-			options:     DomainValidatorOptions{WithScheme: true, AllowedSchemes: []string{"https"}},
-			input:       "tinyauth://example.com",
-			errorFunc: func(t *testing.T, e error) {
-				assert.ErrorContains(t, e, "input url is missing scheme")
-			},
-		},
-		{
-			description: "With port enabled but without scheme and url with https should work",
 			options:     DomainValidatorOptions{WithPort: true},
-			input:       "https://example.com",
+			input:       "example.com",
-			expected:    "example.com",
-		},
-		{
-			description: "With port enabled but without scheme and url with http should work",
-			options:     DomainValidatorOptions{WithPort: true},
-			input:       "http://example.com",
-			expected:    "example.com",
-		},
-		{
-			description: "With port enabled but without scheme and url with port should work",
-			options:     DomainValidatorOptions{WithPort: true},
-			input:       "example.com:8080",
 			expected:    "example.com",
 		},
 	}
@@ -153,7 +150,7 @@ func TestDomainValidator_Validate(t *testing.T) {
 			expected:    "foo:foo",
 			actual:      "bar.com",
 			errorFunc: func(t *testing.T, e error) {
-				assert.ErrorContains(t, e, "failed to parse input url:")
+				assert.ErrorContains(t, e, "failed to parse host:")
 			},
 		},
 		{
@@ -161,7 +158,7 @@ func TestDomainValidator_Validate(t *testing.T) {
 			expected:    "example.com",
 			actual:      "foo:foo",
 			errorFunc: func(t *testing.T, e error) {
-				assert.ErrorContains(t, e, "failed to parse input url:")
+				assert.ErrorContains(t, e, "failed to parse host:")
 			},
 		},
 		{
@@ -180,70 +177,22 @@ func TestDomainValidator_Validate(t *testing.T) {
 			actual:      "https://example.com",
 		},
 		{
-			description: "Port validation without ports and schemes disabled should fail",
+			description: "Port validation with ports enabled and empty ports should work",
 			options:     DomainValidatorOptions{WithPort: true},
 			expected:    "example.com",
 			actual:      "example.com",
-			errorFunc: func(t *testing.T, e error) {
-				assert.ErrorContains(t, e, "port validation is enabled but port is missing in input url and schemes are not enabled")
-			},
 		},
 		{
-			description: "Port validation with no port and http should pass",
+			description: "Port validation should pass with same port",
-			options:     DomainValidatorOptions{WithPort: true, WithScheme: true, AllowedSchemes: []string{"http"}},
-			expected:    "http://example.com",
-			actual:      "http://example.com",
-		},
-		{
-			description: "Port validation with no port and https should pass",
-			options:     DomainValidatorOptions{WithPort: true, WithScheme: true, AllowedSchemes: []string{"https"}},
-			expected:    "https://example.com",
-			actual:      "https://example.com",
-		},
-		{
-			description: "Port validation with port and no scheme should pass with same port",
 			options:     DomainValidatorOptions{WithPort: true},
 			expected:    "example.com:8080",
 			actual:      "example.com:8080",
 		},
 		{
-			description: "Domains with unknown scheme and port enabled but no port should fail",
+			description: "Port enabled with scheme and matching port should pass",
-			options:     DomainValidatorOptions{WithPort: true, WithScheme: true},
-			expected:    "ssh://example.com:22",
-			actual:      "ssh://example.com",
-			errorFunc: func(t *testing.T, e error) {
-				assert.ErrorContains(t, e, "port validation is enabled but port is missing in input url and schemes are not enabled")
-			},
-		},
-		{
-			description: "Domains with unknown scheme and port enabled but no port should fail, reverse",
-			options:     DomainValidatorOptions{WithPort: true, WithScheme: true},
-			expected:    "ssh://example.com",
-			actual:      "ssh://example.com:22",
-			errorFunc: func(t *testing.T, e error) {
-				assert.ErrorContains(t, e, "port validation is enabled but port is missing in input url and schemes are not enabled")
-			},
-		},
-		{
-			description: "Port enabled but no scheme and https scheme should pass",
 			options:     DomainValidatorOptions{WithPort: true},
-			expected:    "https://example.com",
+			expected:    "https://example.com:443",
-			actual:      "https://example.com",
+			actual:      "https://example.com:443",
-		},
-		{
-			description: "Port enabled but no scheme and http scheme should pass",
-			options:     DomainValidatorOptions{WithPort: true},
-			expected:    "http://example.com",
-			actual:      "http://example.com",
-		},
-		{
-			description: "Port validation with port and no scheme should fail with different port",
-			options:     DomainValidatorOptions{WithPort: true},
-			expected:    "example.com:8080",
-			actual:      "example.com:8081",
-			errorFunc: func(t *testing.T, e error) {
-				assert.ErrorIs(t, e, ErrPortMismatch)
-			},
 		},
 		{
 			description: "Failure to format expected domain should fail",
@@ -274,10 +223,13 @@ func TestDomainValidator_Validate(t *testing.T) {
 			actual:      "https://example.com",
 		},
 		{
-			description: "Valid domains with matching ports should pass",
+			description: "Valid domains with non matching ports should fail",
 			options:     DomainValidatorOptions{WithPort: true},
 			expected:    "example.com:8080",
-			actual:      "example.com:8080",
+			actual:      "example.com:8085",
+			errorFunc: func(t *testing.T, e error) {
+				assert.ErrorIs(t, e, ErrPortMismatch)
+			},
 		},
 		{
 			description: "Valid domains without ports or schemes should pass",