feat(aetherd): establish per-session authorization boundary

[rfoust]

Summary

Continues #3849 after merged #5391. This is the first, deliberately small sub-PR of authenticated non-TX control: establish an explicit per-session authorization boundary before any receive-control handler is exposed.

• Default sessions are unauthenticated and fail negotiation closed. Only trusted embedding/transport code can select an authorization context; client names, JSON claims and session IDs cannot grant access.
• The existing current-user local endpoint explicitly authorizes observers, preserving its seven observe-only capabilities. Authenticated sessions with no grants can negotiate and refresh capabilities, but cannot read or subscribe to resources.
• Enforce observe permission at resource dispatch, direct subscription methods and event delivery. Keep negotiation identity private to the service/session.
• Terminal, idempotent revocation clears subscriptions and queued observations before signaling the transport; the local server synchronously aborts unwritten output. Other clients are unaffected.
• Share the production output binding with socket-free tests that independently buffer frames and verify synchronous transport purge, queued flush/drain ordering, and callback lifetime safety.
• Update the protocol/catalogue and contributor status documentation.

This does not complete the authentication/non-TX milestone. Credential verification/provisioning, typed receive intents, discovery/connect, remote transport, meters, desktop migration and Stage 4 TX arbitration remain separate work. There are no new control/TX grants, mutation methods, persisted settings, or dependencies. Revocation is an owning-thread lifecycle hook, not a new wire/admin operation; already-delivered bytes cannot be recalled.

Constitution principle honored

Principle VII — authorize at the protocol boundary and fail closed before resource processing. Principle VI — no transmit method or grant exists. Principle XI — socket-free behavioral regression tests are mutation-checked rather than relying on source-text assertions.

Test plan

• ARM64 macOS RelWithDebInfo build using the required local toolchain and RADE enabled: AetherSDR, aetherd, and the four focused test targets. Verify host/system ARM64, no RNNoise x86 sources, and ARM64 executables.
• control_protocol_codec_test, control_authorization_test, and control_resource_service_test: socket-free checks for negotiation, grant denial, session isolation, queued/new event revocation, revocation before hello and during resync, and preservation of existing resource behavior.
• Existing, unchanged local_control_server_test: exercises our own QLocalServer current-user transport; no fake radio. Requires a permitted local socket bind; passed outside the Codex sandbox. No new socket-owning test is introduced.
• Mutation checks in an isolated worktree: unauthorized default observer, authentication implying observe, no-op revocation, and removed resource.get grant enforcement each make the new test fail. Restored production source passes.
• Transport regression mutation checks: removing the production revocation hookup or changing it to a queued connection each fails the synchronous buffer-purge assertion. Restoring the exact production source passes. Coverage also stops batched writes on revocation, session/transport destruction, and write failure.
• Strict engine boundary, test registration, frozen CI-gate integrity, touchpoint manifest, and whitespace checks.
• An earlier focused security diff review found no findings in the authorization patch before the output-binding/test-coverage revision; that scan does not cover this later revision. No Windows/Linux runtime or live-radio proof claimed; this patch neither changes GUI wiring nor connects/controls a radio.
• The new test is registered in tests/tests.cmake and enters the normal unfiltered main/weekly suites. The frozen per-PR test allow-list is unchanged.

Review follow-up (f7474b2c)

• Enforce single binding, non-null context/callbacks, and caller/endpoint thread affinity in release builds. Keep AutoConnection; do not force a cross-thread socket abort.
• Deny unauthenticated parsed hellos before parameter/version negotiation; malformed envelopes retain protocol errors.
• Clarify that revocation and the no-grants context are staged hooks without production callers. Keep that staging decision with the maintainer.
• Document observes() and takePendingFrames() permission checks as defence-in-depth guards not independently mutation-pinned in any currently reachable state; defensively clear pending byte accounting on denied drains.
• ARM64 app/daemon builds, all four focused tests, and static checks pass. Four follow-up mutations (remove once guard, caller-thread guard, context-affinity guard, or restore old hello ordering) each fail; restored source passes.
• Focused security scan of this follow-up's three changed production files: no findings. This supplements, but does not replace, the earlier scoped reviews. No radio/TX or Windows/Linux runtime proof claimed.
• Current-main merge tree is conflict-free; control sources and focused test sources are identical to the locally tested versions. CI must rerun for the new head.

