dashboard/app: return 4xx instead of 5xx for user requests

* dashboard/app: return 4xx instead of 5xx for user requests

5xx category signals the Internal Server Errors and require server developers attention.
4xx category means client side problem and doesn't require server developers attention.
Added tests.

4 files changed, 82 insertions, 9 deletions

diff --git a/dashboard/app/access.go b/dashboard/app/access.go
index 7266750f3..e37e08be7 100644
--- a/dashboard/app/access.go
+++ b/dashboard/app/access.go
@@ -117,7 +117,7 @@ func checkCrashTextAccess(c context.Context, r *http.Request, field string, id i
 	if len(crashes) != 1 {
 		err := fmt.Errorf("checkCrashTextAccess: found %v crashes for %v=%v", len(crashes), field, id)
 		if len(crashes) == 0 {
-			err = ErrDontLog{err}
+			err = fmt.Errorf("%v: %w", err, ErrClientNotFound)
 		}
 		return nil, nil, err
 	}
@@ -142,7 +142,7 @@ func checkJobTextAccess(c context.Context, r *http.Request, field string, id int
 		err := fmt.Errorf("checkJobTextAccess: found %v jobs for %v=%v", len(keys), field, id)
 		if len(keys) == 0 {
 			// This can be triggered by bad user requests, so don't log the error.
-			err = ErrDontLog{err}
+			err = fmt.Errorf("%v: %w", err, ErrClientNotFound)
 		}
 		return err
 	}
diff --git a/dashboard/app/app_test.go b/dashboard/app/app_test.go
index a57e2493b..9d2ff4dd7 100644
--- a/dashboard/app/app_test.go
+++ b/dashboard/app/app_test.go
@@ -431,6 +431,53 @@ func TestRedirects(t *testing.T) {
 	c.expectOK(err)
 }
 
+func TestResponseStatusCode(t *testing.T) {
+	tests := []struct {
+		whatURL      string
+		wantRespCode int
+	}{
+		{
+			"/text?tag=CrashLog&x=13354bf5700000",
+			http.StatusNotFound,
+		},
+		{
+			"/text?tag=CrashReport&x=17a2bedcb00000",
+			http.StatusNotFound,
+		},
+		{
+			"/text?tag=ReproSyz&x=107e219b700000",
+			http.StatusNotFound,
+		},
+		{
+			"/text?tag=ReproC&x=1762ad64f00000",
+			http.StatusNotFound,
+		},
+		{
+			"/text?tag=CrashLog",
+			http.StatusBadRequest,
+		},
+		{
+			"/text?tag=CrashReport",
+			http.StatusBadRequest,
+		},
+		{
+			"/text?tag=ReproC",
+			http.StatusBadRequest,
+		},
+		{
+			"/text?tag=ReproSyz",
+			http.StatusBadRequest,
+		},
+	}
+
+	c := NewCtx(t)
+	defer c.Close()
+
+	for _, test := range tests {
+		checkResponseStatusCode(c, AccessUser, test.whatURL, test.wantRespCode)
+	}
+}
+
 func checkLoginRedirect(c *Ctx, accessLevel AccessLevel, url string) {
 	to, err := user.LoginURL(c.ctx, url)
 	if err != nil {
@@ -448,6 +495,14 @@ func checkRedirect(c *Ctx, accessLevel AccessLevel, from, to string, status int)
 	c.expectEQ(httpErr.Headers["Location"], []string{to})
 }
 
+func checkResponseStatusCode(c *Ctx, accessLevel AccessLevel, url string, status int) {
+	_, err := c.AuthGET(accessLevel, url)
+	c.expectNE(err, nil)
+	httpErr, ok := err.(HTTPError)
+	c.expectTrue(ok)
+	c.expectEQ(httpErr.Code, status)
+}
+
 // Test purging of old crashes for bugs with lots of crashes.
 func TestPurgeOldCrashes(t *testing.T) {
 	if testing.Short() {
diff --git a/dashboard/app/handler.go b/dashboard/app/handler.go
index 58f4ff339..2cb0d2739 100644
--- a/dashboard/app/handler.go
+++ b/dashboard/app/handler.go
@@ -7,6 +7,7 @@ import (
 	"bytes"
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"sort"
@@ -52,14 +53,18 @@ func handleContext(fn contextHandler) http.Handler {
 				http.Redirect(w, r, redir.Error(), http.StatusFound)
 				return
 			}
+
+			status := http.StatusInternalServerError
 			logf := log.Errorf
-			if _, dontlog := err.(ErrDontLog); dontlog {
+			var clientError *ErrClient
+			if errors.As(err, &clientError) {
 				// We don't log these as errors because they can be provoked
 				// by invalid user requests, so we don't wan't to pollute error log.
 				logf = log.Warningf
+				status = clientError.HTTPStatus()
 			}
 			logf(c, "%v", err)
-			w.WriteHeader(http.StatusInternalServerError)
+			w.WriteHeader(status)
 			if err1 := templates.ExecuteTemplate(w, "error.html", data); err1 != nil {
 				combinedError := fmt.Sprintf("got err \"%v\" processing ExecuteTemplate() for err \"%v\"", err1, err)
 				http.Error(w, combinedError, http.StatusInternalServerError)
@@ -69,10 +74,23 @@ func handleContext(fn contextHandler) http.Handler {
 }
 
 type (
-	ErrDontLog  struct{ error }
+	ErrClient   struct{ error }
 	ErrRedirect struct{ error }
 )
 
+var ErrClientNotFound = &ErrClient{errors.New("resource not found")}
+var ErrClientBadRequest = &ErrClient{errors.New("bad request")}
+
+func (ce *ErrClient) HTTPStatus() int {
+	switch ce {
+	case ErrClientNotFound:
+		return http.StatusNotFound
+	case ErrClientBadRequest:
+		return http.StatusBadRequest
+	}
+	return http.StatusInternalServerError
+}
+
 func handleAuth(fn contextHandler) contextHandler {
 	return func(c context.Context, w http.ResponseWriter, r *http.Request) error {
 		if err := checkAccessLevel(c, r, config.AccessLevel); err != nil {
diff --git a/dashboard/app/main.go b/dashboard/app/main.go
index 97025c080..621a2db16 100644
--- a/dashboard/app/main.go
+++ b/dashboard/app/main.go
@@ -346,7 +346,7 @@ func handleAdmin(c context.Context, w http.ResponseWriter, r *http.Request) erro
 func handleBug(c context.Context, w http.ResponseWriter, r *http.Request) error {
 	bug, err := findBugByID(c, r)
 	if err != nil {
-		return ErrDontLog{err}
+		return fmt.Errorf("%v, %w", err, ErrClientNotFound)
 	}
 	accessLevel := accessLevel(c, r)
 	if err := checkAccessLevel(c, r, bug.sanitizeAccess(accessLevel)); err != nil {
@@ -501,14 +501,14 @@ func handleTextImpl(c context.Context, w http.ResponseWriter, r *http.Request, t
 	if x := r.FormValue("x"); x != "" {
 		xid, err := strconv.ParseUint(x, 16, 64)
 		if err != nil || xid == 0 {
-			return ErrDontLog{fmt.Errorf("failed to parse text id: %v", err)}
+			return fmt.Errorf("failed to parse text id: %v: %w", err, ErrClientBadRequest)
 		}
 		id = int64(xid)
 	} else {
 		// Old link support, don't remove.
 		xid, err := strconv.ParseInt(r.FormValue("id"), 10, 64)
 		if err != nil || xid == 0 {
-			return ErrDontLog{fmt.Errorf("failed to parse text id: %v", err)}
+			return fmt.Errorf("failed to parse text id: %v: %w", err, ErrClientBadRequest)
 		}
 		id = xid
 	}
@@ -519,7 +519,7 @@ func handleTextImpl(c context.Context, w http.ResponseWriter, r *http.Request, t
 	data, ns, err := getText(c, tag, id)
 	if err != nil {
 		if strings.Contains(err.Error(), "datastore: no such entity") {
-			err = ErrDontLog{err}
+			err = fmt.Errorf("%v: %w", err, ErrClientBadRequest)
 		}
 		return err
 	}