Merge pull request #51 from projectdiscovery/issue-50-double-encoding

fix double url encoding in path

Author: Mzack9999

.github/workflows/build-test.yml

@@ -21,4 +21,8 @@ jobs:
         uses: actions/checkout@v3
 
       - name: Test
-        run: go test ./...
+        run: go test ./...
+
+      - name: Run Example
+        run: go run .
+        working-directory: examples/

README.md

@@ -4,30 +4,35 @@ Heavily inspired from [https://github.com/hashicorp/go-retryablehttp](https://gi
 
 ### Usage
 
-```go
-package main
-
-import (
-	"fmt"
-	"io/ioutil"
-
-	"github.com/projectdiscovery/retryablehttp-go"
-)
-
-func main() {
-	opts := retryablehttp.DefaultOptionsSpraying
-	// opts := retryablehttp.DefaultOptionsSingle // use single options for single host
-	client := retryablehttp.NewClient(opts)
-	resp, err := client.Get("https://example.com")
-	if err != nil {
-		panic(err)
-	}
-	defer resp.Body.Close()
-
-	data, err := io.ReadAll(resp.Body)
-	if err != nil {
-		panic(err)
-	}
-	fmt.Printf("Data: %v\n", string(data))
-}
+Example of using `retryablehttp` in Go Code is available in [examples](examples/) folder
+Examples of using Nuclei From Go Code to run templates on targets are provided in the examples folder.
+
+
+
+
+### url encoding and parsing issues
+
+`retryablehttp.Request` by default handles some [url encoding and parameters issues](https://github.com/projectdiscovery/utils/blob/main/url/README.md). since `http.Request` internally uses `url.Parse()` to parse url specified in request it creates some inconsistencies for below urls and other non-RFC compilant urls 
+
+```
+// below urls are either normalized or returns error when used in `http.NewRequest()`
+https://scanme.sh/%invalid
+https://scanme.sh/w%0d%2e/
+scanme.sh/with/path?some'param=`'+OR+ORDER+BY+1--
 ```
+All above mentioned cases are handled internally in `retryablehttp`.
+
+
+### request with unsafe urls
+ 
+`retryablehttp` allows creating requests with unsafe urls but requires some extra steps if `path` of url contains encoded characters (ex: `/%invalid/path`).
+
+- `Request.Prepare()` method should be called before request is executed and this applies a quick fix to avoid double url encoding (ex: `%e5` => `%25e5`)
+
+Note: this is a optional feature and only required if we want to allow unsafe urls. If `Request.Prepare()` is not called it follows the standard behaviour.
+
+### Note
+It is not recommended to update `url.URL` instance of `Request` once a new request is created (ex `req.URL.Path = xyz`) due to internal logic or urls.
+In any case if it is not possible to follow above point due to some reason helper methods are available to reflect such changes
+
+- `Request.Update()` commits any changes made to query parameters (ex: `Request.URL.Query().Add(x,y)`)

examples/main.go

@@ -0,0 +1,25 @@
+package main
+
+import (
+	"fmt"
+	"io"
+
+	"github.com/projectdiscovery/retryablehttp-go"
+)
+
+func main() {
+	opts := retryablehttp.DefaultOptionsSpraying
+	// opts := retryablehttp.DefaultOptionsSingle // use single options for single host
+	client := retryablehttp.NewClient(opts)
+	resp, err := client.Get("https://scanme.sh")
+	if err != nil {
+		panic(err)
+	}
+	defer resp.Body.Close()
+
+	data, err := io.ReadAll(resp.Body)
+	if err != nil {
+		panic(err)
+	}
+	fmt.Printf("Data: %v\n", string(data))
+}

go.mod

@@ -4,7 +4,7 @@ go 1.18
 
 require (
 	github.com/Mzack9999/go-http-digest-auth-client v0.6.1-0.20220414142836-eb8883508809
-	github.com/projectdiscovery/utils v0.0.4-0.20230117135930-7371ae6a739d
+	github.com/projectdiscovery/utils v0.0.7
 	golang.org/x/net v0.5.0
 )
 

go.sum

@@ -8,14 +8,10 @@ github.com/gorilla/css v1.0.0/go.mod h1:Dn721qIggHpt4+EFCcTLTU/vk5ySda2ReITrtgBl
 github.com/microcosm-cc/bluemonday v1.0.21 h1:dNH3e4PSyE4vNX+KlRGHT5KrSvjeUkoNPwEORjffHJg=
 github.com/microcosm-cc/bluemonday v1.0.21/go.mod h1:ytNkv4RrDrLJ2pqlsSI46O6IVXmZOBBD4SaJyDwwTkM=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
-github.com/projectdiscovery/utils v0.0.4-0.20221201124851-f8524345b6d3 h1:sOvfN3xHLiBMb6GJ3yDxBmPnN0dh3xllaQXQYo7CFUo=
-github.com/projectdiscovery/utils v0.0.4-0.20221201124851-f8524345b6d3/go.mod h1:PCwA5YuCYWPgHaGiZmr53/SA9iGQmAnw7DSHuhr8VPQ=
-github.com/projectdiscovery/utils v0.0.4-0.20230117121210-1eaffe0d0834 h1:ehoX21rVDm+i7/o8OpTTtDdbesHshF0AD13gbc21wBA=
-github.com/projectdiscovery/utils v0.0.4-0.20230117121210-1eaffe0d0834/go.mod h1:PCwA5YuCYWPgHaGiZmr53/SA9iGQmAnw7DSHuhr8VPQ=
-github.com/projectdiscovery/utils v0.0.4-0.20230117132455-e51a5b2e562c h1:+iHkNvGP/1Cbq6lo8htaQZd3fWch8E9OKD0xTUwS+Zo=
-github.com/projectdiscovery/utils v0.0.4-0.20230117132455-e51a5b2e562c/go.mod h1:PCwA5YuCYWPgHaGiZmr53/SA9iGQmAnw7DSHuhr8VPQ=
-github.com/projectdiscovery/utils v0.0.4-0.20230117135930-7371ae6a739d h1:iB/n2/NL4oh1IaEcqX6pBxj0WHfYN7finzNOKVNVISM=
-github.com/projectdiscovery/utils v0.0.4-0.20230117135930-7371ae6a739d/go.mod h1:PCwA5YuCYWPgHaGiZmr53/SA9iGQmAnw7DSHuhr8VPQ=
+github.com/projectdiscovery/utils v0.0.7 h1:jqDuZedy3t66o6ejQUXjgNWbyAHqiBqLAUDkst9DA2M=
+github.com/projectdiscovery/utils v0.0.7/go.mod h1:PCwA5YuCYWPgHaGiZmr53/SA9iGQmAnw7DSHuhr8VPQ=
+github.com/projectdiscovery/utils v0.0.8-0.20230206142604-469c07cf5050 h1:PwtYD40LMJag5jpB3F2bi1y4tLAMUPIeuWO37txfbOI=
+github.com/projectdiscovery/utils v0.0.8-0.20230206142604-469c07cf5050/go.mod h1:PCwA5YuCYWPgHaGiZmr53/SA9iGQmAnw7DSHuhr8VPQ=
 github.com/saintfish/chardet v0.0.0-20120816061221-3af4cd4741ca h1:NugYot0LIVPxTvN8n+Kvkn6TrbMyxQiuvKdEwFdR9vI=
 github.com/saintfish/chardet v0.0.0-20120816061221-3af4cd4741ca/go.mod h1:uugorj2VCxiV1x+LzaIdVa9b4S4qGAcH6cbhh4qVxOU=
 github.com/stretchr/testify v1.8.1 h1:w7B6lhMri9wdJUVmEZPGGhZzrYTPvgJArz7wNPgYKsk=

request.go

@@ -107,6 +107,18 @@ func (r *Request) Update() {
 	updateScheme(r.URL.URL)
 }
 
+// Prepares request (applies hot patch if any. Ex: Path Unescape to prevent double url encoding)
+// calling multiple times may have unexpected results unlike Update() method
+func (r *Request) Prepare() {
+	// hot patch to avoid url path encoding issues
+	// by default we decode encoded/escaped path and internally http.Request encodes them again
+	// this avoid double url encoding (or reencoding or path)
+	if rawPath, err := url.QueryUnescape(r.URL.Path); err == nil {
+		r.URL.Path = rawPath
+	}
+	r.Update()
+}
+
 // SetURL updates request url (i.e http.Request.URL) with given url
 func (r *Request) SetURL(u *urlutil.URL) {
 	r.URL = u
@@ -215,23 +227,29 @@ func FromRequestWithTrace(r *http.Request) (*Request, error) {
 }
 
 // NewRequest creates a new wrapped request.
-func NewRequest(method, url string, body interface{}) (*Request, error) {
+func NewRequestFromURL(method string, urlx *urlutil.URL, body interface{}) (*Request, error) {
+	return NewRequestFromURLWithContext(context.Background(), method, urlx, body)
+}
+
+// NewRequestWithContext creates a new wrapped request with context
+func NewRequestFromURLWithContext(ctx context.Context, method string, urlx *urlutil.URL, body interface{}) (*Request, error) {
 	bodyReader, contentLength, err := getReusableBodyandContentLength(body)
 	if err != nil {
 		return nil, err
 	}
 
-	urlx, err := urlutil.Parse(url)
-	if err != nil {
-		return nil, err
-	}
-	httpReq, err := http.NewRequest(method, url, nil)
+	// we provide a url without path to http.NewRequest at start and then replace url instance directly
+	// because `http.NewRequest()` internally parses using `url.Parse()` this removes/overrides any
+	// patches done by urlutil.URL in unsafe mode (ex: https://scanme.sh/%invalid)
+	// Note: this does not have any impact on actual path when sending request
+	// `http.NewRequestxxx` internally only uses `u.Host` and all other data is stored in `url.URL` instance
+	httpReq, err := http.NewRequestWithContext(ctx, method, "https://"+urlx.Host, nil)
 	if err != nil {
 		return nil, err
 	}
+	urlx.Update()
 	httpReq.URL = urlx.URL
 	updateScheme(httpReq.URL)
-
 	// content-length and body should be assigned only
 	// if request has body
 	if bodyReader != nil {
@@ -242,31 +260,22 @@ func NewRequest(method, url string, body interface{}) (*Request, error) {
 	return &Request{httpReq, urlx, Metrics{}, nil}, nil
 }
 
-// NewRequestWithContext creates a new wrapped request with context
-func NewRequestWithContext(ctx context.Context, method, url string, body interface{}) (*Request, error) {
-	bodyReader, contentLength, err := getReusableBodyandContentLength(body)
+// NewRequest creates a new wrapped request
+func NewRequest(method, url string, body interface{}) (*Request, error) {
+	urlx, err := urlutil.Parse(url)
 	if err != nil {
 		return nil, err
 	}
+	return NewRequestFromURL(method, urlx, body)
+}
 
+// NewRequest creates a new wrapped request with given context
+func NewRequestWithContext(ctx context.Context, method, url string, body interface{}) (*Request, error) {
 	urlx, err := urlutil.Parse(url)
 	if err != nil {
 		return nil, err
 	}
-	httpReq, err := http.NewRequestWithContext(ctx, method, url, nil)
-	if err != nil {
-		return nil, err
-	}
-	httpReq.URL = urlx.URL
-	updateScheme(httpReq.URL)
-	// content-length and body should be assigned only
-	// if request has body
-	if bodyReader != nil {
-		httpReq.ContentLength = contentLength
-		httpReq.Body = bodyReader
-	}
-
-	return &Request{httpReq, urlx, Metrics{}, nil}, nil
+	return NewRequestFromURLWithContext(ctx, method, urlx, body)
 }
 
 func updateScheme(u *url.URL) {

request_test.go

@@ -1,7 +1,10 @@
 package retryablehttp_test
 
 import (
+	"bufio"
+	"bytes"
 	"os"
+	"strings"
 	"testing"
 
 	"github.com/projectdiscovery/retryablehttp-go"
@@ -35,3 +38,51 @@ func TestRequestUrls(t *testing.T) {
 		}
 	}
 }
+
+func TestEncodedPaths(t *testing.T) {
+
+	// test this on all valid crlf payloads
+	payloads := []string{"%00", "%0a", "%0a%20", "%0d", "%0d%09", "%0d%0a", "%0d%0a%09", "%0d%0a%20", "%0d%20", "%20", "%20%0a", "%20%0d", "%20%0d%0a", "%23%0a", "%23%0a%20", "%23%0d", "%23%0d%0a", "%23%0a", "%25%30", "%25%30%61", "%2e%2e%2f%0d%0a", "%2f%2e%2e%0d%0a", "%2f..%0d%0a", "%3f", "%3f%0a", "%3f%0d", "%3f%0d%0a", "%e5%98%8a%e5%98%8d", "%e5%98%8a%e5%98%8d%0a", "%e5%98%8a%e5%98%8d%0d", "%e5%98%8a%e5%98%8d%0d%0a", "%e5%98%8a%e5%98%8d%e5%98%8a%e5%98%8d"}
+
+	// create url using below data and payload
+	suffix := "/path?param=true"
+
+	for _, v := range payloads {
+		exURL := "https://scanme.sh/" + v + suffix
+		req, err := retryablehttp.NewRequest("GET", exURL, nil)
+		if err != nil {
+			t.Fatalf("got %v with payload %v", err.Error(), v)
+		}
+
+		req.Prepare()
+		bin, err := req.Dump()
+		if err != nil {
+			t.Errorf("failed to dump request body for payload %v got %v", v, err)
+		}
+
+		relPath := getPathFromRaw(bin)
+		payload := strings.TrimSuffix(relPath, suffix)
+		payload = strings.TrimPrefix(payload, "/")
+
+		if v != payload {
+			t.Errorf("something went wrong expected `%v` in outgoing request but got-----\n%v\n------", v, string(bin))
+		}
+	}
+}
+
+func getPathFromRaw(bin []byte) (relpath string) {
+	buff := bufio.NewReader(bytes.NewReader(bin))
+readline:
+	line, err := buff.ReadString('\n')
+	if err != nil {
+		return
+	}
+	if strings.Contains(line, "HTTP/1.1") {
+		parts := strings.Split(line, " ")
+		if len(parts) == 3 {
+			relpath = parts[1]
+			return
+		}
+	}
+	goto readline
+}