fix(security): scope PausePollersForToken to requesting workspace (#329) - #335
Merged
Merged
Conversation
Severity MEDIUM. Follow-up to #319/#327. After encrypting bot_token in channel_config, PausePollersForToken was rewritten to fetch every enabled channel across all workspaces and decrypt each in Go (since `channel_config->>'bot_token' = $1` can no longer match ciphertext). That put every tenant's plaintext token in the Go process's memory on every discovery call — a blast-radius problem if a heap dump, profiler endpoint, or future core-dump path ever leaked process memory. Fix: scope the lookup to the requesting workspace. Discover handler now requires workspace_id in the request body and passes it through PausePollersForToken(workspaceID, botToken). Only the caller's own channels are ever decrypted. Changes: - platform/internal/channels/manager.go — PausePollersForToken(ws, tok); SQL predicate now `workspace_id = $1 AND enabled = true`. Reload() keeps the unscoped query (it legitimately starts every workspace's pollers at platform boot). - platform/internal/handlers/channels.go — Discover body struct gains workspace_id field, rejected with 400 if missing, passed to PausePollersForToken. - canvas/src/components/tabs/ChannelsTab.tsx — send workspace_id in the discover request body (the component already holds it as a prop). - platform/internal/handlers/channels_test.go — add TestChannelHandler_Discover_329_RequiresWorkspaceID; update the two existing Discover tests to include workspace_id so they exercise their actual assertion (unsupported-type / invalid-token) instead of bouncing at the new scope gate. Closes #329 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
HongmingWang-Rabbit
left a comment
HongmingWang-Rabbit
left a comment
Contributor
Author
There was a problem hiding this comment.
Dev Lead Code Review — ✅ Approved
Issue: #329 — PausePollersForToken decrypts all workspaces' bot tokens on every /discover call
What I checked
- SQL scoping (
manager.go):WHERE enabled = true AND workspace_id = $1— correct. Only the requesting workspace's channels are loaded and decrypted. Blast-radius for a heap dump now bounded to one tenant. ✅ - Empty-value guard:
workspaceID ==check added toPausePollersForTokenalongside existingbotToken ==guard. ✅ - Handler enforcement (
channels.go Discover): Returns 400"workspace_id is required"before any DB call if field missing. No unscoped query ever fires. ✅ - Frontend (
ChannelsTab.tsx):workspace_id: workspaceIdadded to discover POST —workspaceIdcomes from component props bound to the specific workspace panel being configured. ✅ Reload()intentionally unscoped: Comment correctly justifies this — startup legitimately needs all workspaces' pollers. ✅- Tests:
TestChannelHandler_Discover_329_RequiresWorkspaceIDtests the 400 path before DB; existing tests updated withworkspace_id. ✅
Minor flag (non-blocking — follow-up issue recommended)
The handler accepts any workspace_id from the request body without verifying the caller is authorized for that workspace. An authenticated user from workspace A could pass workspace B's ID and pause workspace B's pollers — a targeted DoS, not a data leak. The bot tokens themselves are never returned, so no confidentiality breach. But if another workspace's ID is passed, PausePollersForToken will silently no-op (no matching pollers in the caller's workspace), so the practical impact is: poller not paused, Telegram returns 409, and the user sees a conflict error. Lower severity than it sounds — but worth a inline comment or a follow-up issue.
Verdict: The core fix is correct and the security improvement is real. Merge.
Merged
6 tasks
HongmingWang-Rabbit
pushed a commit
that referenced
this pull request
Jun 12, 2026
#335, supersedes #1608) Empirical finding (a6e3ff018, 2026-05-20): molecule-core's runtime_image_pins table (mig 047) has never had a writer in any repo. The reader at handlers/runtime_image_pin.go has been hitting sql.ErrNoRows on every workspace provision since mig 047 landed, silently falling through to the :latest path. CP's parallel table (CP mig 027) is the de-facto and only SSOT — it has the writer (POST /cp/admin/runtime-image/promote), the reader, the hard-gate (RFC internal#541 Step 2), seeded post-suspension digests (CP mig 028), and the admin endpoints. This PR ratifies that reality. Note: this is a fresh rebase against current main (tip f17375a). PR #1608 was cut from a base before #1585 (RFC#596 Phase 2 dual-push) landed, so merging it would silently revert the publish-runtime.yml Gitea-PyPI-primary path. Sub-agent a5521785 flagged this on PR #1608 comment 41389. The substantive Go logic is identical to PR #1608; the only difference is the base. What - Add 20260520120000_drop_runtime_image_pins.up.sql / .down.sql to drop the unused table. Care zone PRESERVED: workspaces.runtime_image_digest column + its partial index untouched (earmarked for a future stale-workspace panel per RFC internal#617 §3). - Delete handlers/runtime_image_pin.go (the dead reader) + handlers/runtime_image_pin_test.go. - handlers/workspace_provision.go: replace resolveRuntimeImage(ctx, payload.Runtime) with Image: "" (the dead reader was already returning "" on every call). Rewire the surviving db.DB.QueryRow on this call site to QueryRowContext so the provision-timeout ctx stays load-bearing. - Doc comments in provisioner/provisioner.go + provisioner/registry.go updated to point at CP as the SSOT instead of the dead local table. - Add db/migration_20260520_drop_runtime_image_pins_test.go — static- file pin that up.sql DROPs runtime_image_pins, does NOT touch the care-zone column / index, and that the dead reader files cannot be re-added without failing the test. - Hygiene: prune the now-stranded mock.ExpectQuery("SELECT digest FROM runtime_image_pins") rows in handlers/handlers_test.go and handlers/workspace_provision_test.go (the dead reader is gone, so the mock expectation can never fire). Provisioner test comment updated to reflect CP-as-SSOT. Why Two parallel-named tables with structurally incompatible schemas, only one ever written — that is exactly the kind of internal drift feedback_no_single_source_of_truth was written about for non-vendor surfaces. The deletion is reversible (down.sql recreates the table) and the only behavior change is "ctx is now propagated into the workspace_dir DB lookup", which is a small correctness nudge. Verification - [x] go vet ./internal/handlers/... ./internal/db/... ./internal/provisioner/... — clean - [x] go build ./... — clean - [x] go test ./internal/handlers/ ./internal/db/ ./internal/provisioner/ — all pass (16.5s + 0.2s + 0.3s) - [x] New regression tests assert the care-zone column is not touched + the dead reader cannot return - [x] Empirical grep cross-check: no writer for runtime_image_pins in molecule-core; no reader for workspaces.runtime_image_digest anywhere (both confirmed in RFC internal#617 §1 + §3) - [x] Verified clean rebase: branch parent is current main tip (f17375a), NOT pre-#1585 stale base. Diff vs main contains ONLY the migration-drop work — no .gitea/workflows/publish-runtime.yml regression. Tier tier:medium + area:schema — schema/migration change. Reversible by re-running the down-migration. Two-eye review reviewers: core-be (read path / Go) + core-qa (migration correctness). Cascade plan to ~6 live tenant DBs per RFC internal#617 §7 + feedback_image_promote_is_not_user_live (verify on at least 2 tenants post-deploy). Memory consulted: feedback_no_single_source_of_truth, feedback_image_promote_is_not_user_live, feedback_verify_actual_endstate_not_ack_follow_sop, reference_package_distribution_open_ecosystem_dual_push. RFC: https://git.moleculesai.app/molecule-ai/internal/issues/617 Supersedes: https://git.moleculesai.app/molecule-ai/molecule-core/pulls/1608 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
Tighten #327 per #329.
PausePollersForTokenwas decrypting every tenant's bot token on every discovery call. Scope the lookup to the requesting workspace so only the caller's own channels ever hit the plaintext path.Changes
channels/manager.go—PausePollersForToken(workspaceID, botToken); SQL gainsworkspace_id = $1.handlers/channels.go—Discoverrequiresworkspace_idin body (400 otherwise), passes through.canvas/ChannelsTab.tsx— sendworkspace_idin the discover POST body.TestChannelHandler_Discover_329_RequiresWorkspaceID; existing tests updated to sendworkspace_idso they exercise their intended assertions.Reload()intentionally keeps the unscoped query — it legitimately needs every workspace's pollers at platform boot.Test plan
go test -race ./internal/channels/... ./internal/handlers/...— all greennpm test -- --runin canvas — 482/482 greenCloses #329