From 346236d81bae1aa6335be13fc8a5fb5c746821b0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 18 Jul 2026 07:39:17 +0000 Subject: [PATCH] feat(formula): warn on `date` field == temporal-function silent-miss (#3183) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `Field.date` deserializes as a "YYYY-MM-DD" string (ADR-0053 Phase 1), and cel-js's equality hard-codes `string == ` to false, so `record.due_date == today()` compiles clean but silently never matches. cel-js offers no operator-layer hook to fix the comparison, so this adds a build-time advisory warning (the established ADR-0032 guardrail strategy) that turns the silent runtime miss into a loud, self-correcting build signal. `validateExpression` reuses the shared `ExprSchemaHint.fieldTypes` (the same per-field type map the #1928 tier-4 soundness check already threads through `@objectstack/lint`) to flag `==`/`!=` between a `date` field and today()/daysFromNow()/daysAgo()/now(), with a message pointing at the working idioms (date(...), a range, or daysBetween). Warning severity — the write/validation path may carry a real Date, so it never fails the build. Restricted to `type: 'date'` (unambiguously a string); `datetime` excluded to avoid false positives. Ordering operators already work (they fault→hydrate) and are not flagged. Complements the #1874 flow-scheduling lint, which targets a different root cause (record-change triggers firing only on the exact write day). A runtime fix (normalizing the peer of a temporal operand in the data layer) stays tracked in #3183 — a naive "hydrate date fields to Date" version would trade this silent-miss for another (breaking `dateField == "literal"`). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01SuiM565BZ3TR1VD3prMguB --- .changeset/date-field-equality-guardrail.md | 41 +++++++++ packages/formula/src/cel-engine.test.ts | 34 +++++++- packages/formula/src/cel-engine.ts | 67 +++++++++++++++ packages/formula/src/validate.test.ts | 65 ++++++++++++++- packages/formula/src/validate.ts | 37 ++++++++- .../lint/src/validate-expressions.test.ts | 83 +++++++++++++++++++ 6 files changed, 323 insertions(+), 4 deletions(-) create mode 100644 .changeset/date-field-equality-guardrail.md diff --git a/.changeset/date-field-equality-guardrail.md b/.changeset/date-field-equality-guardrail.md new file mode 100644 index 0000000000..a8e7ff3024 --- /dev/null +++ b/.changeset/date-field-equality-guardrail.md @@ -0,0 +1,41 @@ +--- +"@objectstack/formula": minor +--- + +feat(formula): warn when a `date` field is compared to a temporal function with `==`/`!=` (#3183) + +A `Field.date` deserializes as a `YYYY-MM-DD` **string** (ADR-0053 Phase 1), and +cel-js's equality hard-codes `string == ` to `false` — it returns +`false` for a string left operand without ever consulting a registered overload, +and refuses cross-type object equality (`@marcbachmann/cel-js` `overloads.js` +`isEqual`). So the most natural "is it due today" predicate — + +```cel +record.due_date == today() // silently false, even when due_date IS today +record.due_date != today() // silently true for a same-day record +``` + +— compiles clean, throws nothing, and silently never matches. Same silent-miss +family as #1928; **timezone-independent** (fails identically at UTC) and +cross-cutting (formulas, validation, RLS, flow/action/sharing/hook predicates). + +cel-js gives no operator-layer hook to fix the comparison, so this adds a +**build-time advisory warning** (the established ADR-0032 guardrail strategy) +rather than a runtime behavior change. `validateExpression` reuses the shared +`ExprSchemaHint.fieldTypes` (the same per-field type map the #1928 tier-4 +soundness check already threads through `@objectstack/lint`) to flag a `==`/`!=` +between a `date` field (`record.`/`previous.`/bare) and +`today()`/`daysFromNow()`/`daysAgo()`/`now()`, with a self-correcting message +pointing at the working idioms: `date(record.d) == today()`, a range +(`>= … && <= …`), or `daysBetween(today(), record.d) == 0`. + +Warning severity — never fails the build (the write/validation path may carry a +real `Date`). Restricted to `type: 'date'` (unambiguously a string); `datetime` +is excluded to avoid false positives. Ordering operators (`>=`/`<=`/`<`/`>`) +already work — cel-js *throws* for them, tripping the engine's existing +string-hydration retry — so they are not flagged. + +A runtime fix (normalizing the peer of a temporal operand in the data layer) +remains tracked in #3183; a naive "hydrate date fields to `Date`" version would +trade this silent-miss for another (breaking `dateField == "2026-06-20"`), so it +needs its own design. diff --git a/packages/formula/src/cel-engine.test.ts b/packages/formula/src/cel-engine.test.ts index fc0a0f05d5..41e9559581 100644 --- a/packages/formula/src/cel-engine.test.ts +++ b/packages/formula/src/cel-engine.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest'; -import { celEngine } from './cel-engine'; +import { celEngine, temporalEqualityFields } from './cel-engine'; import { CEL_STDLIB_FUNCTIONS } from './validate'; import type { Expression } from '@objectstack/spec'; @@ -510,4 +510,36 @@ describe('celEngine', () => { expect(unresolved, `advertised functions that fault at runtime:\n${unresolved.join('\n')}`).toEqual([]); }); }); + + // #3183 — AST walk backing the date-equality guardrail. Returns field names + // compared with `==`/`!=` directly against a temporal function; the validator + // filters these by field type. AST-based, so no ReDoS on adversarial source. + describe('temporalEqualityFields (#3183)', () => { + it('finds the field on either side, for all four temporal functions', () => { + expect(temporalEqualityFields('record.due == today()')).toEqual(['due']); + expect(temporalEqualityFields('today() != record.due')).toEqual(['due']); + expect(temporalEqualityFields('record.due == daysFromNow(3)')).toEqual(['due']); + expect(temporalEqualityFields('record.due != daysAgo(7)')).toEqual(['due']); + expect(temporalEqualityFields('previous.due == now()')).toEqual(['due']); + expect(temporalEqualityFields('due == today()')).toEqual(['due']); // bare (flattened) + }); + + it('returns nothing for the working idioms or ordering comparisons', () => { + expect(temporalEqualityFields('date(record.due) == today()')).toEqual([]); + expect(temporalEqualityFields('record.due >= today()')).toEqual([]); + expect(temporalEqualityFields('daysBetween(today(), record.due) == 0')).toEqual([]); + expect(temporalEqualityFields('record.a == record.b')).toEqual([]); + }); + + it('de-duplicates and finds fields nested in a compound predicate', () => { + expect(temporalEqualityFields('record.due == today() || record.due == daysFromNow(1)')).toEqual(['due']); + expect(temporalEqualityFields('record.a == today() && b != now()').sort()).toEqual(['a', 'b']); + }); + + it('is linear on adversarial input (the CodeQL ReDoS repros) and returns []', () => { + // These would drive the previous regex O(n²); the AST walk parses or bails fast. + expect(temporalEqualityFields('$'.repeat(5000))).toEqual([]); + expect(temporalEqualityFields('now('.repeat(2000))).toEqual([]); + }); + }); }); diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 8e12e7079f..324a484fca 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -325,6 +325,73 @@ export function firstTypeMismatch( } } +/** cel-js temporal functions that return a calendar Timestamp (for #3183). */ +const TEMPORAL_FNS = new Set(['today', 'daysFromNow', 'daysAgo', 'now']); + +/** A cel-js AST node is `{ op, args }`; `args` is a node[], or a leaf string. */ +type CelNode = { op: string; args: unknown }; + +function isCelNode(v: unknown): v is CelNode { + return typeof v === 'object' && v !== null && typeof (v as CelNode).op === 'string'; +} + +/** True when `node` is a call to a temporal function (`today()`/`daysFromNow(…)`/…). */ +function isTemporalCall(node: unknown): boolean { + return isCelNode(node) && node.op === 'call' + && Array.isArray(node.args) && typeof node.args[0] === 'string' + && TEMPORAL_FNS.has(node.args[0]); +} + +/** + * If `node` is a field reference — `record.` / `previous.` (member access) + * or a bare `` (flattened flow scope) — return the field name ``, else null. + */ +function fieldRefName(node: unknown): string | null { + if (!isCelNode(node)) return null; + if (node.op === 'id' && typeof node.args === 'string') return node.args; // bare `` + if (node.op === '.' && Array.isArray(node.args) && node.args.length === 2) { + const [base, member] = node.args; + if (isCelNode(base) && base.op === 'id' + && (base.args === 'record' || base.args === 'previous') + && typeof member === 'string') { + return member; + } + } + return null; +} + +/** + * #3183 — field names compared with `==`/`!=` DIRECTLY against a temporal + * function (`today()`/`daysFromNow()`/`daysAgo()`/`now()`), found by walking the + * cel-js AST (never a regex on the source — no ReDoS). The caller filters these + * by field type; a `Field.date` reads back as a string and cel-js equality never + * matches it against a timestamp, so such a comparison silently misses. The + * `date(…)`/`datetime(…)`/`timestamp(…)` coercions are NOT temporal calls, so the + * fixed idiom `date(record.d) == today()` yields nothing. Returns `[]` on a parse + * fault (compile() reports those) or when there is no such comparison. + */ +export function temporalEqualityFields(source: string): string[] { + if (typeof source !== 'string' || !source.trim()) return []; + let ast: unknown; + try { + ast = (recordScopeEnv ??= buildScopedEnv([])).parse(source).ast; + } catch { + return []; + } + const out = new Set(); + const visit = (node: unknown): void => { + if (!isCelNode(node)) return; + if ((node.op === '==' || node.op === '!=') && Array.isArray(node.args) && node.args.length === 2) { + const [left, right] = node.args; + if (isTemporalCall(left)) { const f = fieldRefName(right); if (f) out.add(f); } + if (isTemporalCall(right)) { const f = fieldRefName(left); if (f) out.add(f); } + } + if (Array.isArray(node.args)) for (const child of node.args) visit(child); + }; + visit(ast); + return [...out]; +} + /** Coerce cel-js's BigInt-flavored return into spec-friendly JS values. */ function coerce(value: unknown): unknown { if (typeof value === 'bigint') { diff --git a/packages/formula/src/validate.test.ts b/packages/formula/src/validate.test.ts index 56242441d8..770b6499b9 100644 --- a/packages/formula/src/validate.test.ts +++ b/packages/formula/src/validate.test.ts @@ -200,9 +200,12 @@ describe('validateExpression (ADR-0032)', () => { expect(validateExpression('value', 'record.amount * 2 - 50', schema).warnings).toHaveLength(0); }); - it('does NOT warn on a date field compared to today()/daysFromNow()', () => { + it('does NOT warn on a date field with an ORDERING comparison (they hydrate at runtime)', () => { + // Ordering ops fault → the engine's string-hydration retry fires → they work. + // (Equality `==`/`!=` does NOT — that is the #3183 silent-miss, covered in its + // own block below; this tier-4 check leaves it to the #3183 guardrail.) expect(validateExpression('predicate', 'record.due <= daysFromNow(30)', schema).warnings).toHaveLength(0); - expect(validateExpression('predicate', 'record.due == today()', schema).warnings).toHaveLength(0); + expect(validateExpression('predicate', 'record.due >= today()', schema).warnings).toHaveLength(0); }); it('does NOT warn on a select field ordered against a number (option values may be numeric codes)', () => { @@ -320,3 +323,61 @@ describe('inferExpressionType — coarse value-type of a formula', () => { expect(inferExpressionType('undeclared_field + 1')).toBe('unknown'); // bare ref, no fields given }); }); + +// #3183 — a `Field.date` reads back as a "YYYY-MM-DD" string, and cel-js's +// equality hard-codes `string == ` to false, so `record.due_date == +// today()` silently never matches. Advisory warning that guides the author to a +// working idiom. Derived from the shared `fieldTypes` hint (type === 'date'). +describe('temporal date-equality guardrail (#3183)', () => { + const schema = { + objectName: 'task', + fields: ['due_date', 'status'] as const, + fieldTypes: { due_date: 'date', status: 'text' }, + scope: 'record', + } as const; + const dateWarns = (src: string, s: Record = schema) => + validateExpression('predicate', src, s as never).warnings + .filter(w => /calendar-day \(date\) field/.test(w.message)); + + it('warns on `dateField == today()` (field on the left)', () => { + const r = validateExpression('predicate', 'record.due_date == today()', schema); + expect(r.ok).toBe(true); // advisory — never fails the build + expect(r.errors).toHaveLength(0); + const w = r.warnings.filter(x => /calendar-day \(date\) field/.test(x.message)); + expect(w).toHaveLength(1); + expect(w[0].message).toMatch(/date\(record\.due_date\) == today\(\)/); + }); + + it('warns on `today() == dateField` and on daysFromNow/daysAgo/now', () => { + expect(dateWarns('today() == record.due_date')).toHaveLength(1); + expect(dateWarns('record.due_date == daysFromNow(3)')).toHaveLength(1); + expect(dateWarns('record.due_date != daysAgo(7)')).toHaveLength(1); + expect(dateWarns('record.due_date == now()')).toHaveLength(1); + }); + + it('warns on a bare date-field ref (flattened flow-condition scope)', () => { + const flat = { objectName: 'task', fields: ['due_date'], fieldTypes: { due_date: 'date' } }; + expect(dateWarns('due_date == today()', flat)).toHaveLength(1); + }); + + it('does NOT warn on the working idioms', () => { + expect(dateWarns('date(record.due_date) == today()')).toHaveLength(0); + expect(dateWarns('record.due_date >= today() && record.due_date <= today()')).toHaveLength(0); + expect(dateWarns('daysBetween(today(), record.due_date) == 0')).toHaveLength(0); + }); + + it('does NOT warn on ordering comparisons (they fault→hydrate and work)', () => { + expect(dateWarns('record.due_date >= today()')).toHaveLength(0); + expect(dateWarns('record.due_date < daysFromNow(30)')).toHaveLength(0); + }); + + it('does NOT warn on a non-date field, or when fieldTypes is absent', () => { + expect(dateWarns('record.status == today()')).toHaveLength(0); // status is text, not date + expect(dateWarns('record.due_date == today()', { objectName: 'task', fields: ['due_date'], scope: 'record' })) + .toHaveLength(0); // no fieldTypes → check skipped + }); + + it('de-duplicates repeated references to the same field', () => { + expect(dateWarns('record.due_date == today() || record.due_date == daysFromNow(1)')).toHaveLength(1); + }); +}); diff --git a/packages/formula/src/validate.ts b/packages/formula/src/validate.ts index 868f634d57..6955b51900 100644 --- a/packages/formula/src/validate.ts +++ b/packages/formula/src/validate.ts @@ -17,7 +17,7 @@ * This validator detects that specific mistake and returns the exact fix. */ -import { celEngine, firstUndeclaredReference, firstTypeMismatch, inferCelType, type FieldCelType } from './cel-engine'; +import { celEngine, firstUndeclaredReference, firstTypeMismatch, inferCelType, temporalEqualityFields, type FieldCelType } from './cel-engine'; import { templateEngine } from './template-engine'; export type FieldRole = 'predicate' | 'value' | 'template'; @@ -144,6 +144,37 @@ function typeSoundnessWarning( }; } +/** + * #3183 — flag `==`/`!=` between a calendar-day (`date`) field and a temporal + * function (`today()`/`daysFromNow()`/`daysAgo()`/`now()`). A `Field.date` reads + * back as a `YYYY-MM-DD` string (ADR-0053 Phase 1), and cel-js's equality + * (`overloads.js` `isEqual`) treats a string and a timestamp as unequal without + * consulting any overload, so the comparison silently never matches. Advisory + * `warning` only — on the write/validation path the value may be a real `Date` — + * and no-op unless `schema.fieldTypes` marks the referenced field `date`. Derives + * the date fields from the shared `fieldTypes` hint (no separate plumbing). + */ +function checkTemporalDateEquality( + source: string, + schema: ExprSchemaHint | undefined, + warnings: ExprValidationError[], +): void { + const fieldTypes = schema?.fieldTypes; + if (!fieldTypes) return; + // AST-based (no regex on the raw source → no ReDoS); filter to `date` fields. + for (const field of temporalEqualityFields(source)) { + if (fieldTypes[field] !== 'date') continue; + warnings.push({ + source, + message: + `\`${field}\` is a calendar-day (date) field, stored as a "YYYY-MM-DD" string, so comparing it to a ` + + `timestamp function with \`==\`/\`!=\` silently never matches (CEL treats a string and a timestamp as ` + + `unequal). Wrap the field to coerce it — \`date(record.${field}) == today()\` — or use a range ` + + `(\`record.${field} >= today() && record.${field} <= today()\`) or \`daysBetween(today(), record.${field}) == 0\`.`, + }); + } +} + /** A bare `{x}` that is NOT part of a `{{x}}` mustache hole. */ const SINGLE_BRACE_RE = /(?:^|[^{])\{\s*([A-Za-z_$][\w.$]*)\s*\}(?!\})/; /** `record.` / `previous.` head references for field-existence. */ @@ -310,6 +341,10 @@ export function validateExpression( } else { checkFieldExistence(source, schema, errors); checkRoleCatalog(source, schema, errors); + // #3183 — date-field `==`/`!=` a temporal function silently never matches. + // Scope-independent (wrong in both record and flattened sites), so run it + // outside the scope branch below. + checkTemporalDateEquality(source, schema, warnings); if (schema?.scope === 'record') { // In a `record`-scoped site a bare top-level identifier is a silent bug — // it must be `record.` (#1928). Hard error. diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index 02cd256183..d940c55a2c 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -472,4 +472,87 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(issues.some(i => /columnName/.test(i.message))).toBe(false); }); }); + + // #3183 — `date` field `== today()` silently never matches (cel-js equality). + // The stack validator threads each object's field types into the shared + // validator, which flags the pattern with an advisory (non-blocking) warning. + describe('date-field equality guardrail (#3183)', () => { + const objects = [{ + name: 'task', + fields: { due_date: { type: 'date' }, title: { type: 'text' } }, + }]; + const dateWarns = (issues: { message: string }[]) => + issues.filter(i => /calendar-day \(date\) field/.test(i.message)); + + it('warns on a formula field comparing a date field to today()', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'task', + fields: { + due_date: { type: 'date' }, + is_due_today: { type: 'formula', formula: 'record.due_date == today()' }, + }, + }], + }); + const w = dateWarns(issues); + expect(w).toHaveLength(1); + expect(w[0].severity).toBe('warning'); + expect(w[0].where).toMatch(/object 'task'.*formula/); + }); + + it('warns on a validation-rule predicate, and never fails the build (advisory)', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'task', + fields: { due_date: { type: 'date' } }, + validations: [{ name: 'due_check', expression: 'record.due_date != today()' }], + }], + }); + const w = dateWarns(issues); + expect(w).toHaveLength(1); + expect(w.every(i => i.severity === 'warning')).toBe(true); + }); + + it('warns on a flattened flow condition using a bare date field', () => { + const issues = validateStackExpressions({ + objects, + flows: [{ + name: 'due_flow', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'task' } }, + { id: 'check', type: 'decision', config: { condition: 'due_date == today()' } }, + ], + edges: [], + }], + }); + expect(dateWarns(issues)).toHaveLength(1); + }); + + it('does not warn on the working idioms or ordering comparisons', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'task', + fields: { + due_date: { type: 'date' }, + due_soon: { type: 'formula', formula: 'record.due_date >= today() && record.due_date <= daysFromNow(7)' }, + is_today: { type: 'formula', formula: 'date(record.due_date) == today()' }, + }, + }], + }); + expect(dateWarns(issues)).toHaveLength(0); + }); + + it('does not warn on a datetime field (may deserialize as a real instant)', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'task', + fields: { + signed_at: { type: 'datetime' }, + just_signed: { type: 'formula', formula: 'record.signed_at == now()' }, + }, + }], + }); + expect(dateWarns(issues)).toHaveLength(0); + }); + }); });