From 34292b022efe8459c78aec07bd360be326e303bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ga=C4=87e=C5=A1a?= Date: Mon, 26 Aug 2024 13:13:25 +0000 Subject: [PATCH] fix space path handling (#2577) * fix space path handling --- app/api/middleware/encode/encode.go | 18 ++++++++---- app/api/middleware/logging/logging.go | 16 +++++++++++ app/api/request/connector.go | 9 +----- app/api/request/gitspace.go | 9 +----- app/api/request/infra_provider.go | 9 +----- app/api/request/label.go | 4 +-- app/api/request/pipeline.go | 17 ++---------- app/api/request/publickey.go | 9 +----- app/api/request/repo.go | 9 +----- app/api/request/secret.go | 9 +----- app/api/request/space.go | 9 +----- app/api/request/template.go | 16 ++--------- app/api/request/util.go | 40 ++++++++++----------------- app/request/request.go | 24 ++++++---------- app/router/api.go | 2 +- app/router/git.go | 2 +- 16 files changed, 67 insertions(+), 135 deletions(-) diff --git a/app/api/middleware/encode/encode.go b/app/api/middleware/encode/encode.go index f666c75ca..118052406 100644 --- a/app/api/middleware/encode/encode.go +++ b/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 diff --git a/app/api/middleware/logging/logging.go b/app/api/middleware/logging/logging.go index 8ad5e93b8..d0afcf90d 100644 --- a/app/api/middleware/logging/logging.go +++ b/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) + }) + } +} diff --git a/app/api/request/connector.go b/app/api/request/connector.go index e7965db77..2a2f9a3d8 100644 --- a/app/api/request/connector.go +++ b/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) - if err != nil { - return "", err - } - - // paths are unescaped - return url.PathUnescape(rawRef) + return PathParamOrError(r, PathParamConnectorRef) } diff --git a/app/api/request/gitspace.go b/app/api/request/gitspace.go index 74dd0ef7d..b4f024dcd 100644 --- a/app/api/request/gitspace.go +++ b/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) - if err != nil { - return "", err - } - - // paths are unescaped - return url.PathUnescape(rawRef) + return PathParamOrError(r, PathParamGitspaceIdentifier) } diff --git a/app/api/request/infra_provider.go b/app/api/request/infra_provider.go index fcabe52e2..d9f293a69 100644 --- a/app/api/request/infra_provider.go +++ b/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) - if err != nil { - return "", err - } - - // paths are unescaped - return url.PathUnescape(rawRef) + return PathParamOrError(r, PathParamInfraProviderConfigIdentifier) } diff --git a/app/api/request/label.go b/app/api/request/label.go index 39cc36086..84dd7d125 100644 --- a/app/api/request/label.go +++ b/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) { diff --git a/app/api/request/pipeline.go b/app/api/request/pipeline.go index 4a4502c3b..20713aea9 100644 --- a/app/api/request/pipeline.go +++ b/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) - if err != nil { - return "", err - } - - // paths are unescaped - return url.PathUnescape(rawRef) + return PathParamOrError(r, PathParamPipelineIdentifier) } 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) - if err != nil { - return "", err - } - - // paths are unescaped - return url.PathUnescape(rawRef) + return PathParamOrError(r, PathParamTriggerIdentifier) } diff --git a/app/api/request/publickey.go b/app/api/request/publickey.go index 9d867b772..b5a35a3f0 100644 --- a/app/api/request/publickey.go +++ b/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) - if err != nil { - return "", err - } - - // paths are unescaped - return url.PathUnescape(identifier) + return PathParamOrError(r, PathParamPublicKeyIdentifier) } // ParseListPublicKeyQueryFilterFromRequest parses query filter for public keys from the url. diff --git a/app/api/request/repo.go b/app/api/request/repo.go index 2227ff3e9..0e665ed37 100644 --- a/app/api/request/repo.go +++ b/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) - if err != nil { - return "", err - } - - // paths are unescaped - return url.PathUnescape(rawRef) + return PathParamOrError(r, PathParamRepoRef) } // ParseSortRepo extracts the repo sort parameter from the url. diff --git a/app/api/request/secret.go b/app/api/request/secret.go index 80c3c8739..0e27822c6 100644 --- a/app/api/request/secret.go +++ b/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) - if err != nil { - return "", err - } - - // paths are unescaped - return url.PathUnescape(rawRef) + return PathParamOrError(r, PathParamSecretRef) } diff --git a/app/api/request/space.go b/app/api/request/space.go index e2778ef29..9ec55761a 100644 --- a/app/api/request/space.go +++ b/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) - if err != nil { - return "", err - } - - // paths are unescaped and lower - return url.PathUnescape(rawRef) + return PathParamOrError(r, PathParamSpaceRef) } // ParseSortSpace extracts the space sort parameter from the url. diff --git a/app/api/request/template.go b/app/api/request/template.go index 041dd1f23..b16fb798c 100644 --- a/app/api/request/template.go +++ b/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) - if err != nil { - return "", err - } - - // paths are unescaped - return url.PathUnescape(rawRef) + return PathParamOrError(r, PathParamTemplateRef) } func GetTemplateTypeFromPath(r *http.Request) (string, error) { - templateType, err := PathParamOrError(r, PathParamTemplateType) - if err != nil { - return "", err - } - - return templateType, nil + return PathParamOrError(r, PathParamTemplateType) } diff --git a/app/api/request/util.go b/app/api/request/util.go index 24753a853..5ed45d9c4 100644 --- a/app/api/request/util.go +++ b/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) - if !ok { + val, err := PathParam(r, paramName) + 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) - if !ok { + val, err := PathParam(r, paramName) + 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. diff --git a/app/request/request.go b/app/request/request.go index 734021121..1f0a7ca25 100644 --- a/app/request/request.go +++ b/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) - 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) - } + l := len(oldPrefix) - // 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 } diff --git a/app/router/api.go b/app/router/api.go index fa9015e29..c811f617e 100644 --- a/app/router/api.go +++ b/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()) diff --git a/app/router/git.go b/app/router/git.go index 4615ed768..e58f804f1 100644 --- a/app/router/git.go +++ b/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())