Repository navigation
Harden the instruction check and the model gateway's spend cap - #2586
Merged
Merged
Conversation
3 tasks done
pournasserian
added a commit
that referenced
this pull request
Oct 19, 2026
…fusal tests (#2617) Closes #2600 ## What Hardening from the review of #2558 (`SystemInitiator`, ADR-0012): low-severity gaps, none exploitable today, that future engine or plugin code could have used to act beyond what ADR-0012 allows. - **Reset links** (`IdentityPasswordReset.IssueAsync`): an agent or an engine is refused with `user_people_only`, as managing users already is. No token is issued and nothing is logged. - **Only Core's engines exist**: - `SystemInitiator`'s constructor is internal to Core. Code gets an engine from the named ones (`Checks`, `Retention`). - `InitiatorKinds.RestoreSystem` reads a stored row back only as one of the product's engines, with its own id; any other component name is refused as a fault. - **Engines are an allow-list for jobs**: - `EngineJobs` in Core lists each job handler that may run as an engine, with that engine. Today the check run is listed as `checks`. - The job worker asks it before it runs a handler, and fails the job with `job_engine_not_allowed`, without running it, when: - its handler names an engine it is not listed for, or - an engine enqueued the job and its handler names no engine. Code running inside an engine's scope, such as a check type, would otherwise pass the engine on through a job. - A listed handler runs as its own engine whoever enqueued its job. A handler that names no engine runs as whoever enqueued its job when that is a person or an agent, as before. - **Docs**: ADR-0012 states the allow-list, that a plugin's job handler never runs as an engine (EXT-03), and that check types, a plugin's included, run inside the check engine's scope, bounded by the engine's limits. It also corrects the list of what only a person does. CONTRIBUTING.md says the same for job handlers. - **Tests for refusals that had none**, with an engine or an agent as initiator: - grouping an operation into a session's set, at the directory level - disabling and enabling a user - revoking an agent credential - deactivating a check - issuing a reset link - an audit row and a job row stored with another engine's id, and a row naming no engine of the product, read back ## Why The review of #2558 found three gaps: - `IssueAsync` did not check its caller. - The worker ran a handler as any `SystemInitiator` its `RunAs` returned, and anyone could make one. - Several refusals had no test. Its first review here found two more: code running as an engine could pass the engine on through a job it enqueued, and reading a row could make up an engine. Nothing exploits these today, since only Core code runs, but ADR-0012's "no caller can borrow an engine's permissions" should hold for the code that comes later, plugins included. Choices worth a look: - **Keyed by the handler's type.** A plugin cannot make an object of Core's internal `CheckRunJob`, so it cannot pass as it. Code in the same process could still get around this through reflection; the allow-list stops a handler from claiming an engine, not code set on misuse. Per-plugin permissions come with the plugin SDK (EXT-03, M3). - **Fail, don't fall back.** A job refused this way fails rather than running as its enqueuer or as no one, so the mistake shows on the job instead of running with different permissions than its author expected. - **Check types still run in the engine's scope.** Running them as the agent whose set they judge would record the agent for what the engine does, which ADR-0012 rejects. The engine's own limits bound them. - **Later**: when routing publishes as an engine (F-RTE), jobs that publishing enqueues for handlers outside Core, such as webhook deliveries, would be refused by this rule; #2632 tracks how they should run. - **Merge after #2586.** ADR-0012's model-spend line describes `OnBehalfOf` from #2586. ## Tests Unit: - `EngineJobs`: - the check run runs as `checks` - a handler that names no engine runs as a person's or an agent's job - an unlisted handler that claims an engine is refused - a job an engine enqueued for a handler that names no engine is refused - `SystemInitiator`: one stable id per component, read back only for the product's engines; no public constructor. Integration, on SQLite and PostgreSQL: - A plugin handler that claims `checks` fails its job with `job_engine_not_allowed` and never runs. - A probe check type running as the check engine during a submission is refused a reset link. The job it enqueues for an unlisted handler fails unrun, and an agent is refused a reset link too. Nothing is logged as issued. - The refusals and mismatched rows listed above. - The check run still runs as the engine (`ChangeSetChecksTests`). The full suite ran locally against a `postgres:17` container as well as SQLite, and markdownlint passes. No new package and no migration. ## Checklist - [x] Linked to an issue a maintainer has confirmed - [x] Targets `v2` - [x] Tests or docs updated where behaviour changed
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
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.
Closes #2579
What
Hardening from the post-merge review of #2535, #2545 and #2551 (F-CHK #2420).
instructions, SEC-03):CheckContent.AllText): property names as well as values, and an entity's fact metadata, such as a source's reference, as well as its value.Freeto say it charges nothing. Otherwise every call costs $0 and no spend cap refuses one. The host stops at startup with what to change.ModelRequest.ChargedTobecomesOnBehalfOf, the person or agent whose work an engine asks the model for. An agent's call counts against its cap and a person's against none. The gateway refuses (model_call_unaccounted), recording and sending nothing:CheckInput.OnBehalfOf), so LLM-judged checks judge a person's set uncapped and an agent's against its cap. With the oldChargedTo, an engine judging a person's set would have been refused.Models__PromptRetentionmust be longer than the longest profile timeout.Free, who pays, retention, and what the instruction check reads.Why
The review found that the SEC-03 check could be bypassed with a key or a look-alike letter. It also found that the SEC-04 spend cap was silently off when prices were left out, that a missing usage block zeroed a call's cost, that system and initiator-less calls went uncapped, and that a purge could be undone by a call in flight.
Choices worth a look:
Free, not a default price. A made-up price would make caps refuse or allow at random. An endpoint on this machine, such as Ollama, needs neither.SystemInitiator.Retention(its own issue), and a spend lock across processes (Add the model gateway with profiles, recording, spend caps and the prompt store #2545 records that it works within one).Tests
Unit:
CheckRunnerpasses whose work it judges to the model.Integration, on SQLite and PostgreSQL where a database is involved:
Freeone and a local one do not, and a retention no longer than a profile's timeout stops it.The full suite ran locally against a
postgres:17container as well as SQLite, and markdownlint passes. No new package and no migration.Checklist
v2