refactor(streams): decide binary stream auth on isDummy

[dirkwa]

Follow-up to #2937, from a CodeRabbit finding there about shouldAllowWrite(request, 'streams').

What problem does this solve?

The binary stream upgrade handler used the presence of methods as a proxy for two different questions, and got both wrong:

if (app.securityStrategy && typeof app.securityStrategy.shouldAllowWrite === 'function') {
  if (app.securityStrategy.authorizeWS) { ... }
  else { ...shouldAllowWrite(request, 'streams')... }   // never runs
} else { ...allow unauthenticated... }                   // never runs

shouldAllowWrite and authorizeWS are both defined by every strategy in the tree — tokensecurity assigns them, and dummysecurity has a no-op authorizeWS and a shouldAllowWrite returning true. So both fallbacks are unreachable:

• The else labelled "Security is disabled, allow connection without authentication" never runs, because shouldAllowWrite is a function under dummy security too.
• The else passing the string 'streams' never runs either. That one would have been a crash if it did: shouldAllowWrite immediately reads delta.context and delta.updates.find().

Traced against the built output rather than by reading:

outer if (security enabled?) -> true (takes the AUTH path)
  inner if (authorizeWS?)    -> true (calls authorizeWS - a NO-OP under dummy)
  skPrincipal after no-op    -> undefined
  => principal becomes       -> { identifier: 'unknown' }

So today a security-disabled server takes the authentication path, calls a no-op, finds no principal, and falls back to 'unknown' — arriving at roughly the right outcome through entirely the wrong route.

What this changes

Ask isDummy(), which is what the first branch was trying to ask, and call authorizeWS directly. Both dead branches go.

Behaviour is unchanged except under dummy security, where the connection is now labelled 'unauthenticated' instead of 'unknown'. StreamPrincipal.identifier is recorded on the client and never consulted for an authorization decision, and 'unauthenticated' is the more accurate of the two.

Depends on #2937

authorizeWS is optional on master's local WebSocketSecurityStrategy, so calling it unguarded does not compile there. #2937 removes that local interface and makes authorizeWS non-optional on the shared contract. This PR is based on that branch and should merge after it.

Tested

test/binary-stream-auth.ts — 5 passing, covering the paths that matter here: unauthenticated rejected with 401, invalid token rejected with 401, and valid tokens accepted via cookie, query parameter and Authorization header.

Full run: npm run test-only 1171 passing, 0 failing. 0 type errors, build and ci-lint clean.

Summary

• Consolidates security strategy types into the shared SecurityStrategy contract.
• Adds shared SkPrincipal, WSConnection, and LoginResponse interfaces.
• Refactors WebSocket authentication to use isDummy() and direct authorizeWS calls.
• Labels dummy-security connections as unauthenticated.
• Preserves authentication failure handling with HTTP 401 responses.
• Reuses shared security types across WebSocket and token security modules.
• Keeps unsupported login responses at HTTP 501.
• Supports HTTP requests and WebSocket connections in write and admin authorization methods.

[dirkwa]

Note on scope: this targets master but carries #2937's commit as well, since it builds on it — GitHub cannot base a PR on another PR's branch across forks.

Only e698d9d6 belongs to this PR (src/api/streams/index.ts). 070e29d9 is #2937 and should merge there first; once it does, this diff reduces to the single commit.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 15d33730-903b-44b0-847a-247dd90fe7ac

📥 Commits

Reviewing files that changed from the base of the PR and between b2f9a2d and 7320b58.

📒 Files selected for processing (1)

• src/security.ts

---

📝 Walkthrough

Walkthrough

The PR consolidates security interfaces in src/security.ts, updates token security to use shared WebSocket and login types, and changes stream authentication and WebSocket login handling to use the expanded SecurityStrategy contract.

Changes

Security strategy and WebSocket authentication

Layer / File(s)	Summary
Shared security contracts src/security.ts	Adds shared principal, WebSocket connection, and login response interfaces. Expands SecurityStrategy with WebSocket authorization, login capability, token verification, write authorization, and optional admin access.
Token security adaptation src/tokensecurity.ts	Reuses shared security types. Allows admin and write authorization for HTTP requests or WebSocket connections.
WebSocket authentication and login flow src/api/streams/index.ts, src/interfaces/ws.ts	Allows unauthenticated dummy-security connections. Uses authorizeWS for other strategies. Returns HTTP 401 on authorization failure and HTTP 501 when login is unsupported.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• Remove unreachable binary stream authorization fallback and define shouldAllowWrite delta contract #2939 — Relates to the removed stream authorization fallback and the updated SecurityStrategy.shouldAllowWrite contract.

Possibly related PRs

• SignalK/signalk-server#2937 — Modifies the same shared security types and WebSocket authentication flow.
• SignalK/signalk-server#2754 — Modifies WebSocket authentication preparation and authorizeWS() usage.
• SignalK/signalk-server#2568 — Modifies WebSocket login handling and the SecurityStrategy contract.

Sequence Diagram(s)

