Normalize URL query string before executing HTTP requests

- Make sure query strings parameters are encoded
- As opposed to the standard library, do not append equal sign
for query parameters with empty value
- Strip URL fragments like Web browsers

3 files changed, 47 insertions, 18 deletions

diff --git a/http/client/client.go b/http/client/client.go
index 175fe9f..d38859a 100644
--- a/http/client/client.go
+++ b/http/client/client.go
@@ -22,6 +22,7 @@ import (
 	"miniflux.app/errors"
 	"miniflux.app/logger"
 	"miniflux.app/timer"
+	url_helper "miniflux.app/url"
 	"miniflux.app/version"
 )
 
@@ -37,7 +38,8 @@ var (
 
 // Client is a HTTP Client :)
 type Client struct {
-	url                 string
+	inputURL            string
+	requestURL          string
 	etagHeader          string
 	lastModifiedHeader  string
 	authorizationHeader string
@@ -47,6 +49,18 @@ type Client struct {
 	Insecure            bool
 }
 
+func (c *Client) String() string {
+	return fmt.Sprintf(
+		`InputURL=%q RequestURL=%q ETag=%s LastModified=%q BasicAuth=%v UserAgent=%q`,
+		c.inputURL,
+		c.requestURL,
+		c.etagHeader,
+		c.lastModifiedHeader,
+		c.authorizationHeader != "" || (c.username != "" && c.password != ""),
+		c.userAgent,
+	)
+}
+
 // WithCredentials defines the username/password for HTTP Basic authentication.
 func (c *Client) WithCredentials(username, password string) *Client {
 	if username != "" && password != "" {
@@ -115,7 +129,12 @@ func (c *Client) PostJSON(data interface{}) (*Response, error) {
 }
 
 func (c *Client) executeRequest(request *http.Request) (*Response, error) {
-	defer timer.ExecutionTime(time.Now(), fmt.Sprintf("[HttpClient] url=%s", c.url))
+	defer timer.ExecutionTime(time.Now(), fmt.Sprintf("[HttpClient] inputURL=%s", c.inputURL))
+
+	logger.Debug("[HttpClient:Before] Method=%s %s",
+		request.Method,
+		c.String(),
+	)
 
 	client := c.buildClient()
 	resp, err := client.Do(request)
@@ -162,21 +181,15 @@ func (c *Client) executeRequest(request *http.Request) (*Response, error) {
 		EffectiveURL:  resp.Request.URL.String(),
 		LastModified:  resp.Header.Get("Last-Modified"),
 		ETag:          resp.Header.Get("ETag"),
+		Expires:       resp.Header.Get("Expires"),
 		ContentType:   resp.Header.Get("Content-Type"),
 		ContentLength: resp.ContentLength,
 	}
 
-	logger.Debug("[HttpClient:%s] URL=%s, EffectiveURL=%s, Code=%d, Length=%d, Type=%s, ETag=%s, LastMod=%s, Expires=%s, Auth=%v",
+	logger.Debug("[HttpClient:After] Method=%s %s; Response => %s",
 		request.Method,
-		c.url,
-		response.EffectiveURL,
-		response.StatusCode,
-		resp.ContentLength,
-		response.ContentType,
-		response.ETag,
-		response.LastModified,
-		resp.Header.Get("Expires"),
-		c.username != "",
+		c.String(),
+		response,
 	)
 
 	// Ignore caching headers for feeds that do not want any cache.
@@ -190,7 +203,8 @@ func (c *Client) executeRequest(request *http.Request) (*Response, error) {
 }
 
 func (c *Client) buildRequest(method string, body io.Reader) (*http.Request, error) {
-	request, err := http.NewRequest(method, c.url, body)
+	c.requestURL = url_helper.RequestURI(c.inputURL)
+	request, err := http.NewRequest(method, c.requestURL, body)
 	if err != nil {
 		return nil, err
 	}
@@ -238,5 +252,5 @@ func (c *Client) buildHeaders() http.Header {
 
 // New returns a new HTTP client.
 func New(url string) *Client {
-	return &Client{url: url, userAgent: DefaultUserAgent, Insecure: false}
+	return &Client{inputURL: url, userAgent: DefaultUserAgent, Insecure: false}
 }
diff --git a/http/client/response.go b/http/client/response.go
index e62b5ab..122c40c 100644
--- a/http/client/response.go
+++ b/http/client/response.go
@@ -6,6 +6,7 @@ package client // import "miniflux.app/http/client"
 
 import (
 	"bytes"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"regexp"
@@ -24,10 +25,24 @@ type Response struct {
 	EffectiveURL  string
 	LastModified  string
 	ETag          string
+	Expires       string
 	ContentType   string
 	ContentLength int64
 }
 
+func (r *Response) String() string {
+	return fmt.Sprintf(
+		`StatusCode=%d EffectiveURL=%q LastModified=%q ETag=%s Expires=%s ContentType=%q ContentLength=%d`,
+		r.StatusCode,
+		r.EffectiveURL,
+		r.LastModified,
+		r.ETag,
+		r.Expires,
+		r.ContentType,
+		r.ContentLength,
+	)
+}
+
 // IsNotFound returns true if the resource doesn't exists anymore.
 func (r *Response) IsNotFound() bool {
 	return r.StatusCode == 404 || r.StatusCode == 410
@@ -105,8 +120,8 @@ func (r *Response) EnsureUnicodeBody() (err error) {
 	return err
 }
 
-// String returns the response body as string.
-func (r *Response) String() string {
+// BodyAsString returns the response body as string.
+func (r *Response) BodyAsString() string {
 	bytes, _ := ioutil.ReadAll(r.Body)
 	return string(bytes)
 }
diff --git a/http/client/response_test.go b/http/client/response_test.go
index 818fd81..6a2d8c2 100644
--- a/http/client/response_test.go
+++ b/http/client/response_test.go
@@ -95,7 +95,7 @@ func TestToString(t *testing.T) {
 	input := `test`
 	r := &Response{Body: strings.NewReader(input)}
 
-	if r.String() != input {
+	if r.BodyAsString() != input {
 		t.Error(`Unexpected ouput`)
 	}
 }
@@ -140,7 +140,7 @@ func TestEnsureUnicodeWithHTMLDocuments(t *testing.T) {
 			t.Fatalf(`Unicode conversion error for %q - %q: %v`, tc.filename, tc.contentType, parseErr)
 		}
 
-		isUnicode := utf8.ValidString(r.String())
+		isUnicode := utf8.ValidString(r.BodyAsString())
 		if isUnicode != tc.convertedToUnicode {
 			t.Errorf(`Unicode conversion %q - %q, got: %v, expected: %v`,
 				tc.filename, tc.contentType, isUnicode, tc.convertedToUnicode)