From a3e3561467a552b1ecf4dd3da94749febbbe9cac Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 13 Apr 2017 15:18:20 +0100 Subject: [PATCH] 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 | 129 ++++++++++++------ .../matrix-org/rageshake/submit_test.go | 79 +++++++++++ 2 files changed, 166 insertions(+), 42 deletions(-) create mode 100644 src/github.com/matrix-org/rageshake/submit_test.go diff --git a/src/github.com/matrix-org/rageshake/submit.go b/src/github.com/matrix-org/rageshake/submit.go index 4dc0758..574cab4 100644 --- a/src/github.com/matrix-org/rageshake/submit.go +++ b/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) - return - } - 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) + + p := parseRequest(w, req) + if p == nil { + // parseRequest already wrote an error return } - // 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" - } - - if err := s.saveReport(req.Context(), p); err != nil { + 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 - 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, - } + issueReq := buildGithubIssueRequest(p, listingURL) 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("{}")) diff --git a/src/github.com/matrix-org/rageshake/submit_test.go b/src/github.com/matrix-org/rageshake/submit_test.go new file mode 100644 index 0000000..18a8e9a --- /dev/null +++ b/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") + } +}