natsukium/felis
Watch
1
Code Issues 27 Pull requests 1 Projects Releases Packages Wiki Activity Actions

[v0.1/P0] Enforce outbound frame and operation-specific payload limits #16

New issue
Closed
opened 2026-09-03 16:18:00 +09:00 by natsukium · 1 comment
natsukium commented 2026-09-03 16:18:00 +09:00
Owner
Copy link

Parent: #12 (P0.1). Related to #5 and #10.

Why

Receivers enforce DEFAULT_MAX_BODY, but senders narrow usize to u32 without preserving that limit. Treating the 64 MiB framing cap as every operation's policy also leaves ordinary inputs and descriptors unnecessarily large.

Scope

  • Make frame encoding and writing fallible.
  • Reject oversized outbound bodies before integer narrowing or header writes.
  • Define semantic limits for bridge lines, input/paste bytes, argv/environment, search patterns, and retarget descriptors.
  • Return typed local or protocol errors appropriate to each surface.

Acceptance criteria

  • No outbound frame can exceed DEFAULT_MAX_BODY or truncate its length.
  • Each listed operation has a documented limit below the framing backstop.
  • Boundary and oversized cases are tested on encode and write paths.
  • CLI/bridge machine errors remain parseable when a semantic limit is exceeded.
  • IPC, CLI, and security docs agree on the limits.
Parent: #12 (P0.1). Related to #5 and #10. ## Why Receivers enforce `DEFAULT_MAX_BODY`, but senders narrow `usize` to `u32` without preserving that limit. Treating the 64 MiB framing cap as every operation's policy also leaves ordinary inputs and descriptors unnecessarily large. ## Scope - Make frame encoding and writing fallible. - Reject oversized outbound bodies before integer narrowing or header writes. - Define semantic limits for bridge lines, input/paste bytes, argv/environment, search patterns, and retarget descriptors. - Return typed local or protocol errors appropriate to each surface. ## Acceptance criteria - [ ] No outbound frame can exceed `DEFAULT_MAX_BODY` or truncate its length. - [ ] Each listed operation has a documented limit below the framing backstop. - [ ] Boundary and oversized cases are tested on encode and write paths. - [ ] CLI/bridge machine errors remain parseable when a semantic limit is exceeded. - [ ] IPC, CLI, and security docs agree on the limits.
natsukium commented 2026-09-03 20:35:11 +09:00
Author
Owner
Copy link

Triage plan (2026-09-03)

