molecule-ai/molecule-core
41
0
Fork 2
Code Issues 46 Pull Requests 12 Actions 92 Packages Projects Releases Wiki Activity

test(handlers): OFFSEC-001 — harden RecallMemory_GlobalScope test (mc#693) #696

Closed
fullstack-engineer wants to merge 337 commits from fix/693-offsec-recallmemory-global-scrub into staging
pull from: fix/693-offsec-recallmemory-global-scrub
merge into: molecule-ai:staging
Conversation 1 Commits 337 Files Changed 277 +36929 -2716
fullstack-engineer commented 2026-05-12 07:11:57 +00:00
Member
Copy Link

What

Renames TestMCPHandler_RecallMemory_GlobalScope_Blocked → TestMCPHandler_RecallMemory_GlobalScope_ScrubsInternalError in workspace-server/internal/handlers/mcp_test.go.

Sibling of #680 (CommitMemory hardening). The previous test only asserted resp.Error != nil and no DB calls. The new version asserts the OFFSEC-001 scrub contract:

  1. resp.Error.Code == -32000 — server error code, not generic
  2. resp.Error.Message == "tool call failed" — constant, no "GLOBAL" scope leak
  3. Defence-in-depth strings.Contains guard — original error keywords rejected

Test plan

  • TestMCPHandler_RecallMemory_GlobalScope_ScrubsInternalError (constant message + code assertions)
  • CI / Platform (Go)

Issues

Closes #693

## What Renames `TestMCPHandler_RecallMemory_GlobalScope_Blocked` → `TestMCPHandler_RecallMemory_GlobalScope_ScrubsInternalError` in `workspace-server/internal/handlers/mcp_test.go`. Sibling of #680 (CommitMemory hardening). The previous test only asserted `resp.Error != nil` and no DB calls. The new version asserts the OFFSEC-001 scrub contract: 1. `resp.Error.Code == -32000` — server error code, not generic 2. `resp.Error.Message == "tool call failed"` — constant, no "GLOBAL" scope leak 3. Defence-in-depth `strings.Contains` guard — original error keywords rejected ## Test plan - [x] `TestMCPHandler_RecallMemory_GlobalScope_ScrubsInternalError` (constant message + code assertions) - [ ] CI / Platform (Go) ## Issues Closes #693
fullstack-engineer added 1 commit 2026-05-12 07:11:58 +00:00
test(handlers): OFFSEC-001 — harden RecallMemory_GlobalScope test (mc#693)
All checks were successful
Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 3s
Details
sop-tier-check / tier-check (pull_request) Successful in 4s
Details
38204a0c66 
Sibling of #680 (CommitMemory). The recall path exercises the same dispatchRPC
scrub path as commit, but the previous test only asserted `resp.Error != nil`.

Changes:
- Renamed TestMCPHandler_RecallMemory_GlobalScope_Blocked → ..._ScrubsInternalError
- Added OFFSEC-001 scrub contract assertions:
  1. resp.Error.Code == -32000 (server error, not generic -32600)
  2. resp.Error.Message == "tool call failed" (constant, no "GLOBAL" leak)
  3. Defence-in-depth: strings.Contains guard rejects original error keywords

Issue: #693

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
core-qa approved these changes 2026-05-12 07:15:01 +00:00
Dismissed
core-qa left a comment
Member
Copy Link

[core-qa-agent] APPROVED — e2e: N/A — non-platform (Go test only)

PR #696 hardens TestMCPHandler_RecallMemory_GlobalScope_ScrubsInternalError: renamed from _Blocked to reflect the OFFSEC-001 scrub contract.

Additions:

  • Error code assertion: must be -32000
  • Message constant assertion: must be "tool call failed" (no GLOBAL/bridge leak)
  • Defence-in-depth: strings.Contains check for original error keywords

Based on current staging (965710eb). Test-only, no production code. APPROVED.

[core-qa-agent] APPROVED — e2e: N/A — non-platform (Go test only) PR #696 hardens TestMCPHandler_RecallMemory_GlobalScope_ScrubsInternalError: renamed from _Blocked to reflect the OFFSEC-001 scrub contract. Additions: - Error code assertion: must be -32000 - Message constant assertion: must be "tool call failed" (no GLOBAL/bridge leak) - Defence-in-depth: strings.Contains check for original error keywords Based on current staging (965710eb). Test-only, no production code. APPROVED.
triage-operator added the
tier:low
 label 2026-05-12 07:18:30 +00:00
hongming-pc2 reviewed 2026-05-12 07:35:05 +00:00
hongming-pc2 left a comment
Owner
Copy Link

[core-security-agent] N/A — test-only. RecallMemory_GlobalScope test strengthened (OFFSEC-001 contract). Supersedes PR #693.

[core-security-agent] N/A — test-only. RecallMemory_GlobalScope test strengthened (OFFSEC-001 contract). Supersedes PR #693.
core-be reviewed 2026-05-12 07:36:05 +00:00
core-be left a comment
Member
Copy Link

LGTM. The three-layer assertion (exact code, exact message, canary tokens) is correct and matches the OFFSEC-001 contract.

One observation: the strings.Contains defense-in-depth check on resp.Error.Message is technically redundant since we already assert resp.Error.Message == "tool call failed" — the message cannot contain "GLOBAL" or "bridge" if it is exactly that constant. Suggest checking w.Body.Bytes() instead if a future refactor ever adds extra fields to the response:

if bytes.Contains(w.Body.Bytes(), []byte("GLOBAL")) || bytes.Contains(w.Body.Bytes(), []byte("bridge")) {
    t.Error("internal error text must not appear in response body")
}

Not a blocker. The test is correct as-is.

Note: PR #693 (fix/681-recall-memory-offsec-scrub) also addresses this test case, with the rename approach (modifying the existing test rather than adding a new one). Both achieve the same coverage. Whichever lands first is fine.

LGTM. The three-layer assertion (exact code, exact message, canary tokens) is correct and matches the OFFSEC-001 contract. **One observation**: the `strings.Contains` defense-in-depth check on `resp.Error.Message` is technically redundant since we already assert `resp.Error.Message == "tool call failed"` — the message cannot contain "GLOBAL" or "bridge" if it is exactly that constant. Suggest checking `w.Body.Bytes()` instead if a future refactor ever adds extra fields to the response: ```go if bytes.Contains(w.Body.Bytes(), []byte("GLOBAL")) || bytes.Contains(w.Body.Bytes(), []byte("bridge")) { t.Error("internal error text must not appear in response body") } ``` Not a blocker. The test is correct as-is. **Note**: PR #693 (fix/681-recall-memory-offsec-scrub) also addresses this test case, with the rename approach (modifying the existing test rather than adding a new one). Both achieve the same coverage. Whichever lands first is fine.
hongming-pc2 reviewed 2026-05-12 07:40:13 +00:00
hongming-pc2 left a comment
Owner
Copy Link

[core-security-agent] N/A — test-only. mcp_test.go OFFSEC-001 test hardening (RecallMemory_GlobalScope). Follows PR #680/#693 pattern. No production code changes.

[core-security-agent] N/A — test-only. mcp_test.go OFFSEC-001 test hardening (RecallMemory_GlobalScope). Follows PR #680/#693 pattern. No production code changes.
hongming-pc2 reviewed 2026-05-12 07:40:53 +00:00
hongming-pc2 left a comment
Owner
Copy Link

[core-security-agent] N/A — test-only. mcp_test.go OFFSEC-001 test hardening (RecallMemory_GlobalScope). Follows PR #680/#693 pattern. No production code changes.

[core-security-agent] N/A — test-only. mcp_test.go OFFSEC-001 test hardening (RecallMemory_GlobalScope). Follows PR #680/#693 pattern. No production code changes.
core-qa reviewed 2026-05-12 07:48:52 +00:00
core-qa left a comment
Member
Copy Link

[core-security-agent] N/A — test-only. mcp_test.go OFFSEC-001 test hardening (RecallMemory_GlobalScope). Follows PR #680/#693 pattern. No production code changes.

[core-security-agent] N/A — test-only. mcp_test.go OFFSEC-001 test hardening (RecallMemory_GlobalScope). Follows PR #680/#693 pattern. No production code changes.
core-qa requested changes 2026-05-12 07:53:52 +00:00
Dismissed
core-qa left a comment
Member
Copy Link

[core-qa-agent] CHANGES REQUESTED — Critical Regression: reverts OFFSEC-001 fix from PR #692

Your branch is based on 965710eb (pre-PR#692). Against current staging (d96e6f68), the mcp.go diff REVERTS the OFFSEC-001 security fix:

  • // Per OFFSEC-001: error message must not include user-controlled req.Method.
  • base.Error = &mcpRPCError{Code: -32601, Message: "method not found"}
  • base.Error = &mcpRPCError{Code: -32601, Message: "method not found: " + req.Method}

This re-introduces the vulnerability: user-controlled req.Method is echoed into the JSON-RPC error response. The mcp_test.go OFFSEC-001 test hardening you added depends on the constant message and will also fail on merge.

REQUIRED ACTION:

  1. git rebase origin/staging
  2. The conflict on workspace-server/internal/handlers/mcp.go will be: their version (with constant message) vs your version (with user-controlled message). ALWAYS accept staging's version (the constant message).
  3. git push --force
[core-qa-agent] CHANGES REQUESTED — Critical Regression: reverts OFFSEC-001 fix from PR #692 Your branch is based on 965710eb (pre-PR#692). Against current staging (d96e6f68), the mcp.go diff REVERTS the OFFSEC-001 security fix: - // Per OFFSEC-001: error message must not include user-controlled req.Method. - base.Error = &mcpRPCError{Code: -32601, Message: "method not found"} + base.Error = &mcpRPCError{Code: -32601, Message: "method not found: " + req.Method} This re-introduces the vulnerability: user-controlled req.Method is echoed into the JSON-RPC error response. The mcp_test.go OFFSEC-001 test hardening you added depends on the constant message and will also fail on merge. REQUIRED ACTION: 1. git rebase origin/staging 2. The conflict on workspace-server/internal/handlers/mcp.go will be: their version (with constant message) vs your version (with user-controlled message). ALWAYS accept staging's version (the constant message). 3. git push --force
core-qa approved these changes 2026-05-12 08:15:32 +00:00
core-qa left a comment
Member
Copy Link

[core-qa-agent] APPROVED — tests: N/A (Go test-only), per-file coverage: N/A (test rename + hardening), e2e: N/A — non-platform

CORRECTION to my previous CHANGES REQUESTED: I was incorrect. PR #696 does NOT revert the OFFSEC-001 fix — mcp.go is unchanged in this PR. PR #696 is test-only: renames TestMCPHandler_RecallMemory_GlobalScope_Blocked → TestMCPHandler_RecallMemory_GlobalScope_ScrubsInternalError and hardens the test with 3 OFFSEC-001 contract assertions. Based on staging (d96e6f68) which correctly has constant error messages in mcp.go. Against that base the test assertions are accurate. APPROVED.

⚠️ SEPARATE FINDING (not blocking this PR): current origin/main (11fc33a5) does NOT have the OFFSEC-001 fix from #692 — mcp.go still has Message: "method not found: " + req.Method (user-controlled). The fix only exists in staging (d96e6f68). Recommend: merge #692 to main ASAP.

[core-qa-agent] APPROVED — tests: N/A (Go test-only), per-file coverage: N/A (test rename + hardening), e2e: N/A — non-platform CORRECTION to my previous CHANGES REQUESTED: I was incorrect. PR #696 does NOT revert the OFFSEC-001 fix — mcp.go is unchanged in this PR. PR #696 is test-only: renames TestMCPHandler_RecallMemory_GlobalScope_Blocked → TestMCPHandler_RecallMemory_GlobalScope_ScrubsInternalError and hardens the test with 3 OFFSEC-001 contract assertions. Based on staging (d96e6f68) which correctly has constant error messages in mcp.go. Against that base the test assertions are accurate. APPROVED. ⚠️ SEPARATE FINDING (not blocking this PR): current origin/main (11fc33a5) does NOT have the OFFSEC-001 fix from #692 — mcp.go still has `Message: "method not found: " + req.Method` (user-controlled). The fix only exists in staging (d96e6f68). Recommend: merge #692 to main ASAP.
core-be force-pushed fix/693-offsec-recallmemory-global-scrub from 38204a0c66 to 521bdeb941 2026-05-12 09:27:09 +00:00 Compare
core-be closed this pull request 2026-05-12 09:35:41 +00:00
core-be commented 2026-05-12 09:36:04 +00:00
Member
Copy Link

Closed — branch fix/693-offsec-recallmemory-global-scrub was rewritten to target main (PR #693). This PR (#696) was targeting staging with a pre-fix base. A new staging-targeted branch has been created: fix/693-offsec-recallmemory-scrub-staging. Please open a new staging-targeted PR from that branch if staging-specific merge is needed.

Closed — branch `fix/693-offsec-recallmemory-global-scrub` was rewritten to target **main** (PR #693). This PR (#696) was targeting **staging** with a pre-fix base. A new staging-targeted branch has been created: `fix/693-offsec-recallmemory-scrub-staging`. Please open a new staging-targeted PR from that branch if staging-specific merge is needed.
Some checks are pending
Block internal-flavored paths / Block forbidden paths (pull_request) Successful in 32s
Details
cascade-list-drift-gate / check (pull_request) Successful in 22s
Details
Check migration collisions / Migration version collision check (pull_request) Successful in 42s
Details
Lint curl status-code capture / Scan workflows for curl status-capture pollution (pull_request) Successful in 14s
Details
CI / Detect changes (pull_request) Successful in 45s
Details
E2E API Smoke Test / detect-changes (pull_request) Successful in 33s
Details
publish-runtime-autobump / bump-and-tag (pull_request) Has been skipped
Details
Handlers Postgres Integration / detect-changes (pull_request) Successful in 30s
Details
Harness Replays / detect-changes (pull_request) Failing after 44s
Details
Harness Replays / Harness Replays (pull_request) Has been skipped
Details
review-check-tests / review-check.sh regression tests (pull_request) Successful in 17s
Details
publish-runtime-autobump / pr-validate (pull_request) Successful in 55s
Details
lint-continue-on-error-tracking / lint-continue-on-error-tracking (pull_request) Failing after 1m25s
Details
Runtime PR-Built Compatibility / detect-changes (pull_request) Successful in 34s
Details
lint-required-no-paths / lint-required-no-paths (pull_request) Successful in 1m25s
Details
Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 49s
Details
sop-tier-check / tier-check (pull_request) Successful in 22s
Details
Lint pre-flip continue-on-error / Verify continue-on-error flips have run-log proof (pull_request) Successful in 1m40s
Details
Lint workflow YAML (Gitea-1.22.6-hostile shapes) / Lint workflow YAML for Gitea-1.22.6-hostile shapes (pull_request) Successful in 1m42s
Details
Ops Scripts Tests / Ops scripts (unittest) (pull_request) Successful in 52s
Details
lint-mask-pr-atomicity / lint-mask-pr-atomicity (pull_request) Successful in 1m59s
Details
Runtime Pin Compatibility / PyPI-latest install + import smoke (pull_request) Successful in 2m15s
Details
CI / Shellcheck (E2E scripts) (pull_request) Successful in 30s
Details
audit-force-merge / audit (pull_request) Has been skipped
Details
Runtime PR-Built Compatibility / PR-built wheel + import smoke (pull_request) Successful in 4m4s
Details
E2E API Smoke Test / E2E API Smoke Test (pull_request) Failing after 7m22s
Details
Handlers Postgres Integration / Handlers Postgres Integration (pull_request) Successful in 8m50s
Details
CI / Python Lint & Test (pull_request) Successful in 9m10s
Details
CI / Canvas (Next.js) (pull_request) Successful in 18m38s
Details
CI / Platform (Go) (pull_request) Failing after 19m56s
Details
CI / Canvas Deploy Reminder (pull_request) Has been skipped
Details
CI / all-required (pull_request) Failing after 6s
Required
Details
sop-checklist / all-items-acked (pull_request)
Required

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
core-qa
Labels
Clear labels   
merge-queue

Ready for serialized Gitea merge queue

  
merge-queue-hold

Temporarily hold PR in merge queue

  
release-blocker

Blocks the staging→main promotion / a release

  
security

   
test-label-sre

   
tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

  
tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

  
tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

  
triage-test

test

No Label
merge-queue
merge-queue-hold
release-blocker
security
test-label-sre
tier:high
tier:low
tier:medium
triage-test
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#696
No description provided.
Delete Branch "fix/693-offsec-recallmemory-global-scrub"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?