Claude encountered an error —— View job

I'll analyze this and get back to you.

Claude finished @Yeraze's task —— View job

PR Review: fix(security): scope channel-permission node filter by sourceId (#3745)

Tasks:

• Gather context and understand the PR
• Read changed files
• Review filterNodesByChannelPermission and related code
• Provide review feedback

Summary

This PR correctly identifies and fixes a real security bug: filterNodesByChannelPermission was called without sourceId at several endpoints, causing getUserPermissionSetAsync to return a cross-source union of permissions. A guest with channel-0 access on Source A could see Source B's channel-0 nodes.

The fix is conceptually correct and the approach (forwarding sourceId consistently at each call site) is sound.

Issues Found

🔴 Incomplete Fix: checkNodeChannelAccess in server.ts still missing sourceId

The same bug class is present in server.ts for the three checkNodeChannelAccess calls that don't pass a source id:

src/server/server.ts (three occurrences):
    if (!await checkNodeChannelAccess(nodeId, req.user)) {

checkNodeChannelAccess also calls getUserPermissionSetAsync(user.id, sourceId) — and like filterNodesByChannelPermission, when sourceId is undefined it falls back to a cross-source union. telemetryRoutes.ts has the same pattern for the same three calls (lines 47, 110, 191, 227 in the unchanged file). The v1 routes (positionHistory.ts, v1/telemetry.ts) already pass sourceId correctly.

The telemetry endpoints in server.ts (/traceroute/:nodeId, etc.) and the three in telemetryRoutes.ts have the same shape as the bug being fixed — a guest on Source A with channel-0 permission could query telemetry for Source B's channel-0 nodes.

🟡 getChannelDatabasePermissionsForUserAsSetAsync not scoped by source

Both filterNodesByChannelPermission and checkNodeChannelAccess call getChannelDatabasePermissionsForUserAsSetAsync without sourceId. Virtual channels (>= CHANNEL_DB_OFFSET) have the same potential cross-source leak if virtual-channel permissions are source-specific. If this is intentional (channel DB permissions are global-by-design), a comment explaining that would help prevent future confusion.

Code Quality Observations

The fix looks solid where applied. The naming convention for extracted sourceId variables (nodesSourceId, activeNodesSourceId, pollSourceId, unreadSourceId, telAvailSourceId) is clear and consistent.

Test is well-targeted. nodeEnhancer.test.ts:220-228 is a tight regression — it clears the mock, calls with a specific sourceId, and asserts the permission lookup received that exact sourceId. This directly validates the fix.

Minor nit — test mock doesn't exercise source-scoped behavior. The existing getUserPermissionSetAsync mock in nodeEnhancer.test.ts (lines 32-55) ignores the sourceId parameter (async (userId: number) => {...}). The new regression test at line 220 calls toHaveBeenCalledWith(regularUser.id, 'src-B') which is correct, but if the mock were updated to gate on sourceId as well it would also confirm the node list returned is properly scoped. This is a suggestion, not a blocker.

Test Coverage

The single new test (forwards sourceId to the permission lookup — no cross-source union (#3745)) verifies the core invariant. However:

• No regression test for the server.ts endpoints where checkNodeChannelAccess still omits sourceId (because those aren't fixed yet)
• The pollSourceId path in /poll that calls filterNodesByChannelPermission for memory nodes and DB nodes is covered by the existing test structure but not with a source-scoped integration test

Verdict

The fix is correct and necessary for the specific call sites addressed. However, the same checkNodeChannelAccess bug class exists in server.ts and telemetryRoutes.ts for node-specific endpoints (telemetry, traceroutes, etc.) and was not addressed here. These should either be fixed in this PR or tracked as a follow-up issue.

Thanks for the review — both points addressed in the next push:

1. `checkNodeChannelAccess` not source-scoped (valid — same leak class). The function already accepted/forwarded `sourceId`, but 7 call sites omitted it, so node-specific endpoints (telemetry rates/history, position-history/positions/position-override) had the same cross-source union fallback. Fixed all 7 to forward `req.query.sourceId` (server.ts ×3, telemetryRoutes.ts ×4). The v1 routes already passed it. Added a `checkNodeChannelAccess` regression test asserting the permission lookup receives the exact sourceId.

2. `getChannelDatabasePermissionsForUserAsSetAsync` not source-scoped — intentional. The `channel_database` (server-side decryption PSKs) is global-by-design: `channelDecryptionService` tries every enabled row regardless of source, and migration 063 dropped its dead `sourceId` column. So virtual-channel (>= CHANNEL_DB_OFFSET) permissions are global too. Added an explanatory comment at the call sites to prevent future confusion.

Addressed in ca4b3da: forwarded sourceId at all 7 checkNodeChannelAccess call sites (server.ts ×3, telemetryRoutes.ts ×4) + regression test; documented the channel_database global-by-design exception. Full suite 7550 pass.

Claude finished @Yeraze's task —— View job

PR Review: fix(security): scope channel-permission node filter by sourceId (#3745)

Tasks:

• Gather context and understand the PR
• Read changed files (latest push: ca4b3da)
• Verify checkNodeChannelAccess fixes in server.ts and telemetryRoutes.ts
• Verify regression test in nodeEnhancer.test.ts
• Check nodeEnhancer.ts for explanatory comment
• Provide review feedback

Summary

The follow-up push (ca4b3da) fully addresses both issues raised in the first review. This is a clean, correct security fix.

What Changed

server.ts (×3) and telemetryRoutes.ts (×4): All 7 remaining checkNodeChannelAccess call sites now forward req.query.sourceId as string | undefined. Verified all three endpoints in server.ts (/position-history L1324, /positions L1374, /position-override L1804) and all four in telemetryRoutes.ts (/telemetry/:nodeId L47, /telemetry/:nodeId/rates L110, /telemetry/:nodeId/smarthops L191, /telemetry/:nodeId/linkquality L227).

nodeEnhancer.ts: The filterNodesByChannelPermission and checkNodeChannelAccess functions now have explanatory comments at the getChannelDatabasePermissionsForUserAsSetAsync call sites documenting the global-by-design intent of virtual-channel permissions:

NOT source-scoped by design: the channel_database (server-side decryption PSKs) is global — channelDecryptionService tries every enabled row regardless of source, and migration 063 dropped its dead sourceId column.

nodeEnhancer.test.ts: Two new regression tests added — one for filterNodesByChannelPermission (L220–228) and one for checkNodeChannelAccess (L297–303) — both asserting getUserPermissionSetAsync is called with the exact sourceId argument.

Remaining Observation (non-blocking)

checkNodeChannelAccess calls getNode(nodeNum) without sourceId (L378). In a multi-source deployment, the same nodeNum may exist in multiple sources with different channel assignments. The check fetches whichever source's row happens to come back first, which could mean the wrong channel number is used for the permission check. This is a separate (pre-existing) bug, not introduced by this PR, and the sourceId is available as a parameter — a follow-up could pass it here:

// nodeEnhancer.ts:378
const node = await databaseService.nodes.getNode(nodeNum, sourceId);
//                                                          ^^^^^^^^

databaseService.nodes.getNode already accepts an optional sourceId (second parameter, confirmed in src/db/repositories/nodes.ts:130), so this would be a one-line fix.

Verdict

The core security fixes are complete and correct. The regression tests are tight and targeted. The channel_database global-by-design exception is now documented in-code. The only residual concern is the unsourced getNode call inside checkNodeChannelAccess, which is a pre-existing issue and out of scope for this PR.

The PR is ready to merge. The getNode(nodeNum) unscoped call could be tracked as a follow-up in a new issue.

Addressed the remaining observation in 108f2d8: getNode inside checkNodeChannelAccess is now source-scoped (getNode(nodeNum, sourceId)), so the channel that gates access is resolved from the correct source. Test asserts the scoped lookup. Full suite 7550 pass.

Claude finished @Yeraze's task —— View job

PR Review: fix(security): scope channel-permission node filter by sourceId (#3745)

Tasks:

• Gather context and understand the PR
• Read changed files (latest push: 108f2d8)
• Verify getNode source-scoping fix in nodeEnhancer.ts
• Check regression test coverage
• Provide review feedback

Summary

The latest push (108f2d8) addresses the final remaining observation from the previous review: getNode inside checkNodeChannelAccess is now called with sourceId, ensuring the channel used to gate access is resolved from the correct source. This is a clean, complete fix.

What Was Fixed

nodeEnhancer.ts:381 — getNode(nodeNum, sourceId) now scopes the node lookup to the requesting source. The explanatory comment (lines 378–380) is clear about why this matters:

Scope the node lookup to the requesting source — the same nodeNum can exist in multiple sources with different channel assignments, and the channel drives this permission check (#3745).

nodeEnhancer.test.ts:297–307 — Regression test asserts both getUserPermissionSetAsync AND nodes.getNode are called with the exact sourceId argument. This is tighter than the previous test: it validates the two places inside checkNodeChannelAccess where unscoped calls would break the security invariant.

server.ts and telemetryRoutes.ts — All previously-fixed call sites are unchanged and correctly forward req.query.sourceId as string | undefined.

Verification

Spot-checked the three endpoints in server.ts:

• /nodes/:nodeId/position-history (L1324) ✅ source-scoped
• /nodes/:nodeId/positions (L1374) ✅ source-scoped
• /nodes/:nodeId/position-override (L1804) ✅ source-scoped

All four in telemetryRoutes.ts:

• /telemetry/:nodeId (L47) ✅ source-scoped
• /telemetry/:nodeId/rates (L110) ✅ source-scoped
• /telemetry/:nodeId/smarthops (L191) ✅ source-scoped
• /telemetry/:nodeId/linkquality (L227) ✅ source-scoped

The getNode call in telemetryRoutes.ts:68 (privacy check) was already scoped via a local telSourceId — confirmed unchanged.

No Remaining Issues

The three-push fix series fully closes the cross-source permission leak:

1. filterNodesByChannelPermission call sites — sourced at every leaky endpoint
2. checkNodeChannelAccess call sites — sourced at all 7 endpoints
3. getNode inside checkNodeChannelAccess — now resolves channel from the correct source

The channel_database global-by-design exception is documented in-code at each call site. Regression tests cover both the filterNodesByChannelPermission and checkNodeChannelAccess source-forwarding invariants.

The PR is ready to merge.