Merge pull request #16 from justincjohnson/justincjohnson/fuzz

fix: additional error handling and fuzzing tests

.github/workflows/main.yml

@@ -7,7 +7,7 @@ jobs:
       fail-fast: false
       matrix:
         os: [ "ubuntu" ]
-        go: [ "1.17.x", "1.18.x" ]
+        go: [ "1.18.x", "1.19.x" ]
     env:
       COVERAGES: ""
     runs-on: ${{ matrix.os }}-latest
@@ -25,3 +25,5 @@ jobs:
         run: go test -v ./...
       - name: Check formatted
         run: gofmt -l .
+      - name: Fuzz
+        run: go test ./... -fuzz=Fuzz -fuzztime=1m

.gitignore

@@ -0,0 +1 @@
+/testdata

Makefile

@@ -0,0 +1,19 @@
+FUZZTIME ?= 1m
+
+.PHONY: test
+
+default: all
+
+all: build test
+
+clean:
+	go clean ./...
+
+build:
+	go build ./...
+
+test:
+	go test ./...
+
+fuzz:
+	go test ./... -fuzz=Fuzz -fuzztime $(FUZZTIME)

README.md

@@ -28,19 +28,24 @@ from to [status]
 # Redirect with a 302
 /my-redirect  /              302
 
-# Rewrite a path
-/pass-through /index.html    200
+# Redirect with wildcard (splat placeholder)
+/splat/* /redirected-splat/:splat 301
+
+# Redirect with multiple named placeholder
+/posts/:year/:month/:day/:title  /articles/:year/:month/:day/:title  301
 
 # Show a custom 404 for everything under this path
-/ecommerce/*  /store-closed  404
+/ecommerce/*  /store-closed.html  404
 
-# Single page app rewrite
+# Single page app rewrite (SPA, PWA)
 /*    /index.html   200
-
-# Proxying
-/api/*  https://api.example.com/:splat  200
 ```
 
+## Notes for contributors
+
+- `make all` builds and runs tests
+- `FUZZTIME=1m make fuzz` runs fuzzing for specified amount of time
+
 ---
 
 ## Credit

redirects.go

@@ -126,11 +126,11 @@ func Parse(r io.Reader) (rules []Rule, err error) {
 
 		// missing dst
 		if len(fields) <= 1 {
-			return nil, fmt.Errorf("missing destination path: %q", line)
+			return nil, fmt.Errorf("missing 'to' path: %q", line)
 		}
 
 		if len(fields) > 3 {
-			return nil, fmt.Errorf("must match format `from to [status][!]`")
+			return nil, fmt.Errorf("must match format 'from to [status]'")
 		}
 
 		// src and dst
@@ -140,6 +140,23 @@ func Parse(r io.Reader) (rules []Rule, err error) {
 			Status: 301,
 		}
 
+		// from
+		if !strings.HasPrefix(rule.From, "/") {
+			return nil, fmt.Errorf("'from' path must begin with '/'")
+		}
+
+		if strings.Contains(rule.From, "*") && !strings.HasSuffix(rule.From, "*") {
+			return nil, fmt.Errorf("'from' path can only end with splat")
+		}
+
+		// to
+		if !strings.HasPrefix(rule.To, "/") {
+			_, err := url.Parse(rule.To)
+			if err != nil {
+				return nil, errors.Wrapf(err, "invalid 'to' path")
+			}
+		}
+
 		// status
 		if len(fields) > 2 {
 			code, err := parseStatus(fields[2])

redirects_test.go

@@ -1,14 +1,15 @@
 package redirects
 
 import (
+	"bufio"
 	"bytes"
 	"strings"
 	"testing"
 
 	"github.com/tj/assert"
 )
 
-func TestRule_IsProxy(t *testing.T) {
+func TestRuleIsProxy(t *testing.T) {
 	t.Run("without host", func(t *testing.T) {
 		r := Rule{
 			From: "/blog",
@@ -28,7 +29,7 @@ func TestRule_IsProxy(t *testing.T) {
 	})
 }
 
-func TestRule_IsRewrite(t *testing.T) {
+func TestRuleIsRewrite(t *testing.T) {
 	t.Run("with 3xx", func(t *testing.T) {
 		r := Rule{
 			From:   "/blog",
@@ -59,7 +60,7 @@ func TestRule_IsRewrite(t *testing.T) {
 	})
 }
 
-func Test_Parse(t *testing.T) {
+func TestParse(t *testing.T) {
 	t.Run("with illegal force", func(t *testing.T) {
 		_, err := Parse(strings.NewReader(`
 		/home / 301!
@@ -97,3 +98,72 @@ func Test_Parse(t *testing.T) {
 		assert.Contains(t, err.Error(), "redirects file size cannot exceed")
 	})
 }
+
+func FuzzParse(f *testing.F) {
+	testcases := []string{"/a /b 999\n",
+		"/redirect-one /one.html\n/301-redirect-one /one.html 301\n/302-redirect-two /two.html 302\n/200-index /index.html 200\n/posts/:year/:month/:day/:title /articles/:year/:month/:day/:title 301\n/splat/* /redirected-splat/:splat 301\n/not-found/* /404.html 404\n/* /index.html 200\n",
+		"/a /b 301!\n",
+		"/a200 /b200 200\n/a301 /b301 301\n/a302 /b302 302\n/a303 /b303 303\n/a307 /b307 307\n/a308 /b308 308\n/a404 /b404 404\n/a410 /b410 410\n/a451 /b451 451\n",
+		"hello\n", "/redirect-one /one.html\r\n/200-index /index.html 200\r\n", "a b 2\nc   d 42", "/a/*/b blah", "/from https://example.com 200\n/a/:blah/yeah /b/:blah/yeah"}
+	for _, tc := range testcases {
+		f.Add(tc) // Use f.Add to provide a seed corpus
+	}
+	f.Fuzz(func(t *testing.T, orig string) {
+		rules, err := ParseString(orig)
+		if err != nil {
+			if len(rules) > 0 {
+				t.Errorf("should not return rules on error")
+			}
+		}
+
+		s := bufio.NewScanner(strings.NewReader(orig))
+
+		for s.Scan() {
+			line := strings.TrimSpace(s.Text())
+			fields := strings.Fields(line)
+
+			// Skip comments so we don't have to special case
+			if strings.HasPrefix(line, "#") {
+				continue
+			}
+
+			if err == nil && len(fields) < 2 && line != "" {
+				t.Errorf("should error with less than 2 fields.  orig='%v'", orig)
+				continue
+			}
+
+			if err == nil && len(fields) > 3 {
+				t.Errorf("should error with more than 3 fields.  orig='%v'", orig)
+				continue
+			}
+
+			if err == nil && len(fields) > 0 && !strings.HasPrefix(fields[0], "/") {
+				t.Errorf("should error for from path not starting with '/'.  orig=%v", orig)
+				continue
+			}
+
+			// we already handled these cases
+			if len(fields) < 3 {
+				continue
+			}
+
+			if err == nil && strings.Contains(fields[0], "*") && !strings.HasSuffix(fields[0], "*") {
+				t.Errorf("asterisk in from not at end should error.  orig=%v", orig)
+				continue
+			}
+
+			if err == nil && strings.HasSuffix(fields[2], "!") {
+				t.Errorf("should error for forced redirects.  orig=%v, err=%v", orig, err)
+				continue
+			}
+
+			if err == nil {
+				for _, r := range rules {
+					if !isValidStatusCode(r.Status) {
+						t.Errorf("should error for invalid status code.  orig=%v", orig)
+					}
+				}
+			}
+		}
+	})
+}