Fix is_locked bypass: 11 endpoints leak or allow manipulation of locked data

[beastoin]

Closes #6511. Extends #6092/#6147 audit coverage.

Adds is_locked enforcement to 11 endpoints across 4 router files that either leaked locked data or allowed write operations on locked items:

• action_items.py (5 gaps): pending-sync now filters locked items; sync-batch skips locked; share rejects locked with 402; public shared preview skips locked; accept pre-validates before burning token with rollback on race
• mcp.py (2 gaps): delete/edit memory now check is_locked via new _validate_mcp_memory helper
• mcp_sse.py (2 gaps): delete/edit memory now check is_locked using ToolExecutionError (-32002)
• folders.py (2 gaps): single move and bulk move now reject locked conversations with 402
• redis_db.py: added undo_accept_task_share for accept token rollback

27 new unit tests covering all gaps (locked rejected, unlocked passes, edge cases).

---

by AI for @beastoin

[greptile-apps]

Greptile Summary

This PR adds is_locked enforcement to 11 previously unguarded endpoints across action_items.py, mcp.py, mcp_sse.py, and folders.py, and introduces undo_accept_task_share for rollback on the token-burn race in the accept flow. The security fixes are correct in intent, but undo_accept_task_share is missing the @try_catch_decorator used by every other Redis function in the file — if Redis is unavailable during rollback, the function throws instead of failing silently, so the caller's raise HTTPException(status_code=402) is never reached and the user gets a 500 with a consumed, irrecoverable token.

Confidence Score: 4/5

Safe to merge after adding @try_catch_decorator to undo_accept_task_share; all other lock guards are correct.

One P1 defect: missing @try_catch_decorator on undo_accept_task_share causes a 500 (instead of 402) and permanently consumes the share token when Redis errors during rollback. All other changes are correct and well-tested. The fix is a one-line decorator addition.

backend/database/redis_db.py — undo_accept_task_share needs @try_catch_decorator

Important Files Changed

Filename	Overview
backend/database/redis_db.py	Adds undo_accept_task_share for rollback, but missing @try_catch_decorator unlike all other Redis helpers — exception on r.srem() propagates instead of failing silently, breaking the "best-effort" contract and consuming the user's share token without sending the 402.
backend/routers/action_items.py	Adds is_locked guards to 5 endpoints: pending-sync filter, sync-batch skip, share reject, shared preview filter, and accept with pre-validate + rollback; logic is correct but each eligible task is fetched twice (pre-check + copy loop).
backend/routers/folders.py	Adds is_locked 402 guard to single-move and bulk-move endpoints; straightforward and correct.
backend/routers/mcp.py	Introduces _validate_mcp_memory helper and applies it to delete and edit endpoints; clean and correct.
backend/routers/mcp_sse.py	Adds not-found and is_locked checks before delete_memory and edit_memory tool executions using ToolExecutionError codes; correct.
backend/tests/unit/test_lock_bypass_fixes.py	27 new unit tests covering all lock bypass gaps; added to test.sh at line 74 so CI will pick them up.
backend/tests/unit/test_task_sharing.py	Tests for accept-share locked-item scenarios including the race-condition rollback path; already in test.sh at line 34.

Sequence Diagram

sequenceDiagram
    participant Client
    participant accept_endpoint as accept_shared_action_items
    participant Redis
    participant Firestore

    Client->>accept_endpoint: POST /v1/action-items/accept-share
    accept_endpoint->>Redis: get_task_share(token)
    Redis-->>accept_endpoint: uid, task_ids
    loop Pre-validate (NEW)
        accept_endpoint->>Firestore: get_action_item(sender_uid, task_id)
        Firestore-->>accept_endpoint: item or None
    end
    alt No eligible items
        accept_endpoint-->>Client: 402 All tasks locked
    end
    accept_endpoint->>Redis: try_accept_task_share(token, uid)
    Redis-->>accept_endpoint: accepted or already_accepted
    alt Already accepted
        accept_endpoint-->>Client: 409 Conflict
    end
    loop Copy eligible tasks
        accept_endpoint->>Firestore: get_action_item(sender_uid, task_id)
        Firestore-->>accept_endpoint: item
        accept_endpoint->>Firestore: create_action_item(uid, new_item)
    end
    alt Race: all items locked (NEW rollback)
        accept_endpoint->>Redis: undo_accept_task_share(token, uid)
        accept_endpoint-->>Client: 402 Tasks no longer available
    end
    accept_endpoint-->>Client: created, count

Loading

_{Reviews (1): Last reviewed commit: "Add tests for is_locked enforcement in M..." | Re-trigger Greptile}

[greptile-apps]

Missing @try_catch_decorator breaks "best-effort" rollback

Every other Redis helper in this file is wrapped with @try_catch_decorator, which swallows exceptions and returns None. undo_accept_task_share lacks that wrapper, so if r.srem() raises (e.g., Redis is down), the exception propagates to the caller in accept_shared_action_items, the raise HTTPException(status_code=402, ...) line is never executed, and the user gets a 500 with their share token permanently consumed — they cannot re-accept.

Suggested change

	def undo_accept_task_share(token: str, uid: str):
	"""Rollback a task share acceptance (best-effort). Used when post-claim validation fails."""
	key = f'task_share:{token}:accepted'
	r.srem(key, uid)
	@try_catch_decorator
	def undo_accept_task_share(token: str, uid: str):
	"""Rollback a task share acceptance (best-effort). Used when post-claim validation fails."""
	key = f'task_share:{token}:accepted'
	r.srem(key, uid)

[greptile-apps]

Double DB fetch per eligible task

Each task in eligible_ids is fetched from Firestore once in the pre-validation loop (line ~534) and a second time inside the copy loop here (line 552). The second fetch's result is what actually determines whether the item gets copied, so the first fetch's result is discarded. Passing the already-fetched objects through as (task_id, item) tuples halves the Firestore round-trips while preserving the race-condition re-check.

eligible = []
for task_id in task_ids:
    item = action_items_db.get_action_item(sender_uid, task_id)
    if item and not item.get('is_locked', False):
        eligible.append((task_id, item))

if not eligible:
    raise HTTPException(status_code=402, detail="All shared tasks are locked. A paid plan is required.")

# ... burn token ...

created_ids = []
for task_id, original in eligible:
    if original.get('is_locked', False):  # re-check in case of race
        continue
    ...

[beastoin]

Test results:

• pytest tests/unit/test_task_sharing.py -v — 29/29 pass (8 new is_locked tests)
• pytest tests/unit/test_lock_bypass_fixes.py -k "Mcp or Folder" — 18/18 pass (all new)
• bash test.sh — all pass except 3 pre-existing failures in test_conversation_source_unknown.py (unrelated)
• L1: Backend built and started on branch; all endpoints respond correctly (verified via curl)
• L2: Full API integration chain verified: create → share → preview → accept → sync-batch → folder operations

MCP endpoints (P8-P11) tested via unit tests only — MCP API key auth has no ADMIN_KEY bypass for live testing.

---

by AI for @beastoin

[beastoin]

lgtm