Skip to content

Commit a88e341

Browse files
Aurelioloclaude
Aureliolo
and
claude
committed
fix: address 21 review findings from local agents and CodeRabbit
Backend (ws.py): - Fix ?ticket= empty string bypass (use `is not None`) - Reorder accept before subscribe to prevent subscriber leak - Extract _reject_auth helper to reduce _read_auth_message length - Extract _matches_filters helper to reduce _on_event length - Change _on_event send failure log level from WARNING to ERROR - Fix wrong event name API_WS_SEND_FAILED for subscribe failure - Rename accepted param to already_accepted for clarity Frontend: - Use getErrorMessage (not sanitizeForLog) for UI error state in usePolling - Fix sanitizeForLog: strip C1 controls, handle non-positive maxLen, use codePointAt for supplementary Unicode, catch unstringifiable objects - Add NaN/Infinity guards to formatCurrency/formatNumber - Add 4xx fallback in getErrorMessage for uncommon status codes (405, 410, etc.) - Add console.warn for messages failing isWsEvent validation - Derive VALID_WS_CHANNELS from WS_CHANNELS const array (single source of truth) - Add readonly to WsChannel arrays in WS message types - Update useOptimisticUpdate docs for 3rd null-return case - Consistent conditional params in analytics.ts getTrends Tests: - Import WRITE_ROLES constant in useAuth test (prevent drift) - Extend sanitizeForLog property test with BIDI + C1 control checks - Use idiomatic expect().toThrow in client.test.ts - Simplify first-message WS auth test structure Docs & CI: - Add useLoginLockout to CLAUDE.md hooks description - Enable retry-on-snapshot-warnings in dependency-review workflow Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 82b5fa3 commit a88e341

15 files changed

Lines changed: 127 additions & 79 deletions

File tree

‎.github/workflows/dependency-review.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ jobs:
2222
uses: actions/dependency-review-action@2031cfc080254a8a887f58cffee85186f0e49e48 # v4
2323
with:
2424
fail-on-severity: high
25+
retry-on-snapshot-warnings: true
2526
# LicenseRef-scancode-free-unknown: aiosqlite 0.21.0 -- MIT per classifiers, scancode misdetects
2627
# Python-2.0.1: editorconfig 0.17.1 (via jsbeautifier via litestar[standard])
2728
# MIT-0: cffi 2.0.0 -- permissive (MIT variant, no attribution required)

‎CLAUDE.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ src/synthorg/
122122
web/src/ # React 19 + shadcn/ui + Tailwind CSS dashboard
123123
api/ # Axios client, endpoint modules (18 domains), shared types
124124
components/ # React components: ui/ (shadcn primitives); feature dirs added as pages are built
125-
hooks/ # React hooks (auth, WebSocket, polling, optimistic updates)
125+
hooks/ # React hooks (auth, login lockout, WebSocket, polling, optimistic updates)
126126
lib/ # Utilities (cn() class merging, etc.)
127127
stores/ # Zustand stores (auth, WebSocket, domain shells)
128128
styles/ # Global CSS and Tailwind theme tokens

‎src/synthorg/api/controllers/ws.py‎