Source-grounded triage against main at 69076d42, reviewed through seven rounds of an independent reviewer (pi sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12. Where a "Review amendments" section below conflicts with an earlier section, the amendment is the decision.

Claim check

Accurate against HEAD.

  • Senders narrow without checking. crates/felis-protocol/src/frame.rs:62-68 Frame::encode_to: let body_len = self.body.len() as u32; and crates/felis-transport/src/framing.rs:165-182 FrameWriter::write_frame: let len = LEN_OVERHEAD + frame.body.len() as u32; on the large-body path. A body between 64 MiB and 4 GiB writes a header the receiver rejects (BodyTooLarge, tearing down the connection with the receiver blamed); a body ≥ 4 GiB wraps and writes a corrupt length. codec::encode / encode_correlated (codec.rs:26-28, :83-91) and send/send_unflushed/send_correlated (framing.rs:186-215) are all infallible on size. The client writer task (felis-client/src/main.rs:1735-1746) calls write_frame directly.
  • Receiver-side cap is the only cap. DEFAULT_MAX_BODY = 64 MiB (frame.rs:19), enforced at FrameReader (framing.rs:64) and decode_with_ceiling; REQ-105 (spec.md:58) documents only the receive side.
  • Semantic limits exist for some operations, not the ones listed. Present: MAX_ENV_BASE_ENTRIES/_BYTES (session.rs; ipc.md:703-707), MAX_SESSION_TAGS/MAX_TAG_BYTES (ops.rs:267-270), MAX_CARRIER_* (preface.rs:290-295), MAX_IMAGE_CHUNK_PAYLOAD (image.rs:46), LinkText::CAP. Absent: InputMsg::KeyBytes(Vec<u8>) / Paste(Vec<u8>) (input.rs:85-88), SearchMsg::Query.query: String (search.rs:19-24), SpawnArgs.command/args/cwd/env (session.rs:117-129), RetargetTarget strings (ops.rs:299, endpoint path / SSH destination and args), bridge request lines (cli_bridge.rs:118 BufReader::lines() has no length cap; that surface is #21's).
  • One nuance: the issue says the 64 MiB cap is "treated as every operation's policy". More precisely nobody chose it for those operations; the absence of a semantic limit makes the framing backstop the effective one.

Verdict

accept. No principle question; this is the "strictness at the edge" the design values claim. One refinement on placement: the limits must live in felis-protocol (below daemon/client policy), which is also what #49 requires for receiver-side limits, so create the module once and let #49 extend it.

Approach

felis-protocol

  • frame.rs: Frame::encode_to(&self, out) -> Result<(), FrameError> and encode() likewise, returning FrameError::BodyTooLarge { body_len, ceiling } when self.body.len() > DEFAULT_MAX_BODY as usize (checked before the u32 narrowing, so the ≥ 4 GiB case is caught by the same branch). Keep Frame a plain view; the check is at encode time, not construction, so the daemon's fan-out of one body to many peers pays it once per write, which is where the header is formed.
  • New messages/limits.rs (re-exported from messages.rs), one pub const per surface with a doc line citing the reason for the number:
    • MAX_KEY_BYTES = 4 KiB (one key event or IME commit; kitty keyboard encodings are tens of bytes; an IME commit of a paragraph is ~1 KiB).
    • MAX_PASTE_BYTES = 16 MiB (the largest paste a clipboard realistically holds; must be ≤ #15's PTY_INPUT_BUDGET or split; pick the budget after #15 settles, and pin the relation with a const _: () = assert!(...)).
    • MAX_SEARCH_PATTERN_BYTES = 4 KiB (the regex crate compiles under its own size limit; this bounds the request, not the automaton).
    • MAX_SPAWN_ARGV_ENTRIES = 4096, MAX_SPAWN_ARGV_BYTES = 1 MiB (mirrors the env caps; Linux ARG_MAX is 2 MiB for argv+env together), MAX_SPAWN_PATH_BYTES = 4 KiB for command and cwd (PATH_MAX), MAX_SPAWN_ENV_* reuse the env-base caps.
    • MAX_RETARGET_DESCRIPTOR_BYTES = 64 KiB across the carrier's strings (an SSH destination plus --ssh-args).
    • MAX_BRIDGE_LINE_BYTES = 32 MiB (defined here so #21 and the CLI share it; ≥ a JSON-escaped MAX_PASTE_BYTES plus envelope).
  • WireError::OverLimit { field: &'static str, len: usize, cap: usize } raised from the TryFrom<v1::*> conversions (convert/*.rs) for every field above, so a daemon or client decoding an over-limit body gets a typed, connection-local error before any use. The same constants are checked on the send side by a fn validate(&self) -> Result<(), WireError> on each domain enum, called from FrameWriter::send* and by the CLI/bridge before it builds the request. Encoders stay infallible (to_wire); the check precedes them.

felis-transport: write_frame returns TransportError::BodyTooLarge from the new FrameError on both the scratch and large-body paths, before any header byte is written (so a refused frame leaves the stream consistent and the connection usable). send* propagate WireError::OverLimit as TransportError::Wire.

felis-daemon: the outbound path (write_event_frame, serve.rs:1382-1398) treats BodyTooLarge as a daemon bug: log at error with the event's kind and length, evict the subscriber (same path as the gauge eviction), never panic. Audit the largest emitted bodies against the cap: a RowDelta batch at 2048×2048 with a pathological row codec expansion, and a RegionMsg reply; add a chunking guard where a batch can approach 64 MiB. Inbound: OverLimit on Search → Conn::Error { InvalidRequest } on the stream; on Create/Retarget → AttachFailure::SpawnFailed / ops error reply with the field name in detail; on Input (fire-and-forget, no reply channel) → connection-local close with ConnError::Wire, since a conforming client checks before sending.

felis-client-core / felis-cli: sessions send, sessions spawn, search, and switch/retarget check validate() before dialing and report the CLI invalid_request machine error kind (already in the vocabulary, CHANGELOG Unreleased), naming the limit; the bridge emits the same kind as an error object on the request id (error_object, cli_bridge.rs:194-197), so machine consumers keep a parseable line. The GUI's paste path clamps to MAX_PASTE_BYTES and shows the existing confirm overlay's label path for "paste truncated" or refuses; choose refuse (explicit over silent truncation, principle 4's spirit).

Tests

  • frame.rs: body of exactly DEFAULT_MAX_BODY encodes; DEFAULT_MAX_BODY + 1 returns BodyTooLarge; the same pair on FrameWriter::write_frame through a Vec<u8> sink asserting zero bytes written on refusal (a 64 MiB zeroed Vec is fine for nextest; mark the test #[ignore]-free but single-threaded if memory-bound on CI).
  • limits.rs: at-limit / one-past for each constant through codec::encode + decode round trip (send-side validate and receive-side TryFrom both), plus const _ assertions that every semantic limit is below DEFAULT_MAX_BODY and MAX_PASTE_BYTES ≤ PTY_INPUT_BUDGET.
  • CLI: felis sessions send with an over-limit payload exits 1 with {"error":{"kind":"invalid_request",...}}; bridge golden line.
  • Fuzz: extend the existing framing fuzz target to the encode direction (just fuzz*).

Docs cascade

  • docs/reference/ipc.md: new "Semantic limits" table directly under "Frame layer" (limit, value, applies to, checked by sender / receiver, outcome), stating explicitly that the frame cap is a framing backstop and these are the operation policies; "Session" and "Search" sections reference it.
  • docs/reference/spec.md: REQ-105 gains "enforced on encode as well, before integer narrowing"; new REQ-105a listing the semantic limits.
  • docs/reference/cli.md: per-verb limit and the invalid_request mapping; docs/reference/testing.md "Security tests" → "Frame length ceiling" extended to the encode path.
  • docs/explanation/security-model.md "Daemon IPC": one bullet on why per-operation limits exist beside the framing cap (rejected alternative: a single cap per family).
  • CHANGELOG.md: new limits (user-affecting: paste size, argv size, search pattern), invalid_request on over-limit.
  • skills/felis/SKILL.md: the limits a script must respect for sessions send, spawn, search, and the bridge line cap.

Dependencies

  • None blocking. Coordinate: #49 extends the same limits.rs (land #16 first); #15 fixes PTY_INPUT_BUDGET relative to MAX_PASTE_BYTES; #21 consumes MAX_BRIDGE_LINE_BYTES; #23 owns the machine error-kind vocabulary (invalid_request already exists, so no new kind is needed unless #23 renames). #12's order holds.

Risk/effort

M. Mechanical across four crates; the judgment is in the numbers. Main risk: a cap that breaks a real workflow (a 20 MiB paste, a 5000-entry argv); keep the numbers generous and record the rationale beside each constant so a later change is a one-line decision, not a re-investigation.

Labels

Keep priority/P0, release/v0.1.0: the limits become part of the frozen wire contract (#12 "Contract freeze boundary"), so they must be chosen before the tag even though the u32 truncation itself is a low-likelihood bug.

Review amendments (round 2)

  • MAX_PASTE_BYTES = 16 MiB and #15's PTY_INPUT_BUDGET = 16 MiB, related by a compile-time assert MAX_PASTE_BYTES <= PTY_INPUT_BUDGET in this module. "Or split" is withdrawn; there is no chunking.
## Triage plan (2026-09-03) Source-grounded triage against `main` at `69076d42`, reviewed through seven rounds of an independent reviewer (`pi` sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12. Where a "Review amendments" section below conflicts with an earlier section, the amendment is the decision. ## Claim check Accurate against HEAD. - **Senders narrow without checking.** `crates/felis-protocol/src/frame.rs:62-68` `Frame::encode_to`: `let body_len = self.body.len() as u32;` and `crates/felis-transport/src/framing.rs:165-182` `FrameWriter::write_frame`: `let len = LEN_OVERHEAD + frame.body.len() as u32;` on the large-body path. A body between 64 MiB and 4 GiB writes a header the receiver rejects (`BodyTooLarge`, tearing down the connection with the *receiver* blamed); a body ≥ 4 GiB wraps and writes a corrupt length. `codec::encode` / `encode_correlated` (`codec.rs:26-28`, `:83-91`) and `send`/`send_unflushed`/`send_correlated` (`framing.rs:186-215`) are all infallible on size. The client writer task (`felis-client/src/main.rs:1735-1746`) calls `write_frame` directly. - **Receiver-side cap is the only cap.** `DEFAULT_MAX_BODY = 64 MiB` (`frame.rs:19`), enforced at `FrameReader` (`framing.rs:64`) and `decode_with_ceiling`; REQ-105 (`spec.md:58`) documents only the receive side. - **Semantic limits exist for some operations, not the ones listed.** Present: `MAX_ENV_BASE_ENTRIES`/`_BYTES` (session.rs; `ipc.md:703-707`), `MAX_SESSION_TAGS`/`MAX_TAG_BYTES` (`ops.rs:267-270`), `MAX_CARRIER_*` (`preface.rs:290-295`), `MAX_IMAGE_CHUNK_PAYLOAD` (`image.rs:46`), `LinkText::CAP`. Absent: `InputMsg::KeyBytes(Vec<u8>)` / `Paste(Vec<u8>)` (`input.rs:85-88`), `SearchMsg::Query.query: String` (`search.rs:19-24`), `SpawnArgs.command/args/cwd/env` (`session.rs:117-129`), `RetargetTarget` strings (`ops.rs:299`, endpoint path / SSH destination and args), bridge request lines (`cli_bridge.rs:118` `BufReader::lines()` has no length cap; that surface is #21's). - One nuance: the issue says the 64 MiB cap is "treated as every operation's policy". More precisely nobody *chose* it for those operations; the absence of a semantic limit makes the framing backstop the effective one. ## Verdict **accept.** No principle question; this is the "strictness at the edge" the design values claim. One refinement on placement: the limits must live in `felis-protocol` (below daemon/client policy), which is also what #49 requires for receiver-side limits, so create the module once and let #49 extend it. ## Approach **felis-protocol** - `frame.rs`: `Frame::encode_to(&self, out) -> Result<(), FrameError>` and `encode()` likewise, returning `FrameError::BodyTooLarge { body_len, ceiling }` when `self.body.len() > DEFAULT_MAX_BODY as usize` (checked before the `u32` narrowing, so the ≥ 4 GiB case is caught by the same branch). Keep `Frame` a plain view; the check is at encode time, not construction, so the daemon's fan-out of one body to many peers pays it once per write, which is where the header is formed. - New `messages/limits.rs` (re-exported from `messages.rs`), one `pub const` per surface with a doc line citing the reason for the number: - `MAX_KEY_BYTES = 4 KiB` (one key event or IME commit; kitty keyboard encodings are tens of bytes; an IME commit of a paragraph is ~1 KiB). - `MAX_PASTE_BYTES = 16 MiB` (the largest paste a clipboard realistically holds; must be ≤ #15's `PTY_INPUT_BUDGET` or split; pick the budget after #15 settles, and pin the relation with a `const _: () = assert!(...)`). - `MAX_SEARCH_PATTERN_BYTES = 4 KiB` (the `regex` crate compiles under its own size limit; this bounds the request, not the automaton). - `MAX_SPAWN_ARGV_ENTRIES = 4096`, `MAX_SPAWN_ARGV_BYTES = 1 MiB` (mirrors the env caps; Linux `ARG_MAX` is 2 MiB for argv+env together), `MAX_SPAWN_PATH_BYTES = 4 KiB` for `command` and `cwd` (`PATH_MAX`), `MAX_SPAWN_ENV_*` reuse the env-base caps. - `MAX_RETARGET_DESCRIPTOR_BYTES = 64 KiB` across the carrier's strings (an SSH destination plus `--ssh-arg`s). - `MAX_BRIDGE_LINE_BYTES = 32 MiB` (defined here so #21 and the CLI share it; ≥ a JSON-escaped `MAX_PASTE_BYTES` plus envelope). - `WireError::OverLimit { field: &'static str, len: usize, cap: usize }` raised from the `TryFrom<v1::*>` conversions (`convert/*.rs`) for every field above, so a daemon or client decoding an over-limit body gets a typed, connection-local error before any use. The same constants are checked on the *send* side by a `fn validate(&self) -> Result<(), WireError>` on each domain enum, called from `FrameWriter::send*` and by the CLI/bridge before it builds the request. Encoders stay infallible (`to_wire`); the check precedes them. **felis-transport**: `write_frame` returns `TransportError::BodyTooLarge` from the new `FrameError` on both the scratch and large-body paths, before any header byte is written (so a refused frame leaves the stream consistent and the connection usable). `send*` propagate `WireError::OverLimit` as `TransportError::Wire`. **felis-daemon**: the outbound path (`write_event_frame`, `serve.rs:1382-1398`) treats `BodyTooLarge` as a daemon bug: log at `error` with the event's kind and length, evict the subscriber (same path as the gauge eviction), never panic. Audit the largest emitted bodies against the cap: a `RowDelta` batch at 2048×2048 with a pathological row codec expansion, and a `RegionMsg` reply; add a chunking guard where a batch can approach 64 MiB. Inbound: `OverLimit` on `Search` → `Conn::Error { InvalidRequest }` on the stream; on `Create`/`Retarget` → `AttachFailure::SpawnFailed` / ops error reply with the field name in `detail`; on `Input` (fire-and-forget, no reply channel) → connection-local close with `ConnError::Wire`, since a conforming client checks before sending. **felis-client-core / felis-cli**: `sessions send`, `sessions spawn`, `search`, and `switch`/retarget check `validate()` before dialing and report the CLI `invalid_request` machine error kind (already in the vocabulary, CHANGELOG Unreleased), naming the limit; the bridge emits the same kind as an error object on the request id (`error_object`, `cli_bridge.rs:194-197`), so machine consumers keep a parseable line. The GUI's paste path clamps to `MAX_PASTE_BYTES` and shows the existing confirm overlay's label path for "paste truncated" or refuses; choose refuse (explicit over silent truncation, principle 4's spirit). **Tests** - `frame.rs`: body of exactly `DEFAULT_MAX_BODY` encodes; `DEFAULT_MAX_BODY + 1` returns `BodyTooLarge`; the same pair on `FrameWriter::write_frame` through a `Vec<u8>` sink asserting zero bytes written on refusal (a 64 MiB zeroed `Vec` is fine for nextest; mark the test `#[ignore]`-free but single-threaded if memory-bound on CI). - `limits.rs`: at-limit / one-past for each constant through `codec::encode` + `decode` round trip (send-side `validate` and receive-side `TryFrom` both), plus `const _` assertions that every semantic limit is below `DEFAULT_MAX_BODY` and `MAX_PASTE_BYTES ≤ PTY_INPUT_BUDGET`. - CLI: `felis sessions send` with an over-limit payload exits `1` with `{"error":{"kind":"invalid_request",...}}`; bridge golden line. - Fuzz: extend the existing framing fuzz target to the encode direction (`just fuzz*`). **Docs cascade** - `docs/reference/ipc.md`: new "Semantic limits" table directly under "Frame layer" (limit, value, applies to, checked by sender / receiver, outcome), stating explicitly that the frame cap is a framing backstop and these are the operation policies; "Session" and "Search" sections reference it. - `docs/reference/spec.md`: REQ-105 gains "enforced on encode as well, before integer narrowing"; new REQ-105a listing the semantic limits. - `docs/reference/cli.md`: per-verb limit and the `invalid_request` mapping; `docs/reference/testing.md` "Security tests" → "Frame length ceiling" extended to the encode path. - `docs/explanation/security-model.md` "Daemon IPC": one bullet on why per-operation limits exist beside the framing cap (rejected alternative: a single cap per family). - `CHANGELOG.md`: new limits (user-affecting: paste size, argv size, search pattern), `invalid_request` on over-limit. - `skills/felis/SKILL.md`: the limits a script must respect for `sessions send`, `spawn`, `search`, and the bridge line cap. ## Dependencies - None blocking. Coordinate: **#49** extends the same `limits.rs` (land #16 first); **#15** fixes `PTY_INPUT_BUDGET` relative to `MAX_PASTE_BYTES`; **#21** consumes `MAX_BRIDGE_LINE_BYTES`; **#23** owns the machine error-kind vocabulary (`invalid_request` already exists, so no new kind is needed unless #23 renames). #12's order holds. ## Risk/effort **M.** Mechanical across four crates; the judgment is in the numbers. Main risk: a cap that breaks a real workflow (a 20 MiB paste, a 5000-entry argv); keep the numbers generous and record the rationale beside each constant so a later change is a one-line decision, not a re-investigation. ## Labels Keep `priority/P0`, `release/v0.1.0`: the limits become part of the frozen wire contract (#12 "Contract freeze boundary"), so they must be chosen before the tag even though the `u32` truncation itself is a low-likelihood bug. ## Review amendments (round 2) - `MAX_PASTE_BYTES = 16 MiB` and #15's `PTY_INPUT_BUDGET = 16 MiB`, related by a compile-time assert `MAX_PASTE_BYTES <= PTY_INPUT_BUDGET` in this module. "Or split" is withdrawn; there is no chunking.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
ci/darwin-smoke
No results found.
Labels
Clear labels   
priority/P0
Must be resolved before the v0.1.0 tag

  
priority/P1
Must be settled before the first public contract freezes

  
priority/P2
Explicitly deferred until after v0.1.0

  
release/v0.1.0
Work tracked for the first tagged release

  
status/blocked
Cannot start now: waits on an upstream release, another issue, or a recorded revisit trigger

  
status/planned
Triaged with a source-grounded plan and a passed pi review; ready to implement

  
type/bug
Shipped behavior that contradicts the docs or the code's own intent; needs a product code change

  
type/design
An open scope or trade-off question; needs a recorded decision in the owning explanation doc before code

  
type/test-gap
Documented or reviewed behavior that no test pins; usually split from a review finding, needs no product code change

  
type/tracker
Parent issue that tracks child work

No labels
priority/P0
priority/P1
priority/P2
release/v0.1.0
status/blocked
status/planned
type/bug
type/design
type/test-gap
type/tracker
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
natsukium/felis#16
No description provided.