Skip to content

Commit

Permalink
fix: improved parsing based on fuzzing
Browse files Browse the repository at this point in the history
  • Loading branch information
@lidel
lidel committed Sep 23, 2022
1 parent 18c5b72 commit 8d41a5e
Show file tree
Hide file tree
Showing 2 changed files with 111 additions and 51 deletions.
78 changes: 55 additions & 23 deletions redirects.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ type Rule struct {
// - 200 a rewrite
// - defaults to 301 redirect
//
// When proxying this field is ignored.
//
Status int
}

Expand Down Expand Up @@ -126,36 +124,29 @@ func Parse(r io.Reader) (rules []Rule, err error) {

// missing dst
if len(fields) <= 1 {
return nil, fmt.Errorf("missing 'to' path: %q", line)
return nil, fmt.Errorf("missing 'to' path")
}

if len(fields) > 3 {
return nil, fmt.Errorf("must match format 'from to [status]'")
}

// src and dst
rule := Rule{
From: fields[0],
To: fields[1],
Status: 301,
}

// from
if !strings.HasPrefix(rule.From, "/") {
return nil, fmt.Errorf("'from' path must begin with '/'")
}
// implicit status
rule := Rule{Status: 301}

if strings.Contains(rule.From, "*") && !strings.HasSuffix(rule.From, "*") {
return nil, fmt.Errorf("'from' path can only end with splat")
// from (must parse as an absolute path)
from, err := parseFrom(fields[0])
if err != nil {
return nil, errors.Wrapf(err, "parsing 'from'")
}
rule.From = from

// to
if !strings.HasPrefix(rule.To, "/") {
_, err := url.Parse(rule.To)
if err != nil {
return nil, errors.Wrapf(err, "invalid 'to' path")
}
// to (must parse as an absolute path or an URL)
to, err := parseTo(fields[1])
if err != nil {
return nil, errors.Wrapf(err, "parsing 'to'")
}
rule.To = to

// status
if len(fields) > 2 {
Expand All @@ -182,11 +173,52 @@ func ParseString(s string) ([]Rule, error) {
return Parse(strings.NewReader(s))
}

func parseFrom(s string) (string, error) {
// enforce a single splat
fromSplats := strings.Count(s, "*")
if fromSplats > 0 {
if !strings.HasSuffix(s, "*") {
return "", fmt.Errorf("path must end with asterisk")
}
if fromSplats > 1 {
return "", fmt.Errorf("path can have at most one asterisk")
}
}

// confirm value is within URL path spec
_, err := url.Parse(s)
if err != nil {
return "", err
}

if !strings.HasPrefix(s, "/") {
return "", fmt.Errorf("path must begin with '/'")
}
return s, nil
}

func parseTo(s string) (string, error) {
// confirm value is within URL path spec
u, err := url.Parse(s)
if err != nil {
return "", err
}

// if the value is a patch attached to full URL, only allow safelisted schemes
if !strings.HasPrefix(s, "/") {
if u.Scheme != "http" && u.Scheme != "https" && u.Scheme != "ipfs" && u.Scheme != "ipns" {
return "", fmt.Errorf("invalid URL scheme")
}
}

return s, nil
}

// parseStatus returns the status code.
func parseStatus(s string) (code int, err error) {
if strings.HasSuffix(s, "!") {
// See https://docs.netlify.com/routing/redirects/rewrites-proxies/#shadowing
return 0, fmt.Errorf("forced redirects (or \"shadowing\") are not supported by IPFS gateways")
return 0, fmt.Errorf("forced redirects (or \"shadowing\") are not supported")
}

code, err = strconv.Atoi(s)
Expand Down
84 changes: 56 additions & 28 deletions redirects_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package redirects
import (
"bufio"
"bytes"
"net/url"
"strings"
"testing"

Expand All @@ -22,7 +23,7 @@ func TestRuleIsProxy(t *testing.T) {
t.Run("with host", func(t *testing.T) {
r := Rule{
From: "/blog",
To: "https://blog.apex.sh",
To: "https://site.example.com",
}

assert.True(t, r.IsProxy())
Expand Down Expand Up @@ -103,20 +104,59 @@ 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",
"/ą /ę 301\n",
"/%C4%85 /ę 301\n",
"#/a \n\n/b",
"/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.Add([]byte(tc))
}
f.Fuzz(func(t *testing.T, orig string) {
rules, err := ParseString(orig)
f.Fuzz(func(t *testing.T, orig []byte) {
rules, err := Parse(bytes.NewReader(orig))
if err != nil {
if len(rules) > 0 {
if rules != nil {
t.Errorf("should not return rules on error")
}
t.Skip()
}

s := bufio.NewScanner(strings.NewReader(orig))
for _, r := range rules {
if !isValidStatusCode(r.Status) {
t.Errorf("should error for invalid status code. orig=%q", orig)
}

if !strings.HasPrefix(r.From, "/") {
t.Errorf("should error for 'from' path not starting with '/'. orig=%q", orig)
}
_, err := url.Parse(r.From)
if err != nil {
t.Errorf("should error for 'from' path not parsing as relative URL. from=%q, orig=%q", r.From, orig)
}

fromSplats := strings.Count(r.From, "*")
if fromSplats > 0 {
if fromSplats > 1 {
t.Errorf("more than one asterisk in 'from' should error. orig=%q", orig)
}
if !strings.HasSuffix(r.From, "*") {
t.Errorf("asterisk in 'from' not at end should error. orig=%q", orig)
}
}

// if does not start with / we assume it is a valid url
to, err := url.Parse(r.To)
if err != nil {
t.Errorf("should error for 'to' path not parsing as a path or URL. to=%q, orig=%q", to, orig)
}
if !strings.HasPrefix(r.To, "/") {
if to.Scheme != "http" && to.Scheme != "https" && to.Scheme != "ipfs" && to.Scheme != "ipns" {
t.Errorf("should error for 'to' URL with scheme other than safelisted ones: url=%q, scheme=%q, orig=%q", to, to.Scheme, orig)
}
}
}

s := bufio.NewScanner(bytes.NewReader(orig))

for s.Scan() {
line := strings.TrimSpace(s.Text())
Expand All @@ -127,43 +167,31 @@ func FuzzParse(f *testing.F) {
continue
}

if err == nil && len(fields) < 2 && line != "" {
t.Errorf("should error with less than 2 fields. orig='%v'", orig)
if len(fields) < 2 && line != "" {
t.Errorf("should error with less than 2 fields. orig=%q", orig)
continue
}

if err == nil && len(fields) > 3 {
t.Errorf("should error with more than 3 fields. orig='%v'", orig)
if len(fields) > 3 {
t.Errorf("should error with more than 3 fields. orig=%q", 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)
if len(fields) > 0 && !strings.HasPrefix(fields[0], "/") {
t.Errorf("should error for from path not starting with '/'. orig=%q", orig)
continue
}

// we already handled these cases
if len(fields) < 3 {
if len(fields) > 0 && strings.Contains(fields[0], "*") && !strings.HasSuffix(fields[0], "*") {
t.Errorf("asterisk in from not at end should error. orig=%q", orig)
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)
if len(fields) > 2 && strings.HasSuffix(fields[2], "!") {
t.Errorf("should error for forced redirects. orig=%q, err=%v", orig, err)
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)
}
}
}
}
})
}

0 comments on commit 8d41a5e

Please sign in to comment.