diff --git a/cyclops/handlers.go b/cyclops/handlers.go index dcd8a71..970b275 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) @@ -327,10 +339,10 @@ func makeConditionalClause(cond, filter, tag, omitTag, sort, limit, offset strin if cond != "" { b.WriteString(" where ") - // XXX injection risk, but only by way of the 'cond' query parameter, - // 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. + // XXX injection risk, but only by way of 'cond', which every caller + // interpolates unchanged. A condition that arrived as 'jsonCond' has + // been built by ParseCond from a validated structure and is safe. The + // risk goes away when 'cond' is withdrawn. b.WriteString(cond) } @@ -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 { @@ -587,13 +609,42 @@ func (server *ModCyclopsServer) handleDropSet(w http.ResponseWriter, req *http.R // ----------------------------------------------------------------------------- +// RecordSelection is the set of records that an operation applies to: a +// condition, in either of the two forms, narrowed by a filter and by tags. +// Neither operation sorts or pages, so the sort and offset that the clause +// builders accept are always empty here. +type RecordSelection struct { + Cond string `json:"cond"` + JSONCond json.RawMessage `json:"jsonCond"` + Filter string `json:"filter"` + Tag string `json:"tag"` + OmitTag string `json:"omitTag"` +} + +// conditional renders the selection as the conditional part of a command. A +// limit of "*" omits the limit from the command altogether. +func (s *RecordSelection) conditional(limit string) (string, error) { + cond, err := resolveCond(s.Cond, s.JSONCond) + if err != nil { + return "", err + } + return makeConditionalClause(cond, s.Filter, s.Tag, s.OmitTag, "", limit, "") +} + +// selectFrom renders the selection as a "select * from " over the named +// set, which is how records to be added are identified. +func (s *RecordSelection) selectFrom(from, limit string) (string, error) { + cond, err := resolveCond(s.Cond, s.JSONCond) + if err != nil { + return "", err + } + return makeSelectClause("*", from, cond, s.Filter, s.Tag, s.OmitTag, "", limit, "") +} + type AddRecords struct { - From string `json:"from"` - Cond string `json:"cond"` - Filter string `json:"filter"` - Tag string `json:"tag"` - OmitTag string `json:"omitTag"` - Limit string `json:"limit"` + From string `json:"from"` + RecordSelection + Limit string `json:"limit"` } func (server *ModCyclopsServer) handleAddObjects(w http.ResponseWriter, req *http.Request, caption string) error { @@ -612,17 +663,7 @@ func (server *ModCyclopsServer) handleAddObjects(w http.ResponseWriter, req *htt if limit == "" { limit = "*" // Omit "limit" from the command when the request did not specify one } - clause, err := makeSelectClause( - "*", - params.From, - params.Cond, - params.Filter, - params.Tag, - params.OmitTag, - "", // Sort - limit, // "*" omits "limit" completely when none was requested - "", // Offset - ) + clause, err := params.selectFrom(params.From, limit) if err != nil { return fmt.Errorf("could not make select clause: %w", err) } @@ -640,13 +681,7 @@ func (server *ModCyclopsServer) handleAddObjects(w http.ResponseWriter, req *htt // ----------------------------------------------------------------------------- -type RemoveRecords struct { - Cond string `json:"cond"` - Filter string `json:"filter"` - Tag string `json:"tag"` - OmitTag string `json:"omitTag"` - Limit string `json:"limit"` -} +type RemoveRecords = RecordSelection func (server *ModCyclopsServer) handleRemoveObjects(w http.ResponseWriter, req *http.Request, caption string) error { setName, err := ident("set", chi.URLParam(req, "setName")) @@ -660,15 +695,8 @@ func (server *ModCyclopsServer) handleRemoveObjects(w http.ResponseWriter, req * return fmt.Errorf("%s: %w", caption, err) } - clause, err := makeConditionalClause( - params.Cond, - params.Filter, - params.Tag, - params.OmitTag, - "", // Sort - "*", // Special-case value to omit "limit" completely - "", // Offset - ) + // "*" omits the limit from the command completely: removal has no limit. + clause, err := params.conditional("*") if err != nil { return fmt.Errorf("could not make conditional clause: %w", err) } diff --git a/cyclops/handlers_test.go b/cyclops/handlers_test.go index b17ce6f..46973d7 100644 --- a/cyclops/handlers_test.go +++ b/cyclops/handlers_test.go @@ -187,6 +187,60 @@ func assertHTTPStatus(t *testing.T, err error, want int) { } } +// The status an *HTTPError carries must be the status the client is actually +// sent. Handlers wrap the errors they return, so this only holds if +// runWithErrorHandling unwraps rather than type-asserting; when it did not, +// every 400 below reached the client as a 500. +func TestClientSeesHTTPErrorStatus(t *testing.T) { + jsonCond := url.QueryEscape(`{"type":"xyzzy"}`) + cases := []struct { + name string + run func(*ModCyclopsServer, *httptest.ResponseRecorder) + want int + }{ + {"retrieve with a bad jsonCond", func(s *ModCyclopsServer, rr *httptest.ResponseRecorder) { + req := retrieveRequest("users", "fields=id&jsonCond="+jsonCond) + s.runWithErrorHandling(rr, req, s.handleRetrieve, "retrieve") + }, http.StatusBadRequest}, + + {"create filter with a bad jsonCond", func(s *ModCyclopsServer, rr *httptest.ResponseRecorder) { + req := jsonRequest(`{"name":"active","jsonCond":{"type":"xyzzy"}}`, nil) + s.runWithErrorHandling(rr, req, s.handleCreateFilter, "create filter") + }, http.StatusBadRequest}, + + {"add objects with a bad jsonCond", func(s *ModCyclopsServer, rr *httptest.ResponseRecorder) { + req := jsonRequest(`{"from":"src","jsonCond":{"type":"xyzzy"}}`, map[string]string{"setName": "dest"}) + s.runWithErrorHandling(rr, req, s.handleAddObjects, "add objects") + }, http.StatusBadRequest}, + + {"remove objects with a bad jsonCond", func(s *ModCyclopsServer, rr *httptest.ResponseRecorder) { + req := jsonRequest(`{"jsonCond":{"type":"xyzzy"}}`, map[string]string{"setName": "dest"}) + s.runWithErrorHandling(rr, req, s.handleRemoveObjects, "remove objects") + }, http.StatusBadRequest}, + + {"create filter with a bad template", func(s *ModCyclopsServer, rr *httptest.ResponseRecorder) { + req := jsonRequest(`{"name":"active","cond":"age>18","template":"x; drop filter y"}`, nil) + s.runWithErrorHandling(rr, req, s.handleCreateFilter, "create filter") + }, http.StatusBadRequest}, + + // An error that carries no status is still a server fault. + {"CCMS unreachable", func(s *ModCyclopsServer, rr *httptest.ResponseRecorder) { + req := retrieveRequest("users", "fields=id") + s.runWithErrorHandling(rr, req, s.handleRetrieve, "retrieve") + }, http.StatusInternalServerError}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + fake := &fakeCCMS{resp: okResponse(), err: errors.New("connection refused")} + server := newTestServer(fake) + rr := httptest.NewRecorder() + c.run(server, rr) + assertStatus(t, rr, c.want) + }) + } +} + // retrieveCommandFor runs handleRetrieve over the given query string and returns // the command that reached CCMS. func retrieveCommandFor(t *testing.T, rawQuery string) string { @@ -280,7 +334,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 +843,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. @@ -1051,6 +1215,91 @@ func TestHandleRemoveObjects(t *testing.T) { assertStatus(t, rr, http.StatusNoContent) } +// handlerUnderTest is one of the body-carrying handlers that accepts a +// condition, named so failures say which one. +type condHandler struct { + name string + call func(*ModCyclopsServer, *httptest.ResponseRecorder, *http.Request) error + params map[string]string + prefix string // the body members that precede the condition + wantFmt string // the whole command, with %s where the condition goes +} + +var condHandlers = []condHandler{ + { + name: "add objects", + call: func(s *ModCyclopsServer, rr *httptest.ResponseRecorder, req *http.Request) error { + return s.handleAddObjects(rr, req, "add objects") + }, + params: map[string]string{"setName": "dest"}, + prefix: `"from":"src",`, + wantFmt: "insert into dest select * from src where %s;", + }, + { + name: "remove objects", + call: func(s *ModCyclopsServer, rr *httptest.ResponseRecorder, req *http.Request) error { + return s.handleRemoveObjects(rr, req, "remove objects") + }, + params: map[string]string{"setName": "dest"}, + // The double space is as in TestHandleRemoveObjects above. + wantFmt: "delete from dest where %s;", + }, +} + +// Both handlers accept a structured condition in place of the CCMS one, and +// render it through the same code the other entry points use. +func TestHandleObjectsJSONCond(t *testing.T) { + cases := map[string]string{ + `{"type":"term","field":"age","rel":"gt","value":18}`: `age > 18`, + `{"type":"term","field":"author","rel":"eq","value":"Adams, John"}`: `author = 'Adams, John'`, + `{"type":"term","field":"title","rel":"contains","value":"'; drop set dest; --"}`: `title ilike '%''; drop set dest; --%'`, + } + for _, h := range condHandlers { + for jsonCond, wantCond := range cases { + t.Run(h.name+"/"+wantCond, func(t *testing.T) { + fake := &fakeCCMS{resp: okResponse()} + server := newTestServer(fake) + + body := `{` + h.prefix + `"jsonCond":` + jsonCond + `}` + rr := httptest.NewRecorder() + if err := h.call(server, rr, jsonRequest(body, h.params)); err != nil { + t.Fatalf("%s returned error: %v", h.name, err) + } + assertEqual(t, "command sent to CCMS", fake.lastCmd, fmt.Sprintf(h.wantFmt, wantCond)) + assertStatus(t, rr, http.StatusNoContent) + }) + } + } +} + +// And both reject the same client mistakes, without sending a command. +func TestHandleObjectsJSONCondErrors(t *testing.T) { + cases := map[string]string{ + `"cond":"age>18","jsonCond":{"type":"term","field":"age","rel":"gt","value":18}`: `only one of 'cond' and 'jsonCond' may be supplied`, + `"jsonCond":{"type":"xyzzy"}`: `unknown clause type "xyzzy"`, + `"jsonCond":{"type":"term","field":"age; drop set x","rel":"gt","value":18}`: `invalid field identifier`, + } + for _, h := range condHandlers { + for members, wantErr := range cases { + t.Run(h.name+"/"+wantErr, func(t *testing.T) { + fake := &fakeCCMS{resp: okResponse()} + server := newTestServer(fake) + + body := `{` + h.prefix + members + `}` + err := h.call(server, httptest.NewRecorder(), jsonRequest(body, h.params)) + if err == nil { + t.Fatalf("%s: expected an error for body %s", h.name, 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) + } + }) + } + } +} + func TestHandleDeleteProject(t *testing.T) { fake := &fakeCCMS{resp: okResponse()} server := newTestServer(fake) diff --git a/cyclops/server.go b/cyclops/server.go index 5e11994..a21d9a1 100644 --- a/cyclops/server.go +++ b/cyclops/server.go @@ -1,5 +1,6 @@ package cyclops +import "errors" import "fmt" import "net/http" import "time" @@ -181,12 +182,13 @@ func (server *ModCyclopsServer) runWithErrorHandling(w http.ResponseWriter, req } if err != nil { - var status int - switch e := err.(type) { - case *HTTPError: - status = e.status - default: - status = http.StatusInternalServerError + // Use errors.As rather than a type assertion: handlers wrap the + // errors they return for context, so the *HTTPError carrying the + // status is usually not the outermost error. + status := http.StatusInternalServerError + var httpErr *HTTPError + if errors.As(err, &httpErr) { + status = httpErr.status } w.WriteHeader(status) _, _ = fmt.Fprintln(w, err.Error()) 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"
    • +