Merge pull request #262 from akrainiouk/req.url-encoding-fix

req.url: fixed consistency with Fastly implementation

interpreter/variable/all.go

@@ -39,6 +39,17 @@ func NewAllScopeVariables(ctx *context.Context) *AllScopeVariables {
 	}
 }
 
+// return unescaped path value from the specified URL
+// this function makes the best effort to get the raw path
+// even when standard EscapedPath() chooses escaped version
+func getRawUrlPath(u *url.URL) string {
+	result := u.EscapedPath()
+	if u.RawPath != "" && result != u.RawPath {
+		result = u.RawPath
+	}
+	return result
+}
+
 // nolint: funlen,gocognit,gocyclo
 func (v *AllScopeVariables) Get(s context.Scope, name string) (value.Value, error) {
 	req := v.ctx.Request
@@ -490,7 +501,7 @@ func (v *AllScopeVariables) Get(s context.Scope, name string) (value.Value, erro
 		}
 		return &value.String{Value: id}, nil
 	case REQ_TOPURL: // FIXME: what is the difference of req.url ?
-		u := req.URL.Path
+		u := req.URL.EscapedPath()
 		if v := req.URL.RawQuery; v != "" {
 			u += "?" + v
 		}
@@ -499,7 +510,7 @@ func (v *AllScopeVariables) Get(s context.Scope, name string) (value.Value, erro
 		}
 		return &value.String{Value: u}, nil
 	case REQ_URL:
-		u := req.URL.Path
+		u := getRawUrlPath(req.URL)
 		if v := req.URL.RawQuery; v != "" {
 			u += "?" + v
 		}
@@ -509,19 +520,19 @@ func (v *AllScopeVariables) Get(s context.Scope, name string) (value.Value, erro
 		return &value.String{Value: u}, nil
 	case REQ_URL_BASENAME:
 		return &value.String{
-			Value: filepath.Base(req.URL.Path),
+			Value: filepath.Base(getRawUrlPath(req.URL)),
 		}, nil
 	case REQ_URL_DIRNAME:
 		return &value.String{
-			Value: filepath.Dir(req.URL.Path),
+			Value: filepath.Dir(getRawUrlPath(req.URL)),
 		}, nil
 	case REQ_URL_EXT:
-		ext := filepath.Ext(req.URL.Path)
+		ext := filepath.Ext(getRawUrlPath(req.URL))
 		return &value.String{
 			Value: strings.TrimPrefix(ext, "."),
 		}, nil
 	case REQ_URL_PATH:
-		return &value.String{Value: req.URL.Path}, nil
+		return &value.String{Value: getRawUrlPath(req.URL)}, nil
 	case REQ_URL_QS:
 		return &value.String{Value: req.URL.RawQuery}, nil
 	case REQ_VCL:
@@ -718,7 +729,7 @@ func (v *AllScopeVariables) Set(s context.Scope, name, operator string, val valu
 	case REQ_REQUEST:
 		return v.Set(s, "req.method", operator, val)
 	case REQ_URL:
-		u := v.ctx.Request.URL.Path
+		u := getRawUrlPath(v.ctx.Request.URL)
 		if query := v.ctx.Request.URL.RawQuery; query != "" {
 			u += "?" + query
 		}
@@ -735,6 +746,7 @@ func (v *AllScopeVariables) Set(s context.Scope, name, operator string, val valu
 		}
 		// Update request URLs
 		v.ctx.Request.URL.Path = parsed.Path
+		v.ctx.Request.URL.RawPath = parsed.RawPath
 		v.ctx.Request.URL.RawQuery = parsed.RawQuery
 		v.ctx.Request.URL.RawFragment = parsed.RawFragment
 		return nil

interpreter/variable/all_test.go

@@ -0,0 +1,90 @@
+package variable
+
+import (
+	"github.com/google/go-cmp/cmp"
+	"github.com/ysugimoto/falco/interpreter/context"
+	"github.com/ysugimoto/falco/interpreter/value"
+	"net/http"
+	"net/url"
+	"testing"
+)
+
+func createScopeVars(urlStr string) *AllScopeVariables {
+	parsedUrl, _ := url.Parse(urlStr)
+	return &AllScopeVariables{
+		ctx: &context.Context{
+			Request: &http.Request{
+				URL: parsedUrl,
+			},
+		},
+	}
+}
+
+func getValue(t *testing.T, testIndex int, vars *AllScopeVariables, varName string) *value.String {
+	result, err := vars.Get(context.RecvScope, varName)
+	if err != nil {
+		t.Errorf("[%d] Unexpected error: %s", testIndex, err)
+	}
+	return value.Unwrap[*value.String](result)
+}
+
+type Expect struct {
+	url      string
+	path     string
+	dirname  string
+	basename string
+	ext      string
+}
+
+func TestReqUrl(t *testing.T) {
+	tests := []struct {
+		input  string
+		expect Expect
+	}{
+		{
+			input:  "/foo/bar.baz",
+			expect: Expect{"/foo/bar.baz", "/foo/bar.baz", "/foo", "bar.baz", "baz"},
+		},
+		{
+			input:  "/f%6Fo/b%61r.b%61z",
+			expect: Expect{"/f%6Fo/b%61r.b%61z", "/f%6Fo/b%61r.b%61z", "/f%6Fo", "b%61r.b%61z", "b%61z"},
+		},
+		{
+			input:  "/fo&/ba*r.b$z",
+			expect: Expect{"/fo&/ba*r.b$z", "/fo&/ba*r.b$z", "/fo&", "ba*r.b$z", "b$z"},
+		},
+		{
+			input:  "/a b/c d.ex t",
+			expect: Expect{"/a b/c d.ex t", "/a b/c d.ex t", "/a b", "c d.ex t", "ex t"},
+		},
+		// TODO: Fastly does handle this case but in falco it leads to a segfault.
+		//{
+		//	input:  "/a%nnb/c%nn d.ex%nnt",
+		//	expect: Expect{"/a%nnb/c%nnd.ex%nnt", "/a%nnb/c%nnd.ex%nnt", "/a%nnb", "c%nnd.ex%nnt", "ex%nnt"},
+		//},
+	}
+	for i, test := range tests {
+		vars := createScopeVars(test.input)
+		expect := test.expect
+		url := getValue(t, i, vars, REQ_URL)
+		if diff := cmp.Diff(url.Value, expect.url); diff != "" {
+			t.Errorf("Return value unmatch, diff=%s", diff)
+		}
+		path := getValue(t, i, vars, REQ_URL_PATH)
+		if diff := cmp.Diff(path.Value, expect.path); diff != "" {
+			t.Errorf("Return value unmatch, diff=%s", diff)
+		}
+		dirname := getValue(t, i, vars, REQ_URL_DIRNAME)
+		if diff := cmp.Diff(dirname.Value, expect.dirname); diff != "" {
+			t.Errorf("Return value unmatch, diff=%s", diff)
+		}
+		basename := getValue(t, i, vars, REQ_URL_BASENAME)
+		if diff := cmp.Diff(basename.Value, expect.basename); diff != "" {
+			t.Errorf("Return value unmatch, diff=%s", diff)
+		}
+		ext := getValue(t, i, vars, REQ_URL_EXT)
+		if diff := cmp.Diff(ext.Value, expect.ext); diff != "" {
+			t.Errorf("Return value unmatch, diff=%s", diff)
+		}
+	}
+}