Checklist

• No new flat-key AppSettings calls (no settings changes).
• Clean-room implementation.
• No meter UI changes.
• Documentation updated; no CHANGELOG.md entry.
• Security-sensitive work reviewed; no GHSA applies to this preparatory authorization change.
• Signed commit verified locally (f79ef219).
• CI passes.

[Copilot]

🔵 Needs a closer look

It changes a security-sensitive authorization boundary and transport revocation/lifetime behavior that merits final human review despite strong targeted test coverage.

Pull request overview

This PR advances the aetherd Stage-3 control protocol by introducing an explicit per-session authorization context that is supplied by trusted transport/embedding code (not by any hello fields), and by making revocation terminal with synchronous local-transport output purge behavior.

Changes:

• Add SessionAuthorization to ControlSession, defaulting sessions to unauthenticated (fail hello closed) unless a trusted transport explicitly authorizes them.
• Enforce observe grant checks consistently across service dispatch, direct subscribe/unsubscribe entry points, and event delivery; implement terminal, idempotent revocation that clears subscriptions/queued events and aborts transport output.
• Add a new socket-free authorization/revocation regression test target and update protocol/catalogue + contributor documentation.

File summaries

File	Description
tests/tests.cmake	Registers new control_authorization_test target for socket-free auth/revocation coverage.
tests/control_resource_service_test.cpp	Updates tests to use new ControlSession API (sessionId(), isNegotiated()) and explicit observer authorization.
tests/control_authorization_test.cpp	Adds comprehensive socket-free tests for auth boundary, grant enforcement, revocation semantics, and output binding lifetime behavior.
src/core/control/LocalControlServer.h	Removes now-unneeded per-socket drain helper declaration after moving output wiring into ControlSession.
src/core/control/LocalControlServer.cpp	Constructs observer-authorized sessions for the current-user endpoint and binds transport output via ControlSession::bindOutputTransport().
src/core/control/ControlSession.h	Introduces SessionAuthorization, negotiation/auth/observe helpers, revocation, and transport output binding API.
src/core/control/ControlSession.cpp	Implements authorization state, negotiation completion, observation gating, terminal revocation, and shared transport output binding logic.
src/core/control/ControlService.h	Documents that authorization is supplied by trusted transport context and cannot be granted by hello.
src/core/control/ControlService.cpp	Requires authenticated context for hello, filters capabilities by grant, and enforces observe permission before resource processing.
docs/aetherd-control-resource-v1-catalogue.md	Updates v1 catalogue to reflect observe-grant requirements and terminal revocation semantics.
docs/aetherd-control-protocol-v1-design.md	Documents the new immutable per-session authorization model and revocation lifecycle behavior.
AGENTS.md	Updates project guide status section to reflect the new auth boundary and revocation behavior.

Review details

• Files reviewed: 12/12 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[aethersdr-agent]

Issue fit

No fixes/closes #NNNN in the body, so there is no linked issue to map requirements against — I reviewed against the PR's own stated intent and against the two design documents it amends. This is a slice of the already-ratified aetherd control-protocol staging (the design doc and catalogue exist on main; #5391 landed the observe-only resource layer immediately before this), and it is narrowing an existing surface rather than adding a user-visible feature, so I do not read GOVERNANCE.md as requiring a fresh RFC. The stated intent — make authorization an explicit, transport-supplied property of ControlSession instead of an implicit consequence of having negotiated — is fully delivered by the diff, and the docs it edits are accurate to the code (I checked each claim; the one imprecision is a nit below).

Scope

File / group	What it changes	Claimed?	Verdict
src/core/control/ControlSession.{h,cpp}	SessionAuthorization enum, isAuthenticated/canObserve/isRevoked, sessionId+negotiated public fields → accessors, revokeAuthorization(), bindOutputTransport(), observe guards in subscribe/unsubscribe/takePendingFrames/observes	Yes	In scope
src/core/control/ControlService.{h,cpp}	Revoked/unauthenticated guards, per-method observationError(), grant-filtered capabilities()	Yes	In scope
src/core/control/LocalControlServer.{h,cpp}	Constructs sessions as Observer; drainSessionOutput() deleted in favour of bindOutputTransport()	Partly — the drain rewiring is a refactor, not an authorization change	In scope, but see note
AGENTS.md, docs/aetherd-control-protocol-v1-design.md, docs/aetherd-control-resource-v1-catalogue.md	Prose matching the new behaviour	Yes	In scope
tests/control_authorization_test.cpp, tests/tests.cmake	New socket-free target	Yes	In scope
tests/control_resource_service_test.cpp	Mechanical: pass Observer, sessionId → sessionId()	Yes	In scope

The drainSessionOutput → bindOutputTransport move is the only row that isn't literally "authorization", but it is load-bearing for it: revocation has to be able to purge the transport's own unwritten bytes, which the old two-connect wiring in acceptConnections() had no hook for. I would not ask to unbundle it. No CHANGELOG.md touch (correct). No new settings keys, no new wire method, no capability-map change (the seven advertised capabilities are byte-identical for an authorized observer — verified against the diff and the existing socket test's assertion list).

Socket test disclosure: this PR adds and modifies no socket-owning test. tests/control_authorization_test.cpp is socket-free by construction — its BufferedTransport is an injected QObject sink, not a synthetic radio/firmware peer, and tests.cmake compiles only ControlProtocolCodec/ControlResourceStore/ControlService/ControlSession with Qt6::Core. The existing socket-owning local_control_server_test (our own server, legitimate) is untouched but is behaviourally affected by the drain rewrite; see below.

Blockers

None.

Nits (non-blocking)

• Hello validates params and versions before it checks authorization (ControlService.cpp:141, inline). The resource methods deliberately put observationError() ahead of onlyKeys() — the new test even comments "authorization precedes schema processing or resource lookup, so denied callers learn no resource/subscription state." The hello path does the opposite, so an unauthenticated caller can distinguish request.invalid_params from protocol.version_unsupported (which echoes supported: [1]) from auth.required. What leaks is negotiation metadata only, not resource state, so this is a consistency/doc nit rather than a hole — but the design doc's flat "The default context is unauthenticated: hello returns auth.required" is true only for a well-formed v1 hello. Either move the check up or soften the sentence.
• bindOutputTransport says "Bind once" but nothing enforces it (ControlSession.h:55, inline). A second call duplicates all three connections; abortTransport would then fire twice per revocation/overflow. Contract is honoured by the single production caller, so this is a guard-rail request, not a defect.
• revokeAuthorization() and AuthenticatedWithoutGrants have no production caller. The body discloses this ("these are the lifecycle hooks for subsequent authenticated non-TX control"), and I agree the hooks are cheaper to land with their tests than to bolt on later — recording it so the maintainer can price it deliberately: roughly half the new code and five of six new test functions exercise a path nothing in the shipping app reaches yet.
• The revoked-session guard sits ahead of parsing, so its error reply carries an empty id rather than echoing the client's. Terminal path, so nothing depends on correlating it.
• control_authorization_test is correctly not added to any ctest -R gate in ci.yml (frozen list, #5405), so it runs on full-suite/sanitizers only. Green checks on f79ef21 therefore do not mean this test ran.

What I tried to break

• Can a client grant itself anything over the wire? No. validateHelloParams rejects auth outright with auth.invalid and rejects grants as an unknown key, both before completeNegotiation(); capabilities() reads session.canObserve(), which is a function of the constructor-supplied enum and can never be reached from request JSON. The new test's bearer-token case also asserts the token string never appears in the reply — I confirmed the error detail carries only the field name.
• Can a stolen session id transfer permissions? No — request.sessionId != session->sessionId() is checked per-connection before method dispatch, and the id is a fresh QUuid minted only inside completeNegotiation().
• Does anything still read the removed public negotiated/sessionId fields? Grepped the whole head tree: no residual uses, and no ControlSession is constructed anywhere outside LocalControlServer.cpp and the two test files — so the Unauthenticated default argument cannot silently mis-authorize a forgotten call site; it fails closed.
• Does the queue leak when takePendingFrames() early-returns {} without clearing? I traced every enqueue path: observes() gates both store slots and itself returns false when !canObserve(), and requireResync() is only reachable from enqueueCoalesced. So a non-empty m_pending implies canObserve() was true, and the only transition to false is revokeAuthorization(), which clears the queue and m_pendingBytes first. No orphaned bytes, no stuck m_pendingBytes against the overflow bound.
• Lifetime during the drain batch. The outputReady lambda re-checks session/transport (both QPointer) inside the loop, so destroying either endpoint or revoking from within writeFrame stops the batch — the four Stop:: cases assert exactly that. The authorizationRevoked connection is deliberately direct while outputOverflow stays queued; I walked the production consequence — socket->abort() emits disconnected synchronously, which only ever defers dropClient via singleShot(0), so the ControlSession cannot be destroyed underneath the emit that is still on the stack. That deferral already existed for the same reason and is untouched.
• Is the rewritten drain path still covered end-to-end? Yes, and by an existing registered test rather than only the new socket-free one: local_control_server_test subscribes over the real QLocalSocket and asserts a resource.changed frame arrives after resourceStore().upsert() (lines ~214–231), and drives the overflow-abort path at maxQueuedOutputBytes = 1 twice. That was my main worry about deleting drainSessionOutput — the old version's m_clients.find() staleness check is now supplied by binding the socket as the connection context object, which is equivalent and strictly tighter.
• Would the new tests pass with the guard broken? Emptying observationError() out of subscribe() fails the directDenied assertion; making canObserve() return isAuthenticated() fails the whole noGrants block including the empty-grants capability comparison; making takePendingFrames() ignore revocation fails emptyAtRevocation and the delivered.isEmpty() check. They are not self-confirming.
• Every prose claim in the three edited docs was checked against the code — "grant_denied before checking parameters", "revocation discards pending observations", "idempotent and terminal even before negotiation or while a resync notice is pending", "aborts synchronously to discard unwritten output" (Qt's QLocalSocket::abort() does discard the write buffer). All hold; only the hello-ordering sentence above is loose.

All of the above is reasoned from reading the head checkout at /tmp/aetherclaude/pr-5434; I am headless and did not build, run ctest, or drive a socket. No CodeGuard or Copilot findings were supplied, which means either a clean scan or no scan — I am not reporting it as clean.

Recommendation

Approve with nits. This is the rare security-boundary change that actually fails closed at every seam I could find: the default enum value is Unauthenticated, the grant is supplied by the constructor rather than derivable from any wire field, both the service and the direct ControlSession entry points enforce it, and revocation clears state before it notifies anyone. The three nits are consistency and guard-rail items, none of which change behaviour for a real client. Next step is a maintainer call on whether landing revokeAuthorization() ahead of its production caller is the shape they want; if yes, this is mergeable as-is, and I would only ask for the one-sentence doc softening on the hello ordering.

Thanks for the doc updates landing in the same commit as the code — the catalogue and design-doc paragraphs made the intended boundary easy to test against rather than infer.

---

_{🤖 aethersdr-agent · cost: $5.9328 · model: claude-opus-5}

[ten9876]

Verdict: no blockers. Comment-only.

Issue fit. No linked issue; this is a self-declared sub-slice of #3849, following #5391. GOVERNANCE.md exempts "bug fixes with a clear root cause" from the RFC requirement, and the architectural direction here was already ratified by the aetherd Stage-3 design doc that this PR extends, so the process shape is fine. The PR body's own scoping — an authorization boundary and a revocation lifecycle hook, with no wire revocation method, no control/TX grant, no settings, no dependencies — matches the diff exactly.

Scope

File / group	Claimed in body?	Verdict
ControlSession.{h,cpp} — SessionAuthorization, negotiation/observe accessors, revokeAuthorization(), bindOutputTransport()	Yes	In scope
ControlService.{h,cpp} — auth required for hello, grant-filtered capabilities, observe check on the three resource methods	Yes	In scope
LocalControlServer.{h,cpp} — Observer at construction, output wiring moved into the session, drainSessionOutput deleted	Yes	In scope
tests/control_authorization_test.cpp + tests/tests.cmake	Yes	In scope; socket-free, registered, enters the unfiltered suites
tests/control_resource_service_test.cpp	Yes	In scope — purely mechanical (sessionId → sessionId(), negotiated → isNegotiated(), explicit Observer); I diffed every changed line and no assertion was weakened
docs/aetherd-control-protocol-v1-design.md, docs/aetherd-control-resource-v1-catalogue.md, AGENTS.md	Yes	In scope

Single commit, single author date, nothing bundled. No CHANGELOG.md entry (correct — it is release-prep only). No settings keys, no credentials, no capability-map changes, no CI edits. Everything in the diff is explained by the stated change. The body's checklist holds up on every item I could check.

One deleted-behavior check: LocalControlServer::drainSessionOutput() is removed, and with it the m_clients.find(socket) guard it used to do before writing. That is safe — sendFrame() already rejects a null or UnconnectedState socket, the new connection's context object is the socket rather than the server, and dropClient is still deferred through QTimer::singleShot(0, …). Strictly narrower lifetime than before, not wider.

Blockers

None.

Nits (all non-blocking)

1. Two of the new guards are not pinned by any test — see the inline comment on ControlSession.cpp. I ran seven mutations; five were caught, these two were not.
2. bindOutputTransport's same-thread invariant is debug-only — see inline.
3. revokeAuthorization() and AuthenticatedWithoutGrants have no production caller — see inline. Disclosed in the body as a lifecycle hook, so this is a maintainer's call on landing surface ahead of its consumer, not a defect.
4. AGENTS.md phrasing — see inline.

What I tried to break (and what held)

Built the PR head clean (RelWithDebInfo, Ninja, Linux/Qt6) and ran the tests rather than reading them.

• The tests are real. Seven deliberate mutations of the production source, rebuilt and re-run each time. Caught: dropping the isAuthenticated() check in hello; making revokeAuthorization() a no-op; deleting the observationError() check from resource.get; changing the authorizationRevoked → abort connection to Qt::QueuedConnection; deleting the direct subscribe()/unsubscribe() guards. Restored source passes. The queued-connection mutation being caught is the interesting one — the synchronous-purge property really is asserted, not just asserted-about.
• "No behavior change for the existing local endpoint" — verified end-to-end, not argued. Built aetherd, ran it with an isolated XDG_RUNTIME_DIR, and drove the real QLocalServer from a Python client. hello still returns grants: ["observe"] and exactly the seven capabilities (server.read, radioSession.read, slice.read, panadapter.read, resource.get, resource.subscribe, resource.unsubscribe); resource.get and resource.subscribe on server both return the live snapshot at revision 3. The unchanged local_control_server_test also passes on the head.
• Tried to smuggle authorization through hello on fresh connections. {"auth":{"scheme":"bearer","token":"SECRET123"}} → auth.invalid ("authentication is not accepted by this local endpoint"), and I byte-checked the response: the token is not echoed back. {"grants":["transmit","observe"]} → request.invalid_params with field: "grants". Neither negotiates.
• Looked for a use-after-free in the new synchronous abort path. authorizationRevoked → abortTransport is deliberately a direct connection, and in LocalControlServer that runs socket->abort(), which can emit disconnected synchronously → dropClient → destroy the Client and its ControlSession while revokeAuthorization() is still on the stack. It holds: the disconnected handler defers dropClient behind QTimer::singleShot(0, …). The test's Stop::DestroySession / Stop::DestroyTransport cases cover the analogous teardown-inside-the-write-callback shapes, and the QPointer re-check inside the drain loop covers the rest.
• Looked for a queue-accounting leak from takePendingFrames() early-returning without clearing m_pending/m_pendingBytes. Not reachable: the only enqueue path is observes() → canObserve(), requireResync() is only reachable from enqueueCoalesced(), m_authorization is const, m_sessionId is write-once, and revocation clears the queue. So it is robustness, not a bug — but it is the same code that the mutation run showed nothing pins.
• Ran the repo gates: check_engine_boundary, check_test_registration, check_ci_test_gate, check_network_timeouts all pass, and gen_touchpoint_manifest.py regenerates to a clean tree.
• Did not drive the GUI. LocalControlServer is instantiated only by src/aetherd/main.cpp:29; nothing in the AetherSDR binary constructs a ControlSession. The automation bridge cannot reach this change, so the aetherd socket session above is the equivalent end-to-end proof. No radio was connected and no TX path exists in this surface.

Test-boundary preflight: the new control_authorization_test is socket-free — the bind() in BufferedTransport is a local helper name, not a socket bind, and the target compiles only the codec/store/service/session sources. No new socket-owning test is introduced; local_control_server_test is unchanged. Its tests.cmake block correctly says so.

[ten9876]

Approved.

Every blocker-shaped concern raised on this PR — mine, Copilot's, and the agent's — is addressed in f7474b2c, and I re-ran the adversarial pass against that commit rather than taking the replies on faith. I pushed one commit of my own (b6c45b80, docs-only) for the last nit.

What I re-verified on f7474b2c

Mutation battery — the fixes are pinned, not just present. Rebuilt and re-ran control_authorization_test against five deliberate breakages of the production source:

N1 single-bind guard removed                : CAUGHT
N2 caller/endpoint affinity checks removed  : CAUGHT
N3 hello auth check moved back after params : CAUGHT
N4 revoke abort forced Qt::QueuedConnection : CAUGHT
N5 denied-path queue clear removed          : survived (disclosed in the body)

Restored source passes. N5 is the one guard that cannot be pinned without a test backdoor into private state, because the state it defends is unreachable; declining to add that backdoor was the right call, and the new comment carries the invariant instead. Combined with the earlier run against f79ef219, ten of twelve mutations are caught.

The hello reorder does not regress the shipping endpoint. This was the change I most expected to have a tail, since it moves an authorization check across version negotiation. Built aetherd, ran it against an isolated XDG_RUNTIME_DIR, drove the production QLocalServer from a raw socket client:

hello {"versions":[1]}                      -> grants ['observe'], 7 capabilities
resource.get  server                        -> health "ok"
resource.subscribe server                   -> sub-1
hello {"versions":[2]}                      -> protocol.version_unsupported
hello {"versions":[]}                       -> request.invalid_params
hello {...,"auth":{"bearer","SECRET"}}      -> auth.invalid (token not echoed)
hello {...,"grants":["transmit"]}           -> request.invalid_params

An Observer session still gets real version negotiation, so the new early auth.required is reachable only for Unauthenticated contexts — which the current-user transport never constructs. No behavior change for existing clients, confirmed against the daemon rather than argued from the diff.

The binding contract holds where it matters. testOutputBindingContract covers the case I would have missed: a caller on a foreign thread whose endpoint affinity nonetheless matches. Removing either half of that check fails the test. Retaining AutoConnection plus a release-build guard is the correct resolution of the Copilot suggestion — forcing Qt::DirectConnection would have traded a lost guarantee for a cross-thread socket->abort(), which is worse.

Everything else: all four control tests pass (control_authorization_test, control_resource_service_test, control_protocol_codec_test, and the unchanged socket-binding local_control_server_test). check_engine_boundary, check_test_registration, check_ci_test_gate, check_network_timeouts, check_a11y all pass; gen_touchpoint_manifest.py regenerates to a clean tree; no trailing whitespace in the diff. The new logging category follows house convention (lcControlSession / "aether.control.session").

What I pushed

b6c45b80 — docs only, no wording changes. Both paragraphs edited in f7474b2c were left mid-reflow: AGENTS.md broke after "Credential" at 31 columns, and the protocol design's authorization paragraph ran to 99 columns on the sentence about invalid envelopes. Rewrapped both to the ~78-column fill the surrounding prose uses.

Scope, one more time

Still clean across both commits. Two commits, one author, no bundled work, no CHANGELOG.md entry, no settings keys, no credentials, no capability-map changes, no CI edits, no new socket-owning test. The AuthenticatedWithoutGrants / revokeAuthorization() staging question was the one thing needing a ruling and I have made it on the thread: accepted, on the strength of the fail-closed default and the explicit staged-status wording in AGENTS.md and the design doc.

Nice work on the turnaround — the replies were specific about which mutation proves which guard, which made this pass fast to verify. Enabling auto-merge.