diff --git a/docs/API-INPUT-VALIDATION.md b/docs/API-INPUT-VALIDATION.md new file mode 100644 index 000000000..8c6f756ed --- /dev/null +++ b/docs/API-INPUT-VALIDATION.md @@ -0,0 +1,140 @@ +# Operator API input validation + +**This page defines what the engine's operator API accepts for each kind of data item you send it.** +It covers the control plane: the ids, connection names, time bounds and search terms an operator or a +client sends to the engine. It does not cover message payloads. Those are the data plane, and they +have their own documents: [HL7-VALIDATION.md](HL7-VALIDATION.md) and [CODESETS.md](CODESETS.md). + +The rules live in one module, [`messagefoundry/api/validation.py`](../messagefoundry/api/validation.py). +Everything below is quoted from it, and `tests/test_api_input_validation.py` fails if this page and +that module ever disagree. + +A value that breaks one of these rules gets an HTTP 422 with the field named. The engine refuses it +before the value reaches a database query, a filesystem path, a log line or a CSV export. + +--- + +## The rules + +| Data item | What it may be | Where you send it | +|---|---|---| +| Resource id | Exactly 32 lowercase hex characters | `message_id`, `file_id`, `approval_id`, `preset_id`, `user_id`, and each entry of an export `ids` list | +| Digest id | Exactly 64 lowercase hex characters | `attachment_id`, `session_id` | +| Custom role id | `custom:` then 32 lowercase hex characters | `role_id` on the `/roles/custom` routes | +| Role id | A lowercase word, or a custom role id | Each entry of a `roles` list | +| Permission id | `area:action`, lowercase letters and underscores | Each entry of a `permissions` list | +| Connection name | A letter, then letters, digits, `_` and `-`, up to 256 characters | `{name}` in a path, and `channel_id`, `destination_name`, `to`, `source`, `connection` | +| Time bound | A finite number from 0 up to 4102444800 (2100-01-01 UTC) | `received_from`, `received_to`, `since`, `until` | +| Free text | Printable text, no control characters, up to 512 characters | `content`, `field_value` | +| Vocabulary token | Letters and underscores, up to 64 characters | `status`, and each `kind` on the event routes | +| Message type | Printable text, no control characters, up to 64 characters | `message_type` | +| Control id | Printable text, no control characters, up to 256 characters | `control_id`, and `actor` on the audit routes | +| HL7 field path | A three-character segment id, a field number, then optional component and subcomponent numbers, such as `PID-3` or `PID-5.1` | `field_path` | +| Email address | One `@`, a local part with no spaces, and a dotted domain, up to 254 characters | `recipient_override` | + +"Printable text, no control characters" means every character except the C0 range, DEL, and the C1 +range. In practice: no NUL, no tab, no carriage return, no line feed. + +Some lists have a length of their own. A message export may name at most 100000 ids explicitly, the +same ceiling as its `limit`. An events request may filter on at most 32 event kinds. A directory +group mapping, and a counter-reset request, may carry at most 1000 entries. + +**Which words and codes exist is not decided here.** The role names, the permission catalog and the +status vocabulary belong to the engine. These rules decide only the shape a value may take, so a +value that could not be a member is refused early and one that merely does not exist gets a 404. + +--- + +## Why the rules are drawn where they are + +**An id is minted by the engine, never typed by a person.** Every id above comes back to you in an +earlier response. So the rule can be exact, and being exact is what makes it useful: no `.`, `/`, `\` +or NUL survives it, which is why an id can never be read as a file path. The upload store already +shipped this rule for one id; the module generalizes that rule rather than writing a second one. + +**A connection name is wider than the VS Code extension allows, on purpose.** The extension's wizard +rejects a hyphen. Four connection names shipped in this repository contain one, so adopting the +extension's narrower rule would make four connections unreachable through the API. The rule here +admits them. What it still excludes earns its place: path characters, whitespace, control characters, +and the quoting characters a value would need to carry meaning into a URL or a query. + +**A time bound must be finite, and that was the gap.** A lower bound of zero does not exclude +infinity. Before this rule, `?received_from=inf` was accepted and reached a database query, and the +audit routes accepted `?since=nan` as well. A NaN bound is the worse of the two, because every +comparison against it is false, so the filter would return nothing rather than fail. + +**Free text can only be ruled on by what it must not contain.** A search term is whatever an operator +typed to find a patient, so no alphabet rule fits it. What does fit is the control characters. These +terms reach the search audit record, the application log and, through the audit export, a CSV file. A +carriage return or a line feed in one of them would forge a second record in any of the three. + +That rule costs one capability, and this page states it rather than hiding it: a search term can no +longer span an HL7 segment separator, because that separator is a carriage return. The console's +search box is a single-line input, so no shipped client could send one. + +**Two items keep a rule that is not a pattern, and the pattern in front of them does not replace it.** + +1. A reload `config_dir` is confined by an allow-list, because the loader executes Python from that + directory. The shape rule adds the NUL a path check can be truncated by. **The allow-list is still + the control.** +2. A log `level` is checked against the engine's own level names, which is why a wrong one gets a + 400. The shape rule only keeps an arbitrary-length string out of that error message. + +**One rule is documented here but enforced elsewhere.** The HL7 field path grammar belongs to +`messagefoundry.parsing.peek.parse_path`, and `messagefoundry.store.content_search.make_spec` applies +it at every point the API accepts a `field_path`. A malformed path is already a 400. Copying that +pattern into the API models would create a second definition of a rule that has one. + +--- + +## What these rules do not cover + +**The data plane is untouched.** A message body reaching the engine over MLLP, a file, TCP, HTTP or a +database poll is validated by the parsing and connector rules, not by anything on this page. The +edit-and-resubmit body is the one place a message body arrives over the API, and it deliberately +keeps a size bound and no alphabet rule: an HL7 v2 body is separated by carriage returns. + +**The web console declares its own bounds for the same items.** The console's `/ui` routes carry at +least 17 of their own parameter declarations for items this page governs, and they are hand-written +copies rather than references to this module. They can drift. Two of them are a different data item +that happens to share a name: the console's `received_from` and `received_to` are `datetime-local` +strings from a browser form, not the epoch numbers the engine API takes. + +**The console reaches some handlers in process, which skips this validation entirely.** The console +is mounted inside the engine and calls a set of handler callables directly rather than over HTTP. A +direct call runs no request validation. Where the console builds an engine request model, these rules +do apply. Where it calls a handler with plain values, they do not. + +**Several hundred response fields carry no rule, and they should not.** A response field is something +the engine emits, not something you send. It is not an input, so an unbounded response field is not a +gap. Counting one as a gap manufactures a number that cannot be closed. + +**These other input surfaces are not covered here.** The engine's inbound HTTP listener, the command +line, and the `connections.toml` file each accept operator input and each carry their own rules or +their own absence of rules. Establishing what they should be is separate work. + +**The engine does not yet enforce the connection-name rule at registration.** A connection registered +in code or in `connections.toml` under a name this page rejects would be created and then would not +be reachable through the API. Nothing in the shipped samples, harness or tests has such a name. + +--- + +## Two questions this page does not answer + +**Does the standard ask that rules be written down, or that they exist to be written?** This page and +the module behind it take the harder reading and do both. Whether the softer reading would also have +been acceptable is a method question. It is recorded in BACKLOG #1108 and is not settled here. + +**Does the existing HL7 and codeset documentation satisfy the same requirement for the data plane?** +That would make this a control-plane question rather than a whole-product one. It is the second open +question in that item, and this page deliberately does not fold the data plane into its answer. + +--- + +## The generated schema is not this document + +The engine can produce an OpenAPI schema, and that schema does carry every pattern above. It is off +by default, and turning it on would not be a documentation change. A schema lists types and +constraints. It does not say why a rule is drawn where it is, what it costs, or what it does not +cover, which is what the three sections above are for. Turning it on widens the network surface, so +leave `[api].expose_docs` at its default unless you have a separate reason. diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 55cc19edd..0f3bad85e 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -7528,6 +7528,24 @@ The refusal names the scheme it found. A base URL with no scheme fails here too, **Proposed work, by subject, all unallocated:** the operator-surface input-validation reference, scoped to the full input surface measured above rather than to `api/`; the engine-side connection-name grammar, adopting the rule the IDE already enforces at `ide/src/connectionWizardModel.ts:103` (note it has exactly ONE call site, `ide/src/connectionQuickInput.ts:112` -- the IDE's webview authoring path applies neither it nor the port range); the email-address validation rule at its three acceptance points, where `grep -rn EmailStr messagefoundry/` returns zero against a control of 26 `Field` uses in `auth_models.py`; the connector port-range rule the IDE and `[logging].forward_port` already enforce; and an input-rule doc-drift gate in the established house style, AST-walking `transports/` and the CLI as well as `api/` and the console routes, with a planted omission that must fire. Also owed: a record correction on the denominator, and a reconciliation note on the two readings applied to one section on one date. +**2026-09-04: THE CONTROL-PLANE BUILD SHIPPED. THE ITEM STAYS OPEN, because its closing act is not this seat's to perform** -- the recorded act is `scorecard-rescore`, which the ASVS Tracker performs in the vault, and only then does a Lander flip the banner. Two seats, neither of them the builder's. + +**What shipped.** `messagefoundry/api/validation.py` is the single authority for the operator API's control-plane input rules, and `docs/API-INPUT-VALIDATION.md` is the reference a deploying operator reads, linked from `docs/README.md` under "Security reviewers". Rules were decided and applied for every category this item names -- engine-minted ids, connection names, glob patterns (see below), epoch time bounds, and free-text search fields -- plus the role, permission and vocabulary values sitting beside them. `tests/test_api_input_validation.py` (24 tests) pins each pattern, sweeps the shipped connection names, and carries a doc-drift check with a doctored-text control that must fail. + +**Measured with ONE instrument, the app's own generated OpenAPI schema, at the parent commit and at the tip.** Route parameters, 101 both sides: carrying a structural rule 5 -> 65, length-bound-only 25 -> 4, **unbounded strings 36 -> 0**, uncapped arrays of unbounded strings 3 -> 0. Request-body properties across the 31 models reachable from a route, 92 both sides: carrying a structural rule 0 -> 40, length-bound-only 61 -> 27, unbounded strings 1 -> 0, uncapped or unbounded collections 8 -> 0. The earlier AST census that counted `Query(max_length=...)` kwargs was retired mid-task because it stopped answering the question once the constraints moved into the annotation. + +**Two measurements decided rules that a reasonable reader would otherwise have drawn wrong.** First, the connection-name grammar is deliberately WIDER than the IDE rule this item proposes adopting: 4 of the 108 distinct connection names in `samples/`, `harness/`, `tests/` and `messagefoundry/` carry a hyphen (`FILE-OUT_ACME_ADT`, `FILE-OUT_Coverage`, `FILE-OUT_EXAMPLE_ADT`, `FILE-OUT_Test_ADT`), and `ide/src/connectionWizardModel.ts`'s `^[A-Za-z][A-Za-z0-9_]*$` rejects all four, so adopting it verbatim would have made four shipped connections unreachable through the API. Second, `ge=0` does not exclude infinity: `?received_from=inf` was accepted and reached a store bind, and `/audit` declared no bound at all, so `?since=nan` was accepted -- a NaN bound is the worse of the two, because every comparison against it is false and the filter silently returns nothing. + +**One correction to this item's own proposed work.** It lists the email rule at "three acceptance points". Only ONE of the three is an operator-supplied address the API accepts as an address: `AlertTestEmailRequest.recipient_override`, which now carries an `EmailAddress` rule. `UserCreateRequest.email` and `UserUpdateRequest.email` were deliberately NOT changed, and the reason is measured: `tests/` fixtures use `bob@x`, `dr.who@x`, `j@x` and a whitespace-only value, all of which the shipped rule rejects. Whether a user's profile address must carry a dotted domain is a real question about intranet addressing, not a bug, and it needs deciding before the rule moves there. + +**The glob category has no subject on this surface, and that is the finding rather than a skipped limb.** A glob is a file-connector setting consumed at `messagefoundry/transports/file.py:776-778` (`directory.glob`/`rglob`); the only console-to-`connections.toml` write seam is `POST /connections/{name}/flag`, which carries `flagged` and `direction` and nothing else. `grep -rniE "pattern|glob|wildcard"` over `api/models.py` returns one unrelated hit. So the glob rule is a connection-configuration rule, and enforcing it belongs with the `connections.toml` surface. + +**What remains, stated so nobody reads the above as more than it is.** (a) The web console's `/ui` routes declare at least 17 of their own parameter bounds for items this module now governs; they are hand-written copies, they can drift, and two of them (`received_from`/`received_to`) are a DIFFERENT data item -- browser `datetime-local` strings, not epoch numbers. (b) The console reaches several engine handlers through the in-process seam rather than over HTTP, and a direct call runs no request validation at all; where it builds a request model the rules do apply, and two such sites were fixed here. (c) The engine does not enforce the connection-name rule at REGISTRATION, so a name authored outside the rule would be created and then be unreachable through the API; nothing shipped has such a name. (d) The inbound HTTP listener, the CLI's 153 `add_argument` calls, and `connections.toml` are untouched. (e) The 27 request-body properties still carrying a length bound alone are passwords, display names, message bodies and other free text where an alphabet rule would be wrong, not missing. + +**Two questions this build deliberately did not settle, both of them this item's own.** Whether the verb asks only that rules be written down or that they exist to be written -- the build took the harder reading and did both, which does not decide the method question. And whether the existing HL7 and codeset documentation discharges the verb for the DATA plane, which would make this a control-plane-only cell. The data plane is untouched here on purpose; `docs/API-INPUT-VALIDATION.md` names both questions in its own text so a reader cannot mistake the page for an answer to them. + +**A finding that generalises past this item: ADDING AN INPUT RULE CAN SILENTLY DISARM AN EXISTING TEST, and the near-miss here is worth the paragraph.** `packaging/messagefoundry-webconsole/tests/test_webui.py::test_error_banner_escapes_hostile_input` was the reflected-XSS regression guard for **every** rerender-with-error path in the console. It worked by posting a hostile role id, relying on `_validate_roles` echoing it back into the 400 detail, and asserting the banner escaped it. The new role-id rule refuses that value at `RolesUpdateRequest`, so it never reaches `_validate_roles` and the route renders its own fixed `"invalid input"`. **Two of that test's three assertions still passed** -- the 400 status and `hostile not in r.text` -- and only the escaped-form assertion went red. Deleting it, which is the obvious "fix", would have retired the guard for every other path as an unnoticed side effect of an unrelated change. It was instead split: the route test pins the stronger new property with a well-formed-but-unknown role id as the control that the other arm still runs, and the escaping is re-armed directly against `pages.user_detail_page`, where no upstream rule can disarm it. The same hazard applies to the seven other tests this build had to update, and to whoever does the console, transport-plane or CLI halves next: a test that reaches its subject THROUGH a value an input rule now rejects stops measuring its subject while still passing. + ## 1109. research an honest pass for ASVS 2.2.1 -- positive validation that does not sacrifice the tolerance the HL7 default exists to protect > ๐Ÿ”ข **Re-scored 2026-08-20 -> P2.** Value **7/10** ยท Difficulty **6/10** ยท _big bet_. Both limbs stand -- validation.strict ships False at config/models.py:667 and api/models.py carries no model_config across 84 models, against five extra=forbid declarations in config/models.py. Value 7 not 8: the 2.2.1 cell is graded at LEVEL 1, so rung 8's ASVS L3 Partial limb does not reach it, and an authenticated loopback-bound API silently ignoring unknown body keys is not rung 8's production blind spot with no workaround -- it is rung 7, a real gap an operator cannot close from outside the app. Difficulty 6 stands: two independent limbs, a method ruling on which clause binds an L1 requirement inside an L3 assessment, and an API-side positive-validation change that would reach the console and apiclient callers too. _(was 8/10 ยท 8/10.)_ diff --git a/docs/README.md b/docs/README.md index db3cd2c3d..b4a290607 100644 --- a/docs/README.md +++ b/docs/README.md @@ -87,6 +87,7 @@ catalog, and **[../CHANGELOG.md](../CHANGELOG.md)**, which is authoritative for | [../.github/SECURITY.md](../.github/SECURITY.md) | **Vulnerability disclosure policy.** Report here. | | [SECURITY.md](SECURITY.md) | Authentication and RBAC โ€” *not* the disclosure policy, despite the name. | | [PHI.md](PHI.md) | Where PHI can and cannot go, and what the engine guarantees. | +| [API-INPUT-VALIDATION.md](API-INPUT-VALIDATION.md) | What the operator API accepts for each kind of data item you send it โ€” ids, connection names, time bounds, search terms โ€” and which input surfaces those rules do *not* reach. Control plane only; message payloads are `HL7-VALIDATION.md`. | | [ASVS-L2-PHASE0-CHANGES.md](ASVS-L2-PHASE0-CHANGES.md) | ยง4 key/crypto inventory and ยง5 communications inventory, both CI-drift-guarded. ยง1โ€“ยง3 are a historical phase changelog. | | [Secure_Development_Standards.md](Secure_Development_Standards.md) | The standards the build process holds itself to. | | [SECURITY-LOOSENING.md](SECURITY-LOOSENING.md) | The inverse of a hardening guide: every `[security]` switch defaults to the protective position, and this is what moving one off it costs. | diff --git a/messagefoundry/api/app.py b/messagefoundry/api/app.py index e6648d36d..82e468098 100644 --- a/messagefoundry/api/app.py +++ b/messagefoundry/api/app.py @@ -191,6 +191,19 @@ require_step_up, ws_token, ) +from messagefoundry.api.validation import ( + MAX_EVENT_KINDS, + MAX_EXPORT_IDS, + ConnectionName, + ControlIdFilter, + DigestId, + EpochSeconds, + EventKindFilter, + LayeredPresetIds, + MessageTypeFilter, + ResourceId, + StatusFilter, +) # NOTE: the web console (messagefoundry_webconsole) is deliberately NOT imported at module scope # (ADR 0065 / Option B). It is a GUARDED import inside create_app's serve_ui tail (mounted via @@ -2024,7 +2037,7 @@ async def _dual_role_control( @app.post("/connections/{name}/start") async def start_connection( - name: str, + name: ConnectionName, request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_paced(Permission.CONNECTIONS_CONTROL)), @@ -2033,7 +2046,7 @@ async def start_connection( @app.post("/connections/{name}/stop") async def stop_connection( - name: str, + name: ConnectionName, request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_paced(Permission.CONNECTIONS_CONTROL)), @@ -2042,7 +2055,7 @@ async def stop_connection( @app.post("/connections/{name}/restart") async def restart_connection( - name: str, + name: ConnectionName, request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_paced(Permission.CONNECTIONS_CONTROL)), @@ -2053,7 +2066,7 @@ async def restart_connection( @app.post("/connections/{name}/flag") async def set_connection_flag( - name: str, + name: ConnectionName, req: ConnectionFlagRequest, request: Request, engine: Engine = Depends(_get_engine), @@ -2082,7 +2095,7 @@ async def set_connection_flag( @app.get("/connections/{name}/metadata", response_model=ConnectionMetadata) async def connection_metadata( - name: str, + name: ConnectionName, request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require(Permission.MONITORING_READ)), @@ -2132,7 +2145,7 @@ async def connection_metadata( @app.post("/connections/{name}/test", response_model=ConnectionTestResult) async def connection_test( - name: str, + name: ConnectionName, request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_paced(Permission.CONNECTIONS_TEST)), @@ -2181,7 +2194,7 @@ async def connection_test( @app.post("/connections/{name}/test-credential", response_model=ConnectionTestResult) async def connection_test_credential( - name: str, + name: ConnectionName, request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_paced(Permission.CONNECTIONS_TEST)), @@ -2250,7 +2263,7 @@ async def connection_test_credential( @app.post("/connections/{name}/purge", response_model=PurgeResult | PendingApprovalResponse) async def purge_connection( - name: str, + name: ConnectionName, response: Response, request: Request, engine: Engine = Depends(_get_engine), @@ -2379,9 +2392,9 @@ async def list_connection_events( request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require(Permission.MONITORING_READ)), - connection: str | None = Query(None, max_length=256), - kind: list[str] | None = Query(None), - since: float | None = Query(None, ge=0), + connection: ConnectionName | None = Query(None), + kind: list[EventKindFilter] | None = Query(None, max_length=MAX_EVENT_KINDS), + since: EpochSeconds | None = Query(None), limit: int = Query(100, ge=1, le=1000), ) -> list[ConnectionEventInfo]: """The Corepoint-style connection/transport event log (#46), newest first โ€” **metadata only, @@ -2403,12 +2416,12 @@ async def list_connection_events( @app.get("/connections/{name}/events", response_model=list[ConnectionEventInfo]) async def list_connection_events_for( - name: str, + name: ConnectionName, request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require(Permission.MONITORING_READ)), - kind: list[str] | None = Query(None), - since: float | None = Query(None, ge=0), + kind: list[EventKindFilter] | None = Query(None, max_length=MAX_EVENT_KINDS), + since: EpochSeconds | None = Query(None), limit: int = Query(100, ge=1, le=1000), ) -> list[ConnectionEventInfo]: """The connection/transport event log scoped to one connection (#46), newest first.""" @@ -2729,8 +2742,8 @@ async def list_dead_letters( request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_phi_read(Permission.MESSAGES_READ)), - channel_id: str | None = Query(None, max_length=256), - destination_name: str | None = Query(None, max_length=256), + channel_id: ConnectionName | None = Query(None), + destination_name: ConnectionName | None = Query(None), limit: int = Query(50, ge=1, le=500), offset: int = Query(0, ge=0), ) -> DeadLetterList: @@ -2823,7 +2836,7 @@ async def list_approvals( @app.post("/approvals/{approval_id}/approve", response_model=ApprovalDecisionResult) async def approve_action( - approval_id: str, + approval_id: ResourceId, request: Request, identity: Identity = Depends(require_paced(Permission.APPROVALS_APPROVE)), gate: ApprovalGate | None = Depends(_get_gate), @@ -2842,7 +2855,7 @@ async def approve_action( @app.post("/approvals/{approval_id}/reject", response_model=ApprovalDecisionResult) async def reject_action( - approval_id: str, + approval_id: ResourceId, request: Request, identity: Identity = Depends(require_paced(Permission.APPROVALS_APPROVE)), gate: ApprovalGate | None = Depends(_get_gate), @@ -3019,12 +3032,12 @@ async def list_messages( request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_phi_read(Permission.MESSAGES_READ)), - channel_id: str | None = Query(None, max_length=256), - status: str | None = Query(None, max_length=64), - message_type: str | None = Query(None, max_length=64), - control_id: str | None = Query(None, max_length=256), - received_from: float | None = Query(None, ge=0), - received_to: float | None = Query(None, ge=0), + channel_id: ConnectionName | None = Query(None), + status: StatusFilter | None = Query(None), + message_type: MessageTypeFilter | None = Query(None), + control_id: ControlIdFilter | None = Query(None), + received_from: EpochSeconds | None = Query(None), + received_to: EpochSeconds | None = Query(None), limit: int = Query(50, ge=1, le=500), offset: int = Query(0, ge=0), ) -> MessageList: @@ -3171,10 +3184,10 @@ async def search_messages_get( identity: Identity = Depends(require_step_up(Permission.MESSAGES_READ)), field_path: str | None = Query(None, max_length=32), target: str = Query("both", pattern="^(raw|summary|both)$"), - channel_id: str | None = Query(None, max_length=256), - status: str | None = Query(None, max_length=64), - message_type: str | None = Query(None, max_length=64), - control_id: str | None = Query(None, max_length=256), + channel_id: ConnectionName | None = Query(None), + status: StatusFilter | None = Query(None), + message_type: MessageTypeFilter | None = Query(None), + control_id: ControlIdFilter | None = Query(None), limit: int = Query(50, ge=1, le=500), scan_limit: int = Query(DEFAULT_CONTENT_SCAN_LIMIT, ge=1, le=MAX_CONTENT_SCAN_LIMIT), ) -> MessageSearchResults: @@ -3345,13 +3358,13 @@ async def export_messages_get( identity: Identity = Depends( require_step_up(Permission.MESSAGES_EXPORT, Permission.MESSAGES_VIEW_RAW) ), - ids: list[str] = Query(default=[]), # noqa: B006 โ€” FastAPI repeated ?ids= (save-selected) + ids: list[ResourceId] = Query(default=[], max_length=MAX_EXPORT_IDS), # noqa: B006 โ€” FastAPI repeated ?ids= field_path: str | None = Query(None, max_length=32), target: str = Query("both", pattern="^(raw|summary|both)$"), - channel_id: str | None = Query(None, max_length=256), - status: str | None = Query(None, max_length=64), - message_type: str | None = Query(None, max_length=64), - control_id: str | None = Query(None, max_length=256), + channel_id: ConnectionName | None = Query(None), + status: StatusFilter | None = Query(None), + message_type: MessageTypeFilter | None = Query(None), + control_id: ControlIdFilter | None = Query(None), limit: int = Query(1000, ge=1, le=100_000), scan_limit: int = Query(DEFAULT_CONTENT_SCAN_LIMIT, ge=1, le=MAX_CONTENT_SCAN_LIMIT), ) -> StreamingResponse: @@ -3406,7 +3419,7 @@ async def export_messages_post( @app.get("/messages/{message_id}", response_model=MessageDetail) async def get_message( - message_id: str, + message_id: ResourceId, request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_phi_read(Permission.MESSAGES_VIEW_RAW)), @@ -3499,8 +3512,8 @@ async def get_message( @app.get("/messages/{message_id}/attachments/{attachment_id}") async def download_attachment( - message_id: str, - attachment_id: str, + message_id: ResourceId, + attachment_id: DigestId, request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_phi_read(Permission.MESSAGES_VIEW_RAW)), @@ -3583,7 +3596,7 @@ async def download_attachment( @app.get("/messages/{message_id}/responses", response_model=MessageResponses) async def get_message_responses( - message_id: str, + message_id: ResourceId, request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_phi_read(Permission.MESSAGES_READ)), @@ -3636,7 +3649,7 @@ async def get_message_responses( @app.get("/messages/{message_id}/outbound", response_model=OutboundPayloads) async def get_message_outbound( - message_id: str, + message_id: ResourceId, request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_phi_read(Permission.MESSAGES_VIEW_RAW)), @@ -3680,7 +3693,7 @@ async def get_message_outbound( @app.post("/messages/{message_id}/replay", response_model=ReplayResult) async def replay_message( - message_id: str, + message_id: ResourceId, request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_step_up(Permission.MESSAGES_REPLAY)), @@ -3712,7 +3725,7 @@ async def replay_message( @app.post("/messages/{message_id}/resend", response_model=ResendResult) async def resend_message( - message_id: str, + message_id: ResourceId, body: ResendRequest, request: Request, engine: Engine = Depends(_get_engine), @@ -3796,7 +3809,7 @@ async def resend_message( @app.post("/messages/{message_id}/edit-resend", response_model=EditResendResult) async def edit_resend_message( - message_id: str, + message_id: ResourceId, body: EditResendRequest, request: Request, engine: Engine = Depends(_get_engine), @@ -4273,13 +4286,13 @@ async def browse_uploaded_file( @app.get("/uploads/{file_id}/messages", response_model=UploadedMessagesResult) async def browse_uploaded_file_get( request: Request, - file_id: str, + file_id: ResourceId, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_step_up(Permission.FILES_BROWSE)), field_path: str | None = Query(None, max_length=32), target: str = Query("both", pattern="^(raw|summary|both)$"), - message_type: str | None = Query(None, max_length=64), - control_id: str | None = Query(None, max_length=256), + message_type: MessageTypeFilter | None = Query(None), + control_id: ControlIdFilter | None = Query(None), limit: int = Query(50, ge=1, le=500), offset: int = Query(0, ge=0), ) -> UploadedMessagesResult: @@ -4305,7 +4318,7 @@ async def browse_uploaded_file_get( @app.post("/uploads/{file_id}/messages/search", response_model=UploadedMessagesResult) async def browse_uploaded_file_post( request: Request, - file_id: str, + file_id: ResourceId, criteria: UploadedMessageSearchRequest, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_step_up(Permission.FILES_BROWSE)), @@ -4331,7 +4344,7 @@ async def browse_uploaded_file_post( @app.post("/uploads/{file_id}/resend", response_model=UploadResendResult) async def resend_uploaded_message( request: Request, - file_id: str, + file_id: ResourceId, body: UploadResendRequest, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_step_up(Permission.FILES_BROWSE)), @@ -4395,7 +4408,7 @@ async def resend_uploaded_message( @app.delete("/uploads/{file_id}", response_model=UploadDeleteResult) async def delete_uploaded_file( request: Request, - file_id: str, + file_id: ResourceId, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_step_up(Permission.FILES_DELETE)), ) -> UploadDeleteResult: @@ -4504,7 +4517,7 @@ async def create_search_preset( @app.delete("/search/presets/{preset_id}", response_model=SearchPresetDeleteResult) async def delete_search_preset( - preset_id: str, + preset_id: ResourceId, request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require(Permission.MESSAGES_READ)), @@ -4528,7 +4541,7 @@ async def layered_search( request: Request, engine: Engine = Depends(_get_engine), identity: Identity = Depends(require_step_up(Permission.MESSAGES_READ)), - presets: str = Query(..., max_length=1024), + presets: LayeredPresetIds = Query(..., max_length=1024), limit: int = Query(50, ge=1, le=500), scan_limit: int = Query(DEFAULT_CONTENT_SCAN_LIMIT, ge=1, le=MAX_CONTENT_SCAN_LIMIT), ) -> MessageSearchResults: diff --git a/messagefoundry/api/auth_models.py b/messagefoundry/api/auth_models.py index 0b296127d..4ad23e28b 100644 --- a/messagefoundry/api/auth_models.py +++ b/messagefoundry/api/auth_models.py @@ -6,8 +6,17 @@ from pydantic import BaseModel, Field +from messagefoundry.api.validation import ( + MAX_MAP_ENTRIES, + ConnectionName, + PermissionId, + RoleId, +) + # Upper bounds on free-text request fields (API-INPUT): reject absurd inputs before they reach the # store or argon2. Generous vs any legitimate value; the password cap also bounds argon2 work. +# The ITEM rules for the id-shaped lists below live in `api/validation.py`, with the rest of the +# operator API's input rules (BACKLOG #1108, docs/API-INPUT-VALIDATION.md). _NAME_MAX = 256 _PASSWORD_MAX = 1024 _GROUP_MAX = 512 @@ -83,7 +92,7 @@ class UserPermissions(BaseModel): class ChannelScope(BaseModel): """A user's per-channel RBAC scope. ``None`` = all channels; a list = exactly those connections.""" - channels: list[str] | None = Field(default=None, max_length=512) + channels: list[ConnectionName] | None = Field(default=None, max_length=512) class UserCreateRequest(BaseModel): @@ -91,7 +100,7 @@ class UserCreateRequest(BaseModel): password: str = Field(max_length=_PASSWORD_MAX) display_name: str | None = Field(default=None, max_length=_NAME_MAX) email: str | None = Field(default=None, max_length=_NAME_MAX) - roles: list[str] = Field(default=[], max_length=64) + roles: list[RoleId] = Field(default=[], max_length=64) class UserUpdateRequest(BaseModel): @@ -101,7 +110,7 @@ class UserUpdateRequest(BaseModel): class RolesUpdateRequest(BaseModel): - roles: list[str] = Field(max_length=64) + roles: list[RoleId] = Field(max_length=64) class PasswordChangeRequest(BaseModel): @@ -189,7 +198,7 @@ class CustomRoleRequest(BaseModel): display_name: str = Field(max_length=_NAME_MAX) description: str | None = Field(default=None, max_length=_NAME_MAX) - permissions: list[str] = Field(max_length=64) + permissions: list[PermissionId] = Field(max_length=64) class CustomRoleInfo(BaseModel): @@ -205,7 +214,7 @@ class AdGroupMapEntry(BaseModel): class AdGroupMap(BaseModel): - entries: list[AdGroupMapEntry] + entries: list[AdGroupMapEntry] = Field(max_length=MAX_MAP_ENTRIES) class AdGroupScopeEntry(BaseModel): @@ -216,7 +225,7 @@ class AdGroupScopeEntry(BaseModel): class AdGroupScopeMap(BaseModel): - entries: list[AdGroupScopeEntry] + entries: list[AdGroupScopeEntry] = Field(max_length=MAX_MAP_ENTRIES) class AuditEntry(BaseModel): diff --git a/messagefoundry/api/auth_routes.py b/messagefoundry/api/auth_routes.py index b01d3055c..f9953da0f 100644 --- a/messagefoundry/api/auth_routes.py +++ b/messagefoundry/api/auth_routes.py @@ -67,6 +67,14 @@ require_step_up, require_step_up_action, ) +from messagefoundry.api.validation import ( + ActionFilter, + ActorFilter, + CustomRoleId, + DigestId, + EpochSeconds, + ResourceId, +) from messagefoundry.auth import ( BUILTIN_ROLE_PERMISSIONS, ROLE_METADATA, @@ -480,7 +488,7 @@ async def my_security_events( @app.delete("/me/sessions/{session_id}", response_model=SimpleMessage) async def revoke_my_session( - session_id: str, + session_id: DigestId, service: AuthService = Depends(_service), # 7.5.2 (ASVS): terminating a session needs a fresh PASSWORD re-proof BOUND TO THIS ACTION # (BACKLOG #1149) โ€” single-use, so the login-seeded window no longer satisfies it. Still the @@ -579,7 +587,7 @@ async def create_custom_role( @app.put("/roles/custom/{role_id}", response_model=CustomRoleInfo) async def update_custom_role( - role_id: str, + role_id: CustomRoleId, body: CustomRoleRequest, service: AuthService = Depends(_service), identity: Identity = Depends(require_step_up(Permission.USERS_MANAGE)), @@ -605,7 +613,7 @@ async def update_custom_role( @app.delete("/roles/custom/{role_id}", response_model=SimpleMessage) async def delete_custom_role( - role_id: str, + role_id: CustomRoleId, service: AuthService = Depends(_service), identity: Identity = Depends(require_step_up(Permission.USERS_MANAGE)), ) -> SimpleMessage: @@ -628,7 +636,7 @@ async def list_users( @app.get("/users/{user_id}/permissions", response_model=UserPermissions) async def get_user_permissions( - user_id: str, + user_id: ResourceId, service: AuthService = Depends(_service), _: Identity = Depends(require(Permission.USERS_READ)), ) -> UserPermissions: @@ -678,7 +686,7 @@ async def create_user( @app.patch("/users/{user_id}", response_model=SimpleMessage) async def update_user( - user_id: str, + user_id: ResourceId, body: UserUpdateRequest, service: AuthService = Depends(_service), # 7.5.1 (ASVS): the one broad-admin route promoted to ACTION-binding (fresh single-use grant @@ -720,7 +728,7 @@ async def update_user( @app.delete("/users/{user_id}", response_model=SimpleMessage) async def delete_user( - user_id: str, + user_id: ResourceId, service: AuthService = Depends(_service), identity: Identity = Depends(require_step_up(Permission.USERS_MANAGE)), ) -> SimpleMessage: @@ -737,7 +745,7 @@ async def delete_user( @app.delete("/users/{user_id}/sessions", response_model=SimpleMessage) async def admin_revoke_user_sessions( - user_id: str, + user_id: ResourceId, service: AuthService = Depends(_service), identity: Identity = Depends(require_step_up(Permission.USERS_MANAGE)), ) -> SimpleMessage: @@ -749,7 +757,7 @@ async def admin_revoke_user_sessions( @app.put("/users/{user_id}/roles", response_model=SimpleMessage) async def set_user_roles( - user_id: str, + user_id: ResourceId, body: RolesUpdateRequest, service: AuthService = Depends(_service), identity: Identity = Depends(require_step_up(Permission.USERS_MANAGE)), @@ -771,7 +779,7 @@ async def set_user_roles( @app.post("/users/{user_id}/reset-password", response_model=PasswordResetResponse) async def reset_user_password( - user_id: str, + user_id: ResourceId, service: AuthService = Depends(_service), # BACKLOG #1148 (ASVS 7.5.1): the proof must be BOUND TO THIS ACTION and single-use, not # the login-seeded window. require_step_up_ACTION, never the reauth_only variant -- that @@ -803,7 +811,7 @@ async def reset_user_password( @app.post("/users/{user_id}/reset-mfa", response_model=SimpleMessage) async def reset_user_mfa( - user_id: str, + user_id: ResourceId, service: AuthService = Depends(_service), # BACKLOG #1148 (ASVS 7.5.1). This is the sharper of the two: one call clears the TOTP # secret, every recovery code and every passkey on the target account. @@ -853,7 +861,7 @@ async def reset_user_mfa( @app.get("/users/{user_id}/channel-scope", response_model=ChannelScope) async def get_channel_scope( - user_id: str, + user_id: ResourceId, service: AuthService = Depends(_service), _: Identity = Depends(require(Permission.USERS_MANAGE)), ) -> ChannelScope: @@ -864,7 +872,7 @@ async def get_channel_scope( @app.put("/users/{user_id}/channel-scope", response_model=SimpleMessage) async def set_channel_scope( - user_id: str, + user_id: ResourceId, body: ChannelScope, service: AuthService = Depends(_service), identity: Identity = Depends(require_step_up(Permission.USERS_MANAGE)), @@ -957,12 +965,12 @@ async def list_audit( service: AuthService = Depends(_service), _: Identity = Depends(require(Permission.AUDIT_READ)), limit: int = Query(100, ge=1, le=1000), - actor: str | None = Query(None, max_length=256), - action: str | None = Query(None, max_length=128), - since: float | None = Query( + actor: ActorFilter | None = Query(None), + action: ActionFilter | None = Query(None), + since: EpochSeconds | None = Query( None, description="inclusive lower bound on the epoch-float ts" ), - until: float | None = Query( + until: EpochSeconds | None = Query( None, description="inclusive upper bound on the epoch-float ts" ), ) -> AuditList: @@ -985,12 +993,12 @@ async def export_audit( identity: Identity = Depends(require(Permission.AUDIT_EXPORT)), format: str = Query("csv", pattern="^csv$"), limit: int = Query(10000, ge=1, le=1_000_000), - actor: str | None = Query(None, max_length=256), - action: str | None = Query(None, max_length=128), - since: float | None = Query( + actor: ActorFilter | None = Query(None), + action: ActionFilter | None = Query(None), + since: EpochSeconds | None = Query( None, description="inclusive lower bound on the epoch-float ts" ), - until: float | None = Query( + until: EpochSeconds | None = Query( None, description="inclusive upper bound on the epoch-float ts" ), ) -> StreamingResponse: diff --git a/messagefoundry/api/models.py b/messagefoundry/api/models.py index 4f8f7a929..3621d25a6 100644 --- a/messagefoundry/api/models.py +++ b/messagefoundry/api/models.py @@ -22,6 +22,21 @@ from pydantic import BaseModel, Field from messagefoundry.api.phi_gate import PhiGatedModel +from messagefoundry.api.validation import ( + MAX_EXPORT_IDS, + MAX_MAP_ENTRIES, + ConnectionName, + ControlIdFilter, + DisplayLabel, + EmailAddress, + FilesystemPath, + IdempotencyKey, + LogLevelName, + MessageTypeFilter, + ResourceId, + SearchText, + StatusFilter, +) from messagefoundry.config.ai_policy import ( AiDataScope, AiMode, @@ -97,16 +112,21 @@ class MessageSearchRequest(BaseModel): :mod:`messagefoundry.store.content_search`, and importing them here would drag the engine into every process that imports these models (ADR 0088 keeps the apiclient engine-free), so the route resolves the default and enforces the ceiling instead of the field doing it. + + ``field_path`` carries a length bound only, and deliberately so: its grammar is + :func:`messagefoundry.parsing.peek.parse_path`, applied eagerly by + :func:`messagefoundry.store.content_search.make_spec` at every acceptance point, so a malformed + path is already a 4xx. A pattern here would be a second definition of a rule that has one. """ - content: str | None = Field(None, max_length=512) + content: SearchText | None = None field_path: str | None = Field(None, max_length=32) - field_value: str | None = Field(None, max_length=512) + field_value: SearchText | None = None target: Literal["raw", "summary", "both"] = "both" - channel_id: str | None = Field(None, max_length=256) - status: str | None = Field(None, max_length=64) - message_type: str | None = Field(None, max_length=64) - control_id: str | None = Field(None, max_length=256) + channel_id: ConnectionName | None = None + status: StatusFilter | None = None + message_type: MessageTypeFilter | None = None + control_id: ControlIdFilter | None = None limit: int = Field(50, ge=1, le=500) scan_limit: int | None = Field(None, ge=1) @@ -116,7 +136,7 @@ class MessageExportRequest(MessageSearchRequest): console's *save-selected*) beside the inherited search criteria (*save-all*), and raises ``limit`` to the export route's own ceiling.""" - ids: list[str] = Field(default_factory=list) + ids: list[ResourceId] = Field(default_factory=list, max_length=MAX_EXPORT_IDS) limit: int = Field(1000, ge=1, le=100_000) @@ -125,12 +145,12 @@ class UploadedMessageSearchRequest(BaseModel): browse takes no channel/status filter and pages with ``offset``, so it is a sibling of :class:`MessageSearchRequest` rather than a subclass of it.""" - content: str | None = Field(None, max_length=512) + content: SearchText | None = None field_path: str | None = Field(None, max_length=32) - field_value: str | None = Field(None, max_length=512) + field_value: SearchText | None = None target: Literal["raw", "summary", "both"] = "both" - message_type: str | None = Field(None, max_length=64) - control_id: str | None = Field(None, max_length=256) + message_type: MessageTypeFilter | None = None + control_id: ControlIdFilter | None = None limit: int = Field(50, ge=1, le=500) offset: int = Field(0, ge=0) @@ -231,12 +251,13 @@ class ResendRequest(BaseModel): ``to`` is the alternate outbound connection; ``source`` (optional) names which delivered destination's stored body to copy when the origin fanned out to several (omit when there was one). - ``idempotency_key`` makes a retry a no-op โ€” a *new* key is a genuine second resend. Values are - bounded so an over-long name can't reach a store query (ASVS 1.3.3).""" + ``idempotency_key`` makes a retry a no-op โ€” a *new* key is a genuine second resend. Values carry + the connection-name rule (BACKLOG #1108), so an over-long or structurally impossible name is + refused before it can reach a store query (ASVS 1.3.3, 2.1.1).""" - to: str = Field(min_length=1, max_length=256) # the alternate outbound connection - idempotency_key: str = Field(min_length=1, max_length=256) - source: str | None = Field(None, max_length=256) # source delivery to copy the body from + to: ConnectionName # the alternate outbound connection + idempotency_key: IdempotencyKey + source: ConnectionName | None = None # source delivery to copy the body from class ResendResult(BaseModel): @@ -263,10 +284,13 @@ class EditResendRequest(BaseModel): it overrides ``reroute``. ``idempotency_key`` makes a retry a no-op โ€” a *new* key is a genuine second resubmit. The ORIGINAL message stays byte-identical either way.""" + # ``raw`` is a MESSAGE BODY โ€” the data plane. It keeps a size bound and no alphabet rule: an HL7 + # v2 body is separated by carriage returns and may carry any encoding the sender used, so the + # control-plane printable rule (BACKLOG #1108) must not reach it. raw: str = Field(min_length=1, max_length=16_000_000) # the edited body (PHI โ€” never echoed) - idempotency_key: str = Field(min_length=1, max_length=256) + idempotency_key: IdempotencyKey reroute: bool = True - to: str | None = Field(None, max_length=256) # optional direct alternate outbound (power-path) + to: ConnectionName | None = None # optional direct alternate outbound (power-path) class EditResendResult(BaseModel): @@ -367,9 +391,10 @@ class AlertInstanceList(BaseModel): class DeadLetterReplayRequest(BaseModel): - # Connection names; bounded so an over-long value can't reach the store query (ASVS 1.3.3). - channel_id: str | None = Field(None, max_length=256) # scope replay to one inbound (None = all) - destination_name: str | None = Field(None, max_length=256) # scope to one outbound (None = all) + # Connection names; they carry the connection-name rule so a value that could not name a + # connection never reaches the store query (ASVS 1.3.3, 2.1.1 โ€” BACKLOG #1108). + channel_id: ConnectionName | None = None # scope replay to one inbound (None = all) + destination_name: ConnectionName | None = None # scope to one outbound (None = all) class DeadLetterReplayResult(BaseModel): @@ -417,7 +442,7 @@ class ReloadRequest(BaseModel): # server's startup --config dir. Any value must resolve within an allowed reload root (the # startup dir or [api].config_reload_roots) โ€” the loader executes Python from it. Length-bounded # (ASVS 1.3.3); the allow-list confinement remains the real control. - config_dir: str | None = Field(None, max_length=4096) + config_dir: FilesystemPath | None = None # dry_run: validate the graph against THIS environment (loads + build-checks connectors, which # resolves env() values for the target) and report the result WITHOUT swapping the live graph. # The promote pre-flight: catch a missing env value / bad spec before it goes live. @@ -520,15 +545,15 @@ class StatsResetTarget(BaseModel): For ``source`` rows ``destination`` is ignored; for ``destination`` rows it is required.""" role: Literal["source", "destination"] - channel_id: str = Field(min_length=1, max_length=256) - destination: str | None = Field(default=None, max_length=256) + channel_id: ConnectionName + destination: ConnectionName | None = None class StatsResetRequest(BaseModel): """Reset the dashboard's cumulative counters for ``targets``, or for every connection (``all``).""" all: bool = False - targets: list[StatsResetTarget] = Field(default_factory=list) + targets: list[StatsResetTarget] = Field(default_factory=list, max_length=MAX_MAP_ENTRIES) class StatsResetResult(BaseModel): @@ -735,7 +760,9 @@ class LogLevelUpdate(BaseModel): """PATCH body for the runtime verbosity control (BACKLOG #171): the new root/uvicorn level. Ephemeral โ€” the override resets on process restart, and NOT on ``/config/reload`` (ADR 0130 ยง1).""" - level: str + # The authority for which names are legal is ``logging_setup.LOG_LEVELS``, which the route calls + # and 4xxs on; this only fixes the shape (BACKLOG #1108). + level: LogLevelName class LogTailPage(BaseModel): @@ -960,7 +987,7 @@ class DrActivateRequest(BaseModel): attestation does NOT prove the restore's vintage or completeness โ€” that rests on the DBA runbook (BACKLOG #102).""" - archive: str | None = None + archive: FilesystemPath | None = None dba_attests_restored: bool = False @@ -1200,7 +1227,7 @@ class AlertTestEmailRequest(BaseModel): when set, redirects this one test send to a single alternate address (operator config, admin-gated); it is never echoed back in the result.""" - recipient_override: str | None = None + recipient_override: EmailAddress | None = None class AlertTestEmailResult(BaseModel): @@ -1280,7 +1307,7 @@ class UploadResendRequest(BaseModel): ``idempotency_key`` is unused today (each inject is a distinct receipt) but reserved for parity.""" index: int = Field(ge=0) - to: str = Field(min_length=1, max_length=256) # the target inbound connection + to: ConnectionName # the target inbound connection class UploadResendResult(BaseModel): @@ -1308,14 +1335,14 @@ class SearchPresetCriteria(BaseModel): ADR 0046 seam). ``content`` / ``field_value`` are PHI-shaped โ€” the preset column is encrypted at rest and every save/recall is step-up-gated + audited.""" - content: str | None = Field(None, max_length=512) + content: SearchText | None = None field_path: str | None = Field(None, max_length=32) - field_value: str | None = Field(None, max_length=512) + field_value: SearchText | None = None target: Literal["raw", "summary", "both"] = "both" - channel_id: str | None = Field(None, max_length=256) - status: str | None = Field(None, max_length=64) - message_type: str | None = Field(None, max_length=64) - control_id: str | None = Field(None, max_length=256) + channel_id: ConnectionName | None = None + status: StatusFilter | None = None + message_type: MessageTypeFilter | None = None + control_id: ControlIdFilter | None = None limit: int = Field(50, ge=1, le=500) @@ -1337,7 +1364,7 @@ class SearchPresetList(BaseModel): class SearchPresetCreateRequest(BaseModel): """Create-or-replace a named preset for the calling user. ``name`` is a per-user unique label.""" - name: str = Field(min_length=1, max_length=128) + name: DisplayLabel criteria: SearchPresetCriteria diff --git a/messagefoundry/api/validation.py b/messagefoundry/api/validation.py new file mode 100644 index 000000000..369085144 --- /dev/null +++ b/messagefoundry/api/validation.py @@ -0,0 +1,248 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Input-validation rules for the operator API's control-plane data items (BACKLOG #1108). + +**This module is the single authority for the rules; the prose lives in +[`docs/API-INPUT-VALIDATION.md`](../../docs/API-INPUT-VALIDATION.md).** The document explains each +rule and why it is drawn where it is; every pattern and ceiling it quotes is defined here and +pinned by ``tests/test_api_input_validation.py``, so the two cannot drift. + +Scope is the **control plane** -- the ids, connection names, time bounds and search terms an +operator sends to the engine's own API. The **data plane** (the HL7, X12, DICOM and other payloads +the engine carries) is a separate surface with its own rules; see ``docs/HL7-VALIDATION.md`` and +``docs/CODESETS.md``. Nothing here applies to a message body. + +**Why a rule and not just a length.** Before this module the API's request bodies carried length and +numeric bounds only -- an AST walk over ``api/models.py`` and ``api/auth_models.py`` found zero +``pattern=`` constraints between them. A length bound says how much of something may arrive; it does +not say what the something is. ASVS 2.1.1 asks for the second. + +**Anchoring, and the trap in it.** Pydantic 2.13 compiles ``pattern=`` with the Rust ``regex`` crate, +not Python's ``re``. Two consequences, both measured, both load-bearing: + +* ``$`` there means end-of-input. It does **not** admit a trailing newline the way Python's ``re`` + does, so ``^...$`` is a true full match and no ``\\Z`` is needed. +* ``\\Z`` is **not** a recognized escape in that engine. A pattern carrying one raises + ``SchemaError`` at class-construction time -- an import-time crash, not a validation failure. + +So do not copy a pattern from here into a :mod:`re` call without re-anchoring it (Python's ``$`` +would then let a trailing newline through), and do not copy the ``\\Z`` from +:data:`messagefoundry.uploads._FILE_ID_RE` into a pydantic ``pattern=``. + +Import weight is a constraint, not an accident: ``api/models.py`` is imported by the engine-free +``apiclient`` (ADR 0088) and through it by the PySide6 harness, so this module depends on nothing +but pydantic and the standard library. +""" + +from __future__ import annotations + +from typing import Annotated + +from pydantic import Field, StringConstraints + +# --- Engine-minted resource ids ------------------------------------------------------------------ +# +# Every id the operator API accepts on a path is minted by the engine and handed to the client in an +# earlier response. No person types one. Two shapes cover all of them: +# +# * 32 lowercase hex -- ``uuid4().hex`` (message, outbox, approval, user, preset) and +# ``secrets.token_hex(16)`` (upload file id). +# * 64 lowercase hex -- a SHA-256 digest (session id, which is the opaque token's hash; attachment +# id, which is the content digest). +# +# The rule earns its place twice over. It refuses a structurally impossible id **before** the value +# reaches a store query or a filesystem join, and it is what makes the by-id upload routes +# non-enumerable claim hold: no ``.``, ``/``, ``\\`` or NUL can survive it. +# ``messagefoundry.uploads._FILE_ID_RE`` already shipped exactly this rule for one id; this +# generalizes it rather than adding a second, differently-spelled copy. + +#: 32 lowercase hex characters -- what ``uuid4().hex`` and ``secrets.token_hex(16)`` mint. +RESOURCE_ID_PATTERN = r"^[0-9a-f]{32}$" + +#: 64 lowercase hex characters -- a SHA-256 hex digest. +DIGEST_ID_PATTERN = r"^[0-9a-f]{64}$" + +#: A custom role id: the ``custom:`` prefix ``messagefoundry.auth.permissions`` defines, then 32 hex. +#: Built-in role ids are words and are never accepted by the ``/roles/custom`` routes. +CUSTOM_ROLE_ID_PATTERN = r"^custom:[0-9a-f]{32}$" + +#: Any role id: a built-in role's lowercase word, or a custom role's prefixed id. Which words exist +#: stays ``messagefoundry.auth.permissions.Role``'s to say; the API decides only the shape, and the +#: routes 404 an id no role carries. Measured against all 6 built-in roles. +ROLE_ID_PATTERN = r"^[a-z]{1,32}$|^custom:[0-9a-f]{32}$" + +#: A permission id, ``area:action``. The catalog is ``Permission``'s; measured against all 28 members. +PERMISSION_ID_PATTERN = r"^[a-z_]{1,32}:[a-z_]{1,32}$" + +ResourceId = Annotated[str, StringConstraints(pattern=RESOURCE_ID_PATTERN)] +DigestId = Annotated[str, StringConstraints(pattern=DIGEST_ID_PATTERN)] +CustomRoleId = Annotated[str, StringConstraints(pattern=CUSTOM_ROLE_ID_PATTERN)] +RoleId = Annotated[str, StringConstraints(pattern=ROLE_ID_PATTERN)] +PermissionId = Annotated[str, StringConstraints(pattern=PERMISSION_ID_PATTERN)] + + +# --- Connection names ---------------------------------------------------------------------------- +# +# A connection name is operator-chosen, authored code-first or in ``connections.toml``, and reaches +# the API both as a path segment (``/connections/{name}/...``) and as a filter value (``channel_id``, +# ``destination_name``, ``to``, ``source``). +# +# The rule admits a leading letter, then letters, digits, underscore and hyphen, to 256 characters. +# It is deliberately WIDER than the grammar the VS Code extension already enforces at +# ``ide/src/connectionWizardModel.ts`` (``^[A-Za-z][A-Za-z0-9_]*$``, no hyphen). Measured over the +# 108 distinct connection names in ``samples/``, ``harness/``, ``tests/`` and ``messagefoundry/``, +# four fail the IDE's rule and all four fail it on a hyphen (``FILE-OUT_ACME_ADT``, +# ``FILE-OUT_Coverage``, ``FILE-OUT_EXAMPLE_ADT``, ``FILE-OUT_Test_ADT``). Adopting the narrower +# grammar here would make four shipped connections unreachable through the API. All 108 pass the +# rule below. +# +# What it excludes is what earns it: ``.``, ``/`` and ``\\`` (so a name can never read as a path or a +# traversal), whitespace and control characters (so a name cannot forge a log line or a CSV field), +# and the quoting and metacharacters ``%``, ``|``, ``&``, ``'``, ``"`` (so a name carries nothing +# into a URL or a downstream query). The 256 ceiling is the bound ``channel_id`` and +# ``destination_name`` already shipped, reused rather than replaced. + +#: A connection name: a leading letter, then letters, digits, ``_`` and ``-``, at most 256 characters. +CONNECTION_NAME_PATTERN = r"^[A-Za-z][A-Za-z0-9_-]{0,255}$" + +ConnectionName = Annotated[str, StringConstraints(pattern=CONNECTION_NAME_PATTERN)] + + +# --- Time ranges --------------------------------------------------------------------------------- +# +# The API's time bounds (``received_from``/``received_to``, ``since``/``until``) are epoch seconds as +# a float, and they reach a store bind. +# +# Two things were missing and both are demonstrable against the shipped code. ``ge=0`` alone does not +# exclude infinity: ``?received_from=inf`` parses to ``float('inf')`` and is accepted. The audit +# routes declare no bound at all, so ``?since=nan`` and ``?since=-inf`` are accepted there too. A +# NaN bound is the worse of the two, because every comparison against it is false, so the filter +# silently returns nothing rather than failing. +# +# The rule is: a finite float, at or after the epoch, at or before 2100-01-01T00:00:00Z. The upper +# ceiling is a sanity bound, not a business one -- it exists so an absurd or overflowed value is +# refused at the edge instead of reaching a query. + +#: 2100-01-01T00:00:00Z. The far end of any timestamp this engine will legitimately be asked about. +EPOCH_SECONDS_MAX = 4_102_444_800.0 + +EpochSeconds = Annotated[float, Field(ge=0.0, le=EPOCH_SECONDS_MAX, allow_inf_nan=False)] + + +# --- Free-text search terms and metadata filters -------------------------------------------------- +# +# ``content`` and ``field_value`` are whatever an operator typed to find a patient, so they are +# PHI-shaped and no alphabet rule can be written for them -- a patient name is any text. What CAN be +# ruled out is a control character. These values reach the search audit record, the application log +# and, through ``/audit/export``, a CSV file; a NUL, CR or LF in one of them forges a second record +# in whichever of those reads a line at a time. +# +# So the rule for free text is: printable, no C0 controls, no DEL, no C1 controls. It is as narrow as +# the data allows and no narrower. It does cost one capability, stated rather than hidden: a needle +# can no longer span an HL7 segment separator, because that separator is CR. The console's search box +# is a single-line input, so nothing today could send one. +# +# The metadata filters divide by what their values actually are. ``status`` is drawn from a closed +# vocabulary the store defines (``MessageStatus``/``OutboxStatus``), all of whose members are letters +# and underscores. ``message_type`` is an HL7 message type such as ``ADT^A01``, so it keeps ``^`` and +# takes the printable rule. ``control_id`` is MSH-10, which a sending system chooses, so it takes the +# printable rule too. +# +# ``field_path`` is NOT here on purpose. Its grammar already ships, in +# ``messagefoundry.parsing.peek.parse_path``, and ``messagefoundry.store.content_search.make_spec`` +# already applies it eagerly at all six of its acceptance points in ``api/app.py``, so a malformed +# path is already a 4xx. A copy here would be a second definition of a rule that has one. + +#: Printable text: no C0 control, no DEL, no C1 control. One or more characters. +PRINTABLE_TEXT_PATTERN = r"^[^\x00-\x1f\x7f-\x9f]+$" + +#: A member of one of the engine's own closed vocabularies -- ``MessageStatus``, ``OutboxStatus``, +#: the connection-event kinds. Every member of all three is letters and underscores only. The +#: vocabulary itself stays the store's to define; this is the shape a member can have. +#: +#: Named "member" and not "token" because bandit's B105 heuristic reads a constant whose name carries +#: "token" and whose value is a string literal as a hardcoded credential. A suppression here would be +#: one more thing a reviewer has to re-derive; the clearer name costs nothing. +VOCABULARY_MEMBER_PATTERN = r"^[A-Za-z_]{1,64}$" + +#: The most event kinds one ``/events`` request may filter on. The vocabulary is smaller than this. +MAX_EVENT_KINDS = 32 + +#: Ceiling for a PHI-shaped free-text needle. The bound ``content``/``field_value`` already shipped. +SEARCH_TEXT_MAX = 512 + +SearchText = Annotated[ + str, StringConstraints(pattern=PRINTABLE_TEXT_PATTERN, max_length=SEARCH_TEXT_MAX) +] +VocabularyMember = Annotated[str, StringConstraints(pattern=VOCABULARY_MEMBER_PATTERN)] + +#: A message/outbox status filter, and a connection-event kind. Both are vocabulary members; they are +#: named separately so a reader looking for the field finds the rule that governs it. +StatusFilter = VocabularyMember +EventKindFilter = VocabularyMember +MessageTypeFilter = Annotated[str, StringConstraints(pattern=PRINTABLE_TEXT_PATTERN, max_length=64)] +ControlIdFilter = Annotated[str, StringConstraints(pattern=PRINTABLE_TEXT_PATTERN, max_length=256)] + +#: An audit ``actor`` (a username) or ``action`` (an event name). Printable, bounded as they ship. +ActorFilter = Annotated[str, StringConstraints(pattern=PRINTABLE_TEXT_PATTERN, max_length=256)] +ActionFilter = Annotated[str, StringConstraints(pattern=PRINTABLE_TEXT_PATTERN, max_length=128)] + +#: An operator-chosen display label, such as a saved preset's name. Printable, bounded as it ships. +DisplayLabel = Annotated[str, StringConstraints(pattern=PRINTABLE_TEXT_PATTERN, max_length=128)] + +#: A client-minted idempotency token. The client chooses the alphabet, so the rule is the printable +#: one: the value reaches a store uniqueness check and an audit record, and nothing else reads it. +IdempotencyKey = Annotated[str, StringConstraints(pattern=PRINTABLE_TEXT_PATTERN, max_length=256)] + + +# --- Values whose real control is elsewhere ------------------------------------------------------- +# +# Two items on this surface already have an enforced rule that is not, and should not be, a pattern. +# The rule below is a shape gate in front of it, not a replacement for it, and saying which is which +# is the point: a compensating control that quietly stood in for the real one would be worse than no +# control at all. +# +# * A reload ``config_dir`` (and a DR ``archive``) is a filesystem path. The real control is the +# allow-list confinement -- the loader executes Python from that directory, and only an allowed +# reload root is accepted. What the shape gate adds is the NUL, which a path check can be +# truncated by, and the other control characters. +# * A log ``level`` is checked against ``messagefoundry.logging_setup.LOG_LEVELS``, which raises so +# the route can 4xx. The shape gate keeps an arbitrary-length string out of that error message. + +#: A filesystem path supplied by an operator. Printable, and bounded where its field already bounds it. +FilesystemPath = Annotated[str, StringConstraints(pattern=PRINTABLE_TEXT_PATTERN, max_length=4096)] + +#: A log level name. The authority is ``logging_setup.LOG_LEVELS``; this only fixes the shape. +LogLevelName = Annotated[str, StringConstraints(pattern=r"^[A-Za-z]{1,16}$")] + +#: An email address, for the one field that accepts one (the alert test-send override). +#: +#: This is a DELIVERABILITY-shaped rule, not a proof of validity: one ``@``, a non-empty local part +#: with no whitespace or control characters, and a dotted domain of letters, digits and hyphens. It +#: is intentionally not RFC 5322 -- that grammar admits quoted local parts and comments that no mail +#: path here would benefit from, and a regex claiming to implement it would be the false-premise +#: control this module is trying to avoid. 254 is the RFC 5321 maximum forward-path length. +EMAIL_ADDRESS_PATTERN = r"^[^\s@\x00-\x1f\x7f-\x9f]+@[A-Za-z0-9]([A-Za-z0-9-]*[A-Za-z0-9])?(\.[A-Za-z0-9]([A-Za-z0-9-]*[A-Za-z0-9])?)+$" + +EmailAddress = Annotated[str, StringConstraints(pattern=EMAIL_ADDRESS_PATTERN, max_length=254)] + + +# --- Bounded id collections ---------------------------------------------------------------------- +# +# ``/messages/export`` takes an explicit ``ids`` selection beside its search criteria. The route caps +# how many rows it will return, so the list of ids it will consider is capped to match: an unbounded +# list is a request the engine sizes from the client's side of the wire. + +#: The most explicitly-selected message ids one export may name -- the route's own ``limit`` ceiling. +MAX_EXPORT_IDS = 100_000 + +#: The most entries one directory-group mapping or one counter-reset request may carry. An uncapped +#: list of nested models is a request whose cost the client sizes; every sibling list here was already +#: capped, and these three were the ones that were not. +MAX_MAP_ENTRIES = 1_000 + +#: ``GET /search/layered`` takes preset ids as one comma-separated value. The route caps the layers +#: it will compose; the pattern makes a non-id impossible before the split. +LAYERED_PRESET_IDS_PATTERN = r"^[0-9a-f]{32}(,[0-9a-f]{32})*$" + +LayeredPresetIds = Annotated[str, StringConstraints(pattern=LAYERED_PRESET_IDS_PATTERN)] diff --git a/messagefoundry_webconsole/routes/search.py b/messagefoundry_webconsole/routes/search.py index 2d00276be..8d08a0661 100644 --- a/messagefoundry_webconsole/routes/search.py +++ b/messagefoundry_webconsole/routes/search.py @@ -237,20 +237,23 @@ async def ui_save_preset( # search page's unlock form on a stale step-up rather than being auto-retried. assert_same_origin(request) form = dict(await _form_pairs(request)) - criteria = SearchPresetCriteria( - content=form.get("content") or None, - field_path=form.get("field_path") or None, - field_value=form.get("field_value") or None, - target=form.get("target") - if form.get("target") in ("raw", "summary", "both") - else "both", # type: ignore[arg-type] - channel_id=form.get("channel_id") or None, - status=form.get("status") or None, - message_type=form.get("message_type") or None, - control_id=form.get("control_id") or None, - limit=50, - ) try: + # Inside the try, unlike before. The criteria model enforces the API's own input rules + # (BACKLOG #1108), so a criterion that breaks one raises HERE; built outside, that + # exception left the route as a 500 instead of the form's own error. + criteria = SearchPresetCriteria( + content=form.get("content") or None, + field_path=form.get("field_path") or None, + field_value=form.get("field_value") or None, + target=form.get("target") + if form.get("target") in ("raw", "summary", "both") + else "both", # type: ignore[arg-type] + channel_id=form.get("channel_id") or None, + status=form.get("status") or None, + message_type=form.get("message_type") or None, + control_id=form.get("control_id") or None, + limit=50, + ) body = SearchPresetCreateRequest(name=form.get("name", ""), criteria=criteria) await core.create_search_preset( body=body, engine=engine, identity=identity, request=request @@ -261,10 +264,17 @@ async def ui_save_preset( pages.message_search(None, error=str(exc.detail), presets=preset_list), status_code=exc.status_code, ) - except ValueError: # pydantic validation (e.g. empty name) + except ValueError: # pydantic: an empty name, or a criterion that breaks its input rule + # The message says WHICH field is at fault only in the generic sense. pydantic's own text + # quotes the offending value, and that value is form input on a PHI-shaped page, so it is + # never rendered. preset_list = await _presets(engine, identity, request) return HTMLResponse( - pages.message_search(None, error="a preset name is required", presets=preset_list), + pages.message_search( + None, + error="a preset name is required, and each criterion must be a valid value", + presets=preset_list, + ), status_code=400, ) return RedirectResponse("/ui/messages/search", status_code=303) diff --git a/messagefoundry_webconsole/routes/uploaded_logs.py b/messagefoundry_webconsole/routes/uploaded_logs.py index 7f852415c..d787a3e61 100644 --- a/messagefoundry_webconsole/routes/uploaded_logs.py +++ b/messagefoundry_webconsole/routes/uploaded_logs.py @@ -20,6 +20,7 @@ from fastapi import Depends, FastAPI, HTTPException, Query, Request from fastapi.responses import HTMLResponse, RedirectResponse, Response +from pydantic import ValidationError from messagefoundry.api._ui_seam import UiDeps from messagefoundry.api.models import UploadedMessageSearchRequest, UploadResendRequest @@ -381,10 +382,23 @@ async def ui_uploaded_log_resend( # destructive or PHI-shaped. ``to`` is a connection name, i.e. a structural locator, which the # engine already writes to the audit store by name on every resend. # - # Bounds live on the Query params now: the request body no longer passes through - # UploadResendRequest's own Field(ge=0) / max_length=256 before reaching us. + # The Query params carry the LENGTH bounds; the model carries the connection-name RULE + # (BACKLOG #1108), which the query declaration deliberately does not repeat -- a second copy + # would be a second definition. So the model can still refuse a value the query accepted, and + # a `to` that could not name a connection is refused HERE, before the engine sees it. assert_same_origin(request) - body = UploadResendRequest(index=index, to=to) + try: + body = UploadResendRequest(index=index, to=to) + except ValidationError: + # Same shape as an engine refusal below, and for the same reason: answering with the + # SUCCESS response would tell the operator a message was injected when none was. The + # rejected value is caller-supplied, so it travels nowhere -- not into the URL, the HTML + # or the log. + _log.warning( + "uploaded-log resend refused: file_id=%s reason=malformed_target", + _log_file_id(file_id), + ) + return _refused("resend_failed") try: await core.resend_uploaded_message( request, file_id=file_id, body=body, engine=engine, identity=identity diff --git a/packaging/messagefoundry-webconsole/tests/test_uploaded_logs_ui.py b/packaging/messagefoundry-webconsole/tests/test_uploaded_logs_ui.py index 327ed4d36..b65cf0f86 100644 --- a/packaging/messagefoundry-webconsole/tests/test_uploaded_logs_ui.py +++ b/packaging/messagefoundry-webconsole/tests/test_uploaded_logs_ui.py @@ -566,6 +566,11 @@ async def test_refusals_are_recorded_server_side( # exc.detail are all caller-supplied text, and writing those to a log is log injection. service = await _service(engine, ("op", Role.OPERATOR), ("op2", Role.OPERATOR)) transport = httpx.ASGITransport(app=_app(engine, service, tmp_path)) + # Two shapes of bad target, and they are refused at DIFFERENT places since BACKLOG #1108, which + # is why both are probed. ``absent`` is a legal connection name that no inbound carries, so it + # reaches the engine and comes back 404. ``bogus`` could not be a connection name at all, so the + # request model refuses it in the console before the engine is called. + absent = "IB_NOPEZQX_ABSENT" bogus = "IB_NOPEZQX" async with httpx.AsyncClient(transport=transport, base_url="http://t") as c: await _login(c, "op") @@ -577,17 +582,23 @@ async def test_refusals_are_recorded_server_side( await _login(c, "op") r = await c.post( f"/ui/uploaded-logs/file/{fid}/resend", - params={"index": "0", "to": bogus}, + params={"index": "0", "to": absent}, follow_redirects=False, ) assert r.status_code == 303 + malformed_target = await c.post( + f"/ui/uploaded-logs/file/{fid}/resend", + params={"index": "0", "to": bogus}, + follow_redirects=False, + ) + assert malformed_target.status_code == 303 # A file_id the CALLER invented, carrying an encoded newline: the target is unknown, so # the engine 404s before it ever validates the id, and the console still has to log # something. The minted-shape guard makes it a fixed placeholder rather than a forged # second log line. forged = await c.post( f"/ui/uploaded-logs/file/{'0' * 32}%0AWARNING-forged-line/resend", - params={"index": "0", "to": bogus}, + params={"index": "0", "to": absent}, follow_redirects=False, ) assert forged.status_code == 303 @@ -598,6 +609,8 @@ async def test_refusals_are_recorded_server_side( assert f"uploaded-log resend refused: file_id={fid} status=404" in lines assert f"uploaded-log delete refused: file_id={fid} status=404" in lines assert "uploaded-log resend refused: file_id=malformed status=404" in lines + # The earlier refusal records the same way: file_id plus a fixed reason, never the value. + assert f"uploaded-log resend refused: file_id={fid} reason=malformed_target" in lines # Nothing caller-supplied reached the log: not the inbound name, not the payload, not a newline. assert not any("IB_NOPEZQX" in line or "alert(1)" in line for line in lines) assert not any("forged" in line or "\n" in line for line in lines) diff --git a/packaging/messagefoundry-webconsole/tests/test_webui.py b/packaging/messagefoundry-webconsole/tests/test_webui.py index f0574d8b5..3574767d3 100644 --- a/packaging/messagefoundry-webconsole/tests/test_webui.py +++ b/packaging/messagefoundry-webconsole/tests/test_webui.py @@ -2807,9 +2807,15 @@ async def test_admin_cookie_not_accepted_on_json_routes(engine: Engine) -> None: assert (await c.delete(f"/users/{boss_id}")).status_code == 401 -async def test_error_banner_escapes_hostile_input(engine: Engine) -> None: - # _validate_roles echoes posted role ids into the 400 detail; the /ui banner must render it - # escaped (reflected-XSS regression guard for every rerender-with-error path). +async def test_a_hostile_role_id_is_refused_before_anything_can_echo_it(engine: Engine) -> None: + # This used to post a hostile role id, rely on _validate_roles echoing it into the 400 detail, + # and assert the banner escaped it. Since BACKLOG #1108 the role-id rule refuses that value at + # RolesUpdateRequest, so it never reaches _validate_roles and the route renders its own fixed + # "invalid input" instead. That is the stronger property and it is what this now pins. + # + # The escaping half did NOT move with it. Losing an XSS regression guard because an upstream + # rule made one route stop delivering hostile text is exactly how a guard goes quiet, so it is + # re-armed directly against the renderer in the test below, where no upstream rule can disarm it. service = await _service(engine) await _add(service, "u1", Role.VIEWER) async with _boss_client(engine, service) as c: @@ -2817,8 +2823,37 @@ async def test_error_banner_escapes_hostile_input(engine: Engine) -> None: hostile = "" r = await _post_pairs(c, f"/ui/users/{uid}/roles", [("roles", hostile)]) assert r.status_code == 400 + # Neither the raw value nor an escaped copy of it: the page never saw it. assert hostile not in r.text - assert "<img src=x onerror=alert(1)>" in r.text + assert "<img src=x onerror=alert(1)>" not in r.text + assert "invalid input" in r.text + # The control: a well-formed role id that no role carries still reaches the handler, so the + # route's OTHER arm is alive and the refusal above is the rule and not a dead route. + ok = await _post_pairs(c, f"/ui/users/{uid}/roles", [("roles", "nosuchrole")]) + assert ok.status_code == 400 and "invalid input" not in ok.text + + +def test_the_error_banner_escapes_hostile_text() -> None: + """The reflected-XSS guard for every rerender-with-error path, armed at the renderer itself. + + Driven directly rather than through a route: a route can stop being able to deliver hostile text + (see above), and when that happens a route-driven guard passes while measuring nothing. + """ + from messagefoundry.api.auth_models import UserSummary + from messagefoundry_webconsole import pages + + user = UserSummary( + id="0" * 32, username="u1", auth_provider="local", disabled=False, roles=["viewer"] + ) + hostile = "" + page = str(pages.user_detail_page(user, [], error=hostile)) + assert hostile not in page + assert "<img src=x onerror=alert(1)>" in page + # The control: an ordinary message still renders, so the assertion above is escaping and not + # the banner having been dropped. + assert "something went wrong" in str( + pages.user_detail_page(user, [], error="something went wrong") + ) async def test_ad_user_carveouts_on_ui_surface(engine: Engine) -> None: diff --git a/tests/test_api.py b/tests/test_api.py index 15cb68f14..464b970b7 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -34,6 +34,13 @@ "PID|1||100^^^H^MR||DOE^JANE\r" ) +# A well-formed message id that no test seeds, and a malformed one. Since BACKLOG #1108 the two get +# DIFFERENT answers, and both matter: an id the store has never seen is a 404, and a value that could +# not be an id at all is refused (422) before any lookup runs. A single "missing" probe conflated +# them, and would go on passing if the id rule were removed. +ABSENT_ID = "0" * 32 +MALFORMED_ID = "missing" + # A transformed outbound body, deliberately distinct from the raw inbound (different sending app + an # extra segment), so a test can prove the /outbound endpoint returns the *transformed* payload โ€” not # the raw โ€” and that it was decrypted at rest (#14). @@ -175,7 +182,8 @@ async def test_message_detail_includes_body_and_records_audit_view( events = await engine.store.events_for(mid) assert any(e["event"] == "viewed" for e in events) - assert (await client.get("/messages/missing")).status_code == 404 + assert (await client.get(f"/messages/{ABSENT_ID}")).status_code == 404 + assert (await client.get(f"/messages/{MALFORMED_ID}")).status_code == 422 async def test_message_outbound_returns_transformed_payload_and_audits( @@ -199,7 +207,8 @@ async def test_message_outbound_returns_transformed_payload_and_audits( assert any(e["event"] == "viewed" for e in await engine.store.events_for(mid)) assert "outbound.read" in [a["action"] for a in await engine.store.list_audit()] - assert (await client.get("/messages/missing/outbound")).status_code == 404 + assert (await client.get(f"/messages/{ABSENT_ID}/outbound")).status_code == 404 + assert (await client.get(f"/messages/{MALFORMED_ID}/outbound")).status_code == 422 async def test_message_outbound_no_deliveries_is_empty_and_unviewed( @@ -251,7 +260,8 @@ async def test_replay_requeues(engine: Engine, client: httpx.AsyncClient) -> Non assert rows[0]["status"] == OutboxStatus.PENDING.value assert rows[0]["attempts"] == 0 - assert (await client.post("/messages/missing/replay")).status_code == 404 + assert (await client.post(f"/messages/{ABSENT_ID}/replay")).status_code == 404 + assert (await client.post(f"/messages/{MALFORMED_ID}/replay")).status_code == 422 async def test_replay_no_deliveries_is_409_and_preserves_error( diff --git a/tests/test_api_auth.py b/tests/test_api_auth.py index 4c67231aa..5606b2370 100644 --- a/tests/test_api_auth.py +++ b/tests/test_api_auth.py @@ -24,6 +24,17 @@ from messagefoundry.store.store import MessageStore PW = "a-strong-test-passphrase" # โ‰ฅ15, no app/vendor terms โ€” satisfies the ASVS policy (WP-3) + +# A well-formed user id no test creates, and a malformed one. Since BACKLOG #1108 the two get +# DIFFERENT answers: an id the store has never seen is a 404, and a value that could not be a user id +# at all is refused (422) before the lookup. Asserting both keeps the 404 arm honest โ€” a single +# "nope" probe would go on passing with the id rule removed. +ABSENT_USER_ID = "0" * 32 +MALFORMED_USER_ID = "nope" + +#: The AD-provisioned fixture's id. Engine-minted ids are 32 hex, so a store row seeded by hand needs +#: that shape too or its own routes refuse the id before the AD branch under test is reached. +AD_USER_ID = "ad9" + "0" * 29 ADT = "MSH|^~\\&|S|F|R|RF|20260604||ADT^A01|MSG1|P|2.5.1\rPID|1||100^^^H^MR||DOE^JANE\r" @@ -510,7 +521,13 @@ async def test_permission_inspector_unknown_user_404(engine: Engine) -> None: await _add(service, "root", Role.ADMINISTRATOR) async with _client(engine, service) as c: admin = _auth((await _login(c, "root")).json()["token"]) - assert (await c.get("/users/does-not-exist/permissions", headers=admin)).status_code == 404 + assert ( + await c.get(f"/users/{ABSENT_USER_ID}/permissions", headers=admin) + ).status_code == 404 + # A value that could not be a user id never reaches the lookup at all (BACKLOG #1108). + assert ( + await c.get(f"/users/{MALFORMED_USER_ID}/permissions", headers=admin) + ).status_code == 422 async def test_audit_query_filters_by_actor_action_and_time(engine: Engine) -> None: @@ -1113,7 +1130,12 @@ async def test_admin_revokes_a_users_sessions(engine: Engine) -> None: uid = (await c.get("/auth/me", headers=_auth(tu))).json()["user_id"] assert (await c.delete(f"/users/{uid}/sessions", headers=admin)).status_code == 200 assert (await c.get("/auth/me", headers=_auth(tu))).status_code == 401 # force-signed-out - assert (await c.delete("/users/nope/sessions", headers=admin)).status_code == 404 + assert ( + await c.delete(f"/users/{ABSENT_USER_ID}/sessions", headers=admin) + ).status_code == 404 + assert ( + await c.delete(f"/users/{MALFORMED_USER_ID}/sessions", headers=admin) + ).status_code == 422 async def test_session_cap_evicts_oldest_on_login(engine: Engine) -> None: @@ -1242,7 +1264,7 @@ async def test_admin_reset_password_endpoint(engine: Engine) -> None: roles=["viewer"], actor="root", ) - await engine.store.create_user(user_id="ad9", username="ad9", auth_provider="ad") + await engine.store.create_user(user_id=AD_USER_ID, username="ad9", auth_provider="ad") async with _client(engine, service) as c: admin_token = (await _login(c, "root")).json()["token"] admin = _auth(admin_token) @@ -1277,9 +1299,16 @@ async def test_admin_reset_password_endpoint(engine: Engine) -> None: # own grant: the gate runs BEFORE the body, so without one these would all be 403 and the # test would stop measuring what it is named for. assert (await _reauth(c, admin_token, purpose="admin_reset_password")).status_code == 200 - assert (await c.post("/users/nope/reset-password", headers=admin)).status_code == 404 + assert ( + await c.post(f"/users/{ABSENT_USER_ID}/reset-password", headers=admin) + ).status_code == 404 + # No malformed-id probe here on purpose. The step-up gate runs BEFORE the path is validated, + # so on this route a malformed id gets 403 (no grant) rather than the 422 the id rule gives. + # The rule is measured on the ungated routes and in tests/test_api_input_validation.py. assert (await _reauth(c, admin_token, purpose="admin_reset_password")).status_code == 200 - assert (await c.post("/users/ad9/reset-password", headers=admin)).status_code == 400 + assert ( + await c.post(f"/users/{AD_USER_ID}/reset-password", headers=admin) + ).status_code == 400 me_id = (await c.get("/auth/me", headers=admin)).json()["user_id"] assert (await _reauth(c, admin_token, purpose="admin_reset_password")).status_code == 200 assert (await c.post(f"/users/{me_id}/reset-password", headers=admin)).status_code == 400 diff --git a/tests/test_api_input_validation.py b/tests/test_api_input_validation.py new file mode 100644 index 000000000..1e45f5737 --- /dev/null +++ b/tests/test_api_input_validation.py @@ -0,0 +1,372 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The operator API's input-validation rules (BACKLOG #1108, ASVS 2.1.1). + +Three jobs, in order of what each protects: + +1. **The rules do what they say.** Every constrained type accepts a legitimate value and refuses a + structurally impossible one. Each rejection is paired with the acceptance that proves the check + can still return the other answer -- a check that refuses everything is indistinguishable from a + correct one until something legitimate arrives. +2. **The measurements the rules were drawn from stay true.** Four connection names shipped in this + repository carry a hyphen; the connection-name rule was widened past the VS Code extension's + grammar because of them. If one is renamed the rule may be narrowed, but nobody should discover + that by accident. +3. **The document and the module cannot drift.** ``docs/API-INPUT-VALIDATION.md`` quotes every bound + in prose. The drift check reads those numbers back out of the page and compares them to the + module, and it carries its own control: doctored text must make it fail. + +Synthetic only. No store, no network, no PHI. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest +from pydantic import BaseModel, ValidationError + +from messagefoundry.api import validation as v +from messagefoundry.api.models import ( + DeadLetterReplayRequest, + EditResendRequest, + MessageExportRequest, + MessageSearchRequest, + ResendRequest, +) +from messagefoundry.auth.permissions import CUSTOM_ROLE_ID_PREFIX +from messagefoundry.parsing.peek import parse_path +from messagefoundry.uploads import _FILE_ID_RE + +_REPO = Path(__file__).resolve().parents[1] +_DOC = _REPO / "docs" / "API-INPUT-VALIDATION.md" + +_HEX32 = "0123456789abcdef" * 2 +_HEX64 = _HEX32 * 2 + + +def _accepts(alias: object, value: object) -> bool: + """Whether the constrained alias accepts ``value``, decided by pydantic and nothing else.""" + + class Probe(BaseModel): + field: alias # type: ignore[valid-type] + + try: + Probe(field=value) + except ValidationError: + return False + return True + + +# --- The pinned rules ----------------------------------------------------------------------------- + + +def test_resource_id_is_thirty_two_lowercase_hex() -> None: + assert v.RESOURCE_ID_PATTERN == r"^[0-9a-f]{32}$" + assert _accepts(v.ResourceId, _HEX32) # control: a real id is still accepted + for bad in ( + _HEX32[:-1], # one short + _HEX32 + "0", # one long + _HEX32.upper(), # wrong case + "../../etc/passwd", + _HEX32[:-2] + "..", + ): + assert not _accepts(v.ResourceId, bad), bad + + +def test_an_otherwise_valid_id_with_a_trailing_newline_is_refused() -> None: + """Pydantic compiles ``pattern=`` with Rust's regex, where ``$`` is end-of-input. + + Python's ``re`` would let this through, which is why ``messagefoundry.uploads._FILE_ID_RE`` needs + ``\\Z`` and this module does not. Both halves are asserted so neither can be "fixed" into the + other's spelling without a red test. + """ + assert _accepts(v.ResourceId, _HEX32) + assert not _accepts(v.ResourceId, _HEX32 + "\n") + assert _FILE_ID_RE.match(_HEX32) is not None + assert _FILE_ID_RE.match(_HEX32 + "\n") is None + assert r"\Z" not in v.RESOURCE_ID_PATTERN + + +def test_the_upload_file_id_rule_and_the_api_resource_id_rule_agree() -> None: + """The API's id rule generalizes the one ``uploads.py`` already shipped; it must not narrow it.""" + for probe in (_HEX32, _HEX32.upper(), "not-an-id", ""): + assert _accepts(v.ResourceId, probe) == (_FILE_ID_RE.match(probe) is not None), probe + + +def test_digest_id_is_sixty_four_lowercase_hex() -> None: + assert v.DIGEST_ID_PATTERN == r"^[0-9a-f]{64}$" + assert _accepts(v.DigestId, _HEX64) + assert not _accepts(v.DigestId, _HEX32) + assert not _accepts(v.DigestId, _HEX64.upper()) + + +def test_custom_role_id_carries_the_prefix_the_auth_package_mints() -> None: + assert v.CUSTOM_ROLE_ID_PATTERN == r"^custom:[0-9a-f]{32}$" + assert CUSTOM_ROLE_ID_PREFIX == "custom:" + assert _accepts(v.CustomRoleId, CUSTOM_ROLE_ID_PREFIX + _HEX32) + assert not _accepts(v.CustomRoleId, _HEX32) # a built-in role id is not a custom one + assert not _accepts(v.CustomRoleId, "administrator") + + +# --- Connection names, and the measurement behind them -------------------------------------------- + +#: The connection names this repository ships that carry a hyphen. The VS Code extension's wizard +#: grammar (``ide/src/connectionWizardModel.ts``) rejects all four; the API rule admits them, and +#: that is the whole reason the two grammars differ. +HYPHENATED_SHIPPED_NAMES = ( + "FILE-OUT_ACME_ADT", + "FILE-OUT_Coverage", + "FILE-OUT_EXAMPLE_ADT", + "FILE-OUT_Test_ADT", +) + +#: The wizard's grammar, transcribed. Not imported -- it is TypeScript -- so it is pinned by the +#: assertion below that it rejects exactly the names the API rule accepts. +_IDE_NAME_RE = re.compile(r"^[A-Za-z][A-Za-z0-9_]*$") + + +def test_connection_name_admits_the_hyphenated_names_the_ide_grammar_rejects() -> None: + assert v.CONNECTION_NAME_PATTERN == r"^[A-Za-z][A-Za-z0-9_-]{0,255}$" + for name in HYPHENATED_SHIPPED_NAMES: + assert _accepts(v.ConnectionName, name), name + # The control: these names really are the disagreement, not an arbitrary set. + assert _IDE_NAME_RE.match(name) is None, name + # ...and the two rules still agree on an ordinary name, so the divergence is the hyphen alone. + assert _accepts(v.ConnectionName, "IB_ACME_ADT") + assert _IDE_NAME_RE.match("IB_ACME_ADT") is not None + + +def test_connection_name_refuses_what_would_carry_meaning_downstream() -> None: + assert _accepts(v.ConnectionName, "OB_DEMO_ORU") + for bad in ( + "", + "../../etc/passwd", + "IB/ACME", + "IB\\ACME", + "IB.ACME", + "IB ACME", + "IB'ACME", + "IB%2FACME", + "IB\nACME", + "IB\x00ACME", + "9_LEADING_DIGIT", + "-LEADING_HYPHEN", + "A" * 257, + ): + assert not _accepts(v.ConnectionName, bad), bad + assert _accepts(v.ConnectionName, "A" * 256) # the ceiling itself is legal + + +def test_the_shipped_connection_names_all_pass_the_rule() -> None: + """A live sweep, so a newly-authored sample that breaks the rule reds here rather than in the API. + + The sweep's own control is the hyphenated set above: if the pattern that finds names stops + matching anything, that assertion fails first and the empty sweep cannot read as a clean one. + """ + found: set[str] = set() + # ``[^"\n]`` so a match cannot run past the end of its line. Without it the pattern splices one + # source line's opening quote to a later line's closing one and reports the text between them as + # a connection name. + factory = re.compile(r"\b(?:inbound|outbound)\(\s*\"([^\"\n]+)\"") + for root in ("samples", "harness", "tests", "messagefoundry"): + for py in (_REPO / root).rglob("*.py"): + found.update(factory.findall(py.read_text(encoding="utf-8", errors="replace"))) + assert found >= set(HYPHENATED_SHIPPED_NAMES), "the sweep stopped finding known names" + bad = sorted(n for n in found if not _accepts(v.ConnectionName, n)) + assert not bad, f"connection names that the API rule would refuse: {bad}" + + +# --- Time bounds ---------------------------------------------------------------------------------- + + +def test_time_bounds_refuse_infinity_and_nan() -> None: + """The gap this rule closes. A lower bound of zero does not exclude ``inf``.""" + assert v.EPOCH_SECONDS_MAX == 4_102_444_800.0 + assert _accepts(v.EpochSeconds, 1_700_000_000.0) # control: a real timestamp still passes + assert _accepts(v.EpochSeconds, 0.0) + for bad in (float("inf"), float("-inf"), float("nan"), "inf", "nan", -1.0, 1e30): + assert not _accepts(v.EpochSeconds, bad), bad + + +# --- Free text and vocabulary tokens --------------------------------------------------------------- + + +def test_free_text_refuses_control_characters_and_keeps_everything_else() -> None: + assert v.SEARCH_TEXT_MAX == 512 + for good in ("SMITH", "O'Brien", "Zoรซ", "a b c", "^~\\&", "x" * 512): + assert _accepts(v.SearchText, good), good + for bad in ("SMITH\x00", "SMITH\nFORGED", "SMITH\rFORGED", "SMITH\tX", "", "x" * 513): + assert not _accepts(v.SearchText, bad), bad + + +def test_vocabulary_tokens_match_the_engines_own_status_values() -> None: + from messagefoundry.store.store import MessageStatus, OutboxStatus + + assert v.VOCABULARY_MEMBER_PATTERN == r"^[A-Za-z_]{1,64}$" + for status in (*MessageStatus, *OutboxStatus): + assert _accepts(v.StatusFilter, status.value), status + assert _accepts(v.StatusFilter, status.value.upper()), status + for bad in ("received;DROP", "received 1", "", "a" * 65): + assert not _accepts(v.StatusFilter, bad), bad + + +def test_message_type_keeps_the_hl7_component_separator() -> None: + """``ADT^A01`` must survive. A narrower alphabet rule here would be wrong, not stricter.""" + assert _accepts(v.MessageTypeFilter, "ADT^A01") + assert _accepts(v.MessageTypeFilter, "ORU^R01^ORU_R01") + assert not _accepts(v.MessageTypeFilter, "ADT^A01\n") + + +def test_email_rule_accepts_an_ordinary_address_and_refuses_the_obvious_breakage() -> None: + for good in ("ops@example.org", "first.last+tag@sub.example.co.uk"): + assert _accepts(v.EmailAddress, good), good + for bad in ( + "ops", + "ops@", + "@example.org", + "ops@example", + "a b@example.org", + "ops@ex\nample.org", + ): + assert not _accepts(v.EmailAddress, bad), bad + + +# --- The rules as the request models apply them ---------------------------------------------------- + + +def test_search_request_applies_the_rules_to_its_own_fields() -> None: + ok = MessageSearchRequest(content="SMITH", channel_id="IB_ACME_ADT", status="processed") + assert ok.channel_id == "IB_ACME_ADT" + with pytest.raises(ValidationError): + MessageSearchRequest(channel_id="../../etc") + with pytest.raises(ValidationError): + MessageSearchRequest(content="SMITH\nFORGED") + with pytest.raises(ValidationError): + MessageSearchRequest(status="processed; DROP TABLE") + + +def test_resend_request_bounds_both_connection_names() -> None: + ok = ResendRequest(to="OB_ACME_ADT", idempotency_key="k-1", source="OB_OTHER") + assert ok.to == "OB_ACME_ADT" + with pytest.raises(ValidationError): + ResendRequest(to="OB/ACME", idempotency_key="k-1") + with pytest.raises(ValidationError): + ResendRequest(to="OB_ACME_ADT", idempotency_key="k\x001") + + +def test_edit_resend_keeps_the_message_body_unconstrained() -> None: + """``raw`` is the data plane. It carries carriage returns by construction and must stay open.""" + body = "MSH|^~\\&|A|B|C|D|20260101||ADT^A01|1|P|2.5\rPID|||123^^^MRN\r" + ok = EditResendRequest(raw=body, idempotency_key="k-1") + assert ok.raw == body + with pytest.raises(ValidationError): + EditResendRequest(raw=body, idempotency_key="k-1", to="OB ACME") + + +def test_dead_letter_replay_bounds_its_two_scopes() -> None: + assert DeadLetterReplayRequest().channel_id is None # the all-channels scope survives + assert DeadLetterReplayRequest(channel_id="IB_ACME_ADT").channel_id == "IB_ACME_ADT" + with pytest.raises(ValidationError): + DeadLetterReplayRequest(destination_name="OB\x00ACME") + + +def test_export_ids_are_ids_and_the_list_is_bounded() -> None: + assert MessageExportRequest(ids=[_HEX32]).ids == [_HEX32] + assert v.MAX_EXPORT_IDS == 100_000 + with pytest.raises(ValidationError): + MessageExportRequest(ids=["not-an-id"]) + with pytest.raises(ValidationError): + MessageExportRequest(ids=[_HEX32] * (v.MAX_EXPORT_IDS + 1)) + + +def test_role_and_permission_ids_admit_every_shipped_value() -> None: + """The shape rule must be wider than the catalogs, which stay the auth package's to define.""" + from messagefoundry.auth.permissions import Permission, Role + + for role in Role: + assert _accepts(v.RoleId, role.value), role + assert _accepts(v.RoleId, CUSTOM_ROLE_ID_PREFIX + _HEX32) + for perm in Permission: + assert _accepts(v.PermissionId, perm.value), perm + for bad in ("Administrator", "admin;DROP", "custom:short", "", "a" * 33): + assert not _accepts(v.RoleId, bad), bad + for bad in ("messages read", "messages", "MESSAGES:READ", "a:b:c"): + assert not _accepts(v.PermissionId, bad), bad + + +def test_the_auth_models_carry_the_same_rules() -> None: + from messagefoundry.api.auth_models import ( + AdGroupMap, + AdGroupMapEntry, + ChannelScope, + CustomRoleRequest, + RolesUpdateRequest, + ) + + assert ChannelScope(channels=["IB_A", "FILE-OUT_Test_ADT"]).channels is not None + assert ChannelScope().channels is None # the all-channels scope survives + with pytest.raises(ValidationError): + ChannelScope(channels=["IB_A", "../../etc"]) + assert RolesUpdateRequest(roles=["viewer"]).roles == ["viewer"] + with pytest.raises(ValidationError): + RolesUpdateRequest(roles=["viewer", "Bad Role"]) + assert CustomRoleRequest(display_name="X", permissions=["messages:read"]).permissions + with pytest.raises(ValidationError): + CustomRoleRequest(display_name="X", permissions=["messages read"]) + # The directory maps were the uncapped lists; a client no longer sizes the request. + entry = AdGroupMapEntry(ad_group="g", role="viewer") + assert AdGroupMap(entries=[entry]).entries == [entry] + with pytest.raises(ValidationError): + AdGroupMap(entries=[entry] * (v.MAX_MAP_ENTRIES + 1)) + + +def test_the_field_path_rule_stays_where_it_already_lives() -> None: + """No pattern is declared for ``field_path``; ``parse_path`` is its single authority.""" + assert "FIELD_PATH" not in dir(v) + assert parse_path("PID-3") == ("PID", 3, None, None) + with pytest.raises(Exception): # noqa: B017 -- HL7PeekError, raised from parsing + parse_path("PID-3; DROP") + + +# --- The page and the module cannot drift ---------------------------------------------------------- + + +def _doc_claims() -> tuple[str, ...]: + """The sentences the page must contain, each BUILT FROM the module constant it reports. + + Built rather than transcribed on purpose: a hard-coded expectation pins the test to a literal, + which then agrees with a page that has drifted away from the code. Each claim also carries enough + surrounding words to be falsifiable -- a bare ``32`` occurs in "32 lowercase hex" and would match + a page that had dropped the event-kind ceiling entirely. + """ + return ( + f"up to {int(v.EPOCH_SECONDS_MAX)}", + f"up to {v.SEARCH_TEXT_MAX} characters", + f"at most {v.MAX_EXPORT_IDS} ids", + f"at most {v.MAX_EVENT_KINDS} event kinds", + f"at most {v.MAX_MAP_ENTRIES} entries", + CUSTOM_ROLE_ID_PREFIX, + ) + + +def _doc_drift(text: str) -> list[str]: + """Which claims the page no longer makes. Empty means the page and the module agree.""" + flat = text.replace(",", "") + return [claim for claim in _doc_claims() if claim not in flat] + + +def test_the_reference_page_states_the_bounds_the_module_enforces() -> None: + assert _doc_drift(_DOC.read_text(encoding="utf-8")) == [] + + +def test_the_drift_check_can_return_the_other_answer() -> None: + """The control. A checker that passes on doctored text is measuring nothing.""" + doctored = _DOC.read_text(encoding="utf-8").replace("4102444800", "9999999999") + assert _doc_drift(doctored) == [f"up to {int(v.EPOCH_SECONDS_MAX)}"] + + +def test_the_reference_page_is_linked_from_the_docs_index() -> None: + index = (_REPO / "docs" / "README.md").read_text(encoding="utf-8") + assert "API-INPUT-VALIDATION.md" in index diff --git a/tests/test_attachment_download_api.py b/tests/test_attachment_download_api.py index c89aa82e0..41f37fb13 100644 --- a/tests/test_attachment_download_api.py +++ b/tests/test_attachment_download_api.py @@ -372,7 +372,13 @@ async def test_ui_delegate_serves_the_sandbox_csp_not_the_console_csp( async def test_download_unknown_message_is_404(client: httpx.AsyncClient) -> None: - assert (await client.get("/messages/missing/attachments/" + "a" * 64)).status_code == 404 + absent = "0" * 32 # a well-formed message id nothing seeded + assert (await client.get(f"/messages/{absent}/attachments/" + "a" * 64)).status_code == 404 + # A message id that could not be one, and an attachment id that is not a 64-hex digest, are each + # refused before the lookup (BACKLOG #1108). Paired with the 404 above so neither arm can pass by + # the route simply refusing everything. + assert (await client.get("/messages/missing/attachments/" + "a" * 64)).status_code == 422 + assert (await client.get(f"/messages/{absent}/attachments/nope")).status_code == 422 async def test_download_unlinked_attachment_is_404( diff --git a/tests/test_upload_api.py b/tests/test_upload_api.py index c40cc2b07..47016b529 100644 --- a/tests/test_upload_api.py +++ b/tests/test_upload_api.py @@ -374,11 +374,19 @@ async def test_browse_and_delete_path_traversal_404(engine: Engine, tmp_path: Pa transport = httpx.ASGITransport(app=app) async with httpx.AsyncClient(transport=transport, base_url="http://t") as c: h = await _login(c, "op") - # A path-traversal-shaped id never resolves to a file โ€” 404, no filesystem touch. - for bad in ("..%2f..%2fetc", "abc", "0" * 31): + # A path-traversal-shaped id never resolves to a file, and no filesystem touch happens. + # WHICH refusal you get depends on how far the value gets, and both are asserted so that a + # later widening of the id rule cannot pass silently: + # * an encoded slash decodes to a path separator, so the router finds no matching route (404); + # * anything else that is not 32 lowercase hex is refused by the file-id rule the request + # edge now carries (422, BACKLOG #1108) rather than by the store's own guard. + r = await c.get("/uploads/..%2f..%2fetc/messages", headers=h) + assert r.status_code == 404, r.status_code + for bad in ("abc", "0" * 31, "0" * 33, ("0" * 31) + "G"): r = await c.get(f"/uploads/{bad}/messages", headers=h) - assert r.status_code == 404, (bad, r.status_code) - # A well-formed but non-existent id is also 404. + assert r.status_code == 422, (bad, r.status_code) + # A well-formed but non-existent id gets past the shape rule and is 404 โ€” the control that + # proves the loop above is measuring the shape and not simply refusing everything. r = await c.delete(f"/uploads/{'0' * 32}", headers=h) assert r.status_code == 404