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

Author: steveiliop56

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) ||
-		errors.Is(err, validators.ErrPortMismatch) {
+	if !errors.Is(err, validators.ErrHostnameMismatch) {
 		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) &&
-				!errors.Is(err, validators.ErrPortMismatch) &&
-				!errors.Is(err, validators.ErrSchemeMismatch) {
+			if !errors.Is(err, validators.ErrHostnameMismatch) {
 				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.
-	// Leave empty to allow any scheme.
+	// Specify a list of allowed schemes if WithScheme is set to true.
 	AllowedSchemes []string
 }
 
@@ -48,53 +46,74 @@ func NewDomainValidator(opts DomainValidatorOptions) *DomainValidator {
 	}
 }
 
-func (v *DomainValidator) getURL(i string) (*url.URL, error) {
-	u, err := url.Parse(i)
-
-	if !v.opts.WithScheme && (err != nil || u.Host == "") {
-		u, err = url.Parse("tinyauth://" + i)
+func (v *DomainValidator) checkScheme(rawURL string) error {
+	if !v.opts.WithScheme {
+		return nil
 	}
 
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse input url: %w", err)
+	if len(v.opts.AllowedSchemes) == 0 {
+		return fmt.Errorf("allowed schemes must be specified")
 	}
 
-	if u.Host == "" {
-		return nil, ErrInvalidURL
+	for _, scheme := range v.opts.AllowedSchemes {
+		if strings.HasPrefix(strings.ToLower(rawURL), strings.ToLower(scheme)+"://") {
+			return nil
+		}
 	}
 
-	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")
+	return fmt.Errorf("invalid scheme")
+
+}
+
+func (v *DomainValidator) getURL(i string) (*url.URL, error) {
+	if i == "" {
+		return nil, fmt.Errorf("url cannot be empty")
 	}
 
 	if v.opts.WithScheme {
-		// Empty scheme means that we parsed the url with the tinyauth:// placeholder
-		if u.Scheme == "tinyauth" {
-			return nil, fmt.Errorf("input url is missing scheme")
+		err := v.checkScheme(i)
+
+		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
 	}
 
-	return u, nil
-}
+	rawURL := i
 
-func (v *DomainValidator) getEffectivePort(u *url.URL) (string, bool) {
-	if u.Port() != "" {
-		return u.Port(), true
+	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
 	}
-	switch u.Scheme {
-	case "http":
-		return "80", true
-	case "https":
-		return "443", true
-	default:
-		return "", false
+
+	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) formatHostname(hostname string) (string, error) {
+func (v *DomainValidator) getHostname(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 !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 {
+		if eu.Port() != au.Port() {
 			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())
 }