Skip to content

fix(identity): allow_from fallthrough for Matrix user IDs with colon#3045

Open
chengzhichao-xydt wants to merge 2 commits into
sipeed:mainfrom
chengzhichao-xydt:codex/allow-from-matrix-colon
Open

fix(identity): allow_from fallthrough for Matrix user IDs with colon#3045
chengzhichao-xydt wants to merge 2 commits into
sipeed:mainfrom
chengzhichao-xydt:codex/allow-from-matrix-colon

Conversation

@chengzhichao-xydt

Copy link
Copy Markdown
Contributor

Problem

allow_from silently rejects messages from Matrix users when using the standard Matrix user ID format (@alice:example.com). Fixes #3044.

Root cause: ParseCanonicalID splits on the first colon, so @alice:example.com is parsed as platform="@alice", id="example.com". Since @alice is not numeric, the code enters the canonical-match branch and builds "@alice:example.com" as the candidate — which doesn't match sender.CanonicalID ("matrix:@alice:example.com"). The function then returned false immediately, never trying the remaining PlatformID or Username match strategies.

Fix

When the canonical match fails but a colon was present, fall through to the remaining match strategies (PlatformID, Username) instead of returning early. This allows Matrix-style IDs to still match via the legacy paths.

Changes

  • pkg/identity/identity.go: refactor early returns into fallthrough

Verification

  • go build ./pkg/identity/... passes
  • go test ./pkg/identity/... passes (all existing tests green)

@afjcjsbx

afjcjsbx commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

Hi @chengzhichao-xydt, thanks for digging into this, I think the underlying bug is real, but I don't think this patch fully fixes it yet.

For Matrix, sender.PlatformID and sender.Username are both set to the full MXID, e.g. @alice:example.com. In MatchAllowed, the fallback path strips the leading @ before doing the legacy checks, so after the canonical match fails it still compares alice:example.com against @alice:example.com, which will not match.

So unless I'm missing something, allow_from: ["@alice:example.com"] still looks broken even with this fallthrough change.

I think we need one of these instead:

  • only treat platform:id as canonical when platform is a recognized platform name
  • or preserve the leading @ for Matrix-style IDs in the fallback path

Could you also add a regression test for the Matrix case in pkg/identity/identity_test.go? The current tests stay green because this scenario is not covered.

ParseCanonicalID splits on first colon, misinterpreting @alice:example.com as platform:id. Two fixes:

1. When canonical match fails, fall through to legacy match strategies instead of returning false.

2. In the legacy path, preserve the leading @ for entries containing a colon, since the @ is part of a Matrix-style user ID, not a username prefix.

Add regression tests for Matrix ID matching via both PlatformID and canonical format. Fixes sipeed#3044.
@chengzhichao-xydt chengzhichao-xydt force-pushed the codex/allow-from-matrix-colon branch from e932e81 to 1ab06b5 Compare June 10, 2026 06:05
@afjcjsbx

Copy link
Copy Markdown
Collaborator

pkg/identity/identity.go The new fallthrough overly broadens the match for canonical platform:id values. Previously, if allowed == "discord:98765432" and the canonical match failed, the function returned false. With this PR, however, it continues into the legacy checks and could authorize a sender from another platform if their PlatformID matches the entire canonical string. Concrete example: a sender with Platform="matrix" and PlatformID="discord:98765432" now matches allow_from: ["discord:98765432"]. Since allow_from is an authorization check, this is a semantic regression: the fallthrough should be restricted solely to the case of raw MXIDs like alice:example.com or to explicitly recognized platform names.

pkg/identity/identity_test.go One of the new tests does not exercise the bug fixed by the PR. The case allowed: "matrix:@alice:example.com" was already the documented workaround and worked previously as well, so it doesn't guard the control flow change introduced here. What is missing, instead, is a test to prevent the regression mentioned above: a canonical mismatch must not fall back to legacy matches simply because sender.PlatformID contains the platform:id string.

…tching

When a canonical "platform:id" entry fails to match, return false
immediately instead of falling through to legacy PlatformID matching.
Previously, an allow-list entry like "discord:98765432" that didn't
match the canonical path could be satisfied by a sender with
Platform=matrix and PlatformID=discord:98765432 — an unintended
authorization widening.

Only Matrix-style IDs ("@user:domain") and numeric compound IDs now
fall through to legacy matching, since their colon is part of the ID,
not a canonical separator.

Adds regression test verifying the security boundary.
@chengzhichao-xydt

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. Addressed in dfc66d0:

  1. Fallthrough scope — When a canonical platform:id entry is recognized as such (platform is not numeric and doesn't start with @) and fails to match, we now return false immediately. Only Matrix-style IDs (@user:domain) and numeric compound IDs fall through to legacy matching. This prevents the authorization widening scenario you described.

  2. Regression test — Added TestMatchAllowed/canonical_mismatch_does_not_fallthrough_to_PlatformID: a sender with Platform=matrix and PlatformID=discord:98765432 does NOT match allow_from: ["discord:98765432"].

@github-actions

Copy link
Copy Markdown

This PR has had no activity for 7 days and has been marked as stale. If you are still working on it, please push an update or leave a comment; otherwise it will be closed automatically in 7 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: allow_from fails for Matrix user IDs containing colon

2 participants