diff --git a/cyclops/cond.go b/cyclops/cond.go new file mode 100644 index 0000000..e475de3 --- /dev/null +++ b/cyclops/cond.go @@ -0,0 +1,585 @@ +package cyclops + +// Decoding of structured search conditions. +// +// A client sends a condition as the JSON tree described by ramls/cond-schema.json +// rather than as a string of CCMS command language. The client therefore never +// authors CCMS syntax: it names a field, an abstract relation and a value, and +// the code here decides how that is spelled. Every identifier is checked against +// the fields the caller declares queryable, and every value is rendered through +// the sanitisation helpers in handlers.go, so a hostile value can only ever end +// up inside a correctly quoted literal. +// +// +// The translation to a CCMS condition is in two stages. DecodeCond turns bytes +// into a tree, checking only what is intrinsic to the format; RenderCond checks +// the tree against a caller-supplied schema and produces the CCMS condition. +// Keeping them apart means the tree can be inspected, logged or rewritten in +// between, and that rendering is generative: no part of the client's input is +// ever copied into the output except as a quoted literal or an allow-listed name. + +import "bytes" +import "encoding/json" +import "fmt" +import "regexp" +import "strings" + +// Limits on the size of a condition tree. +const maxCondDepth = 20 +const maxCondNodes = 200 +const maxCondListValues = 100 + +// FieldKind is the type of a queryable field, used to check that a value is +// of a form the field can meaningfully be compared against. +// "Kind" rather than "Type" because nodes have a type. +// (How does Go not have enums in 2026?) +type FieldKind int + +const ( + FieldString FieldKind = iota + FieldNumber + FieldBoolean + FieldDate + // FieldAny is the kind of a field whose type is not known, which is the + // case for every field when CondSchema.AllowAnyField is set. A value of + // any scalar type may be compared against it. + FieldAny +) + +// String names the kind for use in error messages. An unrecognised kind is +// rendered in the conventional stringer form rather than as a plausible name, +// so that a bad value cannot pass for a real one. +func (k FieldKind) String() string { + switch k { + case FieldString: + return "string" + case FieldNumber: + return "numeric" + case FieldBoolean: + return "boolean" + case FieldDate: + return "date" + case FieldAny: + return "any" + default: + return fmt.Sprintf("FieldKind(%d)", int(k)) + } +} + +// CondSchema declares what a condition is allowed to mention. It is the +// authorisation boundary: a field absent from Fields cannot be queried at all, +// which keeps a syntactically valid condition from reading data the client has +// no business seeing. +type CondSchema struct { + // Fields maps queryable field names to their types. + Fields map[string]FieldKind + + // Filters is the set of filter names that may be referenced. + Filters map[string]bool + + // AllowAnyField and AllowAnyFilter relax the two allow-lists to "any + // syntactically valid identifier". They exist for callers that do not + // yet have a catalogue of fields to hand, and weaken the guarantee + // above to injection-safety alone. + AllowAnyField bool + AllowAnyFilter bool +} + +// Clause is one node of a condition tree: a Junction, a Negation, a Term or a +// FilterRef. Those types are all implementations of this interface: see their +// definitions of the render() function below. +type Clause interface { + render(s *CondSchema, b *strings.Builder) error +} + +// Junction is a conjunction or disjunction of subordinate clauses. +type Junction struct { + Op string // "and" or "or" + Clauses []Clause +} + +// Negation is the negation of a single subordinate clause. +type Negation struct { + Clause Clause +} + +// Term compares a single field against a value. Rel is an abstract relation +// name, not a CCMS operator: the mapping to CCMS is made during rendering. +type Term struct { + Field string + Rel string + Value any // string, json.Number, bool, []any of those, or nil +} + +// FilterRef refers to a named filter by name. +type FilterRef struct { + Name string +} + +// The operators that combine subordinate clauses, and the CCMS keyword each +// becomes. +var junctionOps = map[string]string{ + "and": "and", + "or": "or", +} + +// Relations that compare a field against a single scalar, and the CCMS +// operator each becomes. +var scalarRels = map[string]string{ + "eq": "=", + "ne": "<>", + "lt": "<", + "le": "<=", + "gt": ">", + "ge": ">=", +} + +// Relations that match a substring of a string field, and the printf-like pattern +// each wraps the value in. The value itself is escaped by likePattern first. +var patternRels = map[string]string{ + "contains": "%%%s%%", + "startsWith": "%s%%", + "endsWith": "%%%s", +} + +// Relations that compare a field against a list of scalars. +var listRels = map[string]string{ + "in": "in", + "notIn": "not in", +} + +// Relations that take no value at all. +var nullRels = map[string]string{ + "isNull": "is null", + "isNotNull": "is not null", +} + +// ParseCond decodes a JSON condition and renders it as a CCMS condition in a +// single step. It is the form callers normally want. +func ParseCond(data []byte, s *CondSchema) (string, error) { + c, err := DecodeCond(data) + if err != nil { + return "", err + } + return RenderCond(c, s) +} + +// DecodeCond decodes a condition tree from JSON, checking its shape but not +// yet the names it mentions. +func DecodeCond(data []byte) (Clause, error) { + d := &condDecoder{} + return d.clause(data, "condition", 0) +} + +// RenderCond checks a decoded condition against a schema and renders it as a +// CCMS condition. +func RenderCond(c Clause, s *CondSchema) (string, error) { + if c == nil { + return "", fmt.Errorf("condition is empty") + } + if s == nil { + return "", fmt.Errorf("no condition schema supplied") + } + var b strings.Builder + err := c.render(s, &b) + if err != nil { + return "", err + } + return b.String(), nil +} + +// ----------------------------------------------------------------------------- +// Decoding + +type condDecoder struct { + nodes int +} + +// The four node shapes, each decoded strictly so that a property belonging to +// another shape -- "clauses" on a "not", say -- is an error rather than being +// quietly dropped. +type rawJunction struct { + Type string `json:"type"` + Clauses []json.RawMessage `json:"clauses"` +} + +type rawNegation struct { + Type string `json:"type"` + Clause json.RawMessage `json:"clause"` +} + +type rawTerm struct { + Type string `json:"type"` + Field string `json:"field"` + Rel string `json:"rel"` + Value json.RawMessage `json:"value"` +} + +type rawFilter struct { + Type string `json:"type"` + Name string `json:"name"` +} + +// clause decodes one node, dispatching on its "type" discriminator. path names +// the node's position in the tree so that errors can point at it. +func (d *condDecoder) clause(data []byte, path string, depth int) (Clause, error) { + if depth > maxCondDepth { + return nil, fmt.Errorf("%s: condition nested more than %d deep", path, maxCondDepth) + } + d.nodes++ + if d.nodes > maxCondNodes { + return nil, fmt.Errorf("condition has more than %d clauses", maxCondNodes) + } + + var disc struct { + Type string `json:"type"` + } + err := json.Unmarshal(data, &disc) + if err != nil { + return nil, fmt.Errorf("%s: not a condition clause: %v", path, err) + } + + switch { + case junctionOps[disc.Type] != "": + return d.junction(data, path, depth) + case disc.Type == "not": + return d.negation(data, path, depth) + case disc.Type == "term": + return d.term(data, path) + case disc.Type == "filter": + return d.filter(data, path) + case disc.Type == "": + return nil, fmt.Errorf(`%s: clause has no "type"`, path) + default: + return nil, fmt.Errorf("%s: unknown clause type %q", path, disc.Type) + } +} + +func (d *condDecoder) junction(data []byte, path string, depth int) (Clause, error) { + var raw rawJunction + err := strictUnmarshal(data, &raw) + if err != nil { + return nil, fmt.Errorf("%s: %v", path, err) + } + if len(raw.Clauses) == 0 { + return nil, fmt.Errorf(`%s: %q clause has no subordinate clauses`, path, raw.Type) + } + j := &Junction{Op: raw.Type, Clauses: make([]Clause, len(raw.Clauses))} + for i, sub := range raw.Clauses { + c, err := d.clause(sub, fmt.Sprintf("%s.clauses[%d]", path, i), depth+1) + if err != nil { + return nil, err + } + j.Clauses[i] = c + } + return j, nil +} + +func (d *condDecoder) negation(data []byte, path string, depth int) (Clause, error) { + var raw rawNegation + err := strictUnmarshal(data, &raw) + if err != nil { + return nil, fmt.Errorf("%s: %v", path, err) + } + if len(raw.Clause) == 0 { + return nil, fmt.Errorf(`%s: "not" clause has no subordinate clause`, path) + } + c, err := d.clause(raw.Clause, path+".clause", depth+1) + if err != nil { + return nil, err + } + return &Negation{Clause: c}, nil +} + +func (d *condDecoder) term(data []byte, path string) (Clause, error) { + var raw rawTerm + err := strictUnmarshal(data, &raw) + if err != nil { + return nil, fmt.Errorf("%s: %v", path, err) + } + if raw.Field == "" { + return nil, fmt.Errorf(`%s: term has no "field"`, path) + } + if raw.Rel == "" { + return nil, fmt.Errorf(`%s: term has no "rel"`, path) + } + + t := &Term{Field: raw.Field, Rel: raw.Rel} + + switch { + case nullRels[raw.Rel] != "": + if len(raw.Value) > 0 { + return nil, fmt.Errorf("%s: relation %q takes no value", path, raw.Rel) + } + return t, nil + + case listRels[raw.Rel] != "": + var list []any + err = decodeValue(raw.Value, &list, path) + if err != nil { + return nil, fmt.Errorf("%s: relation %q needs a list of values: %v", path, raw.Rel, err) + } + if len(list) == 0 { + return nil, fmt.Errorf("%s: relation %q needs a non-empty list of values", path, raw.Rel) + } + if len(list) > maxCondListValues { + return nil, fmt.Errorf("%s: relation %q has more than %d values", path, raw.Rel, maxCondListValues) + } + for i, v := range list { + if !isScalar(v) { + return nil, fmt.Errorf("%s.value[%d]: not a string, number or boolean", path, i) + } + } + t.Value = list + return t, nil + + case scalarRels[raw.Rel] != "" || patternRels[raw.Rel] != "": + var v any + err = decodeValue(raw.Value, &v, path) + if err != nil { + return nil, fmt.Errorf("%s: relation %q needs a value: %v", path, raw.Rel, err) + } + if !isScalar(v) { + return nil, fmt.Errorf("%s.value: not a string, number or boolean", path) + } + t.Value = v + return t, nil + + default: + return nil, fmt.Errorf("%s: unknown relation %q", path, raw.Rel) + } +} + +func (d *condDecoder) filter(data []byte, path string) (Clause, error) { + var raw rawFilter + err := strictUnmarshal(data, &raw) + if err != nil { + return nil, fmt.Errorf("%s: %v", path, err) + } + if raw.Name == "" { + return nil, fmt.Errorf(`%s: filter reference has no "name"`, path) + } + return &FilterRef{Name: raw.Name}, nil +} + +// strictUnmarshal decodes exactly one JSON value into v, rejecting properties v +// does not declare, and preserving numbers as json.Number so that a value's +// precision survives the round trip. +func strictUnmarshal(data []byte, v any) error { + dec := json.NewDecoder(bytes.NewReader(data)) + dec.DisallowUnknownFields() + dec.UseNumber() + err := dec.Decode(v) + if err != nil { + return err + } + if dec.More() { + return fmt.Errorf("unexpected trailing data after clause") + } + return nil +} + +// decodeValue decodes a term's value, treating an absent value and an explicit +// null alike: neither is a value, and a null field is expressed with the +// "isNull" relation instead. +func decodeValue(data json.RawMessage, v any, path string) error { + if len(data) == 0 { + return fmt.Errorf("no value given") + } + if string(data) == "null" { + return fmt.Errorf(`value is null (use the "isNull" relation)`) + } + return strictUnmarshal(data, v) +} + +func isScalar(v any) bool { + switch v.(type) { + case string, json.Number, bool: + return true + } + return false +} + +// ----------------------------------------------------------------------------- +// Rendering + +func (j *Junction) render(s *CondSchema, b *strings.Builder) error { + op := junctionOps[j.Op] + if op == "" { + return fmt.Errorf("unknown junction operator %q", j.Op) + } + b.WriteByte('(') + for i, c := range j.Clauses { + if i > 0 { + b.WriteString(" " + op + " ") + } + err := c.render(s, b) + if err != nil { + return err + } + } + b.WriteByte(')') + return nil +} + +func (n *Negation) render(s *CondSchema, b *strings.Builder) error { + b.WriteString("not ") + _, isJunction := n.Clause.(*Junction) + if !isJunction { + b.WriteByte('(') + defer b.WriteByte(')') + } + return n.Clause.render(s, b) +} + +func (f *FilterRef) render(s *CondSchema, b *strings.Builder) error { + if !s.AllowAnyFilter && !s.Filters[f.Name] { + return fmt.Errorf("unknown filter: %q", f.Name) + } + name, err := ident("filter", f.Name) + if err != nil { + return err + } + b.WriteString("filter(" + name + ")") + return nil +} + +func (t *Term) render(s *CondSchema, b *strings.Builder) error { + kind, err := t.fieldKind(s) + if err != nil { + return err + } + field, err := ident("field", t.Field) + if err != nil { + return err + } + + if op := nullRels[t.Rel]; op != "" { + b.WriteString(field + " " + op) + return nil + } + + if pattern := patternRels[t.Rel]; pattern != "" { + return t.renderPattern(pattern, field, kind, b) + } + + if op := listRels[t.Rel]; op != "" { + return t.renderList(op, field, kind, b) + } + + op := scalarRels[t.Rel] + if op == "" { + return fmt.Errorf("unknown relation %q", t.Rel) + } + lit, err := renderLiteral(t.Value, kind, t.Field) + if err != nil { + return err + } + b.WriteString(field + " " + op + " " + lit) + return nil +} + +// renderPattern renders a substring match. The value is escaped as a LIKE +// pattern, wrapped in whichever wildcards the relation calls for, and quoted. +func (t *Term) renderPattern(pattern, field string, kind FieldKind, b *strings.Builder) error { + if kind != FieldString && kind != FieldAny { + return fmt.Errorf("field %q cannot be matched with relation %q", t.Field, t.Rel) + } + str, ok := t.Value.(string) + if !ok { + return fmt.Errorf("relation %q needs a string value", t.Rel) + } + lit, err := sqlString(fmt.Sprintf(pattern, likePattern(str))) + if err != nil { + return err + } + b.WriteString(field + " ilike " + lit) + return nil +} + +// renderList renders a membership test against a parenthesised list of literals. +func (t *Term) renderList(op, field string, kind FieldKind, b *strings.Builder) error { + list, ok := t.Value.([]any) + if !ok { + return fmt.Errorf("relation %q needs a list of values", t.Rel) + } + lits := make([]string, len(list)) + for i, v := range list { + lit, err := renderLiteral(v, kind, t.Field) + if err != nil { + return err + } + lits[i] = lit + } + b.WriteString(field + " " + op + " (" + strings.Join(lits, ", ") + ")") + return nil +} + +// fieldKind resolves the declared type of the term's field, which is also the +// check that the field may be queried at all. An undeclared field is admitted +// only when AllowAnyField is set, and is then of unknown type, so that any +// scalar may be compared against it. +func (t *Term) fieldKind(s *CondSchema) (FieldKind, error) { + kind, ok := s.Fields[t.Field] + if ok { + return kind, nil + } + if !s.AllowAnyField { + return FieldAny, fmt.Errorf("field is not queryable: %q", t.Field) + } + return FieldAny, nil +} + +// renderLiteral renders a scalar as a CCMS literal of the kind the field expects. +func renderLiteral(v any, kind FieldKind, field string) (string, error) { + switch val := v.(type) { + case string: + if kind != FieldString && kind != FieldDate && kind != FieldAny { + return "", fmt.Errorf("field %q needs a %s value, not a string", field, kind) + } + return sqlString(val) + case json.Number: + if kind != FieldNumber && kind != FieldAny { + return "", fmt.Errorf("field %q needs a %s value, not a number", field, kind) + } + return renderNumber(val) + case bool: + if kind != FieldBoolean && kind != FieldAny { + return "", fmt.Errorf("field %q needs a %s value, not a boolean", field, kind) + } + if val { + return "true", nil + } + return "false", nil + default: + return "", fmt.Errorf("field %q: unsupported value type %T", field, v) + } +} + +// decimalRe matches a plain decimal number, the only non-integer numeric form +// with a counterpart in the CCMS grammar. +var decimalRe = regexp.MustCompile(`^-?[0-9]+\.[0-9]+$`) + +// renderNumber renders a JSON number as a CCMS numeric literal. Integers are +// checked by the same rule as every other integer in a command; anything else +// is rendered only if it is a plain decimal, since exponent notation has no +// counterpart in the grammar. +func renderNumber(n json.Number) (string, error) { + s := n.String() + v, err := intval(s) + if err == nil { + return v, nil + } + if decimalRe.MatchString(s) { + return s, nil + } + return "", fmt.Errorf("invalid number: %q", s) +} + +// likePattern escapes the wildcards of a LIKE pattern, so that a value +// containing '%' or '_' matches those characters literally rather than +// silently becoming a wildcard search. +func likePattern(s string) string { + r := strings.NewReplacer(`\`, `\\`, `%`, `\%`, `_`, `\_`) + return r.Replace(s) +} diff --git a/cyclops/cond_test.go b/cyclops/cond_test.go new file mode 100644 index 0000000..683965a --- /dev/null +++ b/cyclops/cond_test.go @@ -0,0 +1,296 @@ +package cyclops + +import "fmt" +import "maps" +import "os" +import "path/filepath" +import "slices" +import "strings" +import "testing" + +// corpusSchema declares the fields and filters used by the test corpus in ramls/examples/condtest. +func corpusSchema() *CondSchema { + return &CondSchema{ + Fields: map[string]FieldKind{ + "title": FieldString, + "author": FieldString, + "note": FieldString, + "availability": FieldString, + "location": FieldString, + "acquired": FieldDate, + "withdrawn_date": FieldDate, + "holdings_count": FieldNumber, + "decision": FieldBoolean, + }, + Filters: map[string]bool{ + "target": true, + "reviewed": true, + }, + } +} + +// Every file in ramls/examples/condtest/valid must render, and the RAML example with it. +func TestCondCorpusValid(t *testing.T) { + files, err := filepath.Glob("../ramls/examples/condtest/valid/*.json") + if err != nil { + t.Fatal(err) + } + files = append(files, "../ramls/examples/cond.json") + if len(files) < 2 { + t.Fatalf("found %d valid test cases; the corpus has gone missing", len(files)) + } + for _, f := range files { + t.Run(filepath.Base(f), func(t *testing.T) { + _, err := ParseCond(readCase(t, f), corpusSchema()) + if err != nil { + t.Fatalf("wrongly rejected: %v", err) + } + // t.Logf("=> %s", cond) + }) + } +} + +// Every file in ramls/examples/condtest/invalid must be rejected, and rejected for the +// reason it was written to probe: a case that starts failing for some unrelated +// reason has quietly stopped testing anything. The expected text need only be a +// distinctive fragment of the error. +var invalidCases = map[string]string{ + "bad-field-name.json": `field is not queryable: "a; drop table root"`, + "ccms-operator-as-rel.json": `unknown relation ">="`, + "empty-field-name.json": `term has no "field"`, + "empty-junction.json": `"and" clause has no subordinate clauses`, + "extra-property.json": `unknown field "cond"`, + "filter-empty-name.json": `filter reference has no "name"`, + "filter-missing-name.json": `filter reference has no "name"`, + "in-given-empty-list.json": `relation "in" needs a non-empty list of values`, + "in-given-nested-list.json": `value[0]: not a string, number or boolean`, + "in-given-scalar.json": `relation "in" needs a list of values`, + "junction-missing-clauses.json": `"and" clause has no subordinate clauses`, + "missing-discriminator.json": `clause has no "type"`, + "missing-value.json": `relation "eq" needs a value`, + "nested-bad-clause.json": `condition.clauses[0]: unknown relation "nope"`, + "nested-injection-as-field.json": `field is not queryable: "1=1 or a"`, + "not-an-object.json": `not a condition clause`, + "not-missing-clause.json": `"not" clause has no subordinate clause`, + "not-with-clauses.json": `unknown field "clauses"`, + "null-value.json": `value is null (use the "isNull" relation)`, + "scalar-rel-given-array.json": `value: not a string, number or boolean`, + "term-missing-field.json": `term has no "field"`, + "unknown-rel.json": `unknown relation "ilike"`, + "unknown-type.json": `unknown clause type "xyzzy"`, + "value-with-is-null.json": `relation "isNull" takes no value`, +} + +func TestCondCorpusInvalid(t *testing.T) { + files, err := filepath.Glob("../ramls/examples/condtest/invalid/*.json") + if err != nil { + t.Fatal(err) + } + // The corpus and the expectations must name exactly the same cases, so that + // neither a new fixture nor a deleted one can slip through unnoticed. + found := make(map[string]bool, len(files)) + for _, f := range files { + name := filepath.Base(f) + found[name] = true + if _, ok := invalidCases[name]; !ok { + t.Errorf("%s: test case has no expected error in invalidCases", name) + } + } + for _, name := range slices.Sorted(maps.Keys(invalidCases)) { + if !found[name] { + t.Errorf("%s: expected error in invalidCases, but no such test case", name) + } + } + for _, f := range files { + name := filepath.Base(f) + t.Run(name, func(t *testing.T) { + want, ok := invalidCases[name] + if !ok { + t.Fatalf("no expected error recorded for this case") + } + cond, err := ParseCond(readCase(t, f), corpusSchema()) + if err == nil { + t.Fatalf("wrongly accepted, rendering as: %s", cond) + } + if !strings.Contains(err.Error(), want) { + t.Fatalf("rejected for the wrong reason:\n got %v\n want it to contain %q", err, want) + } + // t.Logf("rejected: %v", err) + }) + } +} + +// A Junction is exported with exported fields, so a tree can be built in Go +// rather than decoded from JSON. Rendering must therefore validate the operator +// itself: it may not simply write whatever Op holds into the command. Only the +// operators in junctionOps are admissible, and what is written is the keyword +// the table maps to, never the caller's string. +func TestJunctionOperatorNotInjectable(t *testing.T) { + build := func(op string) *Junction { + return &Junction{ + Op: op, + Clauses: []Clause{ + &FilterRef{Name: "target"}, + &FilterRef{Name: "reviewed"}, + }, + } + } + + for op, want := range map[string]string{ + "and": "(filter(target) and filter(reviewed))", + "or": "(filter(target) or filter(reviewed))", + } { + got, err := RenderCond(build(op), corpusSchema()) + if err != nil { + t.Errorf("Op=%q unexpectedly rejected: %v", op, err) + } else if got != want { + t.Errorf("Op=%q rendered as %q, want %q", op, got, want) + } + } + + // Each of these would break out of the operator position, or is simply not + // an operator, and must be refused rather than written into the command. + for _, op := range []string{ + "", + "; drop set root; --", + "and 1=1 and", + "AND", + "not", + ")", + } { + got, err := RenderCond(build(op), corpusSchema()) + if err == nil { + t.Errorf("Op=%q should have been rejected, rendered as %q", op, got) + continue + } + if want := fmt.Sprintf("unknown junction operator %q", op); !strings.Contains(err.Error(), want) { + t.Errorf("Op=%q error = %q, want it to contain %q", op, err.Error(), want) + } + if got != "" { + t.Errorf("Op=%q returned %q alongside its error; want no condition at all", op, got) + } + } +} + +// The three limits on the size of a condition tree. None of these can be +// expressed as a corpus fixture without checking in a very large file, so they +// are exercised by building the JSON here. +func TestCondLimits(t *testing.T) { + term := `{"type":"term","field":"title","rel":"eq","value":"x"}` + + nest := func(n int) string { + doc := term + for range n { + doc = `{"type":"not","clause":` + doc + `}` + } + return doc + } + widen := func(n int) string { + clauses := make([]string, n) + for i := range clauses { + clauses[i] = term + } + return `{"type":"and","clauses":[` + strings.Join(clauses, ",") + `]}` + } + list := func(n int) string { + values := make([]string, n) + for i := range values { + values[i] = `"x"` + } + return `{"type":"term","field":"title","rel":"in","value":[` + strings.Join(values, ",") + `]}` + } + + cases := []struct { + name string + doc string + want string // fragment of the expected error, or "" if it must be accepted + }{ + {"depth within limit", nest(maxCondDepth - 1), ""}, + {"depth over limit", nest(maxCondDepth + 1), fmt.Sprintf("nested more than %d deep", maxCondDepth)}, + {"nodes within limit", widen(maxCondNodes - 1), ""}, + {"nodes over limit", widen(maxCondNodes + 1), fmt.Sprintf("more than %d clauses", maxCondNodes)}, + {"list within limit", list(maxCondListValues), ""}, + {"list over limit", list(maxCondListValues + 1), fmt.Sprintf(`relation "in" has more than %d values`, maxCondListValues)}, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + _, err := ParseCond([]byte(c.doc), corpusSchema()) + switch { + case c.want == "" && err != nil: + t.Errorf("unexpectedly rejected: %v", err) + case c.want != "" && err == nil: + t.Errorf("unexpectedly accepted") + case c.want != "" && !strings.Contains(err.Error(), c.want): + t.Errorf("error = %q, want it to contain %q", err.Error(), c.want) + } + }) + } +} + +// With AllowAnyField set, a field that the schema does not declare is admitted +// and its type is unknown, so any scalar may be compared against it. Fields the +// schema does declare keep their types, and are still checked. +func TestCondAllowAnyField(t *testing.T) { + permissive := &CondSchema{ + Fields: map[string]FieldKind{"holdings_count": FieldNumber}, + AllowAnyField: true, + AllowAnyFilter: true, + } + + accepted := map[string]string{ + `{"type":"term","field":"undeclared","rel":"eq","value":"x"}`: `undeclared = 'x'`, + `{"type":"term","field":"undeclared","rel":"ge","value":3}`: `undeclared >= 3`, + `{"type":"term","field":"undeclared","rel":"eq","value":true}`: `undeclared = true`, + `{"type":"term","field":"undeclared","rel":"contains","value":"x"}`: `undeclared ilike '%x%'`, + `{"type":"filter","name":"undeclared"}`: `filter(undeclared)`, + // A declared field keeps the type it was declared with. + `{"type":"term","field":"holdings_count","rel":"ge","value":3}`: `holdings_count >= 3`, + } + for doc, want := range accepted { + got, err := ParseCond([]byte(doc), permissive) + if err != nil { + t.Errorf("%s: unexpectedly rejected: %v", doc, err) + } else if got != want { + t.Errorf("%s:\n got %q\nwant %q", doc, got, want) + } + } + + // Being permissive about names is not being permissive about syntax: a + // field name that is not an identifier is still refused. + rejected := map[string]string{ + `{"type":"term","field":"a; drop set x","rel":"eq","value":"x"}`: `invalid field identifier`, + `{"type":"term","field":"holdings_count","rel":"eq","value":"three"}`: `needs a numeric value`, + `{"type":"term","field":"holdings_count","rel":"contains","value":"x"}`: `cannot be matched with relation`, + } + for doc, want := range rejected { + _, err := ParseCond([]byte(doc), permissive) + if err == nil { + t.Errorf("%s: unexpectedly accepted", doc) + } else if !strings.Contains(err.Error(), want) { + t.Errorf("%s: error = %q, want it to contain %q", doc, err, want) + } + } + + // Without the flag, an undeclared field is not queryable at all. + strict := &CondSchema{Fields: map[string]FieldKind{"holdings_count": FieldNumber}} + _, err := ParseCond([]byte(`{"type":"term","field":"undeclared","rel":"eq","value":"x"}`), strict) + if err == nil { + t.Error("an undeclared field was accepted without AllowAnyField") + } else if want := `field is not queryable: "undeclared"`; !strings.Contains(err.Error(), want) { + t.Errorf("error = %q, want it to contain %q", err, want) + } + + if got, want := FieldAny.String(), "any"; got != want { + t.Errorf("FieldAny.String() = %q, want %q", got, want) + } +} + +func readCase(t *testing.T, path string) []byte { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + return data +} diff --git a/cyclops/handlers.go b/cyclops/handlers.go index 081b7d9..dcd8a71 100644 --- a/cyclops/handlers.go +++ b/cyclops/handlers.go @@ -327,9 +327,10 @@ func makeConditionalClause(cond, filter, tag, omitTag, sort, limit, offset strin if cond != "" { b.WriteString(" where ") - // XXX injection risk: 'cond' is a free-form condition expression and is - // not sanitised. Safe handling needs AST-based construction (or a - // validating parser) rather than string interpolation. + // 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. b.WriteString(cond) } @@ -427,16 +428,62 @@ func makeSelectClause(fields, setName, cond, filter, tag, omitTag, sort, limit, return b.String(), nil } +// getCondSchema provides the schema against which a 'jsonCond' parameter is +// validated. Until mod-cyclops knows which fields each project exposes, any +// syntactically valid field and filter name is admitted, so what this buys is +// injection-safety rather than authorisation: see the commentary in cond.go. +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 +// '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. +// +// '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") + + if cond != "" && jsonCond != "" { + return "", &HTTPError{ + status: http.StatusBadRequest, + message: "only one of 'cond' and 'jsonCond' may be supplied", + } + } + if jsonCond == "" { + return cond, nil + } + + rendered, err := ParseCond([]byte(jsonCond), getCondSchema()) + if err != nil { + return "", &HTTPError{ + status: http.StatusBadRequest, + message: fmt.Sprintf("invalid 'jsonCond' parameter: %s", err), + } + } + return rendered, nil +} + func makeRetrieveCommand(req *http.Request, countOnly bool) (string, error) { selectFields := req.URL.Query().Get("fields") if countOnly { selectFields = "COUNT(*)" } + cond, err := requestCond(req) + if err != nil { + return "", err + } + selectClause, err := makeSelectClause( selectFields, chi.URLParam(req, "setName"), - req.URL.Query().Get("cond"), + cond, req.URL.Query().Get("filter"), req.URL.Query().Get("tag"), req.URL.Query().Get("omitTag"), diff --git a/cyclops/handlers_test.go b/cyclops/handlers_test.go index 8c25eef..b17ce6f 100644 --- a/cyclops/handlers_test.go +++ b/cyclops/handlers_test.go @@ -6,6 +6,7 @@ import "errors" import "fmt" import "net/http" import "net/http/httptest" +import "net/url" import "reflect" import "strings" import "testing" @@ -173,6 +174,121 @@ func TestHandleRetrieveCCMSError(t *testing.T) { } } +// assertHTTPStatus fails the test when err is not an *HTTPError carrying the +// wanted status. A client mistake must not be reported as a server fault. +func assertHTTPStatus(t *testing.T, err error, want int) { + t.Helper() + var httpErr *HTTPError + if !errors.As(err, &httpErr) { + t.Fatalf("error %v is a %T, want an *HTTPError with status %d", err, err, want) + } + if httpErr.status != want { + t.Errorf("status: got %d want %d", httpErr.status, want) + } +} + +// retrieveCommandFor runs handleRetrieve over the given query string and returns +// the command that reached CCMS. +func retrieveCommandFor(t *testing.T, rawQuery string) string { + t.Helper() + result := ccms.NewResult("ok") + result.AddField("id", "string") + resp := ccms.NewResponse() + resp.AddResult(result) + + fake := &fakeCCMS{resp: resp} + server := newTestServer(fake) + err := server.handleRetrieve(httptest.NewRecorder(), retrieveRequest("users", rawQuery), "retrieve") + if err != nil { + t.Fatalf("handleRetrieve(%q) returned error: %v", rawQuery, err) + } + return fake.lastCmd +} + +// A condition supplied as 'jsonCond' is rendered into the command by ParseCond, +// so the values it carries can only appear as quoted literals. +func TestHandleRetrieveJSONCond(t *testing.T) { + cases := map[string]string{ + `{"type":"term","field":"title","rel":"contains","value":"world"}`: `title ilike '%world%'`, + `{"type":"term","field":"holdings_count","rel":"ge","value":3}`: `holdings_count >= 3`, + `{"type":"term","field":"decision","rel":"eq","value":false}`: `decision = false`, + `{"type":"filter","name":"target"}`: `filter(target)`, + `{"type":"and","clauses":[{"type":"term","field":"title","rel":"contains","value":"world"},` + + `{"type":"term","field":"author","rel":"contains","value":"O'Brien"}]}`: `(title ilike '%world%' and author ilike '%O''Brien%')`, + // The payload that motivated the whole exercise: it must survive as + // data, matched literally, rather than becoming a second statement. + `{"type":"term","field":"note","rel":"contains","value":"'; drop set users; --"}`: `note ilike '%''; drop set users; --%'`, + } + for jsonCond, wantCond := range cases { + t.Run(wantCond, func(t *testing.T) { + got := retrieveCommandFor(t, "fields=id&jsonCond="+url.QueryEscape(jsonCond)) + want := "select id from users where " + wantCond + " limit 100;" + assertEqual(t, "command sent to CCMS", got, want) + }) + } +} + +// The old parameter keeps working untouched while it remains. +func TestHandleRetrieveCondStillInterpolated(t *testing.T) { + got := retrieveCommandFor(t, "fields=id&cond="+url.QueryEscape("title ilike '%world%'")) + assertEqual(t, "command sent to CCMS", got, "select id from users where title ilike '%world%' limit 100;") +} + +// Neither parameter means an unconditional retrieval, exactly as before. +func TestHandleRetrieveNoCond(t *testing.T) { + got := retrieveCommandFor(t, "fields=id") + assertEqual(t, "command sent to CCMS", got, "select id from users limit 100;") +} + +// The two parameters are alternatives, so supplying both is a client error and +// no command is sent. +func TestHandleRetrieveBothConds(t *testing.T) { + fake := &fakeCCMS{resp: okResponse()} + server := newTestServer(fake) + + rawQuery := "fields=id&cond=" + url.QueryEscape("title = 'x'") + + "&jsonCond=" + url.QueryEscape(`{"type":"filter","name":"target"}`) + err := server.handleRetrieve(httptest.NewRecorder(), retrieveRequest("users", rawQuery), "retrieve") + if err == nil { + t.Fatal("expected an error when both 'cond' and 'jsonCond' are supplied") + } + assertHTTPStatus(t, err, http.StatusBadRequest) + assertErrContains(t, err, "only one of 'cond' and 'jsonCond' may be supplied") + if fake.lastCmd != "" { + t.Errorf("a command was sent despite the bad request: %q", fake.lastCmd) + } +} + +// A 'jsonCond' that does not describe a condition is the client's mistake, and +// must be reported as such rather than as a server fault. +func TestHandleRetrieveJSONCondInvalid(t *testing.T) { + cases := map[string]string{ + `title ilike '%world%'`: `not a condition clause`, + `{}`: `clause has no "type"`, + `{"type":"xyzzy"}`: `unknown clause type "xyzzy"`, + `{"type":"term","field":"title","rel":"ilike","value":"x"}`: `unknown relation "ilike"`, + `{"type":"term","field":"title; drop set users","rel":"eq","value":"x"}`: `invalid field identifier: "title; drop set users"`, + } + for jsonCond, wantErr := range cases { + t.Run(wantErr, func(t *testing.T) { + fake := &fakeCCMS{resp: okResponse()} + server := newTestServer(fake) + + rawQuery := "fields=id&jsonCond=" + url.QueryEscape(jsonCond) + err := server.handleRetrieve(httptest.NewRecorder(), retrieveRequest("users", rawQuery), "retrieve") + if err == nil { + t.Fatalf("expected an error for jsonCond=%s", jsonCond) + } + assertHTTPStatus(t, err, http.StatusBadRequest) + assertErrContains(t, err, "invalid 'jsonCond' parameter") + assertErrContains(t, err, wantErr) + if fake.lastCmd != "" { + t.Errorf("a command was sent despite the bad request: %q", fake.lastCmd) + } + }) + } +} + // jsonRequest builds a request carrying the given chi URL params and a JSON // body. Pass a nil params map when no route params are needed, and an empty // body for handlers that don't read one. diff --git a/htdocs/index.html b/htdocs/index.html index 5900886..d98123f 100644 --- a/htdocs/index.html +++ b/htdocs/index.html @@ -89,6 +89,87 @@
+ Retrieval with + a structured condition, + supplied as thejsonCond parameter instead of cond.
+