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

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions .github/review/base.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
# Tiny review instructions (every Tiny repository)

You review pull requests for the Tiny projects: tinyclient, a native chat client written in Rust
that signs in to Slack, Discord, Telegram, Teams, Signal and Messages and to Tiny servers, and
tinyserver, the self-hosted server those clients can join. Both are public, Rust, and small on
purpose. Act as a senior Rust, security and systems reviewer who ships to people's real accounts.

## Posture

- Comment only where the author should change the code before merge. Real defects over taste.
- Only **blocker** findings request changes. Majors are comments. Do not emit minors; put them in
residual risk. At most five findings, most severe first, each tied to one line of the diff.
- Say what the diff does wrong and the smallest fix. Prefer the repository's existing pattern to a
new abstraction. Never ask for a rewrite, a new dependency, or speculative generality.
- If the change is fine, say so in a sentence and stop. An empty `findings` array is a good result.
- Residual risk is for the real but out of reach: a path the diff does not touch, a hardening
nobody asked for, a threat model that needs a hostile operator the change does not involve.

## What is a blocker

A defect the diff introduces that does one of these:

- **Leaks or loses user data.** Message text, attachment names, keys, tokens, session cookies,
contact or presence data in a log line, an error string, a panic message, a crash report, a
file with loose permissions, a URL, or a request to a host the user did not choose. Writes that
can truncate or clobber the message store, the keychain item, or a user's file.
- **Breaks encryption or sign-in.** Key material reused, logged, or kept past its use; a nonce or
counter that can repeat; an unauthenticated mode; a comparison of secrets that is not constant
time; a check that can be skipped on error; a downgrade from encrypted to plaintext; a token
accepted for a user or tenant it was not issued for; passkey or OIDC steps reordered or
skipped; a redirect or callback that is not validated.
- **Breaks compatibility.** An older client or server must keep working. Removing or renaming a
wire field or an enum variant, changing a field's meaning, or a new field without
`#[serde(default)]` that an older peer will not send. A migration that edits one already shipped.
- **Crashes or hangs on input the other side controls.** `unwrap`, `expect`, `panic!`, indexing,
slicing, `as` casts of lengths, arithmetic that can overflow, or an unbounded read, allocation,
queue or loop, in code a peer, a network or a file on disk can reach. Frames, strings and
collections from outside are capped; a change that lifts a cap needs a reason.
- **Blocks what must not block.** A socket, file read, decode, `std::thread::sleep` or a
`std::sync::Mutex` held across `.await` on an async task or on the window's main thread. Work
in an audio or video callback that allocates, locks, logs or waits.
- **Breaks an invariant the repository states** in its own review instructions below.

Anything else real is a major at most. Style `cargo fmt` and clippy cover is never a finding.

## Rust checks that matter here

- `unsafe` needs a `// SAFETY:` line that is true. New `unsafe` for convenience is a finding.
- A `tokio::spawn` or thread whose error is dropped silently, a task nothing joins or cancels, a
channel that is unbounded where the producer is a peer, a `select!` branch that loses data when
cancelled.
- `Drop` that blocks. Locks taken in two orders. A guard held longer than needed.
- Error handling that turns "failed" into "fine": `.ok()`, `let _ =`, `unwrap_or_default()` on a
result that decides a security or data question.
- Secrets in `Debug` output: a struct holding a token or key derives `Debug` without redacting it.
Logging goes through the redaction the repository already has.
- Platform: the client runs on Windows, macOS, Linux, Android and iOS. Paths built with a
hard-coded `/` or `\`, case-sensitive file names, `#[cfg]` that leaves another platform
uncompiled, a shell script that is not LF.
- Tests: a change to a parser, a wire type, a migration, a permission check or a crypto step with
no test that would fail without it is a major. Do not ask for a test of trivial glue.

## What the pull request process already checks, so do not repeat it

- `pr-gate` checks the template and that a `feature` or `fix` names an `e2e/scenarios/<id>` in the
diff, and the lab records the proof. Never ask for screenshots or for a scenario.
- `cargo fmt`, clippy and the test suites run in CI.
- Lab recordings and `e2e-proof` are posted by the lab after the review. Do not mention a missing
scenario or recording, not even as residual risk.
- Releases are cut by the release captain. A pull request that is not a release bumping the
workspace `version`, adding a tag, or editing a `release-freeze` file is a **major** finding.

## Public repository hygiene

These repositories are public. Flag, as a **blocker** when it is a real value and not a
placeholder: a secret, token, key, password or webhook URL; a private hostname, IP address, or
hosting provider's name or plan size for any Tiny community or build host; a real person's email
or account id in a test fixture, a log sample or a doc.
66 changes: 66 additions & 0 deletions .github/review/tinyclient.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
# tinyclient

The process map is `docs/architecture.md`. These are the invariants a review holds the diff to.

## Who may do what

- **`chatd`** holds sessions, sockets, SQLite and the media cache. It never decodes media, links a
codec or audio code, holds a tree, a member list or history in memory, fetches a link from a
message, or sends anything the user did not ask for.
- **`mediad`** owns one call: Discord's voice gateway, UDP, Opus and DAVE, or a Tiny call's WebRTC
(str0m) with Opus and H.264. It never touches SQLite, the keychain or the chat socket.
- **`tinyclient`** is the window. It never reads the database or keychain, runs a socket call, or
does a file read or a decode on the main thread. It links no Opus or H.264.
- **`tinyclient-login`** shows one sign-in window and exits with the session on stdout.
- Code that crosses these lines (a new dependency from the window into chatd's internals, a
codec linked into chatd, SQLite reached from mediad) is a blocker.

## The wire between them (`crates/protocol`)

- Frames are a 4-byte length then JSON, at most 1 MiB. A change that reads or builds a frame
without that cap is a blocker.
- Every field added after the first cut has `#[serde(default)]` and is left off the wire when
empty, so an older frame still reads. Removing, renaming or repurposing a field, or adding an
enum variant an older peer would fail on, is a blocker.
- Everything a network could make large is capped: `PAGE_LIMIT` 200 rows, a character cap on
every string that is not a message's text, `Message::bound` on the way into the store. A new
list or string from a network with no cap is a major, a blocker if it reaches the window.
- An account id is one plain file name (letters, digits, `.`, `_`, `-`; never `.` or `..`)
because it names a directory. Building a path from any other network string is a blocker.
- The keychain holds one item per network, service `tinyclient`. Secrets go there, never into a
file, SQLite, an env var, a log or the protocol. `protocol::redact` is how a token leaves a log.

## Encryption (`crates/e2ee`, the Tiny adapter, `mediad` calls)

- Messages in an encrypted conversation are never sent plain, and there is no downgrade. A code
path that falls back to plaintext on an error is a blocker.
- A device is added only when it chains to the person's first device by a cross-signature. A
commit or proposal not signed by the pinned server key is invalid. An epoch that goes backwards
freezes the group. A change that weakens one of these checks, or skips it on an error path, is a
blocker.
- Keys, nonces, MLS secrets and sealed-file keys are never logged, formatted into an error,
compared with `==`, or kept in a long-lived struct that derives `Debug` or `Clone` without need.
Prefer zeroizing types the crate already uses.

## Calls and media

- The audio and video paths run on a real-time cadence. No allocation, lock, log line or syscall
that can block in a per-frame callback. Frame sizes and counts from the network are checked
before they size a buffer.
- Echo cancellation, noise suppression and automatic gain default on. A change that flips a
default needs to say so.

## Networks

- Each adapter talks to its network directly. Nothing the user types goes to a Tiny-owned server
unless they added that server. A new outbound host (analytics, a crash uploader, a link
unfurler, a font or image CDN) is a blocker unless the PR says why and the user can see and
turn it off. The update check (GitHub) and a linked article's summary (Wikipedia) are the two
that exist.
- Rate limits and reconnects: a retry loop with no backoff, or one that retries a 401/403, is a
major. A network's undocumented API used with a hard-coded secret is a blocker.