Lines changed: 52 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,19 @@ async def _validate_ticket(
100100
return user
101101

102102

103+
async def _reject_auth(
104+
socket: WebSocket[Any, Any, Any],
105+
log_reason: str,
106+
close_reason: str,
107+
*,
108+
code: int = _WS_CLOSE_AUTH_FAILED,
109+
**extra_kwargs: str,
110+
) -> None:
111+
"""Log a warning and close the socket for an auth rejection."""
112+
logger.warning(API_WS_TICKET_INVALID, reason=log_reason, **extra_kwargs)
113+
await socket.close(code=code, reason=close_reason)
114+
115+
103116
async def _read_auth_message( # noqa: PLR0911
104117
socket: WebSocket[Any, Any, Any],
105118
) -> str | None:
@@ -113,43 +126,36 @@ async def _read_auth_message( # noqa: PLR0911
113126
timeout=_WS_AUTH_TIMEOUT_SECONDS,
114127
)
115128
except TimeoutError:
116-
logger.warning(API_WS_TICKET_INVALID, reason="auth_timeout")
117-
await socket.close(code=_WS_CLOSE_AUTH_FAILED, reason="Auth timeout")
129+
await _reject_auth(socket, "auth_timeout", "Auth timeout")
118130
return None
119131
except WebSocketDisconnect:
120132
logger.debug(API_WS_DISCONNECTED, reason="disconnect_during_auth")
121133
return None
122134

123135
if len(data.encode()) > _MAX_WS_MESSAGE_BYTES:
124-
logger.warning(API_WS_TICKET_INVALID, reason="auth_too_large")
125-
await socket.close(
126-
code=_WS_CLOSE_AUTH_FAILED,
127-
reason="Auth message too large",
128-
)
136+
await _reject_auth(socket, "auth_too_large", "Auth message too large")
129137
return None
130138

131139
try:
132140
msg = json.loads(data)
133141
except json.JSONDecodeError:
134-
logger.warning(API_WS_TICKET_INVALID, reason="invalid_auth_json")
135-
await socket.close(code=_WS_CLOSE_AUTH_FAILED, reason="Invalid auth message")
142+
await _reject_auth(socket, "invalid_auth_json", "Invalid auth message")
136143
return None
137144

138145
if not isinstance(msg, dict) or msg.get("action") != "auth":
139146
action = msg.get("action", "") if isinstance(msg, dict) else ""
140-
logger.warning(
141-
API_WS_TICKET_INVALID,
142-
reason="expected_auth_action",
147+
await _reject_auth(
148+
socket,
149+
"expected_auth_action",
150+
"Expected auth action",
143151
action=str(action)[:64],
144152
)
145-
await socket.close(code=_WS_CLOSE_AUTH_FAILED, reason="Expected auth action")
146153
return None
147154

148155
raw_ticket = msg.get("ticket")
149156
ticket: str | None = raw_ticket if isinstance(raw_ticket, str) else None
150157
if not ticket:
151-
logger.warning(API_WS_TICKET_INVALID, reason="missing_ticket_in_auth")
152-
await socket.close(code=_WS_CLOSE_AUTH_FAILED, reason="Missing ticket")
158+
await _reject_auth(socket, "missing_ticket_in_auth", "Missing ticket")
153159
return None
154160

155161
return ticket
@@ -237,6 +243,24 @@ async def _check_ws_role(
237243
return True
238244

239245

246+
def _matches_filters(
247+
event: dict[str, Any],
248+
channel: str,
249+
channel_filters: dict[str, str],
250+
) -> bool:
251+
"""Check whether the event payload matches the active channel filters."""
252+
payload = event.get("payload", {})
253+
if not isinstance(payload, dict):
254+
logger.warning(
255+
API_WS_INVALID_MESSAGE,
256+
channel=channel,
257+
reason="payload_not_dict",
258+
payload_type=type(payload).__name__,
259+
)
260+
return False
261+
return all(payload.get(k) == v for k, v in channel_filters.items())
262+
263+
240264
async def _on_event(
241265
event_data: bytes,
242266
subscribed: set[str],
@@ -276,25 +300,15 @@ async def _on_event(
276300
return
277301

278302
channel_filters = filters.get(channel)
279-
if channel_filters:
280-
payload = event.get("payload", {})
281-
if not isinstance(payload, dict):
282-
logger.warning(
283-
API_WS_INVALID_MESSAGE,
284-
channel=channel,
285-
reason="payload_not_dict",
286-
payload_type=type(payload).__name__,
287-
)
288-
return
289-
if not all(payload.get(k) == v for k, v in channel_filters.items()):
290-
return
303+
if channel_filters and not _matches_filters(event, channel, channel_filters):
304+
return
291305

292306
try:
293307
await socket.send_text(event_data.decode("utf-8"))
294308
except WebSocketDisconnect:
295309
logger.debug(API_WS_SEND_FAILED, reason="client_disconnected")
296310
except Exception:
297-
logger.warning(API_WS_SEND_FAILED, exc_info=True)
311+
logger.error(API_WS_SEND_FAILED, exc_info=True)
298312
await socket.close(code=1011, reason="Internal error")
299313

300314

@@ -308,7 +322,7 @@ async def _authenticate_ws(
308322
"""
309323
ticket_param = socket.query_params.get("ticket")
310324

311-
if ticket_param:
325+
if ticket_param is not None:
312326
user = await _validate_ticket(socket)
313327
if user is None:
314328
return None
@@ -342,9 +356,9 @@ async def _setup_connection(
342356
socket: WebSocket[Any, Any, Any],
343357
user: AuthenticatedUser,
344358
*,
345-
accepted: bool,
359+
already_accepted: bool,
346360
) -> tuple[ChannelsPlugin, Any] | None:
347-
"""Resolve plugin, subscribe to channels, and accept the connection.
361+
"""Resolve plugin, accept the connection, and subscribe to channels.
348362
349363
Returns ``(channels_plugin, subscriber)`` on success, or ``None``
350364
(socket already closed) on failure.
@@ -363,11 +377,15 @@ async def _setup_connection(
363377
await socket.close(code=1011, reason="Internal error")
364378
return None
365379

380+
socket.scope["user"] = user
381+
if not already_accepted:
382+
await socket.accept()
383+
366384
try:
367385
subscriber = await channels_plugin.subscribe(list(ALL_CHANNELS))
368386
except Exception:
369387
logger.error(
370-
API_WS_SEND_FAILED,
388+
API_WS_TRANSPORT_ERROR,
371389
reason="subscribe_failed",
372390
client=str(socket.client),
373391
user_id=user.user_id,
@@ -376,9 +394,6 @@ async def _setup_connection(
376394
await socket.close(code=1011, reason="Internal error")
377395
return None
378396

379-
socket.scope["user"] = user
380-
if not accepted:
381-
await socket.accept()
382397
logger.info(
383398
API_WS_CONNECTED,
384399
client=str(socket.client),
@@ -408,12 +423,12 @@ async def ws_handler(
408423
auth_result = await _authenticate_ws(socket)
409424
if auth_result is None:
410425
return
411-
user, accepted = auth_result
426+
user, already_accepted = auth_result
412427

413428
if not await _check_ws_role(socket, user):
414429
return
415430

416-
setup = await _setup_connection(socket, user, accepted=accepted)
431+
setup = await _setup_connection(socket, user, already_accepted=already_accepted)
417432
if setup is None:
418433
return
419434
channels_plugin, subscriber = setup

‎tests/unit/api/controllers/test_ws.py‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -311,13 +311,13 @@ def test_ws_rejects_bad_first_message_ticket(
311311
"""WS connection with invalid first-message ticket is rejected post-accept."""
312312
from litestar.exceptions import WebSocketDisconnect
313313

314-
def send_bad_ticket() -> None:
314+
def attempt() -> None:
315315
with test_client.websocket_connect("/api/v1/ws") as ws:
316316
ws.send_text(json.dumps({"action": "auth", "ticket": "bogus-ticket"}))
317317
ws.receive_text()
318318

319319
with pytest.raises(WebSocketDisconnect) as exc_info:
320-
send_bad_ticket()
320+
attempt()
321321
assert exc_info.value.code == _WS_CLOSE_AUTH_FAILED
322322

323323
def test_ws_rejects_missing_first_message_auth(
@@ -327,13 +327,13 @@ def test_ws_rejects_missing_first_message_auth(
327327
"""WS connection sending non-auth first message is rejected."""
328328
from litestar.exceptions import WebSocketDisconnect
329329

330-
def send_wrong_action() -> None:
330+
def attempt() -> None:
331331
with test_client.websocket_connect("/api/v1/ws") as ws:
332332
ws.send_text(json.dumps({"action": "subscribe", "channels": ["tasks"]}))
333333
ws.receive_text()
334334

335335
with pytest.raises(WebSocketDisconnect) as exc_info:
336-
send_wrong_action()
336+
attempt()
337337
assert exc_info.value.code == _WS_CLOSE_AUTH_FAILED
338338

339339
def test_ws_accepts_valid_ticket(

‎web/src/__tests__/api/client.test.ts‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -64,15 +64,14 @@ describe('unwrap', () => {
6464
error_detail: testErrorDetail,
6565
success: false,
6666
})
67-
let caught: ApiRequestError | null = null
67+
expect(() => unwrap(response)).toThrow(ApiRequestError)
6868
try {
6969
unwrap(response)
7070
} catch (err) {
71-
caught = err as ApiRequestError
71+
const caught = err as ApiRequestError
72+
expect(caught.message).toBe('Something went wrong')
73+
expect(caught.errorDetail).toEqual(testErrorDetail)
7274
}
73-
expect(caught).toBeInstanceOf(ApiRequestError)
74-
expect(caught!.message).toBe('Something went wrong')
75-
expect(caught!.errorDetail).toEqual(testErrorDetail)
7675
})
7776

7877
it('throws for null body', () => {

‎web/src/__tests__/hooks/useAuth.test.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import { renderHook } from '@testing-library/react'
22
import { useAuth } from '@/hooks/useAuth'
33
import { useAuthStore } from '@/stores/auth'
4-
import type { UserInfoResponse } from '@/api/types'
4+
import type { HumanRole, UserInfoResponse } from '@/api/types'
5+
import { WRITE_ROLES } from '@/utils/constants'
56

67
vi.mock('@/api/endpoints/auth', () => ({
78
login: vi.fn(),
@@ -43,8 +44,9 @@ describe('useAuth', () => {
4344
})
4445

4546
describe('canWrite', () => {
46-
const writeRoles = ['ceo', 'manager', 'board_member', 'pair_programmer'] as const
47-
const readOnlyRoles = ['observer', 'system'] as const
47+
const writeRoles = WRITE_ROLES
48+
const allRoles: readonly HumanRole[] = ['ceo', 'manager', 'board_member', 'pair_programmer', 'observer', 'system']
49+
const readOnlyRoles = allRoles.filter((r) => !(WRITE_ROLES as readonly string[]).includes(r))
4850

4951
for (const role of writeRoles) {
5052
it(`returns canWrite=true for ${role}`, () => {

‎web/src/__tests__/utils/logging.property.test.ts‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,24 @@ describe('sanitizeForLog property tests', () => {
1111
)
1212
})
1313

14-
it('never contains control characters', () => {
14+
it('never contains control or BIDI characters', () => {
15+
const bidiControls = new Set([
16+
0x200b, 0x200c, 0x200d, 0x200e, 0x200f,
17+
0x202a, 0x202b, 0x202c, 0x202d, 0x202e,
18+
0x2066, 0x2067, 0x2068, 0x2069,
19+
0xfff9, 0xfffa, 0xfffb,
20+
])
1521
fc.assert(
1622
fc.property(fc.string(), (input) => {
1723
const result = sanitizeForLog(input)
1824
for (const ch of result) {
19-
const code = ch.charCodeAt(0)
25+
const code = ch.codePointAt(0) ?? 0
2026
expect(code).toBeGreaterThanOrEqual(0x20)
2127
expect(code).not.toBe(0x7f)
28+
// C1 controls
29+
expect(code < 0x80 || code > 0x9f).toBe(true)
30+
// BIDI overrides
31+
expect(bidiControls.has(code)).toBe(false)
2232
}
2333
}),
2434
)

‎web/src/api/endpoints/analytics.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ export async function getTrends(
1111
metric?: TrendMetric,
1212
): Promise<TrendsResponse> {
1313
const response = await apiClient.get<ApiResponse<TrendsResponse>>('/analytics/trends', {
14-
params: { period, metric },
14+
params: period !== undefined || metric !== undefined ? { period, metric } : undefined,
1515
})
1616
return unwrap(response)
1717
}

‎web/src/api/types.ts‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -753,14 +753,10 @@ export interface TriggerMeetingRequest {
753753

754754
// ── WebSocket ────────────────────────────────────────────────
755755

756-
export type WsChannel =
757-
| 'tasks'
758-
| 'agents'
759-
| 'budget'
760-
| 'messages'
761-
| 'system'
762-
| 'approvals'
763-
| 'meetings'
756+
/** All valid WebSocket channel names. Runtime set derived from this in websocket store. */
757+
export const WS_CHANNELS = ['tasks', 'agents', 'budget', 'messages', 'system', 'approvals', 'meetings'] as const
758+
759+
export type WsChannel = typeof WS_CHANNELS[number]
764760

765761
export type WsEventType =
766762
| 'task.created'
@@ -800,18 +796,18 @@ export type WsSubscriptionFilters = Readonly<Record<string, string>>
800796

801797
export interface WsSubscribeMessage {
802798
action: 'subscribe'
803-
channels: WsChannel[]
799+
readonly channels: readonly WsChannel[]
804800
filters?: WsSubscriptionFilters
805801
}
806802

807803
export interface WsUnsubscribeMessage {
808804
action: 'unsubscribe'
809-
channels: WsChannel[]
805+
readonly channels: readonly WsChannel[]
810806
}
811807

812808
export interface WsAckMessage {
813809
action: 'subscribed' | 'unsubscribed'
814-
channels: WsChannel[]
810+
readonly channels: readonly WsChannel[]
815811
}
816812

817813
export interface WsErrorMessage {

‎web/src/hooks/useOptimisticUpdate.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,12 @@ import { getErrorMessage } from '@/utils/errors'
88
* `applyOptimistic` applies the optimistic state and returns a rollback function,
99
* and `serverAction` is the actual server request.
1010
*
11-
* A `null` return means one of two things:
11+
* A `null` return means one of three things:
1212
* - **Server error**: `error` is set with a message. The optimistic state was rolled back.
1313
* - **Already in-flight**: `pending` was true, so the call was a no-op.
14+
* - **Optimistic prepare failed**: `applyOptimistic` threw and the state was reverted.
1415
*
15-
* Callers should check `error` to distinguish server errors from no-op returns.
16+
* Callers should check `error` to distinguish failures from no-op returns.
1617
*/
1718
export function useOptimisticUpdate(): {
1819
pending: boolean

0 commit comments

Comments
 (0)