Skip to content

Commit

Permalink
willnorris#250
Browse files Browse the repository at this point in the history 
Ability to pass remote url encoded (or not).
Option 4 implementation.

Requires `{options}` to be present in request URL.
`x` or `0x0` can be used as no-op options.
The ability to exclude options was not documented in README anyway,
but nevertheless a breaking change.
  • Loading branch information
mattp committed Jan 21, 2021
1 parent 0da684b commit de4d74c
Show file tree
Hide file tree
Showing 4 changed files with 176 additions and 43 deletions.
4 changes: 3 additions & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ See the full list of available options at
### Remote URL ###

The URL of the original image to load is specified as the remainder of the
path, without any encoding. For example,
path and may be URL encoded. For example,
`http://localhost/200/https://willnorris.com/logo.jpg`.

In order to [optimize caching][], it is recommended that URLs not contain query
Expand All @@ -68,6 +68,8 @@ x0.15 | 15% original height, proportional width | <a href="https://imageproxy
200x,q60 | 200px wide, proportional height, 60% quality | <a href="https://imageproxy.willnorris.com/200x,q60/https://willnorris.com/2013/12/small-things.jpg"><img src="https://imageproxy.willnorris.com/200x,q60/https://willnorris.com/2013/12/small-things.jpg" alt="200x,q60"></a>
200x,png | 200px wide, converted to PNG format | <a href="https://imageproxy.willnorris.com/200x,png/https://willnorris.com/2013/12/small-things.jpg"><img src="https://imageproxy.willnorris.com/200x,png/https://willnorris.com/2013/12/small-things.jpg" alt="200x,png"></a>
cx175,cw400,ch300,100x | crop to 400x300px starting at (175,0), scale to 100px wide | <a href="https://imageproxy.willnorris.com/cx175,cw400,ch300,100x/https://willnorris.com/2013/12/small-things.jpg"><img src="https://imageproxy.willnorris.com/cx175,cw400,ch300,100x/https://willnorris.com/2013/12/small-things.jpg" alt="cx175,cw400,ch300,100x"></a>
0x0 | no option | <a href="https://imageproxy.willnorris.com/0x0/https://willnorris.com/2013/12/small-things.jpg"><img src="https://imageproxy.willnorris.com/0x0/https://willnorris.com/2013/12/small-things.jpg" alt="0x0"></a>
x | no option | <a href="https://imageproxy.willnorris.com/x/https://willnorris.com/2013/12/small-things.jpg"><img src="https://imageproxy.willnorris.com/x/https://willnorris.com/2013/12/small-things.jpg" alt="x"></a>

