Undo the android client's mangled submissions (#9)

current riot-android stuffs everything into the 'version' field; I don't really
want that ending up in the github issue (especially the mxid), and I don't want
to have to wait for an update to riot-android to land, so let's unpick
it into 'data'.

src/github.com/matrix-org/rageshake/submit.go

@@ -75,23 +75,14 @@ func (s *submitServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		respond(200, w)
 		return
 	}
-	if length, err := strconv.Atoi(req.Header.Get("Content-Length")); err != nil || length > maxPayloadSize {
+
-		respond(413, w)
+	p := parseRequest(w, req)
-		return
+	if p == nil {
-	}
+		// parseRequest already wrote an error
-	var p payload
-	if err := json.NewDecoder(req.Body).Decode(&p); err != nil {
-		http.Error(w, fmt.Sprintf("Could not decode payload: %s", err.Error()), 400)
 		return
 	}
 
-	// backwards-compatibility hack: current versions of riot-android
+	if err := s.saveReport(req.Context(), *p); err != nil {
-	// don't set 'app', so we don't correctly file github issues.
-	if p.AppName == "" && p.UserAgent == "Android" {
-		p.AppName = "riot-android"
-	}
-
-	if err := s.saveReport(req.Context(), p); err != nil {
 		log.Println("Error handling report", err)
 		http.Error(w, "Internal error", 500)
 		return
@@ -100,6 +91,59 @@ func (s *submitServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	respond(200, w)
 }
 
+// parseRequest attempts to parse a received request as a bug report. If
+// the request cannot be parsed, it responds with an error and returns nil.
+func parseRequest(w http.ResponseWriter, req *http.Request) *payload {
+	length, err := strconv.Atoi(req.Header.Get("Content-Length"))
+	if err != nil {
+		log.Println("Couldn't parse content-length", err)
+		http.Error(w, "Bad content-length", 400)
+		return nil
+	}
+	if length > maxPayloadSize {
+		log.Println("Content-length", length, "too large")
+		http.Error(w, fmt.Sprintf("Content too large (max %i)", maxPayloadSize), 413)
+		return nil
+	}
+	var p payload
+	if err := json.NewDecoder(req.Body).Decode(&p); err != nil {
+		log.Println("Couldn't decode request body", err)
+		http.Error(w, fmt.Sprintf("Could not decode payload: %s", err.Error()), 400)
+		return nil
+	}
+
+	p.Text = strings.TrimSpace(p.Text)
+
+	if p.Data == nil {
+		p.Data = make(map[string]string)
+	}
+
+	// backwards-compatibility hack: current versions of riot-android
+	// don't set 'app', so we don't correctly file github issues.
+	if p.AppName == "" && p.UserAgent == "Android" {
+		p.AppName = "riot-android"
+
+		// they also shove lots of stuff into 'Version' which we don't really
+		// want in the github report
+		for _, line := range strings.Split(p.Version, "\n") {
+			line = strings.TrimSpace(line)
+			if line == "" {
+				continue
+			}
+			parts := strings.SplitN(line, ":", 2)
+			key := strings.TrimSpace(parts[0])
+			val := ""
+			if len(parts) > 1 {
+				val = strings.TrimSpace(parts[1])
+			}
+			p.Data[key] = val
+		}
+		p.Version = ""
+	}
+
+	return &p
+}
+
 func (s *submitServer) saveReport(ctx context.Context, p payload) error {
 	// Dump bug report to disk as form:
 	//  "bugreport-20170115-112233.log.gz" => user text, version, user agent, # logs
@@ -112,13 +156,11 @@ func (s *submitServer) saveReport(ctx context.Context, p payload) error {
 
 	log.Println("Handling report submission; listing URI will be", listingURL)
 
-	userText := strings.TrimSpace(p.Text)
-
 	var summaryBuf bytes.Buffer
 	fmt.Fprintf(
 		&summaryBuf,
 		"%s\n\nNumber of logs: %d\nApplication: %s\nVersion: %s\nUser-Agent: %s\n",
-		userText, len(p.Logs), p.AppName, p.Version, p.UserAgent,
+		p.Text, len(p.Logs), p.AppName, p.Version, p.UserAgent,
 	)
 	for k, v := range p.Data {
 		fmt.Fprintf(&summaryBuf, "%s: %s\n", k, v)
@@ -140,7 +182,6 @@ func (s *submitServer) saveReport(ctx context.Context, p payload) error {
 	}
 
 	// submit a github issue
-
 	ghProj := s.githubProjectMappings[p.AppName]
 	if ghProj == "" {
 		log.Println("Not creating GH issue for unknown app", p.AppName)
@@ -152,30 +193,7 @@ func (s *submitServer) saveReport(ctx context.Context, p payload) error {
 	}
 	owner, repo := splits[0], splits[1]
 
-	var title string
+	issueReq := buildGithubIssueRequest(p, listingURL)
-	if userText == "" {
-		title = "Untitled report"
-	} else {
-		// set the title to the first line of the user's report
-		if i := strings.IndexAny(userText, "\r\n"); i < 0 {
-			title = userText
-		} else {
-			title = userText[0:i]
-		}
-	}
-
-	body := fmt.Sprintf(
-		"User message:\n```\n%s\n```\nVersion: %s\n[Details](%s) / [Logs](%s)",
-		userText,
-		p.Version,
-		listingURL+"/details.log.gz",
-		listingURL,
-	)
-
-	issueReq := github.IssueRequest{
-		Title: &title,
-		Body:  &body,
-	}
 
 	issue, _, err := s.ghClient.Issues.Create(ctx, owner, repo, &issueReq)
 	if err != nil {
@@ -187,6 +205,33 @@ func (s *submitServer) saveReport(ctx context.Context, p payload) error {
 	return nil
 }
 
+func buildGithubIssueRequest(p payload, listingURL string) github.IssueRequest {
+	var title string
+	if p.Text == "" {
+		title = "Untitled report"
+	} else {
+		// set the title to the first line of the user's report
+		if i := strings.IndexAny(p.Text, "\r\n"); i < 0 {
+			title = p.Text
+		} else {
+			title = p.Text[0:i]
+		}
+	}
+
+	body := fmt.Sprintf(
+		"User message:\n```\n%s\n```\nVersion: %s\n[Details](%s) / [Logs](%s)",
+		p.Text,
+		p.Version,
+		listingURL+"/details.log.gz",
+		listingURL,
+	)
+
+	return github.IssueRequest{
+		Title: &title,
+		Body:  &body,
+	}
+}
+
 func respond(code int, w http.ResponseWriter) {
 	w.WriteHeader(code)
 	w.Write([]byte("{}"))

src/github.com/matrix-org/rageshake/submit_test.go

@@ -0,0 +1,79 @@
+/*
+Copyright 2017 Vector Creations Ltd
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package main
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strconv"
+	"strings"
+	"testing"
+)
+
+// testParsePayload builds a /submit request with the given body, and calls
+// parseRequest with it.
+func testParsePayload(t *testing.T, body, contentType string) (p *payload) {
+	req, err := http.NewRequest("POST", "/api/submit", strings.NewReader(body))
+	if err != nil {
+		t.Fatal(err)
+	}
+	req.Header.Set("Content-Length", strconv.Itoa(len(body)))
+	if contentType != "" {
+		req.Header.Set("Content-Type", contentType)
+	}
+	rr := httptest.NewRecorder()
+	p = parseRequest(rr, req)
+
+	if p == nil {
+		t.Error("parseRequest returned nil")
+	}
+	return
+}
+
+func TestEmptyJson(t *testing.T) {
+	body := "{}"
+
+	// we just test it is parsed without errors for now
+	testParsePayload(t, body, "application/json")
+}
+
+// check that we can unpick the json submitted by the android clients
+func TestUnpickAndroidMangling(t *testing.T) {
+	body := `{"text": "test ylc 001",
+"version": "User : @ylc8001:matrix.org\nPhone : Lenovo P2a42\nVector version: 0:6:9\n",
+"user_agent": "Android"
+}`
+	p := testParsePayload(t, body, "")
+	if p.Text != "test ylc 001" {
+		t.Errorf("user text: got %s, want %s", p.Text, "test ylc 001")
+	}
+	if p.AppName != "riot-android" {
+		t.Errorf("appname: got %s, want %s", p.AppName, "riot-android")
+	}
+	if p.Version != "" {
+		t.Errorf("version: got %s, want ''", p.Version)
+	}
+	if p.Data["User"] != "@ylc8001:matrix.org" {
+		t.Errorf("data.user: got %s, want %s", p.Data["User"], "@ylc8001:matrix.org")
+	}
+	if p.Data["Phone"] != "Lenovo P2a42" {
+		t.Errorf("data.phone: got %s, want %s", p.Data["Phone"], "Lenovo P2a42")
+	}
+	if p.Data["Vector version"] != "0:6:9" {
+		t.Errorf("data.version: got %s, want %s", p.Data["Version"], "0:6:9")
+	}
+}