Skip to content

feat(lint): security-fls-unknown-field — a qualified FLS key naming a field the object does not declare - #16998

Merged
baozhoutao merged 4 commits into
mainfrom
claude/issue-16108-fls-key-field-existence
Sep 9, 2026
Merged

feat(lint): security-fls-unknown-field — a qualified FLS key naming a field the object does not declare#16998
baozhoutao merged 4 commits into
mainfrom
claude/issue-16108-fls-key-field-existence

Conversation

@claude

@claude claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Part of #16108 — deliberately Part of, not a closing keyword: the card's own text makes the retirement of hotcrm's local assertion test/authorization-coverage.test.ts conditional on this rule landing and being verified, and that verification is not this PR's to perform. Closing the card on merge would retire the accounting row while the downstream assertion is still the only thing holding the line.

The defect, and the measurement that it stands

A permission set that authors fields: { 'crm_account.description_nope': { readable: false } } declares a mask that can never enforce. PermissionEvaluator.getFieldPermissions keeps a key only when it starts with the object prefix and then reads the remainder as a column name (key.substring(objectName.length + 1)), so a remainder no column answers to contributes nothing to the merged permission map — the field stays as readable and as editable as the object-level grant leaves it, for every holder of the set. security-fls-unqualified-key catches only the bare spelling.

The before leg was run first, as instructed. On the tree at fe2b7554a5, packages/lint built, one stack per leg through validateSecurityPosture:

leg key rules emitted (before) rules emitted (after)
A — control, unqualified description security-fls-unqualified-key, security-private-no-readscope unchanged
B — the card, qualified + dangling crm_account.description_nope security-private-no-readscope only security-fls-unknown-field, security-private-no-readscope
C — control, qualified + real crm_account.description security-private-no-readscope only unchanged
D — the patch round, qualified + empty crm_account. security-private-no-readscope only security-fls-unknown-field, security-private-no-readscope
E — control, empty remainder on an unknown object no_such_object. security-private-no-readscope only unchanged (skip 1)

B and C were indistinguishable before and are distinguished now; A is untouched and still reds under its own id, not under the new one. security-private-no-readscope is the constant info-tier background in all three legs, present in both columns, so it cancels.

What triage estimated, and what it actually was — for #16119

Triage priced the hard part as getting the object's field set into this rule's input layer, and flagged it as the expensive half shared with #16119. That premise did not hold, and the answer is worth carrying to #16119 directly: the field set was already reachable, and not merely reachable — validate-security-posture.ts already imports from object-graph.ts (import { recordsOf } from './object-graph.js'), which is this package's shared field-resolution index. Reaching the field set cost one word added to an existing import statement (indexObjectGraph) and one const before the permission-set loop.

