diff --git a/CHANGELOG.md b/CHANGELOG.md index cd2ff9af..ac8a6e02 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ - Minor UI improvements (updated dark primary btn contrast). +- Writes the status header for JSON responses only if the fields picker succeed or has acceptable fallback. + _This is to allow custom response status code in reaction to failed json writes._ + - Bumped `golang.org/x/*` dependencies to silence security scanners ([#7829](https://github.com/pocketbase/pocketbase/discussions/7829)). diff --git a/tools/picker/pick.go b/tools/picker/pick.go index c58a19a6..84d584ab 100644 --- a/tools/picker/pick.go +++ b/tools/picker/pick.go @@ -3,12 +3,16 @@ package picker import ( "encoding/json/jsontext" "encoding/json/v2" + "errors" + "fmt" "strings" "github.com/pocketbase/pocketbase/tools/search" "github.com/pocketbase/pocketbase/tools/tokenizer" ) +var ErrInvalidModifierData = errors.New("failed to apply some of the field modifiers for the provided data") + // Pick converts data into a []any, map[string]any, etc. (using json marshal->unmarshal) // containing only the fields from the parsed rawFields expression. // @@ -16,6 +20,10 @@ import ( // Nested fields should be listed with dot-notation. // Fields value modifiers are also supported using the `:modifier(args)` format (see Modifiers). // +// In case some data fails to apply against the registered modifiers +// a wrapped [ErrInvalidModifierData] is returned that you can inspect and +// decide to ignore or propagate further up the execution chain. +// // Example: // // data := map[string]any{"a": 1, "b": 2, "c": map[string]any{"c1": 11, "c2": 22}} @@ -41,7 +49,9 @@ func Pick(data any, rawFields string) (any, error) { } var decoded any - if err := json.Unmarshal(encoded, &decoded); err != nil { + + err = json.Unmarshal(encoded, &decoded) + if err != nil { return nil, err } // --- @@ -55,10 +65,16 @@ func Pick(data any, rawFields string) (any, error) { if isSearchResult { if decodedMap, ok := decoded.(map[string]any); ok { - pickParsedFields(decodedMap["items"], parsedFields) + err = pickParsedFields(decodedMap["items"], parsedFields) + if err != nil { + return nil, fmt.Errorf("%w: %w", ErrInvalidModifierData, err) + } } } else { - pickParsedFields(decoded, parsedFields) + err = pickParsedFields(decoded, parsedFields) + if err != nil { + return nil, fmt.Errorf("%w: %w", ErrInvalidModifierData, err) + } } return decoded, nil @@ -94,7 +110,10 @@ func parseFields(rawFields string) (map[string]Modifier, error) { func pickParsedFields(data any, fields map[string]Modifier) error { switch v := data.(type) { case map[string]any: - pickMapFields(v, fields) + err := pickMapFields(v, fields) + if err != nil { + return err + } case []map[string]any: for _, item := range v { if err := pickMapFields(item, fields); err != nil { @@ -112,7 +131,7 @@ func pickParsedFields(data any, fields map[string]Modifier) error { for _, item := range v { if err := pickMapFields(item.(map[string]any), fields); err != nil { - return nil + return err } } } diff --git a/tools/picker/pick_test.go b/tools/picker/pick_test.go index 4f2ea37c..3d809d21 100644 --- a/tools/picker/pick_test.go +++ b/tools/picker/pick_test.go @@ -2,12 +2,26 @@ package picker_test import ( "encoding/json/v2" + "errors" "testing" "github.com/pocketbase/pocketbase/tools/picker" "github.com/pocketbase/pocketbase/tools/search" ) +type brokenModifier struct { +} + +func (m *brokenModifier) Modify(val any) (any, error) { + return nil, errors.New("test_error") +} + +func init() { + picker.Modifiers["broken"] = func(args ...string) (picker.Modifier, error) { + return &brokenModifier{}, nil + } +} + func TestPickFields(t *testing.T) { scenarios := []struct { name string @@ -223,6 +237,13 @@ func TestPickFields(t *testing.T) { false, `{"id":"123","rel":{"id":"456","sub":{"id":"789"},"title":"rel_title"}}`, }, + { + "with modifer.Modify error", + map[string]any{"a": 1}, + "*:broken", + true, + `{"a":1}`, + }, { "invalid excerpt modifier", map[string]any{"a": 1, "b": 2, "c": "test"}, diff --git a/tools/router/event.go b/tools/router/event.go index cff155ca..cc353965 100644 --- a/tools/router/event.go +++ b/tools/router/event.go @@ -189,23 +189,24 @@ const jsonFieldsParam = "fields" // Note that invalid UTF8 characters are mangled for compatibility // with earlier versions and to prevent unnecessary causing a response error. func (e *Event) JSON(status int, data any) error { + // try to pick only the requested fields (currently allowed only for "success" responses) + rawFields := e.Request.URL.Query().Get(jsonFieldsParam) + if rawFields != "" && status >= 200 && status <= 299 { + modified, err := picker.Pick(data, rawFields) + if err == nil { + data = modified + } else if !errors.Is(err, picker.ErrInvalidModifierData) { + // @todo ignore for now modifier data errors to avoid introducing + // breaking changes but once the router is merged with core consider + // at least logging for dev purposes + return err + } + } + e.setResponseHeaderIfEmpty(headerContentType, "application/json") e.Response.WriteHeader(status) - rawFields := e.Request.URL.Query().Get(jsonFieldsParam) - - // error response or no fields to pick - if rawFields == "" || status < 200 || status > 299 { - return json.MarshalWrite(e.Response, data, jsontext.AllowInvalidUTF8(true)) - } - - // pick only the requested fields - modified, err := picker.Pick(data, rawFields) - if err != nil { - return err - } - - return json.MarshalWrite(e.Response, modified, jsontext.AllowInvalidUTF8(true)) + return json.MarshalWrite(e.Response, data, jsontext.AllowInvalidUTF8(true)) } // XML writes an XML response. diff --git a/tools/router/event_test.go b/tools/router/event_test.go index 28f11381..08ab716f 100644 --- a/tools/router/event_test.go +++ b/tools/router/event_test.go @@ -19,6 +19,7 @@ import ( "testing" validation "github.com/pocketbase/ozzo-validation/v4" + "github.com/pocketbase/pocketbase/tools/picker" "github.com/pocketbase/pocketbase/tools/router" ) @@ -220,6 +221,8 @@ func TestEventRemoteIP(t *testing.T) { } func TestFindUploadedFiles(t *testing.T) { + t.Parallel() + scenarios := []struct { filename string expectedPattern string @@ -273,6 +276,8 @@ func TestFindUploadedFiles(t *testing.T) { } func TestFindUploadedFilesMissing(t *testing.T) { + t.Parallel() + body := new(bytes.Buffer) mp := multipart.NewWriter(body) mp.Close() @@ -293,6 +298,8 @@ func TestFindUploadedFilesMissing(t *testing.T) { } func TestEventSetGet(t *testing.T) { + t.Parallel() + event := router.Event{} // get before any set (ensures that doesn't panic) @@ -324,6 +331,8 @@ func TestEventSetGet(t *testing.T) { } func TestEventSetAllGetAll(t *testing.T) { + t.Parallel() + data := map[string]any{ "a": 123, "b": 456, @@ -351,6 +360,8 @@ func TestEventSetAllGetAll(t *testing.T) { } func TestEventString(t *testing.T) { + t.Parallel() + scenarios := []testResponseWriteScenario[string]{ { name: "no explicit content-type", @@ -380,6 +391,8 @@ func TestEventString(t *testing.T) { } func TestEventHTML(t *testing.T) { + t.Parallel() + scenarios := []testResponseWriteScenario[string]{ { name: "no explicit content-type", @@ -409,6 +422,8 @@ func TestEventHTML(t *testing.T) { } func TestEventJSON(t *testing.T) { + t.Parallel() + body := map[string]any{ "a": 123, "b": true, @@ -456,7 +471,93 @@ func TestEventJSON(t *testing.T) { } } +func TestEventJSONPickError(t *testing.T) { + t.Parallel() + + req, err := http.NewRequest(http.MethodGet, "/?fields=a:excerpt(-1)", nil) + if err != nil { + t.Fatal(err) + } + + rec := httptest.NewRecorder() + + event := &router.Event{ + Request: req, + Response: &router.ResponseWriter{ResponseWriter: rec}, + } + + err = event.JSON(200, map[string]any{"a": "test"}) + if err == nil { + t.Fatal("Expected JSON to return modifier args err, got nil") + } + + // ensure that no explicit status code was written yet by attempting to write one + // (should do nothing if it was already written) + rec.WriteHeader(567) + + result := rec.Result() + defer result.Body.Close() + + if result.StatusCode != 567 { + t.Fatalf("Expected custom status code, got %d", result.StatusCode) + } +} + +type brokenModifier struct { +} + +func (m *brokenModifier) Modify(val any) (any, error) { + return nil, errors.New("test_error") +} + +func TestEventJSONIgnoredError(t *testing.T) { + t.Parallel() + + picker.Modifiers["broken_ignore"] = func(args ...string) (picker.Modifier, error) { + return &brokenModifier{}, nil + } + + req, err := http.NewRequest(http.MethodGet, "/?fields=a:broken_ignore", nil) + if err != nil { + t.Fatal(err) + } + + rec := httptest.NewRecorder() + + event := &router.Event{ + Request: req, + Response: &router.ResponseWriter{ResponseWriter: rec}, + } + + err = event.JSON(200, map[string]any{"a": "test"}) + if err != nil { + t.Fatalf("Expected nil JSON result, got error %v", err) + } + + // (should do nothing if it was already written) + rec.WriteHeader(567) + + result := rec.Result() + defer result.Body.Close() + + rawBody, err := io.ReadAll(result.Body) + if err != nil { + t.Fatal(err) + } + + if result.StatusCode != http.StatusOK { + t.Fatalf("Expected %d status code, got %d", http.StatusOK, result.StatusCode) + } + + expected := `{"a":"test"}` + if string(rawBody) != expected { + t.Fatalf("Expected json\n%s\ngot\n%s", expected, rawBody) + } +} + func TestEventXML(t *testing.T) { + t.Parallel() + scenarios := []testResponseWriteScenario[string]{ { name: "no explicit content-type", @@ -486,6 +587,8 @@ func TestEventXML(t *testing.T) { } func TestEventStream(t *testing.T) { + t.Parallel() + scenarios := []testResponseWriteScenario[string]{ { name: "stream", @@ -506,6 +609,8 @@ func TestEventStream(t *testing.T) { } func TestEventBlob(t *testing.T) { + t.Parallel() + scenarios := []testResponseWriteScenario[[]byte]{ { name: "blob", @@ -526,6 +631,8 @@ func TestEventBlob(t *testing.T) { } func TestEventNoContent(t *testing.T) { + t.Parallel() + s := testResponseWriteScenario[any]{ name: "no content", status: 234, @@ -542,6 +649,8 @@ func TestEventNoContent(t *testing.T) { } func TestEventFlush(t *testing.T) { + t.Parallel() + rec := httptest.NewRecorder() event := &router.Event{ @@ -556,6 +665,8 @@ func TestEventFlush(t *testing.T) { } func TestEventRedirect(t *testing.T) { + t.Parallel() + scenarios := []testResponseWriteScenario[any]{ { name: "non-30x status", @@ -580,6 +691,8 @@ func TestEventRedirect(t *testing.T) { } func TestEventFileFS(t *testing.T) { + t.Parallel() + // stub test files // --- dir, err := os.MkdirTemp("", "EventFileFS") @@ -676,6 +789,8 @@ func TestEventFileFS(t *testing.T) { } func TestEventError(t *testing.T) { + t.Parallel() + err := new(router.Event).Error(123, "message_test", map[string]any{"a": validation.Required, "b": "test"}) result, _ := json.Marshal(err, json.Deterministic(true)) @@ -687,6 +802,8 @@ func TestEventError(t *testing.T) { } func TestEventBadRequestError(t *testing.T) { + t.Parallel() + err := new(router.Event).BadRequestError("message_test", map[string]any{"a": validation.Required, "b": "test"}) result, _ := json.Marshal(err, json.Deterministic(true)) @@ -698,6 +815,8 @@ func TestEventBadRequestError(t *testing.T) { } func TestEventNotFoundError(t *testing.T) { + t.Parallel() + err := new(router.Event).NotFoundError("message_test", map[string]any{"a": validation.Required, "b": "test"}) result, _ := json.Marshal(err, json.Deterministic(true)) @@ -709,6 +828,8 @@ func TestEventNotFoundError(t *testing.T) { } func TestEventForbiddenError(t *testing.T) { + t.Parallel() + err := new(router.Event).ForbiddenError("message_test", map[string]any{"a": validation.Required, "b": "test"}) result, _ := json.Marshal(err, json.Deterministic(true)) @@ -720,6 +841,8 @@ func TestEventForbiddenError(t *testing.T) { } func TestEventUnauthorizedError(t *testing.T) { + t.Parallel() + err := new(router.Event).UnauthorizedError("message_test", map[string]any{"a": validation.Required, "b": "test"}) result, _ := json.Marshal(err, json.Deterministic(true)) @@ -731,6 +854,8 @@ func TestEventUnauthorizedError(t *testing.T) { } func TestEventTooManyRequestsError(t *testing.T) { + t.Parallel() + err := new(router.Event).TooManyRequestsError("message_test", map[string]any{"a": validation.Required, "b": "test"}) result, _ := json.Marshal(err, json.Deterministic(true)) @@ -742,6 +867,8 @@ func TestEventTooManyRequestsError(t *testing.T) { } func TestEventInternalServerError(t *testing.T) { + t.Parallel() + err := new(router.Event).InternalServerError("message_test", map[string]any{"a": validation.Required, "b": "test"}) result, _ := json.Marshal(err, json.Deterministic(true)) @@ -753,6 +880,8 @@ func TestEventInternalServerError(t *testing.T) { } func TestEventBindBody(t *testing.T) { + t.Parallel() + type testDstStruct struct { A int `json:"a" xml:"a" form:"a"` B int `json:"b" xml:"b" form:"b"`