豆豆友情提示:这是一个非官方 GitHub 代理镜像,主要用于网络测试或访问加速。请勿在此进行登录、注册或处理任何敏感信息。进行这些操作请务必访问官方网站 github.com。 Raw 内容也通过此代理提供。
Skip to content

fix(matrix): skip pairing-store reads for room auth#67325

Merged
gumadeiras merged 2 commits intomainfrom
codex/matrix-room-read-hardening
Apr 15, 2026
Merged

fix(matrix): skip pairing-store reads for room auth#67325
gumadeiras merged 2 commits intomainfrom
codex/matrix-room-read-hardening

Conversation

@gumadeiras
Copy link
Copy Markdown
Member

Summary

  • Problem: Matrix room traffic still read the DM pairing-store even after room control-command auth stopped using pairing-store entries.
  • Why it matters: behavior was already safe, but the room path still pulled in irrelevant DM-only state.
  • What changed: room traffic now passes an empty pairing-store list into the Matrix access-state resolver; the existing room command regression test now also asserts the pairing store is not read.
  • What did NOT change (scope boundary): room command authorization semantics stay exactly as in fix(matrix): block DM pairing-store entries from authorizing room control commands [AI-assisted] #67294; this only narrows the room data path.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

Root Cause (if applicable)

  • Root cause: handler.ts still read readStoreAllowFrom() for Matrix room traffic even though room control-command authorization had already been hardened to ignore pairing-store entries.
  • Missing detection / guardrail: the existing regression test covered the room command block, but it did not assert the dead read was removed.
  • Contributing context (if known): this is a follow-up narrowing pass after the main auth fix landed.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file: extensions/matrix/src/matrix/monitor/handler.test.ts
  • Scenario the test should lock in: room control-command handling must not call the DM pairing-store reader.
  • Why this is the smallest reliable guardrail: the handler test covers the real room path and can assert both the authorization outcome and the absence of the pairing-store read.
  • Existing test that already covers this (if any): the room command regression from fix(matrix): block DM pairing-store entries from authorizing room control commands [AI-assisted] #67294 now carries the stronger assertion.
  • If no new test is added, why not: N/A

User-visible / Behavior Changes

None. This narrows an internal room auth data path without changing room auth outcomes.

Diagram (if applicable)

Before:
[Matrix room message] -> [read DM pairing-store] -> [ignore result for room command auth]

After:
[Matrix room message] -> [skip DM pairing-store read] -> [same room auth result]

Security Impact (required)

  • New permissions/capabilities? (No)
  • Secrets/tokens handling changed? (No)
  • New/changed network calls? (No)
  • Command/tool execution surface changed? (No)
  • Data access scope changed? (Yes)
  • If any Yes, explain risk + mitigation: room traffic no longer reads DM-only pairing-store state, reducing cross-scope coupling while preserving the existing room auth result.

Repro + Verification

Environment

  • OS: macOS
  • Runtime/container: local repo dev environment
  • Model/provider: N/A
  • Integration/channel (if any): Matrix
  • Relevant config (redacted): room traffic, commands.useAccessGroups=true

Steps

  1. Trigger the Matrix room command path.
  2. Observe whether the handler calls readAllowFromStore().
  3. Assert the room command outcome stays the same.

Expected

  • Room traffic skips the DM pairing-store read.

Actual

  • Before this patch, room traffic still called the DM pairing-store reader even though room command auth ignored the result.

Evidence

Attach at least one:

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

  • Verified scenarios: ran the Matrix handler test suite locally and confirmed the strengthened room command regression passes.
  • Edge cases checked: existing DM pairing-store behavior remains covered by the same handler suite.
  • What you did not verify: skipped broader/full-suite gates per request; this PR is validated with targeted Matrix tests only.

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

  • Backward compatible? (Yes)
  • Config/env changes? (No)
  • Migration needed? (No)
  • If yes, exact upgrade steps:

Risks and Mitigations

  • Risk: room traffic could accidentally stop reading the pairing store on a DM path if the room/DM split were wrong.
    • Mitigation: the change is a single isDirectMessage guard and the targeted Matrix handler suite still passes, covering both room and DM pairing paths.

