Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions .changeset/plugin-keyed-text-bounds.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
---
"@objectstack/plugin-audit": patch
"@objectstack/plugin-security": patch
---

fix(plugin-audit,plugin-security): declare sourced bounds on the four keyed text columns that break MySQL schema-sync (#12059)

Four text columns that a declared index keys on carried no `maxLength`, so
`driver-sql` emitted them `TEXT`. MySQL refuses a TEXT/BLOB column in a key
without a key length (`ER_BLOB_KEY_WITHOUT_LENGTH`): `CREATE TABLE` succeeds,
`ALTER TABLE … ADD INDEX` fails, and the object lands registered-but-broken
with its declared index silently absent.

| Object | Column | Bound | Producer the bound is derived from |
|---|---|---|---|
| `sys_activity` | `record_id` | 255 | the physical `id` column — `driver-sql` creates every primary key as `table.string('id').primary()`, knex's `varchar(255)` |
| `sys_audit_log` | `record_id` | 255 | same |
| `sys_audience_binding_suggestion` | `package_id` | 255 | `sys_permission_set.package_id` (255), which the same boot pass writes the same value into |
| `sys_audience_binding_suggestion` | `permission_set_name` | 100 | `sys_permission_set.name` (100), the column this value resolves against at confirm time |

Each bound is derived from a **named producer** and stated in the declaration
so it is vetoable in review (#11374 route A; PR #12058 is the worked
precedent). None of them narrows anything storable:

- a record id cannot exceed the `varchar(255)` column the id itself lives in,
and the `referenceVia` seed path refuses an unresolvable pointer rather than
storing a natural key verbatim;
- a permission set name longer than 100 is already refused at the write seam
today — measured on a real engine, `ValidationError: API Name must be ≤ 100
characters (got 101)` — so no set with such a name can exist, and a
suggestion naming one could never be confirmed.

Measured at the driver level, shipped declaration vs. the same declaration with
the bounds stripped: `record_id`, `package_id` and `permission_set_name` move
`TEXT` → `varchar(255)` / `varchar(100)`, while `id` reads `varchar(255)` in
both — the transitivity premise, read off a real table rather than assumed.

Existing deployments are not rewritten: a physical `TEXT` column is deliberately
not diffed against `maxLength` (#11431), so no `ALTER` is planned and no value
at rest is truncated. The repair takes effect where the decision is makeable at
all — at `CREATE TABLE` — because no dialect turns a TEXT column into a keyable
one afterwards.

Each plugin also gains a keyed-text-bounds pin driven through its **own
registration path** (`init()` → the manifest `register({ objects })` call),
rather than a hand-written object list: the platform-objects pin enumerates only
that package's exports, which is exactly why these four columns escaped route
A's sweep after ADR-0029 K2 moved the objects out.
26 changes: 26 additions & 0 deletions packages/plugins/plugin-audit/src/objects/sys-activity.object.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -173,11 +173,37 @@ export const SysActivity = ObjectSchema.create({
group: 'Target',
}),

// [#11374 route A] The value is a record id of the object `object_name`
// names — written by `audit-writers.ts` (`record_id: recordId`, the id of
// the very row the mutation touched). The bound is derived by
// referenced-column transitivity from the id itself, never guessed:
// `driver-sql` creates every table's primary key as
// `table.string('id').primary()` — knex's `varchar(255)`, which the driver
// spells out as `DEFAULT_STRING_VARCHAR_CHARS = 255` and names in its own
// error text as "built-in `id` (a varchar(255))". No id this column can
// receive is wider than the column the id lives in.
//
// The seed path cannot widen it either: an unresolvable pointer is
// "refused loudly, never stored verbatim" (`metadata-protocol`'s
// seed-loader), so `referenceVia` resolves a natural key to a real record
// id BEFORE it is stored — a raw external key never lands in this column.
//
// 255 rather than the 100 that `plugin-sharing` and `plugin-approvals`
// chose for their own `record_id`: those narrow below what the id column
// itself accepts, which is safe only for their own writers. 255 is the
// transitive ceiling, so it refuses nothing that is storable today.
// It is also <= the 768-character utf8mb4 key ceiling, so the
// `(object_name, record_id)` index below is expressible on MySQL — which is
// the whole point: unbounded, this column was emitted TEXT, MySQL refused
// the index with `ER_BLOB_KEY_WITHOUT_LENGTH`, and the object landed
// registered-but-broken with the ActivityPointer lookup the read path
// assumes (ADR-0052 §5) silently absent.
record_id: Field.text({
label: 'Record ID',
required: false,
readonly: true,
searchable: true,
maxLength: 255,
// [#11339] The id half of the ActivityPointer pair (ADR-0052 §5): a
// record id of the object `object_name` names on the same row. Declaring
// it makes the pair seedable — a packaged app's seed writes the target's
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -226,11 +226,25 @@ export const SysAuditLog = ObjectSchema.create({
group: 'Target',
}),

// [#11374 route A] The bound is derived by referenced-column transitivity
// from the id this column holds, never guessed: `driver-sql` creates every
// table's primary key as `table.string('id').primary()` — knex's
// `varchar(255)`, which the driver spells out as
// `DEFAULT_STRING_VARCHAR_CHARS = 255` and names in its own error text as
// "built-in `id` (a varchar(255))". The writers enumerated below all stamp
// a real record id of a stored row, so none of them can produce a value
// wider than the column that id lives in. 255 is also <= the 768-character
// utf8mb4 key ceiling, so the `(object_name, record_id)` index below is
// expressible on MySQL — which is the whole point: unbounded, this column
// was emitted TEXT, MySQL refused the index with
// `ER_BLOB_KEY_WITHOUT_LENGTH`, and the object landed registered-but-broken
// with the lookup its own `record_views` list view depends on absent.
record_id: Field.text({
label: 'Record ID',
required: false,
readonly: true,
searchable: true,
maxLength: 255,
description: 'ID of the affected record',
// [#11386] The id half of this object's ActivityPointer pair (ADR-0052
// §5), adopting the #11339 carrier. VERIFIED for THIS object rather than
Expand Down
178 changes: 178 additions & 0 deletions packages/plugins/plugin-audit/src/plugin-keyed-text-bounds.test.ts
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,178 @@
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.

import { describe, it, expect } from 'vitest';
import { AuditPlugin } from './audit-plugin.js';

/**
* #11374 route A, for the objects THIS PLUGIN registers — every text-family
* column a declared index keys on must declare a `maxLength`, because a bound
* is what lets the column be a key at all.
*
* ## Why a second copy of the pin lives here
*
* The original pin is `@objectstack/platform-objects`'
* `platform-keyed-text-bounds.test.ts`, and it enumerates the objects THAT
* package exports. Platform objects that moved out to plugins under ADR-0029 K2
* are outside it by construction, which is exactly how `sys_activity.record_id`
* and `sys_audit_log.record_id` stayed unbounded through route A's sweep: the
* pin could not see them, so nothing failed by name.
*
* That is the same failure the platform pin already survived once at a smaller
* scale (it used to be scoped to `identity/`, and `sys_import_job.created_by`
* in `audit/` escaped it). A pin scoped to a package polices a package, not the
* defect class. The class-level repair — one walk over every package that ships
* platform objects — is engine-lane work tracked separately; until it lands,
* each shipping package carries its own copy so no keyed column is unpoliced.
*
* ## Why it drives `init()` instead of importing the objects
*
* This package's `package.json` declares only the `.` export and the root
* barrel does not re-export `./objects`, so nothing outside the package can
* import `SysActivity` at all — which is why the objects were never measured
* live. Enumerating a hand-written list here would reproduce that blind spot in
* miniature: the list, not the plugin, would define the surface. So the pin
* drives the REAL registration path (`AuditPlugin.init` → the `manifest`
* service's `register({ objects })`) and polices whatever the plugin actually
* contributes to a kernel. An object added to that call is policed the moment
* it is added, with no second edit here.
*
* ## What a red on this file means
*
* A new keyed text-family field arrived without a `maxLength`. Do not silence
* it — derive a bound from the value's producer and declare it (route A's
* shape: a NAMED producer, stated in the declaration so it is vetoable in
* review), or extend the allowlist with a comment naming why no bound exists
* and where the keyability debt is tracked.
*
* On MySQL the cost of a red is not theoretical: the unbounded column is
* emitted `TEXT`, `ALTER TABLE … ADD INDEX` is refused with
* `ER_BLOB_KEY_WITHOUT_LENGTH`, and the object lands registered-but-broken with
* its declared index silently absent.
*/

const TEXT_FAMILY = new Set(['text', 'textarea', 'html', 'markdown']);

/**
* Keyed text-family columns with NO defensible bound. Every entry must name
* why. Entries that stop matching a real keyed unbounded column fail the last
* test, so the list cannot rot. Empty today, deliberately: all four of this
* plugin's keyed text columns have a sourced bound.
*/
const UNBOUNDABLE: ReadonlySet<string> = new Set([]);

type AnyObject = {
name: string;
fields: Record<string, { type?: string; maxLength?: unknown }>;
indexes?: Array<{ fields?: string[]; unique?: boolean | string }>;
};

/**
* The objects `AuditPlugin` really contributes to a kernel, read off the
* manifest registration it performs in `init()`.
*/
async function registeredObjects(): Promise<AnyObject[]> {
const captured: AnyObject[] = [];
const noop = () => {};
const logger = {
info: noop, warn: noop, error: noop, debug: noop,
child() { return logger; },
};
const ctx = {
logger,
getService(name: string) {
if (name === 'manifest') {
return {
register(m: { objects?: AnyObject[] }) {
for (const o of m?.objects ?? []) captured.push(o);
},
};
}
return undefined;
},
registerService: noop,
hook: noop,
} as never;

await new AuditPlugin().init(ctx);
return captured;
}

function keyedTextColumns(o: AnyObject): Array<{ column: string; maxLength: unknown }> {
const keyed = new Set<string>();
for (const ix of o.indexes ?? []) for (const f of ix.fields ?? []) keyed.add(f);
return Object.entries(o.fields ?? {})
.filter(([name, def]) => keyed.has(name) && TEXT_FAMILY.has(def?.type ?? ''))
.map(([column, def]) => ({ column: `${o.name}.${column}`, maxLength: def.maxLength }));
}

describe('plugin-audit keyed text-family columns declare their bound (#11374 route A)', () => {
it('enumerates a real surface through the plugin registration path — the probe is not vacuous', async () => {
// Positive control: if `init()` stops registering objects, or the field /
// index spelling changes so this file stops seeing columns, fail loudly
// instead of passing empty. An empty enumeration is the failure mode that
// let these columns escape route A in the first place.
const objects = await registeredObjects();
expect(objects.map((o) => o.name)).toEqual(
expect.arrayContaining(['sys_audit_log', 'sys_activity', 'sys_comment']),
);

// 5 is MEASURED off this registration surface, not a round number:
// sys_audit_log.{object_name,record_id}, sys_activity.{object_name,record_id},
// sys_comment.thread_id. Every other index on these three objects keys on a
// lookup, select or datetime column, which is not text-family.
const all = objects.flatMap(keyedTextColumns);
expect(all.length).toBeGreaterThanOrEqual(5);
// Three names from THREE DIFFERENT objects, so a future narrowing of the
// enumeration fails here by name rather than by quietly enumerating less.
expect(all.map((c) => c.column)).toContain('sys_activity.record_id');
expect(all.map((c) => c.column)).toContain('sys_audit_log.record_id');
expect(all.map((c) => c.column)).toContain('sys_comment.thread_id');
});

it('every keyed text-family column declares a positive integer maxLength, or is allowlisted by name', async () => {
const objects = await registeredObjects();
const offenders: string[] = [];
for (const o of objects) {
for (const { column, maxLength } of keyedTextColumns(o)) {
if (UNBOUNDABLE.has(column)) continue;
const bounded =
typeof maxLength === 'number' && Number.isInteger(maxLength) && maxLength > 0;
if (!bounded) offenders.push(`${column} (maxLength: ${String(maxLength)})`);
}
}
expect(
offenders,
`keyed text-family column(s) without a declared maxLength — on MySQL their ` +
`declared index cannot be created and the object lands registered-but-broken. ` +
`Declare a sourced bound or extend UNBOUNDABLE with a named reason: ` +
offenders.join(', '),
).toEqual([]);
});

it('the two ActivityPointer id columns carry the referenced-column bound, not just any bound', async () => {
// The bound is not free-floating: 255 is the width of the physical `id`
// column `driver-sql` creates (`table.string('id').primary()`, knex's
// varchar(255) — the driver spells it `DEFAULT_STRING_VARCHAR_CHARS`), so a
// column holding a record id is bounded by transitivity from the id itself.
// Pinned by VALUE because a later edit that "tidies" one of these to a
// narrower sibling convention (100, as plugin-sharing and plugin-approvals
// chose) would silently make the column unable to hold ids that the id
// column itself accepts.
const byName = new Map((await registeredObjects()).map((o) => [o.name, o]));
expect(byName.get('sys_activity')?.fields.record_id?.maxLength).toBe(255);
expect(byName.get('sys_audit_log')?.fields.record_id?.maxLength).toBe(255);
});

it('the UNBOUNDABLE allowlist matches only real, still-unbounded keyed columns', async () => {
const real = new Map(
(await registeredObjects()).flatMap(keyedTextColumns).map((c) => [c.column, c.maxLength]),
);
for (const entry of UNBOUNDABLE) {
expect(real.has(entry), `allowlist entry ${entry} is not a keyed text column any more — remove it`).toBe(true);
expect(
real.get(entry),
`allowlist entry ${entry} now declares a bound — remove it from UNBOUNDABLE`,
).toBeUndefined();
}
});
});
Original file line numberDiff line numberDiff line change
Expand Up@@ -41,17 +41,79 @@ export const SysAudienceBindingSuggestion = ObjectSchema.create({
description: 'UUID of the suggestion row.',
}),

// [#11374 route A] Both key columns below declare a bound derived by
// referenced-column transitivity, and the producer is named per column so
// the derivation is vetoable in review rather than taken on trust. The pair
// is the object's declared unique key `(package_id, permission_set_name,
// anchor)`; unbounded, both were emitted TEXT, MySQL refused the index with
// `ER_BLOB_KEY_WITHOUT_LENGTH`, and the object landed registered-but-broken
// — the per-tenant uniqueness this table depends on silently absent.
//
// The transitivity is not an analogy here, it is the confirm path itself:
// `confirmAudienceBindingSuggestion` resolves the row by
// `find('sys_permission_set', { name: row.permission_set_name })` and, when
// that misses, materializes it via `upsertPackagePermissionSet(ql,
// declared.set, row.package_id)` — which writes these two values into
// `sys_permission_set.name` and `sys_permission_set.package_id`. So a
// suggestion is confirmable exactly when its two key values fit the columns
// `sys_permission_set` declares, and those columns are what bound these.

package_id: Field.text({
label: 'Package',
required: true,
readonly: true,
// Producer: the owning package's manifest id (`manifest.id`, or the
// `_packageId` the metadata layer stamps) — `collectDeclaredSuggestions`
// reads one of those two and `syncAudienceBindingSuggestions` writes it
// here verbatim. Bounded at 255 because the SAME boot pass writes the
// SAME value into `sys_permission_set.package_id` (maxLength: 255), and
// every landed column of this value class agrees: `sys_capability
// .package_id`, `sys_metadata.package_id`, `sys_metadata_commit
// .package_id`. A package id too wide for those cannot own a
// materialized permission set, so it cannot produce a confirmable
// suggestion either. Measured against the in-repo corpus, the longest
// real reverse-domain package id is 57 characters — the floor is cleared
// with room to spare.
maxLength: 255,
description: 'Owning package that ships the suggested permission set (ADR-0086 D3 provenance).',
}),

permission_set_name: Field.text({
label: 'Permission Set',
required: true,
readonly: true,
// Producer: the declared set's own `name` (spec `PermissionSetSchema
// .name`), written here as `d.set.name`. Bounded at 100 by
// referenced-column transitivity from `sys_permission_set.name`
// (maxLength: 100) — the column this value must resolve against, as this
// field's own description says and as the confirm path literally does.
//
// The ceiling is MEASURED at the write seam, not inferred from the
// declaration. On a real ObjectQL engine over a real SqlDriver, inserting
// a longer name into `sys_permission_set` is refused before the driver is
// reached:
// len 101 → ValidationError: API Name must be ≤ 100 characters (got 101)
// len 120 / 255 / 256 / 300 → the same refusal
// (`objectql`'s `record-validator.ts` `max_length` check). So no
// permission set whose name exceeds 100 characters can exist, and a
// suggestion naming one could never be confirmed:
// `confirmAudienceBindingSuggestion` answers `SuggestionStateError`
// ("Permission set '…' is not materialized in sys_permission_set").
// Bounding at 100 therefore refuses nothing that is storable today.
//
// 100 and not the 255 its `package_id` sibling takes: the two columns
// reference DIFFERENT columns and inherit their widths independently.
// The in-package precedent for exactly this shape is
// `sys_user_position.position` — "Position machine name (references
// sys_position.name)", maxLength 100 against `sys_position.name`'s 100.
//
// ⚠️ The bound is transitive, not intrinsic: `PermissionSetSchema.name`
// is `SnakeCaseIdentifierSchema`, which carries `.min(2)` and NO `.max()`,
// so the SPEC does not bound identifier length — every cap on this value
// class comes from the columns that store it. Filed separately; if
// `sys_permission_set.name` is ever widened, the pin beside this file is
// where this bound is re-derived rather than rediscovered on MySQL.
maxLength: 100,
description: 'Name of the suggested permission set (resolved against sys_permission_set at confirm time).',
}),

Expand Down
Loading
Loading