sequenceDiagram
  participant WebSocketClient
  participant StreamApplication
  participant SecurityStrategy
  WebSocketClient->>StreamApplication: Open WebSocket connection
  StreamApplication->>SecurityStrategy: authorizeWS(connection)
  SecurityStrategy-->>StreamApplication: Authorization result
  StreamApplication-->>WebSocketClient: Allow connection or return HTTP 401
  WebSocketClient->>StreamApplication: Submit login request
  StreamApplication->>SecurityStrategy: Check supportsLogin and call login
  SecurityStrategy-->>StreamApplication: LoginResponse or HTTP 501

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the stream authentication refactor and the use of isDummy().
Description check	✅ Passed	The description explains the problem, change, dependency, behavior, and testing results, and includes both required template sections.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/api/streams/index.ts`:
- Around line 123-126: Update the debug call in the
app.securityStrategy.isDummy() branch to avoid eagerly interpolating streamId
when the debug namespace is disabled. Guard the dynamic message construction
with the debug-enabled check, and use a static message plus the stream
identifier when debug logging is not enabled.

In `@src/interfaces/ws.ts`:
- Around line 811-819: Update the login dispatcher to route every
parsedMsg.login request to processLoginRequest, regardless of supportsLogin().
In processLoginRequest, check supportsLogin() before rate limiting or invoking
login, and preserve the existing completed 501 response for unsupported login
requests.

In `@src/security.ts`:
- Around line 289-294: Update the shouldAllowWrite declaration to replace the
any delta parameter with the concrete WsMessage type used by the WebSocket path
and TokenSecurity context/updates access. Remove the JSDoc text explaining the
untyped binary-stream request string, while preserving the existing request
parameter contract.

In `@src/tokensecurity.ts`:
- Around line 549-550: Replace the broad unknown-to-SKRequest cast in
hasAdminAccess and shouldAllowWrite with a local intersection type combining
WSConnection and Pick<SKRequest, 'skPrincipal' | 'skIsAuthenticated'>. Apply
this in src/tokensecurity.ts at lines 549-550 and 1263-1267, preserving access
to only the authentication fields both functions read.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 632e6524-3b43-4e19-bd3a-6c55c9a3da2a

📥 Commits

Reviewing files that changed from the base of the PR and between 704c21f and e698d9d.

📒 Files selected for processing (4)

• src/api/streams/index.ts
• src/interfaces/ws.ts
• src/security.ts
• src/tokensecurity.ts

[dirkwa]

@coderabbitai Two applied, one partially, one rejected.

Guard the dynamic debug message — applied, and to the pre-existing one next to it as well. Both now use the lazy %s form the rest of the file already uses (debug('Matched stream pattern: %s', streamId) etc.), so this is a consistency fix as much as an allocation one. This is a connection-upgrade path rather than a per-delta one, so the cost was small, but there was no reason for these two to be the odd ones out.

Use WSConnection auth state instead of the double cast — applied, slightly differently. WSConnection & Pick<SKRequest, ...> still needs a cast from Request, so instead there is now a small AuthenticatedRequest { skIsAuthenticated?, skPrincipal? } in tokensecurity.ts holding exactly what both functions read. req as unknown as SKRequest becomes req as AuthenticatedRequest in both places — one cast instead of two, and the type says what the check actually needs.

Type the delta parameter — investigated properly this time, and it does not work yet, though the reason has changed. With the 'streams' caller gone there is exactly one call site left, ws.ts:945, passing a WsMessage. So the obvious move is Delta:

• WsMessage.updates is optional, Delta.updates is required, so WsMessage is not assignable to Delta.
• The runtime guarantee is real but unexpressed: processUpdates is only called under if (parsedMsg.updates) at ws.ts:600, and tokensecurity calls delta.updates.find() unguarded.
• WsMessage itself cannot be the type: it lives in ws.ts, which already imports from security.ts, so referencing it there is a circular import.

Narrowing processUpdates to WsMessage & Pick<Delta, 'updates'> compiles at the callee but not at the caller, because the if (parsedMsg.updates) guard does not narrow the object. Making it work means restructuring how inbound messages are typed and narrowed in ws.ts — a real improvement, but a bigger change than this PR, and unrelated to what it is about. The JSDoc now records the actual constraint rather than the stale binary-stream rationale.

Dispatch unsupported login to the 501 — still no, same as on #2937. supportsLogin() and login are set together in every strategy in the tree: tokensecurity.ts:945-946 sets both, dummysecurity.ts:127 returns false and defines no login. The 501 is unreachable and exists only because login is optional on the interface. Removing the dispatcher gate is a behaviour change — unauthenticated clients would start receiving a distinct 501 where they currently get silence — and Signal K websockets open before authentication by design so login can happen over the socket, which makes that path reachable by anyone. It belongs in its own PR if it is wanted at all.

Verified: 0 type errors, build and ci-lint clean, npm run test-only 1171 passing 0 failing, and binary-stream-auth.ts + security.js 58 passing together.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@dirkwa Thanks for the detailed investigation.

The lazy debug formatting and AuthenticatedRequest changes address the applicable findings.

I agree that the delta narrowing needs a separate, focused change in src/interfaces/ws.ts. The JSDoc documents the current runtime constraint without expanding this PR.

I withdraw the unsupported-login concern. Signal K WebSocket connections intentionally open before authentication. The existing silent behavior is therefore part of the intended flow.

🐇

---

✏️ Learnings added

Learnt from: dirkwa
URL: https://github.com/SignalK/signalk-server/pull/2940

Timestamp: 2026-08-10T18:00:40.651Z
Learning: In Signal K WebSocket authentication, connections intentionally open before authentication so clients can log in over the socket. When `SecurityStrategy.supportsLogin()` is false and `login` is unavailable, the dispatcher must retain its existing silent behavior rather than return HTTP 501. `supportsLogin()` and `login` are configured together by the current security strategies.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

_{You are interacting with an AI system.}

[dirkwa]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[dirkwa]

ready for human review