Copilot AI review requested due to automatic review settings April 15, 2026 17:39
@openclaw-barnacle openclaw-barnacle bot added channel: matrix Channel integration: matrix size: XS maintainer Maintainer-authored PR labels Apr 15, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR narrows the Matrix room message handling path by skipping DM pairing-store reads, since room control-command authorization no longer depends on pairing-store entries (per #67294).

Changes:

  • Skip readAllowFromStore() for room traffic by passing an empty storeAllowFrom list when isDirectMessage is false.
  • Strengthen the existing room control-command regression test to assert the pairing-store reader is not invoked.
  • Add a changelog entry documenting the narrowed room auth data path.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
extensions/matrix/src/matrix/monitor/handler.ts Avoids DM pairing-store reads on room traffic by gating the store read on isDirectMessage.
extensions/matrix/src/matrix/monitor/handler.test.ts Adds an assertion that the pairing-store reader is not called in the room control-command regression scenario.
CHANGELOG.md Documents the behavior change for Matrix room traffic.

Comment thread CHANGELOG.md Outdated
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 15, 2026

Greptile Summary

This PR adds a single isDirectMessage guard in handler.ts so room traffic passes [] directly to resolveMatrixMonitorAccessState instead of calling the DM pairing-store reader — a dead read since #67294 already hardened room command auth to ignore pairing-store entries. The existing regression test gains a not.toHaveBeenCalled() assertion to lock in the removal, while all DM-pairing tests remain unchanged and continue to verify the store is still read on the DM path.

  • The new CHANGELOG.md entry is placed at the top of the ### Fixes section instead of the end, which contradicts the project's changelog placement rule (append, don't prepend).

Confidence Score: 5/5

Safe to merge — change is minimal, correct, and well-guarded by the updated regression test.

The only finding is a P2 changelog placement convention violation. The code change itself is clean, logically sound, preserves DM behavior, and is directly verified by the updated test.

CHANGELOG.md — entry ordering should be fixed before merge if the project enforces strict changelog ordering.

Prompt To Fix All With AI
This is a comment left during a code review.
Path: CHANGELOG.md
Line: 11

Comment:
**Changelog entry placed at top of section instead of end**

`CLAUDE.md` says: _"append new entries to the end of the target section (`### Changes` or `### Fixes`); do not insert new entries at the top of a section."_ This entry is inserted before the existing `#67298`, `#67294`, and other already-landed fixes rather than after them.

**Context Used:** CLAUDE.md ([source](https://app.greptile.com/review/custom-context?memory=fd949e91-5c3a-4ab5-90a1-cbe184fd6ce8))

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "matrix: skip pairing-store reads for roo..." | Re-trigger Greptile

Comment thread CHANGELOG.md Outdated
@gumadeiras gumadeiras self-assigned this Apr 15, 2026
@gumadeiras gumadeiras force-pushed the codex/matrix-room-read-hardening branch from b48ff88 to 5e80368 Compare April 15, 2026 17:58
@gumadeiras gumadeiras force-pushed the codex/matrix-room-read-hardening branch from 5e80368 to 121ff3b Compare April 15, 2026 18:08
@gumadeiras gumadeiras merged commit 2bfd808 into main Apr 15, 2026
7 checks passed
@gumadeiras gumadeiras deleted the codex/matrix-room-read-hardening branch April 15, 2026 18:08
@gumadeiras
Copy link
Copy Markdown
Member Author

Merged via squash.

Thanks @gumadeiras!

xudaiyanzi pushed a commit to xudaiyanzi/openclaw that referenced this pull request Apr 17, 2026
Merged via squash.

Prepared head SHA: 121ff3b
Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com>
Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com>
Reviewed-by: @gumadeiras
kvnkho pushed a commit to kvnkho/openclaw that referenced this pull request Apr 17, 2026
Merged via squash.

Prepared head SHA: 121ff3b
Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com>
Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com>
Reviewed-by: @gumadeiras
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: matrix Channel integration: matrix maintainer Maintainer-authored PR size: XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants