Operator hooks: SSO sign-in, sign-out-everywhere, trusted proxies, frame ancestors, /metrics, STT endpoint, reminder webhook, fr-CA - #30
Open
distronode-com wants to merge 21 commits into
Conversation
Calnode's SQL is hand-written and portable apart from placeholder syntax, so supporting a second engine is mostly a matter of not spreading "? vs $n" across 763 call sites. This adds the layer that hides it: - Dialect (sqlite, postgres) selected from the DATABASE_URL scheme, with Dialect.SQL for the few statements that genuinely cannot be shared. - Rebind: a small lexer, not a string replace. A ? inside a string literal, a quoted identifier or a comment is data, and renumbering one corrupts the statement in a way that only shows up at runtime. - DB/Tx wrapping sql.DB/sql.Tx, rebinding every statement on the way through. Query, Exec, QueryRow, their Context forms, Prepare and Begin/BeginTx are all covered, so a call site moving onto the wrapper is a type change and nothing else. Begin returns *db.Tx rather than *sql.Tx deliberately: a transaction that quietly stopped rebinding is the easiest way to reintroduce ? on Postgres. - OpenDB returns the wrapper; Open still returns the bare *sql.DB so nothing outside this package changes yet. The SQLite path is untouched: same pragmas, and the one-connection pool stays because it is the correctness guarantee behind the booking overlap check (ARCHITECTURE section 17), not a tuning choice. Postgres gets an ordinary pool, and openPostgres records what that costs so the gap is not rediscovered later. Migrations move to migrations/sqlite/ unchanged (git detects all 57 as renames); the postgres set lands in the next commit. is_applied is now tested for truth rather than compared to 1, which is the one spelling both engines accept -- goose stores it as INTEGER on SQLite and BOOLEAN on Postgres. pgx v5.10.0 is the driver, via database/sql (stdlib), so nothing here depends on the pgx-native API.
57 files in migrations/postgres, one per SQLite migration and numbered
identically, embedded alongside them. Open picks the directory and the goose
dialect from the DSN, so nothing else has to know which set is in use.
What the translation does, and what it deliberately does not:
- Flag columns stay integers (INTEGER -> SMALLINT), never BOOLEAN. Those columns
are scanned into Go ints across the codebase, so changing the type would break
every one of those scans. Columns that hold real numbers stay INTEGER. A test
reads information_schema and fails on a boolean.
- Timestamp defaults keep TEXT and keep SQLite's exact string format:
strftime('%Y-%m-%dT%H:%M:%fZ','now') becomes
to_char(now() AT TIME ZONE 'UTC', 'YYYY-MM-DD"T"HH24:MI:SS.MS"Z"') and
datetime('now') becomes the seconds-precision equivalent. TIMESTAMPTZ was the
alternative and is wrong here: every time value in this schema is written,
compared, sorted and paginated as a string, and database/sql would hand those
columns back formatted as RFC3339Nano, which sorts differently and would not
match a stored value. A test asserts both formats against the shapes SQLite
writes.
- Partial unique indexes carry over verbatim; a test proves idx_bookings_no_double
still refuses a second booking at the same start time and still allows re-booking
a cancelled slot.
- crypto_keystore.id relied on SQLite's rowid (keyvault.go inserts without an id),
so it becomes BIGINT GENERATED BY DEFAULT AS IDENTITY. server_settings.id does
not: it is seeded explicitly and pinned by CHECK(id = 1), where a sequence would
only mislead. BLOB becomes BYTEA.
- The three table rebuilds become what Postgres can do directly: 00012 drops the
four columns, 00014 and 00042 replace a constraint in place. 00042 therefore
needs neither NO TRANSACTION nor the foreign_keys PRAGMA toggle, both of which
existed only to make SQLite's rebuild safe.
- lower(hex(randomblob(16))) becomes replace(gen_random_uuid()::text, '-', ''),
the same 32 lowercase hex characters with no extension to install.
INSERT OR IGNORE becomes ON CONFLICT DO NOTHING.
- Downs that are no-ops on SQLite stay no-ops here even where Postgres could drop
the column. A down that lands on a different schema per engine is worse than one
that lands on none, and it would make the following up fail on one engine only.
Tests are opt-in on CALNODE_TEST_POSTGRES_DSN and skip cleanly when it is unset,
so upstream CI and any contributor running SQLite is unaffected. Each test runs in
a schema of its own, created and dropped around it, so runs neither collide nor
leave anything behind in an operator's database.
Every other cross-engine test asserts that one specific thing survived the translation. This one asserts that nothing else changed: it migrates SQLite and Postgres to 57 in the same process and compares the table list and each table's column names, so a column quietly dropped, renamed or added in one set fails here rather than in whichever handler reads it. Names only, deliberately. TEXT versus text and SMALLINT versus integer are the translation doing its job, and the types that do matter are already pinned by TestPostgres_flagColumnsStayIntegers. The table-count floor is there because an empty map on either side would satisfy every comparison in the test.
The dialect-aware wrapper rebinds ? to $n on Postgres and is a no-op on SQLite,
so the only way the rest of Calnode benefits is to hold the wrapper rather than
the bare *sql.DB. Method names match database/sql, so this is a type change at
the declarations and almost nothing at the 763 call sites.
Three places did need a decision rather than a substitution:
- internal/connstore.Execer is an interface over QueryRowContext, so both the
wrapper and a bare *sql.DB satisfy it; only its doc comment changed.
destination_test.go opens the sqlite driver directly against a bespoke
fragment schema, and stays on *sql.DB for that reason.
- handler.Readyz passes h.db.DB to db.SchemaReady, which takes a bare pool.
Safe: that statement has no placeholders, so there is nothing to rebind.
- cmd/calnode moved from db.Open + db.Migrate(database) to db.OpenDB and the
handle's own Migrate method, which is what carries the dialect.
SQLite behaviour is unchanged: Rebind returns its input untouched there.
Timestamps first, because they are 38 of the 44 sites. datetime('now') and
strftime('%Y-%m-%dT%H:%M:%fZ','now') are now computed in Go and bound as
parameters, which removes the dialect difference rather than encoding it and
makes the value something a test can predict.
internal/dbtime holds the two layouts. Two, not one: the schema already stores
both shapes, and they are load-bearing. recordings.created_at is the space-
separated form and meeting_consents.decided_at the millisecond form, and
consentWindow converts between them to build a lexicographic BETWEEN; notes'
updated_at is handed to a client verbatim; jobs.run_at written by enqueueJob is
deliberately the space form, which sorts before every T-separated run_at and is
what makes those jobs due immediately. Normalising would have been tidier and
would have quietly changed all three. dbtime_test asserts both layouts against
SQLite's own output rather than against a reading of its documentation.
The rest:
- INSERT OR IGNORE becomes ON CONFLICT DO NOTHING with no conflict target,
which both engines accept and which keeps OR IGNORE's "any unique
constraint" scope.
- COLLATE NOCASE becomes LOWER(col) = LOWER(?). No dialect pair, because
nothing indexes booking_attendees.email: there is no collation for an index
to depend on, so there is no plan to preserve.
- demo.Reset's PRAGMA foreign_keys is SQLite-only now. Postgres has no
equivalent without superuser rights, so that branch wipes with a single
TRUNCATE ... CASCADE over every table, which needs no delete ordering at
all. sqlite_master becomes pg_tables scoped to current_schema(), the one
Dialect.SQL pair added here — current_schema() also keeps it right inside an
isolated test schema.
RETURNING is left alone: both engines support it. No UPDATE or DELETE carries a
LIMIT; that was checked by scanning every backquoted string in the tree rather
than grepping single lines.
SQLite behaviour is unchanged, stored bytes included.
ARCHITECTURE §17 says the app-level double-booking check is free of TOCTOU races because SetMaxOpenConns(1) serialises every transaction. That is true and it is a SQLite property. On a Postgres pool two overlapping bookings can both read "host is free" and both insert, and idx_bookings_no_double is UNIQUE(host_id, start_at) so it only catches an IDENTICAL start time — 10:00-10:30 against 10:15-10:45 are two distinct keys and both inserts satisfy it. lockHosts takes pg_advisory_xact_lock on every host whose availability the transaction is about to decide, before the first hostBusy read. The lock is released by the transaction ending, so there is no unlock to forget and no leak down the several early-return paths these functions have. Per host rather than global, so bookings for different hosts stay concurrent — otherwise this would just reinstate the single writer it replaces. On SQLite it returns immediately and nothing changes. The key is SHA-256 of "calnode:booking:host:" + id, first eight bytes big-endian as int64. Derived in Go rather than with hashtext() so it is readable and testable from Go, and domain-separated so a later advisory lock on some other entity cannot collide by hashing the same raw id. Ids are sorted and deduplicated on a COPY before locking: two transactions needing the same two hosts in opposite orders would deadlock, and Create's HostIDs arrive in round-robin priority order, which decides who gets the booking. Three call sites, where the packet named two. ReassignHost has the identical check-then-write shape — hostBusy's own doc comment names all three write paths — and leaving it out would have left a known double-booking hole. Reschedule locks the primary host BEFORE reading booking_hosts rather than after, because a concurrent ReassignHost holds that same key, and that ordering is what stops a reassignment committing between the host-list read and the UPDATE. isUniqueViolation now matches SQLSTATE 23505 as well as SQLite's message text. It was a substring match on "UNIQUE constraint failed", so on Postgres the index backstop would have surfaced as a 500 rather than ErrDoubleBooked. SQLSTATE because the message is localised by the server's lc_messages. Measured against PostgreSQL 17, 40 rounds of two goroutines racing overlapping slots: with the lock, 40 created / 40 conflicts / 0 overlapping pairs in the database. With the lock disabled, 79 created / 1 conflict / 39 overlapping pairs. The unlocked run is why the index cannot be described as a sufficient guard: it caught one race in forty. internal/dbtest arrives with this rather than with the docs commit, because the committed version of that test needs it. Each test gets its own calnode_test_<random> schema via search_path in the DSN, dropped on cleanup; the test that asserts the isolation actually reaches the server is in that package, because a search_path that silently stopped being forwarded would migrate every package into public and read as flakiness.
22 test helpers across 18 files moved from db.OpenDB("sqlite://:memory:") +
Migrate() to dbtest.Open(t), so which engine the suite exercises is an
environment variable rather than 22 hardcoded DSNs. With
CALNODE_TEST_POSTGRES_DSN unset — every local run, and the existing CI job —
nothing changes: it is the same in-memory SQLite as before.
Four helpers stay on SQLite deliberately: dbtime_test pins SQLite's own timestamp
output, hostlock_internal_test asserts the advisory lock is a no-op there,
connstore/destination_test opens the bare driver against a bespoke fragment
schema, and two health_test cases want an UNMIGRATED database. keyvault_test
keeps its shared-cache DSN, which it has for a reason.
ARCHITECTURE §4 is amended rather than extended, because a new section would have
sat next to prose asserting the opposite. The heading covers both engines; the
pool difference is stated along with the way the wrapper can be bypassed
silently; the rows-cursor gotcha is scoped to SQLite together with the reason the
materialise-first pattern has to stay regardless; and a new subsection states the
double-booking guarantee per engine — the single connection on SQLite, the
advisory lock on Postgres — plus what the partial unique index actually covers,
with the measured 39-double-bookings-without-the-lock figure. §17's first gotcha
and one now-engine-conditional aside in §8 were pulled into line with it.
The new CI job runs the Go half against a postgres:17 service. Separate job, not
a matrix, because only Go is engine-dependent and svelte-check would otherwise
run twice. It carries a pg_isready health check: without one the first connection
races the server's startup and fails as "connection refused", which reads like a
bad DSN rather than a timing problem.
That job will be RED until internal/db/migrations/postgres exists — the database
layer is another worker's file set and it has only the sqlite directory today.
Measured rather than assumed: with the DSN set, dbtest.Open reports "run
migrations: migrations/postgres directory does not exist".
Thirteen sites decided between 409, 400, 404 and 500 by substring-matching
SQLite's error text. On PostgreSQL none of them matched, so a duplicate slug, a
replayed idempotency key and a bad enum value all fell through to
{"error":"internal error"} with a 500. Seven Postgres test failures were this.
internal/db now exports IsUniqueViolation, IsCheckViolation and
IsForeignKeyViolation. PostgreSQL is matched on SQLSTATE (23505 / 23514 / 23503)
because the message is localised by the server's lc_messages, so a server running
in a non-English locale would defeat any text match no matter how carefully it was
written. SQLite keeps a text match, because modernc.org/sqlite does not expose a
code on its error value — but it is now in exactly one place instead of thirteen,
and it is pinned by a test.
A *pgconn.PgError whose code does not match returns false rather than falling
through to the text comparison. Falling through would classify a PostgreSQL error
by whether its message happened to contain another engine's English phrase, which
is the fragility being removed.
The test provokes a REAL violation of each class against the real migrated schema
on whichever engine dbtest is configured for — a duplicate users.email, a booking
with a status the CHECK forbids, a booking referencing a nonexistent event type —
rather than constructing an error value, because a constructed error only proves
the predicate agrees with what the author believed the driver returns, and that
belief being wrong is why this commit exists. It also asserts exactly ONE of the
three predicates matches each violation, and that a missing-table error and a
plain errors.New match none: a predicate that answered true for everything would
have made all thirteen call sites pass and been badly wrong.
internal/booking and internal/handler keep thin local wrappers where a local name
read better at the call site; their bodies now delegate.
grep -rn "constraint failed" --include='*.go' internal/ cmd/ returns three hits,
all of them the constants inside internal/db/constraint.go.
With the constraint predicates in, eight Postgres failures were left, and they were
four unrelated causes rather than one.
Computed booleans. "(user_id = ?) AS owned" and "(archived_at IS NOT NULL) AS
archived" are 0/1 on SQLite and a boolean on PostgreSQL, and the boolean does not
scan into the int the real 0/1 columns beside them use — "converting driver.Value
type bool to a int". Both are CASE WHEN ... THEN 1 ELSE 0 END now: portable, and
the same 0/1 convention the schema uses for stored flags, so the != 0 idiom in the
scanner is untouched.
json_extract in production. replaceReminderJobs deleted a booking's reminder jobs
by filtering on json_extract(payload, '$.booking_id'), a SQLite JSON1 function. On
PostgreSQL the DELETE errored, so rescheduling silently left the old reminder in
place — a real bug, not just a red test. No portable spelling exists, so this is a
Dialect.SQL pair: payload::json ->> 'booking_id', with the cast because the column
is TEXT rather than json.
ORDER BY rowid. webhook_deliveries had no timestamp of its own, so "the 50 most
recent" was rowid DESC. PostgreSQL has no rowid, and the query was not strictly
correct on SQLite either: rowid tracks insertion order only until something
renumbers it, and VACUUM may. Migration 00058 adds created_at to both dirs, the
writer binds it, and the order is created_at DESC, id DESC — the tiebreak because
two deliveries written in the same millisecond would otherwise come back in
whatever order the engine felt like. The default is a constant '' rather than a
timestamp expression because SQLite's ALTER TABLE ADD COLUMN forbids a
parenthesised DEFAULT; rows predating the migration sort last, which is where the
oldest deliveries belong.
randomblob in a gcal test helper: SQLite's id generator, swapped for uid.New(),
which is what every other row in those tests already used.
Two test changes, neither of them an assertion:
- reschedule_test held its own copy of the json_extract expression AND discarded
the Scan error, so "function json_extract does not exist" surfaced two seconds
later as time.Parse failing on "". It now uses the same dialect pair and
reports the query error, so the next engine difference names itself.
- postgres_test pinned the migration count as the literal 57 in two places. It is
now one named constant at 58, so adding a migration is a single edit that
cannot be half-done.
go test -count=1 ./... is green on both engines from this commit: 28 of 28 packages
on SQLite, 28 of 28 on PostgreSQL.
The comment in constraint.go claimed modernc.org/sqlite exposes no error codes. That was wrong. It defines type Error with Code(), populated for every constraint class, and the codes are SQLite's extended result codes. Measured against a real in-memory database rather than read off a doc page: UNIQUE 2067, PRIMARY KEY 1555, CHECK 275, FOREIGN KEY 787, NOT NULL 1299 ⛔ The reason this is worth a commit rather than a tidy-up: a PRIMARY KEY collision reports 1555, NOT 2067, while still saying "UNIQUE constraint failed" in its message. The text match caught both by accident. Switching to Code() == 2067 alone would silently stop recognising primary-key collisions — and Calnode has one that matters: idempotency_keys.idempotency_key is a bare PRIMARY KEY, so every idempotent replay arrives as 1555. Verified by removing 1555 and re-running: the new subtest fails naming the code and the table, and TestCreateBooking_idempotentReplay goes to "replay: 500". So the regression was real, not theoretical. IsUniqueViolation therefore matches BOTH 2067 and 1555. PostgreSQL needs no equivalent change: a primary-key collision is 23505 like any other unique violation, which is why the trap exists on only one side. Constants are named after the SQLite symbols so the numbers are greppable. The text comparison stays as a fallback, and only as a fallback. It covers an error that arrives without its concrete driver type attached — a driver release that changes the type, a layer that reformats rather than wraps — where the message is the only signal left and answering from it beats returning a 500. TestConstraintTextFallback exercises that branch directly, so it is not unexecuted code that reads like an accident. A *pgconn.PgError or a *sqlite.Error whose code does not match is a DEFINITE no and does not fall through: falling through would reintroduce this very trap in reverse, readmitting a 1555 by its message after excluding it by code. Also fixes a flake in the dbtest harness, which is what the two intermittent handler failures under `go test ./...` actually were — not an application fault. Calnode's handlers do several things fire-and-forget (notify hosts, enqueue webhook, enqueue reminders), those goroutines outlive the test body, and closing the pool does not stop an in-flight statement. DROP SCHEMA CASCADE needs an exclusive lock on every object, meets them, and PostgreSQL reports "deadlock detected" (40P01) — visible only under the full run, where packages compete and everything is slower, in a different pair of tests each time. The drop now runs on a pinned connection with lock_timeout set so an attempt fails fast instead of deadlocking, and retries within a bounded budget; a schema that still cannot be dropped is reported, because a leaked schema accumulates on a shared server.
Includes the measured code table, the primary-key trap and how it was proved, and the dbtest teardown deadlock that the two intermittent handler failures actually were.
GET /v1/auth/sso?token=<jwt> takes a short-lived HS256 token from an external identity system that has already authenticated the person and starts an ordinary Calnode session, so they are not asked to log in a second time. Off unless CALNODE_SSO_SHARED_SECRET is set, and 404 when it is not: an instance that has not configured this should be indistinguishable from one that does not implement it, and a feature that can mint a session must not be reachable by default. The token is verified in-tree with crypto/hmac rather than by a JWT library, for the reason internal/livekit signs its own: one algorithm, one key, a fixed claim set, nothing to keep current. HS256 is checked before the signature is compared, because accepting the token's own choice of algorithm is the alg:none downgrade. Two properties carry the security, and both are cheap only because the token is a redirect the browser follows immediately. It may live at most 60s after iat (30s of skew either way, since the two systems are separate hosts), and its jti is claimed in the new sso_nonces table BEFORE the session is created, so a replay inside the window collides on a primary key instead of racing a read-then-write. The worker purges expired nonces in the GC pass it already runs; a row past its expires_at can no longer refuse anything, so keeping it is pure growth. aud must equal BASE_URL. That is what stops a token minted for staging being spent on production when someone shares a secret between the two by mistake. This is the one path that creates a user without an invite, and that is the trade the shared secret buys. On creation the claimed role is applied. On someone who already exists it is not: a workspace's roles are the workspace's business, and a hand-off rewriting them on every sign-in would make the admin UI's role controls advisory. The single exception is bootstrapping an instance with no owner, where the one-owner invariant means there is nothing to displace. An archived account is refused here as it is everywhere else. ?next= is honoured only for a same-origin absolute path, and a bad one is refused rather than cleaned up, because a redirect built from a partially sanitised value is how open redirects survive their own fix. It is also checked before the nonce is claimed, so the caller's own bug does not burn the token. wid is parsed and ignored. A multi-workspace mode will use it to choose which workspace the hand-off lands in; accepting it now means a caller written against that does not need changing to work today.
POST /v1/auth/sessions/revoke-all. With no body it drops every session the
caller has except the one that made the request, which is the action people
actually want when a laptop goes missing: Logout already ends the current
session, and a 30-day cookie on a machine you no longer hold is the thing with
no answer. An API-key caller has no current session, so for them every row goes.
With {"user_id": "..."} it becomes an offboarding tool, on the same tiers
roles.go already enforces: an admin may revoke a member, only the owner may
revoke another admin, and the owner's sessions can be ended only by the owner
(there is exactly one owner, so that is the self branch). The actor's tier is
checked before the target is loaded, so this endpoint's 404 cannot be used to
enumerate user ids.
It also deletes the target's oauth_access_tokens rows. That is what makes it an
offboarding tool rather than a convenience: an MCP connector authenticates with a
bearer token, not the session cookie, so revoking sessions alone would leave an
agent connected with exactly the authority just withdrawn. Both deletes share one
transaction, because a caller told "revoked" must not keep a token on account of
the second statement failing after the first committed.
Rate limits key on the TCP peer, and that is correct for a directly reachable instance and useless behind a fronting CDN, where every visitor arrives from the same few addresses and shares one bucket. TRUSTED_PROXY_CIDRS opts in per network: for a peer inside one of them the client IP comes from CF-Connecting-IP, else from X-Forwarded-For, else the peer. The default does not move, and cannot be moved by a header. A peer that is not in the list has its headers ignored before they are read, so an instance that sets nothing behaves exactly as it did. Two details are the difference between this and a spoofable version: The X-Forwarded-For walk goes RIGHT TO LEFT past trusted hops and returns the first untrusted address, not the leftmost entry. The left of that header is whatever the original client sent, and every well-behaved proxy prepends to it and preserves it, so "leftmost" is a value the client picks. The rightmost non-trusted hop is the last address one of our own proxies actually observed. A hop that does not parse ends the walk and falls back to the peer instead of being skipped. Skipping would let a client inject one malformed entry to push the walk past the real hop onto a value it chose. The cost is that a proxy appending "ip:port" reads as malformed and lands on the peer, which is the safe direction. Resolution is a middleware rather than a package-level setting, computed once in the outermost layer and carried in the request context: a mutable global would leak between server instances in a test binary and would make "which requests are affected" unanswerable from the wiring. remoteIP reads the context when it is there, so the untrusted path is byte-for-byte the old function. A CIDR that does not parse is logged and dropped rather than fatal, and the other entries survive it. The consequence of dropping one is that that hop's headers are not believed, which costs shared rate-limit buckets and never a trusted forgery. audit/claims.yaml's rate-limit-keys-on-tcp-source-address said the headers are "never read", which stops being true the moment this ships. Rewritten to state the condition rather than left to rot into a false claim.
FRAME_ANCESTORS (space-separated, so it reads the same in the env var as in the header) puts Content-Security-Policy: frame-ancestors <list> on the handler under /admin/. Wanted by anyone embedding the console in their own tooling; the alternative today is a reverse proxy rewriting headers. The middleware wraps frontend.Handler() and nothing else. The public booking pages set frame-ancestors 'none' and X-Frame-Options: DENY in their own handlers and must keep doing so unconditionally: they are unauthenticated pages that collect names, emails and card details, and clickjacking one of those is worth more to an attacker than framing a console nobody can reach without a session. No X-Frame-Options is sent beside the CSP. That header has no allow-list form (ALLOW-FROM was implemented by one browser and is dead), so the only value it could carry here is SAMEORIGIN, which the browsers that read it apply INSTEAD of honouring frame-ancestors, breaking the embedding this exists to enable. Unset changes nothing, and what "nothing" is here is worth writing down: /admin/ sends no frame header at all today, so the SPA is framable by default. This does not add a default deny, because an opt-in setting must not smuggle in a behaviour change, and TestAdminSPA_sendsNoFrameHeadersWhenUnset pins the current answer so that the next person to change it does so on purpose. An entry that is not https://host[:port] or 'self' fails config.Validate and the process exits rather than starting. A browser drops a source list it cannot parse, so a typo would leave /admin/ MORE embeddable than the setting unset, with nothing in the response to say so. Wildcards are refused too even though CSP allows them: https://*.example.com trusts every host that name ever points at, including one taken over later, and an operator who needs two hosts can name two hosts.
GET /metrics exposes build identity, requests by surface and status, a duration histogram, job-queue depth, booking lifecycle counts, process start time and two Go runtime gauges. internal/metrics writes the exposition by hand: the format is one page of stable text, and a scrape endpoint is not worth a dependency tree in a binary an operator self-hosts. Same trade as hand-signing LiveKit tokens. It answers 404, not 401, when METRICS_TOKEN is unset or the bearer is wrong, and the body is byte-identical to the mux's own not-found. A 401 confirms the endpoint exists and invites a guess, and what is behind it is a business feed: bookings per hour, request volume by surface, queue depth, on an instance whose whole point is being publicly reachable. An operator who has not set a token has not agreed to publish any of that, so there is nothing to advertise. The token compare is over SHA-256 digests rather than the raw strings, because ConstantTimeCompare returns early on a length mismatch and would otherwise leak the token's length. No rate limit: a scrape runs every few seconds by design, and a limiter tuned for humans would drop samples and leave gaps that read as downtime. The class label is derived from the path PREFIX only, so the label set is closed at five values and a request cannot mint a new series. That is the usual way a metrics endpoint turns into an out-of-memory vector. It costs some precision and the docs say so: POST /v1/bookings is public and unauthenticated and still counts as api, because the alternative is a per-route table that goes stale the first time someone adds a route without noticing. Requests are counted in the existing Logging middleware rather than a wrapper of their own: that is already the one place holding the final status and the elapsed time, and a second measurement of the same request would drift from the log line it is supposed to corroborate. Job depth is read from the jobs table per scrape, because any instance can claim any job, so it is not this process's counter to keep. Booking counts sit in the three shared side-effect functions, which covers the REST, MCP and manage-link paths at once, and they are incremented next to the webhook enqueue but not inside its nil check: an instance with no webhooks configured still has bookings worth counting. A host reassignment is deliberately not counted as a reschedule, even though it does fire booking.rescheduled, because it does not move the meeting and counting it would make the series answer a question nobody asked. All three booking series are emitted at zero rather than appearing on first use. A series that springs into existence makes rate() over a quiet window return nothing instead of 0, which a dashboard renders as "no data" rather than "nothing happened".
The Deepgram host was a hardcoded constant, so meeting audio always went to the provider's global endpoint. STT_BASE_URL points it elsewhere: a regional endpoint, so recordings are transcribed inside one jurisdiction, or a self-hosted deployment of the same API. The default is unchanged and a test pins the exact URL, query included, because a dropped diarize=true returns a transcript with no speaker labels rather than an error. Only the host is configurable. The path, model and options stay in listenPath, so an operator picks a region and not a different request; a trailing slash is trimmed so both spellings of a host behave the same. The base URL is a NewDeepgram parameter rather than a package variable, because configuration reaching a client through a global is configuration nobody can see at the call site. GET /v1/settings/notetaker now reports the effective value as stt_base_url, resolved on read so a Handler built without the setter still names the real default instead of an empty string. It is read-only and env-only, unlike the API key beside it: an admin should be able to see where recording audio is sent without shelling into a container, and should not be able to repoint it from a browser session.
Reminders were email-only, so an integration could hear that a booking was made, moved or cancelled but not that the nudge before it had been sent. reminder.send now enqueues booking.reminder, booking-shaped like its siblings plus hours_before, which the job payload has always carried: an event type can configure several reminders, so a subscriber that cannot tell 24h from 1h cannot act on the event. Fired after the email and only when the email succeeded. The event means "the attendee has been reminded", so emitting it beside a failed send would say something untrue, and the job retries, which would deliver it twice for one reminder. The mirror image also matters: an enqueue failure does NOT fail the job, because the email has already gone and a retry would send a second one. Logged and dropped, the trade every webhook enqueue after a committed side effect makes here. sendReminder's existing early returns are all "no reminder happened" - booking deleted, no longer confirmed, host has reminder emails off - so none of them reaches this either. A test pins the suppressed-email case, since "a job ran" and "someone was reminded" are easy to conflate later. The payload needs the BOOKING's host_id, not the event type's owner: a rotation or a reassignment moves it, and Enqueue selects a subscriber's webhooks by that id. The query gained b.host_id and b.created_at for this and nothing else. hours_before goes through the same per-webhook field selection as everything else rather than being bolted onto the envelope, so it appears in the admin field list and an operator can decline it. It is in defaultFields on the same bargain as the payment fields: omitted at zero, so every other event's payload is byte-identical to before, and a webhook created before this event existed is unchanged. Payment fields are deliberately absent from this event. paymentStatusForWebhook's mapping lives in the handler package, and a reminder is a time notification; payment state is what create and cancel are for. The SPA's event list and field catalog gain the two strings. The embedded build is not rebuilt here, so the admin UI will not offer booking.reminder until someone runs pnpm build - the API accepts it now either way.
A visitor asking for fr-CA got the France copy. fr-CA.json is the first regional locale here, and it is a file rather than a fallback because the differences are real rather than cosmetic: - courriel, not e-mail. The OQLF rejects the France-ism and it reads as foreign. - reporter / report, not reprogrammer / reprogrammation. - renseignements personnels, never données personnelles. That is the statutory term in Quebec, so it is the one a reader expects to see. - no space before ! ? ;, where France puts one. The space before : stays. - conflit d'horaire rather than conflit d'agenda. - month_short_jul is juill., against juil. in France. CLDR itself disagrees on exactly this one abbreviation, which is what TestDateTablesMatchCLDR is for; had the file been a copy of fr it would have failed on that line and nothing else. The 24-hour clock and the day-month date_format are shared with fr. Canadian French also puts a non-breaking space before $ and %, and no key in the file carries either today, so that rule is written into ARCHITECTURE §23 rather than applied to nothing. Generated from fr.json with explicit per-key overrides and verified to have the same keys, the same key order and the same printf verbs, which is what the three guards check. Every other value is inherited verbatim, so a later correction to fr does not silently diverge here. TestResolve gains four cases, and two of them are the ones that matter: fr and fr-FR must be unaffected by fr-CA existing, because that is the half that would break silently for every existing French visitor. The wording is an unreviewed LLM draft, as every non-English locale in this repository is. Structure is verified, the copy is not, and the CHANGELOG says so before anyone markets the language.
postgres_test.go asserts the fully-migrated version twice through this constant, and its own doc comment says it moves with every migration added. This branch adds one, so it is 59. Worth knowing for the next person: those two tests skip when CALNODE_TEST_POSTGRES_DSN is unset, so a migration added and verified on the SQLite lane alone leaves this red and nothing says so until someone runs the Postgres lane.
PROGRESS.md is the working log of the fork this branch was developed on. It is not part of the change and does not belong upstream; the reasoning it holds is in the commit messages.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
Two real issues before merge: a rate-limit spoof path when TRUSTED_PROXY_CIDRS is set, and a concurrent SSO owner-bootstrap race that can break the one-owner invariant.
Reviewed changes Operator hooks (SSO, revoke-all, trusted proxies, frame ancestors, /metrics, STT host, reminder webhook, fr-CA) plus the stacked PostgreSQL dual-engine base (#29).
- SSO hand-off — short-lived HS256 JWT, jti nonce table, optional user create, owner bootstrap when unowned.
- Sign out everywhere — sessions + MCP access tokens; self keeps calling cookie; admin offboarding tiers.
- Trusted proxies — CIDR-gated client IP for rate limits; CF header then XFF right-to-left walk.
- Frame ancestors / metrics / STT / reminder webhook / fr-CA — opt-in or additive, with tests on the security-sensitive paths.
- Postgres stack — dialect wrapper, migrations, constraint codes, host advisory locks for overlap races.
⚠️ Stacked on #29
This branch includes the full Postgres dual-engine stack. Merging before #29 (or an equivalent base) lands will either fail CI or dump that stack into main via this PR. Confirm the intended merge order.
Technical details
# Merge sequencing for stacked PR
## Affected sites
- Entire `internal/db/**` dual-engine surface and booking `lockHosts` — present in this diff as commits before the operator-hooks series.
## Required outcome
- Either merge #29 first and rebase this PR, or treat this PR as the vehicle for both and review/ship the Postgres work as first-class here (not “hooks only”).
## Open questions for the human
- Is #29 still the intended base, or has it been superseded?Grok ｜ 𝕏
Comment on lines
+399
to
+401
| if cf := strings.TrimSpace(r.Header.Get("CF-Connecting-IP")); cf != "" { | ||
| if ip := net.ParseIP(cf); ip != nil { | ||
| return ip.String() |
Contributor
There was a problem hiding this comment.
CF-Connecting-IP is taken at face value for any trusted peer, before the careful XFF right-to-left walk. A client that can reach Calnode through a non-Cloudflare reverse proxy in TRUSTED_PROXY_CIDRS can set that header themselves unless the edge strips it — and then every request keys rate limits on an IP they chose, which is exactly the spoof the XFF walk was written to prevent.
Technical details
# Do not prefer client-settable CF-Connecting-IP for arbitrary trusted peers
## Affected sites
- `internal/server/middleware.go` `resolveClientIP` — CF branch returns immediately
- `TestTrustClientIP_trustedPeerUsesCFConnectingIP` — pins CF winning; no test that a client-injected CF header is ignored when the peer is a generic trusted proxy
- DEPLOY/ARCHITECTURE text that lists CF and XFF as equivalent for any listed CIDR
## Required outcome
- Rate-limit key must not be attacker-chosen when the trusted hop is a normal reverse proxy that forwards client headers.
- Cloudflare-style single-value header is fine only when that hop is known to set/overwrite it (separate allowlist, or CF ranges, or “strip CF at edge unless CDN” documented and default-safe).
## Suggested approach (optional)
- Prefer the XFF walk always; treat CF-Connecting-IP only when explicitly enabled (e.g. `TRUST_CF_CONNECTING_IP=1`) or when the peer is in Cloudflare’s published ranges.
- Or: never read CF-Connecting-IP unless the operator also opts in; document that nginx/Caddy must clear that header unless the CDN sets it.
- Add a test: trusted peer + client-supplied `CF-Connecting-IP` + honest XFF → resolved IP is the XFF client hop, not the CF value.
Comment on lines
+186
to
+198
| if !h.ssoOwnerExists(ctx) { | ||
| isOwner = 1 | ||
| } | ||
| case "admin": | ||
| isAdmin = 1 | ||
| } | ||
| userID = uid.New() | ||
| if _, err := h.db.ExecContext(ctx, ` | ||
| INSERT INTO users (id, email, name, iana_timezone, is_admin, is_owner, email_login) | ||
| VALUES (?, ?, ?, 'UTC', ?, ?, 0)`, | ||
| userID, email, claims.Name, isAdmin, isOwner); err != nil { | ||
| return "", false, err | ||
| } |
Contributor
There was a problem hiding this comment.
Owner bootstrap is check-then-act with no lock and no partial unique constraint. Two concurrent role=owner handoffs on an unowned instance can both see ssoOwnerExists == false and both insert is_owner = 1, breaking the one-owner invariant TransferOwnership maintains in a transaction.
Technical details
# Atomic SSO owner bootstrap
## Affected sites
- `internal/handler/sso.go` `ssoResolveUser` create path (`isOwner = 1` after `ssoOwnerExists`)
- Same file existing-user path: `UPDATE … is_owner = 1` after another `ssoOwnerExists` check (~208–212)
- No DB constraint on a single owner row (unlike the transactional swap in `roles.go` `TransferOwnership`)
## Required outcome
- At most one non-archived owner after any number of concurrent SSO handoffs.
- Prefer failing the second bootstrap cleanly (admin, not owner) over two owners.
## Suggested approach (optional)
- Claim ownership in one statement, e.g. insert/update only when no owner exists (`WHERE NOT EXISTS (SELECT 1 FROM users WHERE is_owner = 1 AND archived_at IS NULL)`), and treat 0 rows as “became admin only”.
- Or take a process/DB advisory lock around bootstrap; or a partial unique index on `is_owner` where true (engine-specific).
- Concurrent test with two owner-role tokens on empty DB asserting `COUNT(*) WHERE is_owner = 1` is 1.
Comment on lines
+177
to
+179
| b.line("# HELP calnode_jobs_failed_total Background jobs that exhausted their retries.") | ||
| b.line("# TYPE calnode_jobs_failed_total counter") | ||
| b.linef("calnode_jobs_failed_total %d", q.Failed) |
Contributor
There was a problem hiding this comment.
calnode_jobs_failed_total is declared # TYPE … counter but the value is a live COUNT(*) of rows currently in status = 'failed' (same shape as calnode_jobs_pending). Counters must only increase and represent cumulative events; this is a gauge of backlog. rate() on it will not mean “failures per second”.
Rename to something like calnode_jobs_failed with TYPE gauge, or expose a true process-lifetime failure counter incremented when a job is marked failed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this adds
Eight small, independent operator-facing features, each its own commit, none of them on by default. Together they let Calnode sit behind an identity system and a reverse proxy an operator already runs. Stacked on #29 (it shares that branch's base); the commits unique to this one start at
auth: sign someone in from an identity system you already run.CALNODE_SSO_SHARED_SECRETGET /v1/auth/sso?token=accepts a short-lived HS256 token (aud,sub,name,role,exp≤ 60 s, one-timejti) minted by the operator's own identity system, creates the user if absent, and opens a session. Unset ⇒ 404.POST /v1/auth/sessions/revoke-allends every session and MCP connector token for the caller.TRUSTED_PROXY_CIDRSFRAME_ANCESTORSDENY.METRICS_TOKENGET /metricsbehind a bearer token; unset ⇒ 404, so it cannot be public by accident.STT_BASE_URLbooking.reminderwebhookhours_beforenaming which one.fr-CAships as its own locale rather than falling back tofr. Structure is verified by the existing locale guards; the wording is an LLM draft with no native review, like the other non-English locales.How it is tested
Every feature has unit tests on both engines (the SSO endpoint's replay, audience and expiry refusals; the proxy CIDR matching with the untrusted-peer case; the metrics 404-when-unset; the reminder payload field).
knownMigrationCountmoves with the one migration this adds (sso_nonces).Compatibility
Nothing changes for a deployment that sets none of the new variables.
🤖 Generated with Claude Code