object-graph.ts is not just a field-name lookup; it already encodes the three skips every field-existence rule in this package takes, and this rule inherits all three rather than deriving a second opinion:

  1. an object this stack does not define (it may be another installed package's);
  2. an object with no readable field map (an ADR-0015 external object, an introspected datasource);
  3. registry-injected system columns — created_at, owner_id where ownership provides one — which are real at runtime and appear in no authored fields, resolved per object via injectedColumnsFor, never the object-independent union.

So #16119 should not budget for input plumbing on the RLS-predicate face either; indexObjectGraph plus resolveFieldPath is already there, and validate-expressions.ts is already a consumer of it.

Boundaries held

  • security-fls-unqualified-key is untouched. Verified as a diff property, not an intention: the existing loop is byte-identical and the new rule is a second, disjoint loop over the same map beneath it — the first skips every key containing a dot, the second skips every key not containing one, so no key reaches both. Two ids, two messages, two prescriptions — an author suppressing one does not suppress the other.
  • hotcrm's test/authorization-coverage.test.ts is not referenced, weakened or retired, and nothing in this diff or its changeset claims coverage that would let it be. This repo does not contain that file; the only thing this PR asserts about it is in the paragraph above.
  • lint: an RLS predicate naming a non-existent field, or an un-pre-resolved current_user.* variable, is reported by nothing — both fail CLOSED at runtime #16119 is not folded in. No RLS-predicate surface is touched.

Design decisions worth a reviewer's eye

Patch round — a fail-open this rule SAW and skipped, on a premise that was false

The first revision carried this line:

if (!flsField) continue; // 'crm_account.' names no field at all — a shape the schema owns

The schema does not own it, and I did not check before writing that. Re-derived here, with a control:

  • packages/spec/src/security/permission.zod.ts declares fields: z.record(z.string(), FieldPermissionSchema).optional() — a bare string key, no pattern;
  • that file contains zero occurrences of refine, superRefine or .regex( (control: the same grep over packages/spec/src/data/object.zod.ts returns 12, so the pattern is not silently matching nothing);
  • packages/lint/src/validate-security-posture.ts is the only reader of permission-set fields keys in the package (one hit across packages/lint/src/*.ts).

So 'crm_account.' was reported by nothing, and at runtime it passes the prefix test and resolves to the empty column name — matching no column. That is the identical fail open this rule exists to close, sitting inside the rule, skipped by a sentence crediting coverage to a component that has none. That sentence is the same accounting trap this card warns about downstream, one level in, and it is the part of this PR I got wrong.

Folded into security-fls-unknown-field, with its own message branch (there is no field name to quote, so the dangling-field wording would have rendered as declares no field ''), a pin on the message, and a pin against the live schema so the premise cannot rot silently: if a future revision adds a key pattern or a refinement, that test reds and the rule's justification is re-opened deliberately instead of the rule quietly becoming a second opinion on the schema.

Deliberately NOT widened. The check sits after the two object skips. An empty field name is in fact unmatchable independently of the object — FieldSchema.name is /^[a-z_][a-z0-9_]*$/, so no package can declare one — but hoisting it above skip 1 would start this rule judging keys whose object half it cannot resolve, which is exactly the disposition '.description' (empty object name) and a mis-cased object name are left to. 'no_such_object.' therefore stays silent, pinned by leg E of the table above and by its own test with a firing control beside it.

Clause-② — re-derived, not copied

My reading is yes, and the changeset grades @objectstack/lint as minor. Derived in two steps rather than taken from the dispatch:

  1. Does this diff reach the published artifact? Measured, not assumed. npm pack --dry-run --json on packages/lint reports 17 entries with dist/ published under the package's files whitelist. Grepping the built artifact for the new rule id: dist/index.js = 1, dist/index.cjs = 1, dist/index.d.ts = 1. Positive control in the same command — security-fls-unqualified-key = 1 in each of the three. Nonsense control — security-fls-nonsense-control = 0 in each of the three. So the zero-vs-one reading is a reading.
  2. Which grade? Not patch: a consumer whose stack carries a dangling FLS key builds clean on 17.3.0 and fails on the next version, at error severity, on both the CLI path and the runtime publish door. A patch cannot break a consumer's build. Not major: nothing is removed or reshaped, the new export is purely additive, and every stack without a dangling key is byte-identically clean. minor is also what this repo's own precedent grades a new gating rule id — .changeset/flow-filter-token-unknown.md and .changeset/lint-field-no-consumers.md are both minor for exactly this shape, while .changeset/chart-measure-unknown-presentation-positions.md is patch for a rule that narrows.

skip-changeset was never in play: the rule ships inside dist/, which is published.

Verification

Run under scripts/pm/os-verify-lock.sh; verdicts quoted from each gate's own printed line, exit codes captured before any pipe. All readings below are on the patched head b84b9e23f2.

Tests and typecheck

  • pnpm --filter @objectstack/lint testVERDICT command-exit 0; 102 files, 3596 tests passed.
  • pnpm --filter @objectstack/lint typecheckVERDICT command-exit 0 (tsc --noEmit plus check:test-typecheck, the latter printing OK — @objectstack/lint's test layer compiles under packages/lint/tsconfig.test.json).

Ablations — CORRECTED, and re-measured on the patched head b84b9e23f2

⚠️ The earlier revision of this section was wrong, and not by a digit. It reported one "always fires" ablation at 7, and described that ablation as "the three skip guards collapsed into an empty-surface fallback". The mutation actually run also deleted the existence check, which the description did not say — so the number was not measured the way the text described it, and the parenthetical gloss ("all four negative controls plus the pre-existing") named five tests while the run reddened seven. The digit was real for a mutation this body did not describe. It is restated below as three separately named legs, each with its mutation given verbatim so any reader can reproduce the count rather than take it.

Each leg: mutate, prove the mutation reached disk by git hash-object differing from the file's HEAD blob, run, then restore with git checkout HEAD -- ABSOLUTE_PATH under an EXIT INT TERM trap and prove the restore by the blob hash returning to a95310e35a84e083b4a87e802c4bd7fde3340b11 with git diff HEAD empty. Scope: src/validate-security-posture.test.ts alone, 122 tests.

leg mutation, verbatim red
A — disarm if (surface.names.has(flsField) || surface.injected.has(flsField)) continue; replaced by a bare continue 7
B — the three skips only, existence check KEPT the two flsGraph guards replaced by const surface = flsGraph.get(flsObject) ?? { names: new Set(), injected: new Set(), fields: new Map() }; 4
C — true always fires: skips AND existence check removed both of the above at once 8

Leg B is the one the review asked for, and it confirms the review's prediction exactly: an all-qualified, all-real permission set emits NOTHING new stays green under it, because collapsing the skips cannot change the verdict on a key whose field genuinely resolves. Its four reds are precisely the skip-dependent ones — accepts object-qualified FLS keys, an object with NO fields at all does not throw, skip 1: an object this stack does not define is never judged, and skip 1 still holds for a truncated key naming an unknown object.

⚠️ One residual difference worth naming rather than smoothing over: the reviewer's leg B also reddened skip 3, and mine does not. That is a mutation difference, not a disagreement about the code — my leg B leaves flsGraph.get in place, so a key on an object that IS in the stack still sees the real injected set and created_at still resolves. Their leg evidently blanked the surface unconditionally. Both are legitimate mutations of "remove the skips"; the mutation text above says which one produced the 4.

Leg C's 8 reconciles with the reviewer's independently measured true-always-fires count of 8, and it is the leg that answers the acceptance criterion by name: an implementation that always fires satisfies every positive test, and eight tests refuse it — including all five negative controls.

Negative controls, all with a firing control beside the zero

  • an all-qualified, all-real permission set emits nothing new — and the same stack with one key repointed at a nonexistent field emits exactly one finding;
  • an object with no fields at all does not throw and is not judged — both spellings, fields absent and fields: {};
  • skip 1 (object not in this stack) and skip 3 (created_at) each emit zero, each measured against a nonsense sibling in the same run that emits one.

Blast radius on the repo's own metadata

The rule was run over the shipped showcase's real security metadata (22 objects, 9 permission sets loaded from examples/app-showcase/src): its six authored FLS keys — three distinct, all on showcase_project — emit zero findings. Firing control in the same run: injecting one dangling key produces exactly one security-fls-unknown-field. examples/app-crm authors no fields map at all. The authoritative sweep is CI's objectstack verify leg over both example apps.

Gates run locally (verdict line quoted, exit captured before any pipe)

gate exit verdict
pnpm check:nul-bytes 0 OK (scanned 8410 text file(s) … no raw ASCII control bytes)
check-empty-changeset --base origin/main 0 1 declaring changeset(s) added
check-changeset-no-major --base origin/main 0 This diff introduces no major bump
check-adr-0087-registration --base origin/main 0 adds no declared-breaking changeset
pnpm check:changeset-gate-self-tests 0 all three self-tests hold
pnpm check:published-files 0 70 publishable package(s) … none narrows its resolvable surface against the merge base
pnpm --filter @objectstack/lint check:doc-security-posture 0 27 ObjectSchema.create example(s) … carry an os validate-clean security posture
pnpm check:cross-package-test-inputs 0 28 package(s) read outside themselves, all declared
pnpm check:test-source-alias 0 73 packages with tests scanned
pnpm check:type-check-coverage 0 76/80 workspace packages type-checked
pnpm check:docs-audit/check-affected-docs 0 no docs owed by this diff

Declared NOT MEASURED, not green — each stopped on a stated prerequisite this container cannot meet, and each says so in its own words rather than passing:

  • check:published-readme-exports — exit 3, PREREQUISITE NOT MET: 44 packages' built type entries are absent, so nothing was judged. Needs a full-workspace build; CI's.
  • check:type-check-debt --re-measure — exit 3, PREREQUISITE NOT MET: 28 workspace dependencies unbuilt. Its self-test and invariant half both passed (see the table).

Lint, as a declared narrowing with all three readings

  • population, read from eslint's own config rather than guessed: 6391 files in scope;
  • narrowed run over the five changed .ts files, count read from --format json: 5 files, 0 errors, 0 warnings;
  • invariance for untouched files: eslint.config.mjs:328 states in as many words that there is no parserOptions.project and no typed @typescript-eslint rules, so no verdict on an untouched file is a function of this diff. The five files also carry zero inline eslint directives, so --no-inline-config (which pnpm lint adds and this run did not) changes nothing for them.

A whole-repo eslint . was additionally run: 104 errors / 30 warnings, none of them inside packages/lint/src — they are pre-existing on the merge base and untouched by this diff.

Not filed, noted here for the reviewer

  • docs/qa/platform-checklist/areas/access-security.json names the sibling rule id and not this one. The previous new rule in this file (security-cbp-ambiguous-relation) is not there either, so there is no established obligation and none is discharged here. Recorded as a residual by the dispatching seat.

  • This module's own header table indexes its rules by id and has never listed security-fls-unqualified-key; the new rule is not added to it either, so the table's coverage is unchanged rather than made inconsistent. Both rules are documented in prose at their push sites, which is how the existing FLS rule is already documented.

  • skills/objectstack-data/SKILL.md tabulates 4 of this module's 14 rule ids. The new one joins the 10 already absent. Not touched: it is a published skill under the line ratchet, and a partial table is not a defect.

  • A key naming an object this stack does not declare (no_such_object.description) is still reported by nothing, and fails open the same way. That is skip 1, and it is deliberate rather than a gap in this rule: the object may be shipped by another installed package, so a package-time linter genuinely cannot answer. It is the same disposition the module's header already records for a guest-bound anchor set (lint: security-anchor-high-privilege keys off permissionSet.isDefault, so an anchor-bound set that does not author that flag is never checked #16110) — judged at bind/install time, not at authoring time. Named here so it is not mistaken for coverage this PR provides.


Generated by Claude Code

…ect does not declare

A permission set key like 'crm_account.description_nope' is stripped of its
object prefix by PermissionEvaluator.getFieldPermissions and the remainder is
looked up as a column, so a remainder no column answers to contributes nothing
to the merged permission map: the declared masking never enforces and the field
stays readable to every holder of the set. security-fls-unqualified-key caught
only the bare spelling; this adds security-fls-unknown-field beside it, without
touching it.

Existence resolves through object-graph.ts, the shared field index this package
already uses, so the rule inherits its three skips (object not in this stack,
no readable field map, registry-injected system columns) rather than deriving a
second opinion about what a field is.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU
@github-actions github-actions Bot added the size/m label Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/lint, touching 3 documentable anchor(s). ⚠️ 1 changed file(s) yielded no anchor (packages/lint/src/index.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

2 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/deployment/validating-metadata.mdx (via AUTHORING_RULES (symbol, a top-level const object))
  • content/docs/permissions/authorization.mdx (via validateSecurityPosture (symbol, a top-level function))

2 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v13.mdx (via validateSecurityPosture (symbol, a top-level function))
  • content/docs/releases/v17.mdx (via AUTHORING_RULES (symbol, a top-level const object), validateSecurityPosture (symbol, a top-level function))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 changed file(s) yielded no anchor (packages/lint/src/index.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 419facddee69d4c4b569f3a8e33fe985b4267160packageMentionDocs.

Which tree this was computed on

This run read content/docs from c58a6f438492e070bce1543a5207cd78a0d35adf — the merge of head b84b9e23f28b05a77c5958253a5ae0a7653a1938 into base 419facddee69d4c4b569f3a8e33fe985b4267160, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin c58a6f438492e070bce1543a5207cd78a0d35adf && git checkout c58a6f438492e070bce1543a5207cd78a0d35adf
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 419facddee69d4c4b569f3a8e33fe985b4267160 b84b9e23f28b05a77c5958253a5ae0a7653a1938 && git checkout -B drift-repro 419facddee69d4c4b569f3a8e33fe985b4267160 && git merge --no-ff b84b9e23f28b05a77c5958253a5ae0a7653a1938

node scripts/docs-audit/affected-docs.mjs --json 419facddee69d4c4b569f3a8e33fe985b4267160

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 419facddee69d4c4b569f3a8e33fe985b4267160 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

…a schema that does not check it

'crm_account.' passed startsWith at runtime and resolved to the empty column
name, matching nothing -- the same fail open security-fls-unknown-field exists
to close. It was skipped with a comment claiming the schema owned the shape.
PermissionSetSchema.fields is z.record(z.string(), FieldPermissionSchema) with
no key regex and no refine in that file, and this loop is the package's only
reader of those keys, so nothing reported it. The false sentence is replaced by
the measurement, and a pin against the live schema keeps the premise from
rotting silently.

Judged after the two object skips, not before them: an empty field name is
unmatchable independently of the object, but hoisting the check would start the
rule judging keys whose object half it cannot resolve.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU

Copy link
Copy Markdown
Collaborator

Contract review (claude-fable-5-1, in-seat) — PR #16998 @ b84b9e23f2

Verdict: PASS. Reviewed-by: consolidated seat session_018rzQyhLGC5iVs11V3TzRs5 (serves CONTRACT_REVIEW_TIER; get_session at 01:35Z). Implemented-by: session_012GKcPZbMoGq7WPzKLfRBTU's os-dev on claude/issue-16108-fls-key-field-existence.

What the diff does, read from the hunks: a second loop over the same fields map beneath security-fls-unqualified-key, disjoint by guard (dot < 0 vs dot >= 0); split on the first dot; skip 1 (!flsGraph.has), skip 2 (!surface), skip 3 (namesinjected); the empty remainder reported by its own message branch, judged after the two object skips so 'no_such_object.' and '.description' keep their skip-1 disposition; indexObjectGraph(stack) built once. security-fls-unqualified-key's loop is byte-identical.

The one question this seat had to settle itself — the evaluator has no field-half wildcard. packages/plugins/plugin-security/src/permission-evaluator.ts getFieldPermissions at origin/main: if (!key.startsWith(\${objectName}.`)) continue; const fieldName = key.substring(objectName.length + 1);— nothing else; the''sentinel in that file is onobjects only (:140, :217). So a key like crm_account.would match no column at runtime and the new rule reporting it is a true finding, not a false one. Control: 0 authored FLS keys with a*field half inexamples/+packages/` (the 4 hits for the regex are unrelated strings).

Clause-② yes, level minor: confirmed. The rule turns a stack that builds clean today into an error at os validate / os compile and the runtime publish door — a narrowing of the accept set of a published rule; minor matches the repo's precedent for a new gating rule id. check-clause2-carriers --pair 16998 (this seat, 02:1xZ): exit 0, "the clause-② declaration is readable in the fixed spelling and both carriers agree".

Evidence read, not taken: the before/after five-leg table (B and C indistinguishable before, distinguished after; A unmoved); three named ablation legs with the mutation text and blob-verified restore (leg C = 8 reds reconciles the always-fires count); negative controls with firing siblings; the schema pin against the live PermissionSetSchema.fields (bare z.string() key) so the empty-remainder premise cannot rot silently; showcase blast radius 6 keys / 0 findings with a one-key firing control.

Record, non-blocking: (1) the module's header rule table still lists neither FLS rule — same as before, not made inconsistent; (2) docs/qa/platform-checklist/areas/access-security.json names the sibling rule only — residual carried by the dispatching seat, no established obligation; (3) Part of is right: hotcrm's test/authorization-coverage.test.ts retirement is hotcrm's verification, not this PR's — on merge this seat posts the landing on hotcrm#1621 and closes #16108 as delivered, since the platform half is complete.

Governed paths: none. Maintainer-only merge: no. Handoff: needs:contract-review cleared on PR and card in the same stroke; auto-merge armed.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants