Repository: gobuffalo/buffalo
Pull request #2322 (2022-09-16 09:39:16)

safer error handling
The `defaultErrorHandler` presently compares the environment against "production" to determine whether it's safe to dump debugging details. When running in staging (or qa, etc.), it is preferable to not have debugging detail since it could be hosted on a public server. This PR reverses the logic so the detailed error message is only used when the environment is "development".

sio4: Hi @Schparky,

Currently, the `main` branch is not configured properly. I am working on the cleanup of v1 release and the main will be used for the next version. Did you make the change based on the main branch? Please use `v1` branch.
sio4: Ah, it looks like you started from the correct branch. I changed the target.
sio4: I tested two things.

1. filed my own PR against v1 branch --> the tests worked fine
2. retry the failed test for this PR after changing merge target --> still failing

The symptom: `actions/checkout@v3` keep trying to merge this PR against the last commit of `main`.

```
  HEAD is now at 7cc1cdc Merge 13d320b2917516df9d7a8674c9f9b7975c75a0c6 into 89ed84d0d3bf2db8474659131231a0f5117eba27
```

89ed84d is the latest commit in the main branch.

sio4: This issue was originally discussed in the Slack channel and I think it could be fine. Even though it changes the behavior of error handling for each environment, the direction of this change is narrowing down the scope of a safe place, it could be better.

* The previous behavior: shows the simple error page for `production` but shows debugging pages for all others
* The changed behavior: shows the simple error page for all other environments, but only shows debugging page when the environment is exactly set as `development`

This is better from the security point of view and allows users have other custom environment names for their various production-ish environments.


I basically agree that those debugging messages SHOULD only be allowed for a specific narrow environment so != development looks better than == production to me. However, regardless the other values are mentioned in the doc or not, behavior change should be handled carefully so I just ask you to consider them if you are fine. ￼


[1] https://gophers.slack.com/archives/C3MSAFD40/p1663318963883059

Patch:
diff --git a/README.md b/README.md
index 80971fd2..acc5d5d4 100644
--- a/README.md
+++ b/README.md
@@ -90,7 +90,7 @@ First, thank you so much for wanting to contribute! It means so much that you ca
 - Consider opening an issue **BEFORE** creating a Pull request (PR): you won't
   lose your time on fixing non-existing bugs, or fixing the wrong bug. Also we
   can help you to produce the best PR!
-- Open a PR against the `main` branch if your PR is for mainstrem or version
+- Open a PR against the `main` branch if your PR is for mainstream or version
   specific branch e.g. `v1` if your PR is for specific version.
   Note that the valid branch for a new feature request PR should be `main`
   while a PR against a version specific branch are allowed only for bugfixes.
diff --git a/errors.go b/errors.go
index 228bd386..ee6a619f 100644
--- a/errors.go
+++ b/errors.go
@@ -174,7 +174,7 @@ func defaultErrorHandler(status int, origErr error, c Context) error {
 	c.Logger().Error(origErr)
 	c.Response().WriteHeader(status)
 
-	if env != nil && env.(string) == "production" {
+	if env != nil && env.(string) != "development" {
 		switch strings.ToLower(requestCT) {
 		case "application/json", "text/json", "json", "application/xml", "text/xml", "xml":
 			defaultErrorResponse = &ErrorResponse{


Test patch:
diff --git a/errors_test.go b/errors_test.go
index bf55f600..10d359d0 100644
--- a/errors_test.go
+++ b/errors_test.go
@@ -66,81 +66,74 @@ func Test_defaultErrorHandler_Logger(t *testing.T) {
 }
 
 func Test_defaultErrorHandler_JSON_development(t *testing.T) {
-	r := require.New(t)
-	app := New(Options{})
-	app.GET("/", func(c Context) error {
-		return c.Error(http.StatusUnauthorized, fmt.Errorf("boom"))
-	})
-
-	w := httptest.New(app)
-	res := w.JSON("/").Get()
-	r.Equal(http.StatusUnauthorized, res.Code)
-	ct := res.Header().Get("content-type")
-	r.Equal("application/json", ct)
-	b := res.Body.String()
-	r.Contains(b, `"code":401`)
-	r.Contains(b, `"error":"boom"`)
-	r.Contains(b, `"trace":"`)
+	testDefaultErrorHandler(t, "application/json", "development")
 }
 
 func Test_defaultErrorHandler_XML_development(t *testing.T) {
-	r := require.New(t)
-	app := New(Options{})
-	app.GET("/", func(c Context) error {
-		return c.Error(http.StatusUnauthorized, fmt.Errorf("boom"))
-	})
+	testDefaultErrorHandler(t, "text/xml", "development")
+}
 
-	w := httptest.New(app)
-	res := w.XML("/").Get()
-	r.Equal(http.StatusUnauthorized, res.Code)
-	ct := res.Header().Get("content-type")
-	r.Equal("text/xml", ct)
-	b := res.Body.String()
-	r.Contains(b, `<response code="401">`)
-	r.Contains(b, `<error>boom</error>`)
-	r.Contains(b, `<trace>`)
-	r.Contains(b, `</trace>`)
-	r.Contains(b, `</response>`)
+func Test_defaultErrorHandler_JSON_staging(t *testing.T) {
+	testDefaultErrorHandler(t, "application/json", "staging")
 }
 
-func Test_defaultErrorHandler_JSON_production(t *testing.T) {
-	r := require.New(t)
-	app := New(Options{})
-	app.Env = "production"
-	app.GET("/", func(c Context) error {
-		return c.Error(http.StatusUnauthorized, fmt.Errorf("boom"))
-	})
+func Test_defaultErrorHandler_XML_staging(t *testing.T) {
+	testDefaultErrorHandler(t, "text/xml", "staging")
+}
 
-	w := httptest.New(app)
-	res := w.JSON("/").Get()
-	r.Equal(http.StatusUnauthorized, res.Code)
-	ct := res.Header().Get("content-type")
-	r.Equal("application/json", ct)
-	b := res.Body.String()
-	r.Contains(b, `"code":401`)
-	r.Contains(b, fmt.Sprintf(`"error":"%s"`, http.StatusText(http.StatusUnauthorized)))
-	r.NotContains(b, `"trace":"`)
+func Test_defaultErrorHandler_JSON_production(t *testing.T) {
+	testDefaultErrorHandler(t, "application/json", "production")
 }
 
 func Test_defaultErrorHandler_XML_production(t *testing.T) {
+	testDefaultErrorHandler(t, "text/xml", "production")
+}
+
+func testDefaultErrorHandler(t *testing.T, contentType, env string) {
 	r := require.New(t)
 	app := New(Options{})
-	app.Env = "production"
+	app.Env = env
 	app.GET("/", func(c Context) error {
 		return c.Error(http.StatusUnauthorized, fmt.Errorf("boom"))
 	})
 
 	w := httptest.New(app)
-	res := w.XML("/").Get()
+	var res *httptest.Response
+	if contentType == "application/json" {
+		res = w.JSON("/").Get().Response
+	} else {
+		res = w.XML("/").Get().Response
+	}
 	r.Equal(http.StatusUnauthorized, res.Code)
 	ct := res.Header().Get("content-type")
-	r.Equal("text/xml", ct)
+	r.Equal(contentType, ct)
 	b := res.Body.String()
-	r.Contains(b, `<response code="401">`)
-	r.Contains(b, fmt.Sprintf(`<error>%s</error>`, http.StatusText(http.StatusUnauthorized)))
-	r.NotContains(b, `<trace>`)
-	r.NotContains(b, `</trace>`)
-	r.Contains(b, `</response>`)
+
+	if env == "development" {
+		if contentType == "text/xml" {
+			r.Contains(b, `<response code="401">`)
+			r.Contains(b, `<error>boom</error>`)
+			r.Contains(b, `<trace>`)
+			r.Contains(b, `</trace>`)
+			r.Contains(b, `</response>`)
+		} else {
+			r.Contains(b, `"code":401`)
+			r.Contains(b, `"error":"boom"`)
+			r.Contains(b, `"trace":"`)
+		}
+	} else {
+		if contentType == "text/xml" {
+			r.Contains(b, `<response code="401">`)
+			r.Contains(b, fmt.Sprintf(`<error>%s</error>`, http.StatusText(http.StatusUnauthorized)))
+			r.NotContains(b, `<trace>`)
+			r.NotContains(b, `</trace>`)
+			r.Contains(b, `</response>`)
+		} else {
+			r.Contains(b, `"code":401`)
+			r.Contains(b, fmt.Sprintf(`"error":"%s"`, http.StatusText(http.StatusUnauthorized)))
+			r.NotContains(b, `"trace":"`)
+		}
+	}
 }
 
 func Test_defaultErrorHandler_nil_error(t *testing.T) {