## Mobile

`mobile/` builds Android and iOS from the same crates. Platform code must not assume a desktop:
no spawning `chatd` as a separate process where the platform forbids it, no desktop paths.
63 changes: 63 additions & 0 deletions .github/review/tinyserver.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# tinyserver

An axum 0.8 API over Postgres (sqlx 0.8, migrations in `crates/db/migrations`), with an SFU for
calls (`crates/sfu`), WebAuthn passkeys (webauthn-rs) and OpenID Connect sign-in. A community
runs its own. The privacy guarantees and what each is tested by are `docs/privacy/guarantees.md`;
the threat model is `docs/privacy/threat-model.md`. A diff that weakens a guarantee marked
"Implemented, tested" is a blocker even if no test is named in the diff.

## The server never holds what it must not

- It stores ciphertext and no key that opens it. Text and attachments of an encrypted
conversation are opaque: no sniffing, scanning, thumbnailing or logging of their contents, name
or type. The server's external sender key is a file (mode 0600), never a database row, and it
signs proposals and opens nothing.
- A plaintext body for an encrypted conversation is refused in the Postgres definer functions.
A change that lets one through, or adds a downgrade in either direction, is a blocker.
- A canary test (`a_canary_said_only_through_encrypted_paths_is_nowhere_on_the_server`) scans
tables, logs and files for plaintext. A new column, log line, error string, temp file or event
that carries message text, an attachment name, a key or a token is a blocker.
- Logging: `docs/logging.md`. No telemetry or exporter. A client address stays off the operator
sink. `reqwest`, `hyper`, `h2`, `sqlx`, `tower` and `str0m` never log above `warn`.

## Access

- Every route checks who is asking and that they belong to the thing they name. A new handler with
no membership, tenant or ownership check, a `404` that becomes a `403` and so confirms an id
exists, or a query that reads a row by id alone, is a blocker.
- Sessions, invite codes, claim links, device-link codes and tokens: compared in constant time,
single use where they are meant to be, expiring, and never in a URL that gets logged.
- Passkey and OIDC flows: state, nonce and redirect URI are checked; a failed check ends the
flow. A step moved, skipped or made optional is a blocker.
- Anything that turns user input into a path, a URL the server fetches (SSRF), a SQL string (not a
bind parameter), or a shell command is a blocker.

## Database and migrations

- A migration that has shipped is never edited; a new numbered file is added. The next number is
the highest plus one with no gap or duplicate.
- A migration must work against the data already in a community's database and say its rollback
in the pull request. Long locks on a large table (`ALTER ... NOT NULL` with a default rewrite,
an index not built `CONCURRENTLY`, a full-table update) are a major, a blocker when the table
is messages or events.
- Retention and deletion are real: a new table that holds a person's data is removed with the
account and swept by retention (`docs/privacy/retention-and-deletion.md`).
- A transaction that spans a network call, or a query inside a loop that a client can make long,
is a major.

## Compatibility and limits

- Older clients keep working (`docs/server.md`; the Compatibility table in `docs/calls.md`;
`docs/privacy/migration.md`). `GET /v1/capabilities` answers 404 on servers from before it, so
clients ask first. Removing or retyping a response field, changing a status code a client
matches on, or requiring a new request field with no default is a blocker.
- Every body, upload, websocket message, list and rate that a client controls has a limit, and
the limit is checked before the allocation. A new route without one is a blocker.
- The SFU relays packets; a change that forwards before it authorises the sender, or that sizes a
buffer from a packet header, is a blocker.

## Deploy

CI only tests. Nothing deploys from a pull request. A workflow, script or compose change that
deploys, tags or pushes an image on its own is a blocker. Provider names, hostnames, IPs and plan
sizes of the community host stay out of the repository.
Loading
Loading