From 135237f0d485b02631eee4b3566d0288d28f4699 Mon Sep 17 00:00:00 2001 From: Narasimha-sc <166327228+Narasimha-sc@users.noreply.github.com> Date: Mon, 6 Jul 2026 19:31:22 +0000 Subject: [PATCH] core: don't re-approve directory channels on stale publicGroupId updateGroupFromLinkData reported profileChanged via a full structural (/=) that includes publicGroupId - an immutable identity field updateGroupProfile never re-syncs. A directory whose stored public_group_id drifts from the link's therefore re-approved the channel on every link check (~30 min). Fix: normalize publicGroupId out of the comparison (mirrors sameGroupProfileInfo), so only fields updateGroupProfile actually manages drive the change signal. The (pg :: PublicGroupProfile) annotation is required (DuplicateRecordFields). Adds a directory regression test: register+approve a channel, corrupt the stored public_group_id via /x /sql, run a link check, assert the channel stays listed (no re-approval). Builds verified: simplex-directory-service and simplex-chat-test both compile. --- ...026-07-06-fix-directory-name-reapproval.md | 30 +++++--- src/Simplex/Chat/Library/Internal.hs | 8 ++- tests/Bots/DirectoryTests.hs | 70 +++++++++++++++++++ 3 files changed, 96 insertions(+), 12 deletions(-) diff --git a/plans/2026-07-06-fix-directory-name-reapproval.md b/plans/2026-07-06-fix-directory-name-reapproval.md index 024ef741c0..ec6acda761 100644 --- a/plans/2026-07-06-fix-directory-name-reapproval.md +++ b/plans/2026-07-06-fix-directory-name-reapproval.md @@ -1,6 +1,6 @@ # Fix: SimpleX directory re-approves a channel every 30 min after a blockchain name is added -Status: proposed — **root cause confirmed by code (deduction, §3); fix in core (§4)** +Status: implemented & verified — **root cause confirmed by code (deduction, §3); fix in core (§4); regression test passes and fails without the fix (§8)** Branch: `nd/fix-directory-names` Base: `origin/master` (`b38015c7b`) @@ -107,7 +107,7 @@ sites — the guard, the `if … then updateGroupProfile`, and the returned flag -- Internal.hs:2986, which normalizes groupPreferences the same way). profileChanged = clearId p /= clearId groupProfile clearId gp@GroupProfile {publicGroup} = - gp {publicGroup = (\pg -> pg {publicGroupId = B64UrlByteString ""}) <$> publicGroup} + gp {publicGroup = (\pg -> (pg :: PublicGroupProfile) {publicGroupId = B64UrlByteString ""}) <$> publicGroup} ``` The `| profileChanged || countChanged || verifyChanged` guard, the @@ -207,16 +207,24 @@ not a directory policy, so it belongs where the comparison lives: `profileChanged` true and stores). `Commands.hs:4358` is the directory path this fix targets. -## 8. Verification plan +## 8. Verification (done) -1. **Regression test** in `tests/Bots/DirectoryTests.hs` (beside the existing - "link/whitespace-only must not re-approve" tests, `:296-326`): register a channel - with one `public_group_id`, then make the fetched link carry a *different* - `publicGroupId` (all other fields equal), run ≥2 link checks, and assert the reg - stays `GRSActive` (no `GRSPendingApproval`, no owner re-approval message). Add a - positive control: a display-name change *does* re-approve. -2. Apply §4. -3. Build the directory service (statically links core); run the directory suite. +Regression test `testLinkCheckStalePublicGroupId` in `tests/Bots/DirectoryTests.hs` +(beside the existing "link/whitespace-only must not re-approve" tests): register + +approve a channel, then corrupt the directory's *stored* `public_group_id` via +`/x /sql chat UPDATE group_profiles SET public_group_id = randomblob(32) …` (so it +differs from the link's; the value stays non-NULL, so `publicGroup` remains `Just` +and the link check still runs), let a link check run (`linkCheckInterval = 1`), and +assert the channel stays listed with no re-approval. + +Results: +- `cabal build simplex-directory-service` — links (the fix compiles into the bot). +- `cabal build simplex-chat-test` — the test compiles. +- Test **passes** with the fix (1 example, 0 failures). +- **Negative control** (fix reverted, test kept): the test **fails** with + `but got: Just "…The channel ID 1 (news) profile changed."` — the exact production + re-approval message. So the test genuinely guards the bug, and the root cause is + demonstrated end-to-end (not just deduced). ## 9. Files touched diff --git a/src/Simplex/Chat/Library/Internal.hs b/src/Simplex/Chat/Library/Internal.hs index c6c7251fc4..cc106ff4d4 100644 --- a/src/Simplex/Chat/Library/Internal.hs +++ b/src/Simplex/Chat/Library/Internal.hs @@ -1479,7 +1479,13 @@ updateGroupFromLinkData user gInfo@GroupInfo {groupProfile = p, groupDomainVerif pure (g'', profileChanged) | otherwise = pure (gInfo, False) where - profileChanged = p /= groupProfile + -- publicGroupId is immutable identity, not content, and is not a column + -- updateGroupProfile syncs (it is set at group creation / by updateRelayGroupKeys). + -- A stale stored value would otherwise make this (/=) re-trigger directory + -- approval on every link check. Normalize it out (mirrors sameGroupProfileInfo). + profileChanged = clearId p /= clearId groupProfile + clearId gp@GroupProfile {publicGroup} = + gp {publicGroup = (\pg -> (pg :: PublicGroupProfile) {publicGroupId = B64UrlByteString ""}) <$> publicGroup} countChanged = case publicGroupData of Just PublicGroupData {publicMemberCount} -> Just publicMemberCount /= localCount _ -> False diff --git a/tests/Bots/DirectoryTests.hs b/tests/Bots/DirectoryTests.hs index 025bfe4f66..074641e2ae 100644 --- a/tests/Bots/DirectoryTests.hs +++ b/tests/Bots/DirectoryTests.hs @@ -99,6 +99,7 @@ directoryServiceTests = do it "should delete channel registration and leave" testDeleteChannelRegistration it "should handle re-registration when already listed" testReregistrationAlreadyListed it "should update subscriber count periodically" testLinkCheckUpdatesCount + it "should not re-approve when only stored publicGroupId is stale" testLinkCheckStalePublicGroupId directoryProfile :: Profile directoryProfile = Profile {displayName = "SimpleX Directory", fullName = "", shortDescr = Nothing, image = Nothing, contactLink = Nothing, peerType = Just CPTBot, preferences = Nothing, badge = Nothing, contactDomain = Nothing} @@ -2307,6 +2308,75 @@ testLinkCheckUpdatesCount ps = do bob <## "You need SimpleX Chat app v6.5 to join." bob <## "3 subscribers" +-- Regression: a stale stored publicGroupId (which updateGroupProfile never re-syncs) +-- differs from the link's on every link check. Before the fix this re-approved the +-- channel every linkCheckInterval; after it, a publicGroupId-only difference is +-- ignored and the channel stays listed. (See plans/2026-07-06-fix-directory-name-reapproval.md) +testLinkCheckStalePublicGroupId :: HasCallStack => TestParams -> IO () +testLinkCheckStalePublicGroupId ps = do + dsLink <- + withNewTestChatCfg ps testCfg serviceDbPrefix directoryProfile $ \ds -> + withNewTestChatCfg ps testCfg "super_user" aliceProfile $ \superUser -> do + connectUsers ds superUser + ds ##> "/ad" + getContactLink ds True + let opts = (mkDirectoryOpts ps [KnownContact 2 "alice"] Nothing Nothing) {linkCheckInterval = 1} + runDirectory testCfg opts $ + withTestChatCfg ps testCfg "super_user" $ \superUser -> do + superUser <## "subscribed 1 connections on server localhost" + withNewTestChatCfg ps testCfg "bob" bobProfile $ \bob -> + withRelay ps $ \relay -> do + bob `connectVia` dsLink + _ <- prepareChannel1Relay "news" bob relay + -- register and approve + bob ##> "/share chat #news @'SimpleX Directory'" + bob <# "@'SimpleX Directory' link to join channel #news (signed):" + _ <- getTermLine bob -- short link + _ <- getTermLine bob -- ownerSig JSON + bob <# "'SimpleX Directory'> Joining the channel news…" + concurrentlyN_ + [ do + relay <## "'SimpleX Directory': accepting request to join group #news..." + relay <## "#news: 'SimpleX Directory' joined the group", + bob <## "#news: relay introduced 'SimpleX Directory_1' in the channel" + ] + bob <# "'SimpleX Directory'> Joined the channel news. Registration is pending approval — it may take up to 48 hours." + bob <# "'SimpleX Directory'> We recommend allowing direct messages, media, voice, and SimpleX links only for group moderators and admins. Use group preferences to set them." + bob <## "Captcha verification is enabled. Use /'filter 1' to change it." + superUser <# "'SimpleX Directory'> bob submitted the channel ID 1:" + superUser <## "news" + superUser <##. "Link to join channel: " + superUser <## "You need SimpleX Chat app v6.5 to join." + superUser <## "1 subscribers" + superUser <## "" + superUser <## "To approve send:" + superUser <# "'SimpleX Directory'> /approve 1:news 1" + let approve = "/approve 1:news 1" + superUser #> ("@'SimpleX Directory' " <> approve) + superUser <# ("'SimpleX Directory'> > " <> approve) + superUser <## " Channel approved!" + bob <# "'SimpleX Directory'> The channel ID 1 (news) is approved and listed in directory - please moderate it!" + bob <## "Please note: if you change the channel profile it will be hidden from directory until it is re-approved." + -- Corrupt the directory's STORED public_group_id so it differs from the link's + -- (simulates the stale identity that updateGroupProfile never re-syncs). Value + -- stays non-NULL, so publicGroup remains Just and the link check still runs. + let sql = "/x /sql chat UPDATE group_profiles SET public_group_id = randomblob(32) WHERE public_group_id IS NOT NULL" + superUser #> ("@'SimpleX Directory' " <> sql) + superUser <# ("'SimpleX Directory'> > " <> sql) + superUser <## "" + -- allow >= 1 link check (interval = 1s) to run + threadDelay 1500000 + -- channel is still listed and active (no re-approval); had it re-approved, + -- bob's search would first receive the "profile has changed" message and this + -- sequence would fail. + bob #> "@'SimpleX Directory' news" + bob <# "'SimpleX Directory'> > news" + bob <## " Found 1 group(s)." + bob <# "'SimpleX Directory'> news" + bob <##. "Link to join channel: " + bob <## "You need SimpleX Chat app v6.5 to join." + bob <## "2 subscribers" + testGetCaptchaStr :: HasCallStack => TestParams -> IO () testGetCaptchaStr _ps = do s0 <- getCaptchaStr 0 ""