fix space path handling (#2577)

* fix space path handling

app/api/middleware/encode/encode.go

@@ -26,7 +26,7 @@ import (
 )
 
 const (
-	EncodedPathSeparator = "%252F"
+	EncodedPathSeparator = "%2F"
 )
 
 // GitPathBefore wraps an http.HandlerFunc in a layer that encodes a path coming
@@ -69,7 +69,7 @@ func TerminatedPathBefore(prefixes []string, next http.Handler) http.Handler {
 
 // pathTerminatedWithMarker function encodes a path followed by a custom marker and returns a request with an
 // updated URL.Path.
-// A non-empty prefix can be provided to encode encode only after the prefix.
+// A non-empty prefix can be provided to encode only after the prefix.
 // It allows our Rest API to handle paths of the form "/spaces/space1/space2/+/authToken"
 //
 // Examples:
@@ -77,12 +77,21 @@ func TerminatedPathBefore(prefixes []string, next http.Handler) http.Handler {
 // Prefix: "" Path: "/space1/.gitness.git" => "/space1%2F.gitness"
 // Prefix: "/spaces" Path: "/spaces/space1/space2/+/authToken" => "/spaces/space1%2Fspace2/authToken".
 func pathTerminatedWithMarker(r *http.Request, prefix string, marker string, markerReplacement string) (bool, error) {
+	// r.URL.Path contains URL-decoded URI path. r.URL.RawPath contains raw URI path. But, if the URI don't contain
+	// any character that needs encoding, the r.URL.RawPath is empty (otherwise it would be equal to r.URL.Path).
+
+	// We work with r.URL.RawPath it is exist, or with r.URL.Path if the r.URL.RawPath is empty.
+	urlPath := r.URL.Path
+	if r.URL.RawPath != "" {
+		urlPath = r.URL.RawPath
+	}
+
 	// In case path doesn't start with prefix - nothing to encode
-	if len(r.URL.Path) < len(prefix) || r.URL.Path[0:len(prefix)] != prefix {
+	if len(urlPath) < len(prefix) || urlPath[0:len(prefix)] != prefix {
 		return false, nil
 	}
 
-	originalSubPath := r.URL.Path[len(prefix):]
+	originalSubPath := urlPath[len(prefix):]
 	path, found := cutOutTerminatedPath(originalSubPath, marker)
 	if !found {
 		// If we don't find a marker - nothing to encode
@@ -90,7 +99,6 @@ func pathTerminatedWithMarker(r *http.Request, prefix string, marker string, mar
 	}
 
 	// if marker was found - convert to escaped version (skip first character in case path starts with '/').
-	// Since replacePrefix unescapes the strings, we have to double escape.
 	escapedPath := path[0:1] + strings.ReplaceAll(path[1:], types.PathSeparator, EncodedPathSeparator)
 
 	prefixWithPath := prefix + path + marker

app/api/middleware/logging/logging.go

@@ -23,6 +23,7 @@ import (
 	"github.com/harness/gitness/logging"
 
 	"github.com/rs/xid"
+	"github.com/rs/zerolog"
 	"github.com/rs/zerolog/hlog"
 )
 
@@ -74,3 +75,18 @@ func HLogAccessLogHandler() func(http.Handler) http.Handler {
 		},
 	)
 }
+
+func URLHandler(fieldKey string) func(next http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			log := zerolog.Ctx(r.Context())
+			log.UpdateContext(func(c zerolog.Context) zerolog.Context {
+				if r.URL.RawPath != "" {
+					return c.Str(fieldKey, r.URL.RawPath+"?"+r.URL.RawQuery)
+				}
+				return c.Str(fieldKey, r.URL.Path+"?"+r.URL.RawQuery)
+			})
+			next.ServeHTTP(w, r)
+		})
+	}
+}

app/api/request/connector.go

@@ -16,7 +16,6 @@ package request
 
 import (
 	"net/http"
-	"net/url"
 )
 
 const (
@@ -24,11 +23,5 @@ const (
 )
 
 func GetConnectorRefFromPath(r *http.Request) (string, error) {
-	rawRef, err := PathParamOrError(r, PathParamConnectorRef)
+	return PathParamOrError(r, PathParamConnectorRef)
-	if err != nil {
-		return "", err
-	}
-
-	// paths are unescaped
-	return url.PathUnescape(rawRef)
 }

app/api/request/gitspace.go

@@ -16,7 +16,6 @@ package request
 
 import (
 	"net/http"
-	"net/url"
 )
 
 const (
@@ -24,11 +23,5 @@ const (
 )
 
 func GetGitspaceRefFromPath(r *http.Request) (string, error) {
-	rawRef, err := PathParamOrError(r, PathParamGitspaceIdentifier)
+	return PathParamOrError(r, PathParamGitspaceIdentifier)
-	if err != nil {
-		return "", err
-	}
-
-	// paths are unescaped
-	return url.PathUnescape(rawRef)
 }

app/api/request/infra_provider.go

@@ -16,7 +16,6 @@ package request
 
 import (
 	"net/http"
-	"net/url"
 )
 
 const (
@@ -24,11 +23,5 @@ const (
 )
 
 func GetInfraProviderRefFromPath(r *http.Request) (string, error) {
-	rawRef, err := PathParamOrError(r, PathParamInfraProviderConfigIdentifier)
+	return PathParamOrError(r, PathParamInfraProviderConfigIdentifier)
-	if err != nil {
-		return "", err
-	}
-
-	// paths are unescaped
-	return url.PathUnescape(rawRef)
 }

app/api/request/label.go

@@ -27,11 +27,11 @@ const (
 )
 
 func GetLabelKeyFromPath(r *http.Request) (string, error) {
-	return EncodedPathParamOrError(r, PathParamLabelKey)
+	return PathParamOrError(r, PathParamLabelKey)
 }
 
 func GetLabelValueFromPath(r *http.Request) (string, error) {
-	return EncodedPathParamOrError(r, PathParamLabelValue)
+	return PathParamOrError(r, PathParamLabelValue)
 }
 
 func GetLabelIDFromPath(r *http.Request) (int64, error) {

app/api/request/pipeline.go

@@ -16,7 +16,6 @@ package request
 
 import (
 	"net/http"
-	"net/url"
 )
 
 const (
@@ -30,13 +29,7 @@ const (
 )
 
 func GetPipelineIdentifierFromPath(r *http.Request) (string, error) {
-	rawRef, err := PathParamOrError(r, PathParamPipelineIdentifier)
+	return PathParamOrError(r, PathParamPipelineIdentifier)
-	if err != nil {
-		return "", err
-	}
-
-	// paths are unescaped
-	return url.PathUnescape(rawRef)
 }
 
 func GetBranchFromQuery(r *http.Request) string {
@@ -61,11 +54,5 @@ func GetLatestFromPath(r *http.Request) bool {
 }
 
 func GetTriggerIdentifierFromPath(r *http.Request) (string, error) {
-	rawRef, err := PathParamOrError(r, PathParamTriggerIdentifier)
+	return PathParamOrError(r, PathParamTriggerIdentifier)
-	if err != nil {
-		return "", err
-	}
-
-	// paths are unescaped
-	return url.PathUnescape(rawRef)
 }

app/api/request/publickey.go

@@ -16,7 +16,6 @@ package request
 
 import (
 	"net/http"
-	"net/url"
 
 	"github.com/harness/gitness/app/api/usererror"
 	"github.com/harness/gitness/types"
@@ -28,13 +27,7 @@ const (
 )
 
 func GetPublicKeyIdentifierFromPath(r *http.Request) (string, error) {
-	identifier, err := PathParamOrError(r, PathParamPublicKeyIdentifier)
+	return PathParamOrError(r, PathParamPublicKeyIdentifier)
-	if err != nil {
-		return "", err
-	}
-
-	// paths are unescaped
-	return url.PathUnescape(identifier)
 }
 
 // ParseListPublicKeyQueryFilterFromRequest parses query filter for public keys from the url.

app/api/request/repo.go

@@ -16,7 +16,6 @@ package request
 
 import (
 	"net/http"
-	"net/url"
 
 	"github.com/harness/gitness/types"
 	"github.com/harness/gitness/types/enum"
@@ -28,13 +27,7 @@ const (
 )
 
 func GetRepoRefFromPath(r *http.Request) (string, error) {
-	rawRef, err := PathParamOrError(r, PathParamRepoRef)
+	return PathParamOrError(r, PathParamRepoRef)
-	if err != nil {
-		return "", err
-	}
-
-	// paths are unescaped
-	return url.PathUnescape(rawRef)
 }
 
 // ParseSortRepo extracts the repo sort parameter from the url.

app/api/request/secret.go

@@ -16,7 +16,6 @@ package request
 
 import (
 	"net/http"
-	"net/url"
 )
 
 const (
@@ -24,11 +23,5 @@ const (
 )
 
 func GetSecretRefFromPath(r *http.Request) (string, error) {
-	rawRef, err := PathParamOrError(r, PathParamSecretRef)
+	return PathParamOrError(r, PathParamSecretRef)
-	if err != nil {
-		return "", err
-	}
-
-	// paths are unescaped
-	return url.PathUnescape(rawRef)
 }

app/api/request/space.go

@@ -16,7 +16,6 @@ package request
 
 import (
 	"net/http"
-	"net/url"
 
 	"github.com/harness/gitness/types"
 	"github.com/harness/gitness/types/enum"
@@ -27,13 +26,7 @@ const (
 )
 
 func GetSpaceRefFromPath(r *http.Request) (string, error) {
-	rawRef, err := PathParamOrError(r, PathParamSpaceRef)
+	return PathParamOrError(r, PathParamSpaceRef)
-	if err != nil {
-		return "", err
-	}
-
-	// paths are unescaped and lower
-	return url.PathUnescape(rawRef)
 }
 
 // ParseSortSpace extracts the space sort parameter from the url.

app/api/request/template.go

@@ -16,7 +16,6 @@ package request
 
 import (
 	"net/http"
-	"net/url"
 )
 
 const (
@@ -25,20 +24,9 @@ const (
 )
 
 func GetTemplateRefFromPath(r *http.Request) (string, error) {
-	rawRef, err := PathParamOrError(r, PathParamTemplateRef)
+	return PathParamOrError(r, PathParamTemplateRef)
-	if err != nil {
-		return "", err
-	}
-
-	// paths are unescaped
-	return url.PathUnescape(rawRef)
 }
 
 func GetTemplateTypeFromPath(r *http.Request) (string, error) {
-	templateType, err := PathParamOrError(r, PathParamTemplateType)
+	return PathParamOrError(r, PathParamTemplateType)
-	if err != nil {
-		return "", err
-	}
-
-	return templateType, nil
 }

app/api/request/util.go

@@ -65,36 +65,21 @@ func GetHeader(r *http.Request, headerName string) (string, bool) {
 // PathParamOrError tries to retrieve the parameter from the request and
 // returns the parameter if it exists and is not empty, otherwise returns an error.
 func PathParamOrError(r *http.Request, paramName string) (string, error) {
-	val, ok := PathParam(r, paramName)
+	val, err := PathParam(r, paramName)
-	if !ok {
+	if err != nil {
+		return "", err
+	}
+	if val == "" {
 		return "", usererror.BadRequestf("Parameter '%s' not found in request path.", paramName)
 	}
 
 	return val, nil
 }
 
-// EncodedPathParamOrError tries to retrieve the parameter from the request and
-// returns the parameter if it exists and is not empty, otherwise returns an error,
-// then it tries to URL decode parameter value,
-// and returns decoded value, or an error on decoding failure.
-func EncodedPathParamOrError(r *http.Request, paramName string) (string, error) {
-	val, err := PathParamOrError(r, paramName)
-	if err != nil {
-		return "", err
-	}
-
-	decoded, err := url.PathUnescape(val)
-	if err != nil {
-		return "", usererror.BadRequestf("Value %s for param %s has incorrect encoding", val, paramName)
-	}
-
-	return decoded, nil
-}
-
 // PathParamOrEmpty retrieves the path parameter or returns an empty string otherwise.
 func PathParamOrEmpty(r *http.Request, paramName string) string {
-	val, ok := PathParam(r, paramName)
+	val, err := PathParam(r, paramName)
-	if !ok {
+	if err != nil {
 		return ""
 	}
 
@@ -102,13 +87,18 @@ func PathParamOrEmpty(r *http.Request, paramName string) string {
 }
 
 // PathParam retrieves the path parameter or returns false if it exists.
-func PathParam(r *http.Request, paramName string) (string, bool) {
+func PathParam(r *http.Request, paramName string) (string, error) {
 	val := chi.URLParam(r, paramName)
 	if val == "" {
-		return "", false
+		return "", nil
 	}
 
-	return val, true
+	val, err := url.PathUnescape(val)
+	if err != nil {
+		return "", usererror.BadRequestf("Failed to decode path parameter '%s'.", paramName)
+	}
+
+	return val, nil
 }
 
 // QueryParam returns the parameter if it exists.

app/request/request.go

@@ -41,31 +41,23 @@ func ReplacePrefix(r *http.Request, oldPrefix string, newPrefix string) error {
 	 * http.StripPrefix initially changed the path only, but that was updated because of official recommendations:
 	 * 		https://github.com/golang/go/issues/18952
 	 */
-	unOldPrefix, err := url.PathUnescape(oldPrefix)
-	if err != nil {
-		return fmt.Errorf("failed to unescape old prefix '%s'", oldPrefix)
-	}
-	unNewPrefix, err := url.PathUnescape(newPrefix)
-	if err != nil {
-		return fmt.Errorf("failed to unescape new prefix '%s'", newPrefix)
-	}
 
-	unl := len(unOldPrefix)
+	l := len(oldPrefix)
-	if len(r.URL.Path) < unl || r.URL.Path[0:unl] != unOldPrefix {
-		return fmt.Errorf("path '%s' doesn't contain prefix '%s'", r.URL.Path, unOldPrefix)
-	}
 
-	// only change RawPath if it exists
 	if r.URL.RawPath != "" {
-		l := len(oldPrefix)
 		if len(r.URL.RawPath) < l || r.URL.RawPath[0:l] != oldPrefix {
 			return fmt.Errorf("raw path '%s' doesn't contain prefix '%s'", r.URL.RawPath, oldPrefix)
 		}
 
 		r.URL.RawPath = newPrefix + r.URL.RawPath[l:]
-	}
+		r.URL.Path = url.PathEscape(r.URL.RawPath)
+	} else {
+		if len(r.URL.Path) < l || r.URL.Path[0:l] != oldPrefix {
+			return fmt.Errorf("path '%s' doesn't contain prefix '%s'", r.URL.Path, oldPrefix)
+		}
 
-	r.URL.Path = unNewPrefix + r.URL.Path[unl:]
+		r.URL.Path = newPrefix + r.URL.Path[l:]
+	}
 
 	return nil
 }

app/router/api.go

@@ -142,7 +142,7 @@ func NewAPIHandler(
 	r.Use(middleware.Recoverer)
 
 	// configure logging middleware.
-	r.Use(hlog.URLHandler("http.url"))
+	r.Use(logging.URLHandler("http.url"))
 	r.Use(hlog.MethodHandler("http.method"))
 	r.Use(logging.HLogRequestIDHandler())
 	r.Use(logging.HLogAccessLogHandler())

app/router/git.go

@@ -48,7 +48,7 @@ func NewGitHandler(
 	r.Use(middleware.Recoverer)
 
 	// configure logging middleware.
-	r.Use(hlog.URLHandler("http.url"))
+	r.Use(logging.URLHandler("http.url"))
 	r.Use(hlog.MethodHandler("http.method"))
 	r.Use(logging.HLogRequestIDHandler())
 	r.Use(logging.HLogAccessLogHandler())