From b07d635eb8a21e23c996dc6719f69aaf57733330 Mon Sep 17 00:00:00 2001 From: Mike Taylor Date: Tue, 1 Sep 2026 12:51:53 +0100 Subject: [PATCH 1/4] Filter creation also accepts JSON cond --- cyclops/handlers.go | 64 +++++++---- cyclops/handlers_test.go | 112 +++++++++++++++++++- htdocs/index.html | 1 + ramls/Makefile | 1 + ramls/cyclops.raml | 4 +- ramls/examples/filter-jsoncond-example.json | 21 ++++ ramls/filter-schema.json | 19 +++- 7 files changed, 196 insertions(+), 26 deletions(-) create mode 100644 ramls/examples/filter-jsoncond-example.json diff --git a/cyclops/handlers.go b/cyclops/handlers.go index dcd8a71..5c8ebb0 100644 --- a/cyclops/handlers.go +++ b/cyclops/handlers.go @@ -134,9 +134,10 @@ func (server *ModCyclopsServer) handleShowFilters(w http.ResponseWriter, req *ht // ----------------------------------------------------------------------------- type CreateFilter struct { - Name string `json:"name"` - Cond string `json:"cond"` - Template string `json:"template"` + Name string `json:"name"` + Cond string `json:"cond"` + JSONCond json.RawMessage `json:"jsonCond"` + Template string `json:"template"` } func (server *ModCyclopsServer) handleCreateFilter(w http.ResponseWriter, req *http.Request, caption string) error { @@ -151,16 +152,27 @@ func (server *ModCyclopsServer) handleCreateFilter(w http.ResponseWriter, req *h return fmt.Errorf("%s: %w", caption, err) } + cond, err := resolveCond(filter.Cond, filter.JSONCond) + if err != nil { + return err + } + command := "create filter " + name - if filter.Cond != "" { - // XXX injection risk: 'cond' is a free-form condition expression and is - // not sanitised; needs AST-based construction. - command += " where " + filter.Cond + if cond != "" { + // XXX injection risk, but only by way of the 'cond' member, which is + // interpolated unchanged. A condition arriving as 'jsonCond' has been + // built by ParseCond from a validated structure and is safe. The risk + // goes away when 'cond' is withdrawn. + command += " where " + cond } if filter.Template != "" { - // XXX injection risk: 'template' is a free-form expression and is not - // sanitised; needs AST-based construction. - command += " template " + filter.Template + // The template is the name of another filter, so the qualified + // "project.filter" form must survive: ident admits the joining '.'. + template, terr := ident("filter template", filter.Template) + if terr != nil { + return &HTTPError{status: http.StatusBadRequest, message: terr.Error()} + } + command += " template " + template } command += ";" server.Log("command", command) @@ -436,39 +448,49 @@ func getCondSchema() *CondSchema { return &CondSchema{AllowAnyField: true, AllowAnyFilter: true} } -// requestCond returns the WHERE condition for a retrieval, which the caller may -// supply either as 'cond', a condition already in CCMS's own language, or as +// resolveCond returns the WHERE condition to use, given the two forms a caller +// may supply it in: 'cond', a condition already in CCMS's own language, and // 'jsonCond', the structured form described by ramls/cond-schema.json. The two -// are alternatives: supplying both is an error, and supplying neither means the -// retrieval is unconditional, as it always has been. +// are alternatives, so supplying both is an error; supplying neither yields the +// empty condition, which every caller treats as "unconditional". // // 'cond' is interpolated into the command unchanged and is therefore an // injection risk; 'jsonCond' is validated and rendered by mod-cyclops itself. // The intention is to withdraw 'cond' once clients have moved over. -func requestCond(req *http.Request) (string, error) { - cond := req.URL.Query().Get("cond") - jsonCond := req.URL.Query().Get("jsonCond") +func resolveCond(cond string, jsonCond json.RawMessage) (string, error) { + // An absent member and an explicit null are alike: neither is a condition. + hasJSON := len(jsonCond) > 0 && string(jsonCond) != "null" - if cond != "" && jsonCond != "" { + if cond != "" && hasJSON { return "", &HTTPError{ status: http.StatusBadRequest, message: "only one of 'cond' and 'jsonCond' may be supplied", } } - if jsonCond == "" { + if !hasJSON { return cond, nil } - rendered, err := ParseCond([]byte(jsonCond), getCondSchema()) + rendered, err := ParseCond(jsonCond, getCondSchema()) if err != nil { return "", &HTTPError{ status: http.StatusBadRequest, - message: fmt.Sprintf("invalid 'jsonCond' parameter: %s", err), + message: fmt.Sprintf("invalid 'jsonCond': %s", err), } } return rendered, nil } +// requestCond resolves the condition for a retrieval, whose caller supplies it +// in query parameters. 'jsonCond' is then the JSON text of a condition, since a +// query parameter cannot itself be structured. +func requestCond(req *http.Request) (string, error) { + return resolveCond( + req.URL.Query().Get("cond"), + json.RawMessage(req.URL.Query().Get("jsonCond")), + ) +} + func makeRetrieveCommand(req *http.Request, countOnly bool) (string, error) { selectFields := req.URL.Query().Get("fields") if countOnly { diff --git a/cyclops/handlers_test.go b/cyclops/handlers_test.go index b17ce6f..70fbf70 100644 --- a/cyclops/handlers_test.go +++ b/cyclops/handlers_test.go @@ -280,7 +280,7 @@ func TestHandleRetrieveJSONCondInvalid(t *testing.T) { t.Fatalf("expected an error for jsonCond=%s", jsonCond) } assertHTTPStatus(t, err, http.StatusBadRequest) - assertErrContains(t, err, "invalid 'jsonCond' parameter") + assertErrContains(t, err, "invalid 'jsonCond'") assertErrContains(t, err, wantErr) if fake.lastCmd != "" { t.Errorf("a command was sent despite the bad request: %q", fake.lastCmd) @@ -789,6 +789,116 @@ func TestHandleCreateFilterNameOnly(t *testing.T) { assertStatus(t, rr, http.StatusNoContent) } +// A filter's condition may be supplied structurally instead. Because the body +// is already JSON, 'jsonCond' is a nested object rather than a string of JSON. +func TestHandleCreateFilterJSONCond(t *testing.T) { + cases := map[string]string{ + `{"type":"term","field":"age","rel":"gt","value":18}`: `create filter active where age > 18;`, + `{"type":"and","clauses":[{"type":"term","field":"age","rel":"ge","value":143100000},` + + `{"type":"term","field":"age","rel":"le","value":201400000}]}`: `create filter active where (age >= 143100000 and age <= 201400000);`, + `{"type":"term","field":"title","rel":"contains","value":"'; drop filter x; --"}`: `create filter active where title ilike '%''; drop filter x; --%';`, + } + for jsonCond, want := range cases { + t.Run(want, func(t *testing.T) { + fake := &fakeCCMS{resp: okResponse()} + server := newTestServer(fake) + + body := `{"name":"active","jsonCond":` + jsonCond + `}` + rr := httptest.NewRecorder() + err := server.handleCreateFilter(rr, jsonRequest(body, nil), "create filter") + if err != nil { + t.Fatalf("handleCreateFilter returned error: %v", err) + } + assertEqual(t, "command sent to CCMS", fake.lastCmd, want) + assertStatus(t, rr, http.StatusNoContent) + }) + } +} + +// As for retrieval, the two forms are alternatives, and a bad structure is the +// client's mistake rather than a server fault. +func TestHandleCreateFilterJSONCondErrors(t *testing.T) { + cases := map[string]string{ + `{"name":"active","cond":"age>18","jsonCond":{"type":"term","field":"age","rel":"gt","value":18}}`: `only one of 'cond' and 'jsonCond' may be supplied`, + `{"name":"active","jsonCond":{"type":"xyzzy"}}`: `unknown clause type "xyzzy"`, + `{"name":"active","jsonCond":{"type":"term","field":"age; drop filter x","rel":"gt","value":18}}`: `invalid field identifier`, + } + for body, wantErr := range cases { + t.Run(wantErr, func(t *testing.T) { + fake := &fakeCCMS{resp: okResponse()} + server := newTestServer(fake) + + err := server.handleCreateFilter(httptest.NewRecorder(), jsonRequest(body, nil), "create filter") + if err == nil { + t.Fatalf("expected an error for body %s", body) + } + assertHTTPStatus(t, err, http.StatusBadRequest) + assertErrContains(t, err, wantErr) + if fake.lastCmd != "" { + t.Errorf("a command was sent despite the bad request: %q", fake.lastCmd) + } + }) + } +} + +// The template names another filter, so it must be an identifier and cannot be +// used to append anything else to the command. +func TestHandleCreateFilterTemplate(t *testing.T) { + t.Run("qualified name survives intact", func(t *testing.T) { + fake := &fakeCCMS{resp: okResponse()} + server := newTestServer(fake) + + body := `{"name":"korea_lit.jurassic","cond":"age>18","template":"korea_lit.mesozoic"}` + err := server.handleCreateFilter(httptest.NewRecorder(), jsonRequest(body, nil), "create filter") + if err != nil { + t.Fatalf("handleCreateFilter returned error: %v", err) + } + assertEqual(t, "command sent to CCMS", fake.lastCmd, + "create filter korea_lit.jurassic where age>18 template korea_lit.mesozoic;") + }) + + for _, template := range []string{ + "mesozoic; drop filter x", + "mesozoic where 1=1", + "mesozoic'", + "1mesozoic", + "meso zoic", + } { + t.Run(template, func(t *testing.T) { + fake := &fakeCCMS{resp: okResponse()} + server := newTestServer(fake) + + body, mErr := json.Marshal(CreateFilter{Name: "active", Cond: "age>18", Template: template}) + if mErr != nil { + t.Fatal(mErr) + } + err := server.handleCreateFilter(httptest.NewRecorder(), jsonRequest(string(body), nil), "create filter") + if err == nil { + t.Fatalf("template %q should have been rejected", template) + } + assertHTTPStatus(t, err, http.StatusBadRequest) + assertErrContains(t, err, fmt.Sprintf("invalid filter template identifier: %q", template)) + if fake.lastCmd != "" { + t.Errorf("a command was sent despite the bad request: %q", fake.lastCmd) + } + }) + } +} + +// An explicit null is not a condition, and must not be mistaken for one. +func TestHandleCreateFilterNullJSONCond(t *testing.T) { + fake := &fakeCCMS{resp: okResponse()} + server := newTestServer(fake) + + rr := httptest.NewRecorder() + body := `{"name":"active","cond":"age>18","jsonCond":null}` + err := server.handleCreateFilter(rr, jsonRequest(body, nil), "create filter") + if err != nil { + t.Fatalf("handleCreateFilter returned error: %v", err) + } + assertEqual(t, "command sent to CCMS", fake.lastCmd, "create filter active where age>18;") +} + func TestHandleDeleteFilter(t *testing.T) { // The identifier is the project-qualified name the filter was created // under, so the '.' must survive validation and reach the command intact. diff --git a/htdocs/index.html b/htdocs/index.html index d98123f..b81f20e 100644 --- a/htdocs/index.html +++ b/htdocs/index.html @@ -72,6 +72,7 @@
  • list of filters in project "literature of North Korea"
    • +