Merge branch '116-dont-log-query-strings' into 'master'

Resolve "Don't log query strings"

Closes #116

See merge request gitlab-org/gitlab-pages!77

2 files changed, 32 insertions, 4 deletions

diff --git a/logging.go b/logging.go
index f3859059..c2980503 100644
--- a/logging.go
+++ b/logging.go
@@ -3,6 +3,7 @@ package main
 import (
 	"fmt"
 	"net/http"
+	"net/url"
 	"time"
 
 	log "github.com/sirupsen/logrus"
@@ -64,19 +65,32 @@ func (l *loggingResponseWriter) WriteHeader(status int) {
 	l.rw.WriteHeader(status)
 }
 
-func (l *loggingResponseWriter) Log(r *http.Request) {
-	fields := log.Fields{
+func (l *loggingResponseWriter) extractLogFields(r *http.Request) log.Fields {
+	referer := r.Referer()
+	parsedReferer, err := url.Parse(referer)
+
+	// The referer query string may contain credentials, so remove if possible
+	if err == nil {
+		parsedReferer.RawQuery = ""
+		referer = parsedReferer.String()
+	}
+
+	return log.Fields{
 		"host":       r.Host,
 		"remoteAddr": r.RemoteAddr,
 		"method":     r.Method,
-		"uri":        r.RequestURI,
+		"uri":        r.URL.Path, //The request query string may contain credentials
 		"proto":      r.Proto,
 		"status":     l.status,
 		"written":    l.written,
-		"referer":    r.Referer(),
+		"referer":    referer,
 		"userAgent":  r.UserAgent(),
 		"duration":   time.Since(l.started).Seconds(),
 	}
+}
+
+func (l *loggingResponseWriter) Log(r *http.Request) {
+	fields := l.extractLogFields(r)
 
 	switch accessLogFormat {
 	case "text":
diff --git a/logging_test.go b/logging_test.go
index bc557a71..8cabbde7 100644
--- a/logging_test.go
+++ b/logging_test.go
@@ -3,6 +3,7 @@ package main
 import (
 	"fmt"
 	"net/http"
+	"net/http/httptest"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -28,6 +29,19 @@ func testLogWithDoubleStatus(ww http.ResponseWriter, r *http.Request) {
 	http.Redirect(&w, r, "/test", 301)
 }
 
+func TestExtractLogFieldsHidesQueryStrings(t *testing.T) {
+	w := httptest.NewRecorder()
+	r := httptest.NewRequest("GET", "/foo?token=bar", nil)
+	r.Header.Set("Referer", "http://invalid.com/bar?token=baz")
+
+	l := newLoggingResponseWriter(w)
+
+	fields := l.extractLogFields(r)
+
+	assert.Equal(t, fields["uri"], "/foo")
+	assert.Equal(t, fields["referer"], "http://invalid.com/bar")
+}
+
 func TestLoggingWriter(t *testing.T) {
 	assert.HTTPBodyContains(t, testLogWithStatus, "GET", "/test", nil, "with-status")
 	assert.HTTPBodyContains(t, testLogWithoutStatus, "GET", "/test", nil, "no-status")