fix(mcp): match toolWhitelist per function, not per registered tool#2016
Open
MWest2020 wants to merge 1508 commits into
Open
fix(mcp): match toolWhitelist per function, not per registered tool#2016MWest2020 wants to merge 1508 commits into
MWest2020 wants to merge 1508 commits into
Conversation
…chival-transfer-hardening, dsar-escalation-and-dpia, semantic-object-handoff-engine); branch code superseded by squash merges
…rest of branch superseded)
… casts in UNION query
Nextcloud 34's app-management page inlines the raw app.svg and recolors it via 'fill: currentcolor' on the <svg> element. A fill carried on <path>/<style>/class beats the inherited value, so the icon rendered white-on-white and vanished from the list. Moving the fill to the <svg> element lets Nextcloud recolor the icon; visuals are unchanged everywhere the icon is shown on a dark/colored surface.
…currentcolor recoloring' (#341) from fix/app-icon-svg-level-fill into development
setSelfMetadata() now applies a client-supplied @self.geo onto the object entity via setGeo(). Unlike organisation/tmlo/folder this is plain descriptive geographical metadata that drives no security or lifecycle decision, so it is client-writable; an explicit null clears it. This is the backend for map widgets (CnObjectGeoWidget) that edit an object's location via a minimal PATCH of @self.geo only.
…e (map widget backend)' (#343) from feat/object-geo-metadata into development
The published css/index.css re-exported only the unscoped globals, and webpack pruned the ESM barrel's CSS import as side-effect-free, so any build resolving the npm dist shipped ZERO scoped SFC CSS. Components rendered data-v-* scope attributes with no matching rules and silently lost every scoped declaration — flex containers fell back to display: block. Fixed upstream in nextcloud-vue#171 (css/index.css now @imports the built stylesheet) and #173 (wrapped dashboard header actions stay right-aligned). Local builds masked the bug for a long time: they alias @conduction/nextcloud-vue to the sibling ../nextcloud-vue/src checkout and compile the SFCs (and their scoped styles) in-app. CI and production have no sibling, so they always resolved the dist — and always shipped the broken CSS. This app had no way to reproduce that: `useLocalLib` was hard-wired to `fs.existsSync(localLib)`, so a developer with a sibling checkout could never build what CI builds. Add the fleet-standard `USE_LOCAL_LIB=false` escape hatch.
`nextcloud/ocp` is a require-dev STUB package (NC API signatures for Psalm /
PHPStan). It was mapped in the RUNTIME `autoload.psr-4`:
"OCP\\": "vendor/nextcloud/ocp/OCP/",
"NCU\\": "vendor/nextcloud/ocp/NCU/"
and lib/AppInfo/Application.php requires vendor/autoload.php — so every request
that touched OpenRegister registered a psr-4 map sending `OCP\*` to the stubs.
The stubs are pinned `^31.0`. On this NC 34 instance that means core's
`OC\Settings\Manager` (which carries `#[\Override]` on `getAdminDelegatedSettings()`)
was compiled against the NC 31 `OCP\Settings\IManager`, which predates that
method. PHP 8.3 raises a COMPILE-TIME fatal:
OC\Settings\Manager::getAdminDelegatedSettings() has #[\Override] attribute,
but no matching parent method exists
That takes down the WHOLE instance, not just this app: `occ` dies, `occ app:list`
returns 0 apps, and every /apps/openregister/api/* route 404s while other apps'
endpoints 500. Observed today — pipelinq's dashboard lost all of its KPI and
chart data because the aggregation endpoints were unreachable.
Moves the stub mapping to `autoload-dev`, where static analysis still resolves it
and a `--no-dev` deploy cannot leak it into the runtime autoloader.
NOTE — this is only half the mitigation. `autoload-dev` is STILL baked into the
generated autoloader by a plain `composer install` (dev mode). In this dev
topology the app checkout IS the served app, so a routine `composer install` here
re-bricks the instance. Same landmine in hermiq and launchpad. A durable fix is
either always deploying `--no-dev`, or pinning `nextcloud/ocp` to the running NC
major so a leak is harmless.
…runtime
Moving the stub mapping to `autoload-dev` (previous commit) narrowed the failure
window but did not close it: `autoload-dev` is STILL baked into the generated
autoloader by a plain `composer install`. In this topology the app checkout IS
the served app, so a routine dev install would re-brick the instance — which is
exactly how it broke today.
The mapping turns out to be entirely redundant. Both analysers already discover
the stubs through their own mechanisms, neither of which consults Composer's
autoloader:
phpstan.neon: scanDirectories: - vendor/nextcloud/ocp
psalm.xml: <extraFiles><directory name="vendor/nextcloud/ocp" /></extraFiles>
plus an explicit <stubs> entry
Verified with the mapping removed:
- `composer dump-autoload` in DEV mode emits ZERO references to nextcloud/ocp
in autoload_psr4.php / autoload_static.php → OCP can no longer leak into the
runtime autoloader under ANY install mode.
- Psalm: 124 errors, ZERO of which mention OCP. All 96 UndefinedClass errors
are Doctrine\DBAL\* and Sabre\VObject\* (NC core 3rdparty), pre-existing and
unrelated. So OCP still resolves for static analysis.
This removes the failure mode rather than narrowing it, and needs no discipline
(`--no-dev`) and no coupling to the running NC major.
…rphans
Deleting a schema that still holds objects was broken in several ways. Diagnosed
live: a "Cow" schema with one object refused to delete (correctly, 409) but the
refusal was unreadable, there was no clean way to delete it, and the UI's
delete-then-unlink ordering corrupted the register on the way past.
## The cascade
`DELETE /api/schemas/{id}?deleteObjects=true` — audit + hard-delete every object,
drop the now-empty magic table(s), delete the schema. Mutually exclusive with
`force` (both -> 400 conflicting-delete-dispositions): force ORPHANS the objects,
deleteObjects REMOVES them; an ambiguous destructive intent is refused rather
than guessed. The no-flag path is byte-for-byte unchanged (409 + objectCount),
and `force` still behaves exactly as before for API clients.
Two phases, because DROP TABLE is DDL and implicitly commits on MySQL/MariaDB
(it cannot be rolled back): phase 1 is one transaction (audit + rows + schema);
phase 2 drops the tables post-commit, best-effort. A failed drop logs a WARNING
and returns success with `tableDropped: false` — it never fails a request whose
data work already succeeded, and never pretends to roll back what it cannot.
A schema can live in MULTIPLE registers, each with its own magic table, and the
guard counts across all of them — so the cascade iterates every one of them.
Dropping only the "current" table would leave behind orphans the guard had
already counted.
## Bugs found and fixed on the way
- `ObjectService::deleteObjectsBySchema()` was a THROWING STUB, so
`POST /api/bulk/{register}/{schema}/delete-objects` had been returning HTTP 500.
The MagicMapper method it needed was already fully implemented — it just was
never wired up. Both bulk routes work again; the misleadingly-named
`bulk#deleteSchema` (which deletes objects, not a schema) is marked @deprecated.
- `MagicMapper::dropTable()` NEVER WORKED. It called
`ConnectionAdapter::quoteIdentifier()`, which does not exist on this Nextcloud —
so it threw "Call to undefined method" on every single call. That is why deleted
schemas always left their magic table behind. Now quotes via the platform, the
pattern the rest of the codebase already uses.
- `SchemaMapper::delete()`'s "objects still attached" guard queried the RETIRED
`openregister_objects` blob table, which is always empty for magic-table
objects. It counted 0 and waved everything through — a dead guard. Re-pointed at
the magic tables (direct query: MagicStatisticsHandler injects SchemaMapper, so
injecting it back would be circular) and given an explicit `force` bypass that
only the `?force=true` disposition may pass.
The three callers behind that dead guard now genuinely refuse. Two of them —
SchemasToolProvider and SchemaTool — are LLM-invokable with no controller in
front: a model acting on an ambiguous instruction must not be able to orphan
data. TablesSchemaSyncService also refuses; it retires read-only mirrors whose
objects live in Nextcloud Tables, so the repaired guard counts 0 and retirement
proceeds unchanged — and if a magic table DOES hold rows, that is real data the
sync job has no mandate to destroy.
- `TablesSchemaSyncService::retireMissing()` unlinked a schema from its register
BEFORE deleting it, and left it unlinked when the delete threw — the exact
orphaned-schema corruption this change exists to close.
- `BulkController` passed `hardDelete` unnormalized (a query/form request delivers
the string "true") into a bool-typed parameter: a latent TypeError that would
have surfaced the moment the stub was fixed.
Live-verified: no-flag 409 unchanged; both flags 400; cascade leaves 0 rows,
0 tables, 0 schema; bulk endpoint 200; 0-object cascade fine.
Tests: 19 new (SchemaDeletionService, SchemaMapper guard, destroy safety).
PHPUnit/Psalm/PHPStan/PHPMD identical to the origin/development baseline — zero
new issues. Also fixed 57 pre-existing PHPCS errors across 8 untouched files.
…its objects, leaving no orphans
The previous commit removed the `OCP\` / `NCU\` psr-4 mapping from composer.json,
which is what let the nextcloud/ocp stubs shadow core's OCP and take the whole
instance down. But the mapping was NOT purely redundant, as I first thought: CI
runs PHPUnit standalone on php:8.3-cli with no Nextcloud, and tests/bootstrap.php
loads only vendor/autoload.php — so OCP\* had nowhere else to come from.
Move the registration to the test entry point, where it belongs: a dev-only
mapping in the dev-only bootstrap, instead of in composer.json where a plain
`composer install` bakes it into the autoloader of the SERVED app.
- CI / standalone: bootstrap registers the stubs when no live NC supplies OCP.
- Served app: composer's autoloader has no OCP mapping under ANY install mode,
so it can never shadow core.
- Static analysis: unaffected — PHPStan reads the stubs via `scanDirectories`,
Psalm via `<extraFiles>`.
Registered AFTER the Doctrine stubs: IQueryBuilder declares constants referencing
Doctrine\DBAL\ParameterType, evaluated as the file is parsed, so the placeholders
must already be in the class table.
Verified against a CI-identical run (php:8.3-cli, no Nextcloud), before vs after:
baseline (mapping in composer): 14058 tests, 115 errors, 17 failures
this change (bootstrap): 14058 tests, 115 errors, 17 failures
Byte-for-byte identical — the pre-existing failures are untouched and nothing
regressed.
…tubs at runtime — they were bricking the whole instance' (#346) from fix/ocp-stubs-not-runtime-autoload into development
…enBuild PAT custody) (#351)
…, trust-config repair Snapshot of uncommitted work that was sitting in the shared dev checkout, committed so it is captured in git (and pushable) before merging origin/development in. The working tree is byte-identical before and after this commit — nothing is rewritten. Contains: - PublicApiCorsMiddleware registration + #[AnonRateLimit] on object create, and the public-create RBAC path in PermissionHandler (+ tests) - lib/Repair/ImportTrustConfigurationRegister.php (new) - credential-provider catalogue additions, EditSchema.vue option enrichment Several of these files had already been re-derived upstream and are byte-identical to origin/development; the merge that follows reconciles them.
…ta190 # Conflicts: # package-lock.json
…cked change proposal(s) Derived from the 2026-07-12 Specter deep-research on scholiq (competitors, user wishes, flows, standards, Nextcloud ecosystem; evidence persisted in the intelligence DB, tag gap-2026-07-12). Changes: notification-delivery-windows
…(scholiq gap-wave 2026-07-12)' (#352) from specs/scholiq-gap-wave-20260712 into development
…der, McpTool attribute (ADR-063)
…, derived provider, McpTool attribute (ADR-063)' (#353) from wip/mcp-platform-abstraction-spec into development
…s, critical bypass Implements openspec change notification-delivery-windows: QueuedNotification table + mapper + migration, dispatcher gate that queues (never drops) suppressed non-critical notifications, per-rule fixed-time digest schedules, critical:true bypass, IANA timezone evaluation with server fallback, NotificationQueueFlushJob; removes dead BatchNotificationJob/NotificationDigest. Tests 14118->14180, 0 new errors/failures; hydra gates pass for touched code.
…able digest queue (wip/notification-delivery-windows)' (#354) from wip/notification-delivery-windows into development
Adds the declarative x-openregister-mcp dialect: registers the key in Schema::ANNOTATION_VOCABULARY so it folds into schema configuration on import, adds McpAnnotationValidator (shape/type checks for enabled, the fixed five-verb CRUD template, per-verb description/scope/hints, and search-only filters cross-checked against real properties), and wires validateMcpAnnotation() into SchemaMapper::cleanObject() after validateHandoffAnnotation(). Default OFF, opt-in per schema; emits no MCP tool and changes no serving surface (that lands in the follow-up or-mcp-derived-tool-provider change). Documents the dialect in docs/features/ai-and-mcp.md and the changelog.
Moves the completed change to openspec/changes/archive/2026-07-12-or-mcp-schema-dialect/ and syncs its spec delta (REQ-DIALECT-001/002/003) into openspec/specs/ai-mcp/spec.md, marking or-mcp-schema-dialect shipped in the ADR-063 in-flight-changes list while keeping or-mcp-derived-tool-provider and or-mcp-tool-attribute noted as in-progress.
…osed-and-vocabulary-drift' (#443) from wip/scratchpad-authz-report into development
…vider Hermiq needs to run a Claude Max/Pro subscription through the official `claude` CLI, which is the ToS-sanctioned path: Anthropic categorically refuses a subscription OAuth token on the direct Messages API (verified 2026-07-16 — HTTP 429 rate_limit_error carrying anthropic-organization-id but NO retry-after and NO anthropic-ratelimit-* counters, byte-identical after 14h of zero usage; a refusal, not a quota). A CLI reads its token from the process ENVIRONMENT. There is no outbound request for the broker to proxy and no header for injectAuth() to substitute into, so the constrained-proxy path cannot express this credential at any host — the host (api.anthropic.com) is perfectly well known. resolveInjectable() correctly returns null for the host-locked anthropic-oauth proxy provider, so today Hermiq cannot obtain the token at all. CredentialController::create() also rejects an unregistered provider with a 400, so a user cannot even save the credential. Adds anthropic-cli: inject_only, no baseUrl, no allowRules. Grouped with the other anthropic providers — both broker branches key on the flag (isInjectOnly()), never on position, so grouping is editorial and a Claude provider should not hide inside a block titled 'generic'. Also generalises $injectOnlyComment. It read as a narrative about OpenConnector's unbounded hosts and said 'the five entries', so a sixth inject-only entry elsewhere would silently falsify the reader's inference that inject_only implies generic-*. inject_only now states the general rule: the broker cannot bound this call, so it refuses to make it — which covers both an unbounded host and a non-HTTP consumer. No PHP: request() already denies inject-only providers and resolveInjectable() already releases only for them, both strictly after Guard 1 (owner/IDOR) and Guard 2 (allowedApps). The new entry inherits correct handling from the flag alone. Version 1.5.0 -> 1.6.0 (6473d7e already took 1.5.0 for the anthropic descriptors; the $injectOnlyComment's 1.4.0 narrative is stale). The trade-off is deliberate and bounded: an inject-only secret leaves OpenRegister into the trusted same-instance app. Forced (a CLI needs it in its env), precedented (the five generic-* entries), and bounded (both guards still run; Doriath keeps custody; app config holds only a credentialRef). Prefer the zero-knowledge anthropic proxy entry wherever it works. Claude Max/Pro is PERSONAL-SCOPE ONLY per the Anthropic ToS — declared here, enforced by the consuming app which owns the only resolution path.
…al provider' (#445) from feat/anthropic-cli-provider into development
…r trusted in-process callers (#450) ADR-064 Rule 4 requires an infrastructure credential (a source/consumer secret) to be organisation-scoped, never personal. But its trusted in-process consumers — openconnector's migration repair step (Phase C) and background sync jobs — run with NO user session, and the original design D3 "no session denies" rule left org-scoped infrastructure credentials unresolvable. Add a sessionless organisation assertion mirroring the existing actingUserId posture (honored ONLY without a session; never settable from request input): - resolveInjectable() gains ?string $actingOrganisationId as its last param, threaded through loadAdmittedCredential() into assertOrganisationMember(). - assertOrganisationMember() now admits on two paths: with a session the session is authoritative and the asserted value is IGNORED (a request-context caller can never escalate via the assertion); sessionless, it admits iff actingOrganisationId equals the credential's organisation. Matching keeps resolution decoupled from any individual user's membership (the point of org scope per ADR-064) — the actingUserId fallback is deliberately not reused. Trust boundary: resolveInjectable() is NOT HTTP-routed, so the assertion is only ever set by in-process code. The routed request() passes null explicitly (comment records the invariant), and CredentialController never reads it — mirroring how it already refuses to forward actingUserId. Tests: CredentialBrokerSessionlessOrganisationTest pins the full contract (match admits, mismatch/null/empty deny, session ignores a wrong/matching assertion in both directions). Mutation-verified: reverting the sessionless admit or dropping the === org match makes a test fail. Also fixes a pre-existing bootstrap gap — DoriathStubs.php (fixtures) was never required by bootstrap-unit-local.php, so DoriathCredentialStoreTest/CredentialStoreResolverTest errored; now the full tests/Unit/Service/Credential/ dir is green (105 tests). Follow-up (not in this change): openconnector's BrokeredCallService must pass actingOrganisationId when resolving org-scoped credentials sessionlessly.
…dation methods Gate 6 (orphan-auth) un-blinding (openregister#444) reports 14 orphan validation methods on origin/development. Triaged each; 4 are superseded by a proven live enforcement point and are removed (with their dead tests), 10 are non-auth value-object predicates / deferred opt-in validators left in place and documented. No method was a live unprotected action — every security-relevant orphan already had a fail-closed live check. Deleted (zero lib/src callers, superseding live check verified): - AuditHandler::validateObjectOwnership (+ private helpers) — LogService::getLogs re-validates register/schema inline behind requireAdmin() - DestructionService::validateDestructionList — DestructionExecutionJob re-checks legal holds fail-closed at execution - SearchQueryHandler::isSearchTrailsEnabled — getEffectiveRecordingMode supersedes - ValidationHandler::validateSchemaObjects — validateAndSaveObjectsBySchema is the live /api/objects/validate path tests/Unit unchanged: 14763 tests, 27 err/17 fail/23 skip (identical before/after).
Sync the orphan-auth-remediation delta into openspec/specs/ (status: done) and move the change to changes/archive/2026-07-16-orphan-auth-remediation/.
…453) from wip/orphan-auth-remediation-archive into development
…ontext (_rbac:false) CredentialBrokerService::mint()'s contract is explicit — "minting is not itself a guarded operation; authorization is the caller's" (the controller runs the provider-catalogue + organisation-admin gate before calling; a repair-step migration is itself system-trusted). But its create saveObject() did NOT pass _rbac:false, so the write was re-gated by RBAC and ran as whatever principal the caller had. A SESSIONLESS caller — an occ/repair migration folding an inline source secret into the broker (openconnector#151) — is the anonymous principal, which OR denies from creating objects, so mint() threw NotAuthorizedException and the migration could never complete. The controller path only worked because it happened to carry an authorized admin session. This also made mint() inconsistent with itself: its rollback delete already passes _rbac:false. Add _rbac:false + _multitenancy:false to the create so mint honours its own contract for every caller, sessionless included. Live-verified on an isolated NC34 instance: before this change the migration's mint failed with NotAuthorizedException (secret correctly left intact, Phase D gate stayed closed); after it, mint→verify→null completes end-to-end (migrated:1, the source's inline apikey nulled, a credentialRef written, and the sessionless resolveInjectable round-trip via openregister#450 matched). The existing mint tests stubbed saveObject() ignoring its arguments, which is why the gap shipped green — the added test asserts the _rbac:false argument, not the return value.
…ject in system context (_rbac:false)' (#454) from wip/mint-sessionless-rbac-bypass into development
…ntainer (fleet-wide silent-empty bug) (#455)
…o canonical specs Deterministic, comment-only repointer retargets @SPEC docblock anchors that pointed at archived change dirs (openspec/changes/<slug>/tasks.md#task-N) to their canonical openspec/specs/<cap>/spec.md[#requirement-<slug>] home. - 3,643 anchors repointed across 695 files (896 anchor-level, 2,747 file-level) - comment-only: 0 non-@SPEC changed lines / 7,030; every file ins==del (1:1) - gate-46 re-verify: OR broken 5,051 -> 1,408 - 1,408 residual-dangling anchors filed for human triage - tool + unit test committed under the change dir
…from wip/spec-anchor-repair into development
…aths
OR resolved `writeOnly` from top-level schema properties only. A secret
nested inside an untyped `object` property was therefore impossible to
protect: `writeOnly: true` is a JSON Schema keyword and can only attach to
a property the schema declares, and blanket-`writeOnly` on the parent
breaks the editors that legitimately read the rest of it back. Live
instances (openconnector#235, the openconnector#147 residual):
- source.configuration.authentication.{client_secret,username,password}
- rule.configuration.authentication.keys (apiKey -> userId impersonation map)
Both were returned in cleartext on every read, for everyone, with no way
to declare otherwise.
Declaration: a schema-level annotation listing dot-paths, e.g.
`x-openregister-writeonly-paths: ["configuration.authentication.keys"]`.
It composes with the existing x-openregister-* machinery and reuses the
dot-path vocabulary the relations mirror is already keyed by. A declared
path strips the value AND its whole sub-tree, which is what covers
`...authentication.keys` whose leaf segments are caller-supplied apiKeys
and cannot be enumerated in advance.
Enforcement rides the SAME hard render boundary as top-level writeOnly —
unconditional, admin included, NOT _rbac-gated (#389: an admin HTTP GET
renders with _rbac: false). Applied at every site that strips top-level
writeOnly, all of which funnel through
PropertyRbacHandler::stripWriteOnlyProperties():
- RenderObject::renderEntity() body strip + @self.relations mirror
- RenderObject::redactWriteOnlyFromRows() cheap list path (both row shapes)
- PropertyRbacHandler::filterReadableProperties() (property-authz path)
Gates now ask RenderObject::schemaHasWriteOnlyRule() so a caller cannot
check one spelling and forget the other. `_render: false` still bypasses
rendering entirely and returns the raw value (the credential migration and
CallService depend on it).
@self.relations: SaveObject::scanForRelations() flattens nested values into
LITERAL dot-path keys, so a nested secret is mirrored there under its full
path — the nested equivalent of the #429 gap. Stripped by exact key and by
`<path>.` prefix.
Validation fails loudly. This is the only configuration key exempt from
#419's per-key isolation: for every other key "drop the bad key, keep the
rest" degrades safely (the feature just doesn't fire), but for this one it
is fail-OPEN — the schema would save, look annotated to a reviewer, and
serve the secret. A malformed path, or one rooted at an undeclared property
(`configuratio.authentication.keys`), aborts the whole save.
Also fixes two pre-existing defects found on the way:
- testSystemContextReadIsNotRedacted asserted the PRE-#389 contract and
had been RED at HEAD, documenting a leak as intended behaviour.
- the canonical spec still said `_rbac: false` MUST return the secret,
which is what the stale test was pinned to. Split the bypass
requirement: property authorization.read bypasses, writeOnly never does.
Tests: 20 new (12 Schema validation, 8 render). Mutation-tested — removing
the nested strip fails 5 tests showing the plaintext across body, mirror,
_rbac:false, fields re-widen and cheap path. Zero new failures vs pristine
HEAD across tests/Unit/Db + tests/Unit/Service/Object (13 pre-existing,
1 fixed). phpcs unchanged vs HEAD (0 errors, 41 warnings).
Consumers: openconnector#235, openconnector#147. Neither is closed —
openconnector must still ADOPT the annotation for its rule/source schemas.
Refs openconnector#235, openconnector#147, openregister#389, openregister#429
…only-paths' (#459) from wip/nested-writeonly-paths into development
#460) An admin LIST/search read returned every `writeOnly` secret in cleartext. #389 established that writeOnly is a hard render-boundary rule that strips unconditionally, admins included, and hardened `RenderObject::renderEntity()`. Its sibling `redactWriteOnlyFromRows()` — the cheap list/search path — kept the pre-#389 bypass: if ($_rbac === false || SystemOperationContext::isActive() === true ...) return; Its comment claimed "same bypass as renderEntity's read-strip", which had been stale since #389 removed exactly that bypass. `ObjectsController` derives `$rbac = ($isAdmin === false)`, so an ADMIN list arrived here with `_rbac: false` and hit the early-return — the disclosure #389 was filed to close, still open on the list path. `_rbac: false` on an HTTP read means "this caller bypasses WHICH OBJECTS it may see", never "this caller may see secrets". Conflating those was the bug. Fix: mirror renderEntity's split. writeOnly (top-level and nested `x-openregister-writeonly-paths`, #459) strips unconditionally via the shared `schemaHasWriteOnlyRule()`/`stripWriteOnlyProperties()` choke point, including the `@self.relations` mirror (#429). Property `authorization.read` stripping and field decryption REMAIN `_rbac`/SystemOperationContext-gated — unchanged. Consumer investigation (the ocon#215/#226 regression risk): no consumer depends on the bypass. `redactWriteOnlyFromRows()` is reachable ONLY via `searchObjectsPaginated`; every internal caller (GraphQLResolver, ObjectsProvider, ContextRetrievalHandler, SearchController, SettingsController) passes `_rbac: true` or the default. The only `_rbac: false` callers are the three ObjectsController list sites, which serialize straight to a JSONResponse. The engines the comment named batch-read via `findAll()`/`find()`, which never enter this method; the sanctioned raw read stays `_render: false`. Nothing to migrate. Tests: the defect hid because tests exercised the helper (with `_rbac: true`, the one value that did strip) or mocked RenderObject out of the controller entirely. Adds ObjectsControllerWriteOnlyListLeakTest — the REAL controller list path with a REAL RenderObject, asserting on the serialized admin response (top-level, nested, and the relations mirror). Mutation-verified: restoring the early-return fails 6 tests, printing `"apiKey":"SECRET_APIKEY_MUST_NOT_LEAK"` in an admin list response. Inverts testCheapPathRowRedactionSkipsSystemContext, which pinned the leak as intended behaviour, and pins that authorization.read gating is untouched. Suites: 4682 tests (was 4675, +7); the 13 reds are byte-identical to an origin/development baseline — all pre-existing, none new. Refs #389, #429, #459. Closes #460.
Fixes three tool defects found by fleet-wide use: CRLF normalisation, raw fragment emission (produced anchors gate-46 still rejects), and the missing self-heal path for already-canonical targets. Adds regression tests for each. gate-46: 1411 -> 1408. Comment-only: every changed lib/src line is an @SPEC tag.
…y fleet-wide use (gate-46 1411 to 1408)' (#461) from wip/spec-anchor-repair-round2 into development
ToolRegistryFacade::listTools() intersected the whitelist against tool
REGISTRY ids while invokeTool() has always resolved a call against FUNCTION
ids (name or dotted mcpId). Consumers store function-level ids — an ADR-035
toolWhitelist, Hermiq's per-agent grants — so the two id spaces disagreed in
both directions:
- An entry naming a real function ("list_schemas" — exactly what Hermiq's
agent tool-catalog offers, and the id space of the 30 built-in
descriptors that carry no mcpId) matched NOTHING and silently resolved to
zero tools, indistinguishable from a tool-less agent.
- An entry naming a tool ("openregister.schema" — one tool, five functions)
handed over every function it owns, delete_schema included, defeating a
caller's read-only intent.
listTools() now applies the whitelist per function, accepting the same forms
invokeTool() does plus the owning tool's registry id (which still admits all
of that tool's functions, so existing coarse grants are unchanged).
Tests: the function-name case fails against the old facade; sibling
exclusion, registry-id compatibility, mcpId resolution and the unknown-entry
case are locked in. 16/16 green, phpcs + psalm clean.
MWest2020
requested review from
Rem-Dam,
SudoThijn,
WilcoLouwerse,
bbrands02,
remko48,
rjzondervan and
rubenvdlinde
as code owners
July 17, 2026 08:12
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Heropend na de migratie van Codeberg naar GitHub (2026-07-14).
Origineel, met review-historie: https://codeberg.org/Conduction/openregister/pulls/464