The [smart crop feature](https://godoc.org/willnorris.com/go/imageproxy#hdr-Smart_Crop)
can best be seen by comparing crops of [this source image][judah-sheets], with
Expand Down
82 changes: 61 additions & 21 deletions data.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,39 +320,36 @@ func (r Request) String() string {

// NewRequest parses an http.Request into an imageproxy Request. Options and
// the remote image URL are specified in the request path, formatted as:
// /{options}/{remote_url}. Options may be omitted, so a request path may
// simply contain /{remote_url}. The remote URL must be an absolute "http" or
// "https" URL, should not be URL encoded, and may contain a query string.
// /{options}/{remote_url}. Options may not be omitted, but `x` or `0x0` can
// be used as noop option (/x/{remote_url}).
// The remote URL must be an absolute "http(s)" URL unless BaseURL is set.
// The remote URL may be URL encoded.
//
// Assuming an imageproxy server running on localhost, the following are all
// valid imageproxy requests:
//
// http://localhost/100x200/http://example.com/image.jpg
// http://localhost/100x200,r90/http://example.com/image.jpg?foo=bar
// http://localhost//http://example.com/image.jpg
// http://localhost/http://example.com/image.jpg
// http://localhost/x/http://example.com/image.jpg
// http://localhost/x/http%3A%2F%2Fexample.com%2Fimage.jpg
func NewRequest(r *http.Request, baseURL *url.URL) (*Request, error) {
var err error
req := &Request{Original: r}

path := r.URL.EscapedPath()[1:] // strip leading slash
req.URL, err = parseURL(path)
if err != nil || !req.URL.IsAbs() {
// first segment should be options
parts := strings.SplitN(path, "/", 2)
if len(parts) != 2 {
return nil, URLError{"too few path segments", r.URL}
}

var err error
req.URL, err = parseURL(parts[1])
if err != nil {
return nil, URLError{fmt.Sprintf("unable to parse remote URL: %v", err), r.URL}
}
// first segment should be options
parts := strings.SplitN(path, "/", 2)
if len(parts) != 2 {
return nil, URLError{"too few path segments", r.URL}
}

req.Options = ParseOptions(parts[0])
req.URL, err = parseURL(parts[1])
if err != nil {
return nil, URLError{fmt.Sprintf("unable to parse remote URL: %v", err), r.URL}
}

req.Options = ParseOptions(parts[0])

if baseURL != nil {
req.URL = baseURL.ResolveReference(req.URL)
}
Expand All @@ -365,8 +362,16 @@ func NewRequest(r *http.Request, baseURL *url.URL) (*Request, error) {
return nil, URLError{"remote URL must have http or https scheme", r.URL}
}

// query string is always part of the remote URL
req.URL.RawQuery = r.URL.RawQuery
// only append original (unencoded) query string
// if we don't have one already. Example:
// http://localhost/x/https%3A%2F%2Fexample.com%2Ffoo%2Fbar%3Fhello%3Dworld?more=query
// should be https://example.com/foo/bar?hello=world
// not https://example.com/foo/bar?more=query
if len(r.URL.RawQuery) > 0 && len(req.URL.RawQuery) == 0 {
// query string is always part of the remote URL
req.URL.RawQuery = r.URL.RawQuery
}

return req, nil
}

Expand All @@ -375,6 +380,41 @@ var reCleanedURL = regexp.MustCompile(`^(https?):/+([^/])`)
// parseURL parses s as a URL, handling URLs that have been munged by
// path.Clean or a webserver that collapses multiple slashes.
func parseURL(s string) (*url.URL, error) {
var err error

// convert http:/example.com to http://example.com
s = reCleanedURL.ReplaceAllString(s, "$1://$2")

s, err = decodeURL(s)
if err != nil {
return nil, err
}

return url.Parse(s)
}

var reAbsURL = regexp.MustCompile(`^https?`)
var reDecodedURL = regexp.MustCompile(`^https?://`)
func decodeURL(s string) (string, error) {
var err error
// don't try to decode unless looks like abs url
if !reAbsURL.MatchString(s) {
return s, nil
}

// perserve original value in case we are not able to decode
decodedUrl := s
maxDecodeAttempts := 3
for i := 0; !reDecodedURL.MatchString(decodedUrl) && i < maxDecodeAttempts; i++ {
decodedUrl, err = url.QueryUnescape(decodedUrl)
if err != nil {
return "", URLError{"remote URL could not be decoded", nil}
}
}
if reDecodedURL.MatchString(decodedUrl) {
return decodedUrl, nil
} else {
// return original value. might be relative url (e.g. https/foo.jpg)
return s, nil
}
}
123 changes: 107 additions & 16 deletions data_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ func TestNewRequest(t *testing.T) {
{"http://localhost//example.com/foo", "", emptyOptions, true},
{"http://localhost//ftp://example.com/foo", "", emptyOptions, true},

// invalid URL because options now required
{"http://localhost/http://example.com/foo", "", emptyOptions, true},

// invalid options. These won't return errors, but will not fully parse the options
{
"http://localhost/s/http://example.com/",
Expand All @@ -136,37 +139,60 @@ func TestNewRequest(t *testing.T) {

// valid URLs
{
"http://localhost/http://example.com/foo",
"http://localhost//http://example.com/foo",
"http://example.com/foo", emptyOptions, false,
},
{
"http://localhost//http://example.com/foo",
"http://localhost/x/http://example.com/foo",
"http://example.com/foo", emptyOptions, false,
},
{
"http://localhost/x/http://example.com/foo",
"http://example.com/foo", emptyOptions, false,
},
{
"http://localhost//https://example.com/foo",
"http://localhost/0x0/https://example.com/foo",
"https://example.com/foo", emptyOptions, false,
},
{
"http://localhost/1x2/http://example.com/foo",
"http://example.com/foo", Options{Width: 1, Height: 2}, false,
},
{
"http://localhost//http://example.com/foo?bar",
"http://localhost/0x0/http://example.com/foo?bar",
"http://example.com/foo?bar", emptyOptions, false,
},
{
"http://localhost/http:/example.com/foo",
"http://localhost/x/http:/example.com/foo",
"http://example.com/foo", emptyOptions, false,
},
{
"http://localhost/http:///example.com/foo",
"http://localhost/x/http:///example.com/foo",
"http://example.com/foo", emptyOptions, false,
},
{ // escaped path
"http://localhost/http://example.com/%2C",
"http://localhost/x/http://example.com/%2C",
"http://example.com/%2C", emptyOptions, false,
},
// unescaped querystring
{
"http://localhost/x/http://example.com/foo/bar?hello=world",
"http://example.com/foo/bar?hello=world", emptyOptions, false,
},
// escaped remote including querystring
{
"http://localhost/x/http%3A%2F%2Fexample.com%2Ffoo%2Fbar%3Fhello%3Dworld",
"http://example.com/foo/bar?hello=world", emptyOptions, false,
},
{
"http://localhost/x/https%3A%2F%2Fexample.com%2Ffoo%2Fbar%3Fhello%3Dworld",
"https://example.com/foo/bar?hello=world", emptyOptions, false,
},
// multi-escaped remote
{
"http://localhost/x/https%25253A%25252F%25252Fexample.com%25252Ffoo%25252Fbar%25253Fhello%25253Dworld",
"https://example.com/foo/bar?hello=world", emptyOptions, false,
},
}

for _, tt := range tests {
Expand Down Expand Up @@ -197,17 +223,82 @@ func TestNewRequest(t *testing.T) {
}

func TestNewRequest_BaseURL(t *testing.T) {
req, _ := http.NewRequest("GET", "/x/path", nil)
base, _ := url.Parse("https://example.com/")

r, err := NewRequest(req, base)
if err != nil {
t.Errorf("NewRequest(%v, %v) returned unexpected error: %v", req, base, err)
tests := []struct {
BaseURL string // base url to use
URL string // input URL to parse as an imageproxy request
RemoteURL string // expected URL of remote image parsed from input
Options Options // expected options parsed from input
ExpectError bool // whether an error is expected from NewRequest
}{
{
"http://example.com/",
"http://localhost/x/foo",
"http://example.com/foo", emptyOptions, false,
},
{
"http://example.com/hello",
"http://localhost/x//foo/bar",
"http://example.com/foo/bar", emptyOptions, false,
},
// if BaseURL doesn't have trailing slash
// URL.ResolveReference will strip last directory
{
"http://example.com/hello/",
"http://localhost/x/foo/bar",
"http://example.com/hello/foo/bar", emptyOptions, false,
},
{
"http://example.com/hello/",
"http://localhost/x/../foo/bar",
"http://example.com/foo/bar", emptyOptions, false,
},
}

want := "https://example.com/path#0x0"
if got := r.String(); got != want {
t.Errorf("NewRequest(%v, %v) returned %q, want %q", req, base, got, want)

for _, tt := range tests {
req, err := http.NewRequest("GET", tt.URL, nil)
if err != nil {
t.Errorf("http.NewRequest(%q) returned error: %v", tt.URL, err)
continue
}
base, err := url.Parse(tt.BaseURL)
if err != nil {
t.Errorf("url.Parse(%q) returned error: %v", tt.BaseURL, err)
continue
}

r, err := NewRequest(req, base)
if tt.ExpectError {
if err == nil {
t.Errorf("NewRequest(%v, %v) did not return expected error", req, base)
}
continue
} else if err != nil {
t.Errorf("NewRequest(%v, %v) returned unexpected error: %v", req, base, err)
continue
}

if got, want := r.URL.String(), tt.RemoteURL; got != want {
t.Errorf("NewRequest(%q, %q) request URL = %v, want %v", tt.URL, tt.BaseURL, got, want)
}
if got, want := r.Options, tt.Options; got != want {
t.Errorf("NewRequest(%q, %q) request options = %v, want %v", tt.URL, tt.BaseURL, got, want)
}
}



// req, _ := http.NewRequest("GET", "/x/path", nil)
// base, _ := url.Parse("https://example.com/")

// r, err := NewRequest(req, base)
// if err != nil {
// t.Errorf("NewRequest(%v, %v) returned unexpected error: %v", req, base, err)
// }

// want := "https://example.com/path#0x0"
// if got := r.String(); got != want {
// t.Errorf("NewRequest(%v, %v) returned %q, want %q", req, base, got, want)
// }

}
10 changes: 5 additions & 5 deletions imageproxy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -400,10 +400,10 @@ func TestProxy_ServeHTTP(t *testing.T) {
code int // expected response status code
}{
{"/favicon.ico", http.StatusOK},
{"//foo", http.StatusBadRequest}, // invalid request URL
{"/http://bad.test/", http.StatusForbidden}, // Disallowed host
{"/http://good.test/error", http.StatusInternalServerError}, // HTTP protocol error
{"/http://good.test/nocontent", http.StatusNoContent}, // non-OK response
{"/x/foo", http.StatusBadRequest}, // invalid request URL
{"/x/http://bad.test/", http.StatusForbidden}, // Disallowed host
{"/x/http://good.test/error", http.StatusInternalServerError}, // HTTP protocol error
{"/x/http://good.test/nocontent", http.StatusNoContent}, // non-OK response
{"/100/http://good.test/png", http.StatusOK},
{"/100/http://good.test/plain", http.StatusForbidden}, // non-image response

Expand Down Expand Up @@ -431,7 +431,7 @@ func TestProxy_ServeHTTP_is304(t *testing.T) {
},
}

req, _ := http.NewRequest("GET", "http://localhost/http://good.test/etag", nil)
req, _ := http.NewRequest("GET", "http://localhost/x/http://good.test/etag", nil)
req.Header.Add("If-None-Match", `"tag"`)
resp := httptest.NewRecorder()
p.ServeHTTP(resp, req)
Expand Down

0 comments on commit de4d74c

Please sign in to comment.