Follow-up issues for PR #105 #112

New Issue

jabber.developer2 · 2026-04-28T14:29:10Z

jabber.developer2 commented

2026-04-28 14:29:10 +00:00

Follow-up issues for PR #105

Issue templates ready to paste into Gitea after PR #105 lands. Each
section is a standalone issue. Headings are suggested titles.

All items below were surfaced during the code review of the upstream
merge in PR #105.

1. Verify XEP-0359 `by` attribute via disco before trusting `stanza-id`

Labels: security · xmpp
Severity: medium (dedup poisoning)

Problem

src/xmpp/message.c:1090 and :1399 accept the by attribute on
<stanza-id> after only checking that the value matches the expected
JID (MUC bare-jid for groupchat, own bare-jid for chat). Per
XEP-0359 §7 Business Rules
and §8 Security Considerations:

Before processing the stanza ID of a message and using it for
deduplication purposes or for MAM catchup, the receiving entity
SHOULD ensure that the stanza ID could not have been faked, by
verifying that the entity referenced in the by attribute does
announce the urn:xmpp:sid:0 namespace in its disco features.

CProof currently skips that disco check. Combined with the new UNIQUE
constraint on archive_id (introduced in 64fcdfaa), this turns a
non-XEP-0359 server into a dedup poisoner: replay any earlier
stanza-id for a different message and the second message is
silently dropped as a duplicate.

Acceptance criteria

Cache disco#info results per JID with a reasonable TTL.
Before accepting <stanza-id by=…> for dedup, look up the
cached features for by and require urn:xmpp:sid:0.
If features are unknown, either delay the dedup decision until
disco completes, or fall back to not deduping that message.
Add a unit / functional test that injects a stanza-id from an
entity not announcing the namespace and asserts it does not
poison the archive.

Out of scope

Reworking dedup itself, MAM catchup pipeline. Scope is limited to the
by-trust gate.

2. Audit existing on-disk account names against the new validator

Labels: compatibility · accounts
Severity: low (UX regression risk)

Problem

src/config/accounts.c:37 introduces a stricter validator on account
names. Existing user installations may already contain account files
with names that the new validator would reject — meaning a working
configuration can become unloadable after upgrade.

Acceptance criteria

Snapshot the set of names produced by all historical CProof
versions (and inherited from upstream Profanity) — gather from
git history of the validator and from any user reports.
Decide one of:
- the validator stays strict and we ship a one-shot rename pass
  on first launch with a clear log message;
- the validator is split into "accept on read, warn on create"
  (lenient inbound, strict outbound).
Document whichever path is picked in CHANGELOG.

3. OTR interop matrix before next release

Labels: otr · testing
Severity: low (nothing observed broken yet)

Problem

src/otr/otr.c:274 carries a TODO: test with other popular messenger to ensure that this logic works correctly. Until that pass is run we
don't know whether the recent OTR changes interoperate with mainstream
clients.

Acceptance criteria

Run an OTR session against at least three of: Gajim, Dino,
Conversations (with OTR plugin), Pidgin.
Cover: handshake, encrypted message, fingerprint verification,
session re-keying, end-of-session.
Capture findings in this issue. If any client diverges, file a
sub-issue per failure mode.

4. Clarify the "similar method" hint in `src/ui/window.c`

Labels: question · refactor

Problem

The PR #105 review noted that _win_ensure_pad_capacity
(window.c, line ~47) overlaps with another method, but the comment
anchor sits on its closing }. The two pad-related candidates we
found — _win_ensure_pad_capacity (grow on append) and win_resize
(handle terminal resize) — have different concerns.

Acceptance criteria

Identify the specific other site that was meant, or close the
issue if the original concern is already covered by
_win_ensure_pad_capacity alone.

5. Re-introduce stricter warnings + sanitizer build, Pikaur-friendly

Labels: build · ci

Background

PR #105 originally landed a build commit (5459e78e8) that:

dropped -Wno-sign-compare and added -Wsign-compare +
-Wconversion to the GCC-specific warning probe loop;
introduced --enable-sanitizers (ASan + UBSan +
-fsanitize=unsigned-integer-overflow) as an opt-in flag;
added escape-hatches under PACKAGE_STATUS=development so the
new sign-warnings did not turn into hard errors immediately
(-Wno-error=sign-conversion, -Wno-error=sign-compare);
routed glib_CFLAGS / gio_CFLAGS through -isystem so macros
like GPOINTER_TO_UINT in glibconfig.h did not generate noise
under -Wconversion.

That commit was reverted (a7ad9ac0a) in favour of master's
0722dc9e3 build(pikaur): Fix failure due to duplicated flag and warnings, which goes the opposite direction — keeps
-Wno-sign-compare, drops -Wnull-dereference and
-D_FORTIFY_SOURCE=2. The two directions cannot trivially coexist:
the warnings PR #105 wants to enable are exactly the ones the Pikaur
fix had to silence to build through.

TODO

Reproduce the original Pikaur failure that motivated
0722dc9e3 (which warning fired, what flag duplication
happened, on which Arch + Pikaur version).
Pick a path that lands the warnings without breaking Pikaur:
- dedupe flags via AX_APPEND_FLAG so a duplicate -Wno-X from
  the makepkg side cannot cancel ours;
- gate hardening flags (-Wnull-dereference, -D_FORTIFY_SOURCE=2)
  behind --enable-hardening or PACKAGE_STATUS=development so
  they only show up where we control the toolchain;
- or scope -Wsign-conversion/-Wconversion per-file via
  #pragma GCC diagnostic until callers are clean.
Re-add --enable-sanitizers as a separate, opt-in option
(it does not conflict with Pikaur and is worth on its own).
Re-route glib_CFLAGS / gio_CFLAGS through -isystem
regardless of the rest — that cleanup is independent.
Restore the development-only -Wno-error=sign-* escape-hatches
once the warnings are enabled again, otherwise -Werror will
hard-fail every dev build.
Document the chosen knobs in CONTRIBUTING.md.

What's already merged

The narrowing-and-conversion code cleanups that 5459e78e8 enabled
remain in tree (commit 9feff00ea); they don't depend on the
warnings being on, they just made the codebase cleaner. So when
the warnings are re-enabled, the flood from the source tree
itself should be much smaller than the original ~327 hits.

6. Clarify the "cleanup later" hint in `src/ui/inputwin.c`

Labels: question · refactor

Problem

The PR #105 review left a "we should also clean it up its definition
later" note anchored on line 1017, which is the closing } of
_inp_rl_win_pagedown_handler. The intended target (which symbol /
prototype / struct field) is not obvious from the anchor alone.

Acceptance criteria

Identify the definition meant.
Open a concrete cleanup issue once the target is known, then
close this one.

7. `_migrate_to_v3` dedupe is non-deterministic and can break LMC chains

Labels: database · correctness
Severity: medium (silent data loss for corrected messages)

Shall be closed by #128

Problem

src/database_sqlite.c _migrate_to_v3 deduplicates rows by

INSERT INTO ChatLogs_v3_migration
SELECT * FROM ChatLogs WHERE archive_id IS NULL
UNION ALL
SELECT * FROM (SELECT * FROM ChatLogs WHERE archive_id IS NOT NULL
               GROUP BY archive_id)

SQLite extension semantics let bare columns appear with GROUP BY — but
the kept row per group is implementation-defined (effectively
arbitrary). When the duplicate set was produced by MAM rebroadcast of a
message that was later corrected via XEP-0308 LMC, the kept row's
replaces_db_id / replaced_by_db_id may point at rows that get
dropped, leaving dangling local FKs in the chain.

Real-world incidence is low (MAM rebroadcast of the same archive_id is
rare on well-behaved servers), but the migration runs once per upgrade
and is irreversible without a backup.

Acceptance criteria

Pick a deterministic row per archive_id group — MAX(id) is the
simplest "keep the latest insert" rule.
After dedupe, sweep replaces_db_id / replaced_by_db_id and
NULL out references to rows that no longer exist (or remap them
to the survivor).
Add a migration unit test that seeds a v2 schema with deliberate
archive_id duplicates including an LMC chain spanning the
duplicates, runs the migration, and asserts no dangling FKs.

Out of scope

Changing the dedup strategy for live inserts (already handled by the
explicit duplicate_check_query and the new archive_id UNIQUE).

8. OTR whitespace-tag stripping uses wrong `memmove` length

Labels: otr · ui · pre-existing
Severity: low (cosmetic, narrow trigger)

Problem

src/otr/otr.c otr_on_message_recv strips the OTR whitespace tag
from an incoming message with

memmove(whitespace_base, whitespace_base + tag_length, tag_length);

The third argument is the tag length, not the length of the message
tail that needs to shift left. Result: the tag area is overwritten with
the first tag_length bytes of the rest of the message, but the rest
itself is not shifted — the user sees <first 24 bytes of body><full body> duplication whenever the opportunistic/always OTR policy
triggers a whitespace-tag handoff.

Pre-existing in origin/master — PR #105 only tightened the
strstr→strncmp precondition next to it, which is why we noticed.

Acceptance criteria

Fix the length: memmove(whitespace_base, whitespace_base + tag_length, strlen(whitespace_base + tag_length) + 1).
Add a unit test for otr_on_message_recv that feeds a message
with a synthetic whitespace tag plus a known body and asserts the
body is preserved verbatim after stripping.

9. `resource` pointer is read after ownership-transfer in presence handler

Labels: xmpp · use-after-free risk · pre-existing
Severity: low (no current UAF, fragile pattern)

Problem

In src/xmpp/presence.c (own-jid branch, around line 624):

connection_add_available_resource(resource);          // hash_table_replace — takes ownership
Resource* resource_for_roster = resource_copy(resource);
…
g_strcmp0(cur_resource, resource->name)               // still deref
resource->name, resource->priority, resource->presence, resource->status

Safe in the current control flow because nothing else does a
g_hash_table_replace with the same key (resource->name) between
the ownership transfer and the last read. Any future change that
adds such a path silently introduces a use-after-free.

Pre-existing pattern, not introduced by PR #105.

Acceptance criteria

Capture the needed values into local variables before
connection_add_available_resource(resource), then deref the
locals afterwards.
Or, document the lifetime invariant inline next to the call
and add a brief assertion (e.g., a g_hash_table_lookup cross-
check) that catches the bad case in debug builds.

10. `omemo_on_connect` discards `_omemo_finalize_identity_load` return

Labels: omemo · error-handling
Severity: low (no user-visible failure observed)

Shall be closed by #128

Problem

src/omemo/omemo.c:263 calls _omemo_finalize_identity_load(account)
and ignores the boolean return. The function returns FALSE only on
files_file_in_account_data_path failure (out-of-disk / permission
denied) — in which case omemo_ctx.loaded is left FALSE and the
caller proceeds as if init succeeded. Subsequent OMEMO operations
hit the omemo_ctx.loaded guards and silently no-op, so the user
sees encryption "not available" with no log line pointing at the
storage failure.

Pre-existing pattern, surfaced during PR #105 review.

Acceptance criteria

Propagate the return; if FALSE, log a user-visible message
(cons_show_error) explaining that OMEMO state could not be
loaded from disk.
Optionally bail out of omemo_on_connect early so we don't
leave the partially-initialised omemo_ctx.signal /
omemo_ctx.store lying around.

11. `_migrate_to_v3`: `INSERT … SELECT *` depends on positional column match

Labels: database · tech-debt
Severity: low

Problem

src/database_sqlite.c _migrate_to_v3 copies rows with

INSERT INTO ChatLogs_v3_migration SELECT * FROM ChatLogs …

This is safe today because the v2 schema has exactly the same column
list / order as the new v3 table. Any out-of-band schema patch between
v2 and v3 (e.g., a side branch that adds a column to ChatLogs) would
silently mis-bind columns at migration time.

Acceptance criteria

Replace SELECT * with an explicit column list matching the v3
destination table. Same change for the WHERE archive_id IS NULL and GROUP BY archive_id branches.
Add a comment near the schema definition noting that schema
changes need a matching update to the migration column list.

# Follow-up issues for PR #105 Issue templates ready to paste into Gitea after PR #105 lands. Each section is a standalone issue. Headings are suggested titles. All items below were surfaced during the code review of the upstream merge in PR #105. --- # 1. Verify XEP-0359 `by` attribute via disco before trusting `stanza-id` **Labels:** security · xmpp **Severity:** medium (dedup poisoning) ### Problem `src/xmpp/message.c:1090` and `:1399` accept the `by` attribute on `<stanza-id>` after only checking that the value matches the expected JID (MUC bare-jid for groupchat, own bare-jid for chat). Per [XEP-0359 §7 Business Rules](https://xmpp.org/extensions/xep-0359.html#business-rules) and §8 Security Considerations: > Before processing the stanza ID of a message and using it for > deduplication purposes or for MAM catchup, the receiving entity > SHOULD ensure that the stanza ID could not have been faked, by > verifying that the entity referenced in the `by` attribute does > announce the `urn:xmpp:sid:0` namespace in its disco features. CProof currently skips that disco check. Combined with the new UNIQUE constraint on `archive_id` (introduced in `64fcdfaa`), this turns a non-XEP-0359 server into a dedup poisoner: replay any earlier `stanza-id` for a different message and the second message is silently dropped as a duplicate. ### Acceptance criteria - [ ] Cache disco#info results per JID with a reasonable TTL. - [ ] Before accepting `<stanza-id by=…>` for dedup, look up the cached features for `by` and require `urn:xmpp:sid:0`. - [ ] If features are unknown, either delay the dedup decision until disco completes, or fall back to not deduping that message. - [ ] Add a unit / functional test that injects a `stanza-id` from an entity not announcing the namespace and asserts it does **not** poison the archive. ### Out of scope Reworking dedup itself, MAM catchup pipeline. Scope is limited to the `by`-trust gate. --- ## 2. Audit existing on-disk account names against the new validator **Labels:** compatibility · accounts **Severity:** low (UX regression risk) ### Problem `src/config/accounts.c:37` introduces a stricter validator on account names. Existing user installations may already contain account files with names that the new validator would reject — meaning a working configuration can become unloadable after upgrade. ### Acceptance criteria - [ ] Snapshot the set of names produced by all historical CProof versions (and inherited from upstream Profanity) — gather from git history of the validator and from any user reports. - [ ] Decide one of: - the validator stays strict and we ship a one-shot rename pass on first launch with a clear log message; - the validator is split into "accept on read, warn on create" (lenient inbound, strict outbound). - [ ] Document whichever path is picked in CHANGELOG. --- ## 3. OTR interop matrix before next release **Labels:** otr · testing **Severity:** low (nothing observed broken yet) ### Problem `src/otr/otr.c:274` carries a `TODO: test with other popular messenger to ensure that this logic works correctly`. Until that pass is run we don't know whether the recent OTR changes interoperate with mainstream clients. ### Acceptance criteria - [ ] Run an OTR session against at least three of: Gajim, Dino, Conversations (with OTR plugin), Pidgin. - [ ] Cover: handshake, encrypted message, fingerprint verification, session re-keying, end-of-session. - [ ] Capture findings in this issue. If any client diverges, file a sub-issue per failure mode. --- ## 4. Clarify the "similar method" hint in `src/ui/window.c` **Labels:** question · refactor ### Problem The PR #105 review noted that `_win_ensure_pad_capacity` (window.c, line ~47) overlaps with another method, but the comment anchor sits on its closing `}`. The two pad-related candidates we found — `_win_ensure_pad_capacity` (grow on append) and `win_resize` (handle terminal resize) — have different concerns. ### Acceptance criteria - [ ] Identify the specific other site that was meant, or close the issue if the original concern is already covered by `_win_ensure_pad_capacity` alone. --- ## 5. Re-introduce stricter warnings + sanitizer build, Pikaur-friendly **Labels:** build · ci ### Background PR #105 originally landed a build commit (`5459e78e8`) that: 1. dropped `-Wno-sign-compare` and added `-Wsign-compare` + `-Wconversion` to the GCC-specific warning probe loop; 2. introduced `--enable-sanitizers` (ASan + UBSan + `-fsanitize=unsigned-integer-overflow`) as an opt-in flag; 3. added escape-hatches under `PACKAGE_STATUS=development` so the new sign-warnings did not turn into hard errors immediately (`-Wno-error=sign-conversion`, `-Wno-error=sign-compare`); 4. routed `glib_CFLAGS` / `gio_CFLAGS` through `-isystem` so macros like `GPOINTER_TO_UINT` in `glibconfig.h` did not generate noise under `-Wconversion`. That commit was reverted (`a7ad9ac0a`) in favour of master's `0722dc9e3 build(pikaur): Fix failure due to duplicated flag and warnings`, which goes the opposite direction — keeps `-Wno-sign-compare`, drops `-Wnull-dereference` and `-D_FORTIFY_SOURCE=2`. The two directions cannot trivially coexist: the warnings PR #105 wants to enable are exactly the ones the Pikaur fix had to silence to build through. ### TODO - [ ] Reproduce the original Pikaur failure that motivated `0722dc9e3` (which warning fired, what flag duplication happened, on which Arch + Pikaur version). - [ ] Pick a path that lands the warnings without breaking Pikaur: - dedupe flags via `AX_APPEND_FLAG` so a duplicate `-Wno-X` from the makepkg side cannot cancel ours; - gate hardening flags (`-Wnull-dereference`, `-D_FORTIFY_SOURCE=2`) behind `--enable-hardening` or `PACKAGE_STATUS=development` so they only show up where we control the toolchain; - or scope `-Wsign-conversion`/`-Wconversion` per-file via `#pragma GCC diagnostic` until callers are clean. - [ ] Re-add `--enable-sanitizers` as a separate, opt-in option (it does not conflict with Pikaur and is worth on its own). - [ ] Re-route `glib_CFLAGS` / `gio_CFLAGS` through `-isystem` regardless of the rest — that cleanup is independent. - [ ] Restore the development-only `-Wno-error=sign-*` escape-hatches once the warnings are enabled again, otherwise `-Werror` will hard-fail every dev build. - [ ] Document the chosen knobs in `CONTRIBUTING.md`. ### What's already merged - The narrowing-and-conversion code cleanups that 5459e78e8 enabled remain in tree (commit `9feff00ea`); they don't depend on the warnings being on, they just made the codebase cleaner. So when the warnings are re-enabled, the flood from the source tree itself should be much smaller than the original ~327 hits. --- ## 6. Clarify the "cleanup later" hint in `src/ui/inputwin.c` **Labels:** question · refactor ### Problem The PR #105 review left a "we should also clean it up its definition later" note anchored on line 1017, which is the closing `}` of `_inp_rl_win_pagedown_handler`. The intended target (which symbol / prototype / struct field) is not obvious from the anchor alone. ### Acceptance criteria - [ ] Identify the definition meant. - [ ] Open a concrete cleanup issue once the target is known, then close this one. --- ## 7. `_migrate_to_v3` dedupe is non-deterministic and can break LMC chains **Labels:** database · correctness **Severity:** medium (silent data loss for corrected messages) Shall be closed by #128 ### Problem `src/database_sqlite.c` `_migrate_to_v3` deduplicates rows by ```sql INSERT INTO ChatLogs_v3_migration SELECT * FROM ChatLogs WHERE archive_id IS NULL UNION ALL SELECT * FROM (SELECT * FROM ChatLogs WHERE archive_id IS NOT NULL GROUP BY archive_id) ``` SQLite extension semantics let bare columns appear with `GROUP BY` — but the kept row per group is *implementation-defined* (effectively arbitrary). When the duplicate set was produced by MAM rebroadcast of a message that was later corrected via XEP-0308 LMC, the kept row's `replaces_db_id` / `replaced_by_db_id` may point at rows that get dropped, leaving dangling local FKs in the chain. Real-world incidence is low (MAM rebroadcast of the same `archive_id` is rare on well-behaved servers), but the migration runs once per upgrade and is irreversible without a backup. ### Acceptance criteria - [ ] Pick a deterministic row per `archive_id` group — `MAX(id)` is the simplest "keep the latest insert" rule. - [ ] After dedupe, sweep `replaces_db_id` / `replaced_by_db_id` and NULL out references to rows that no longer exist (or remap them to the survivor). - [ ] Add a migration unit test that seeds a v2 schema with deliberate `archive_id` duplicates including an LMC chain spanning the duplicates, runs the migration, and asserts no dangling FKs. ### Out of scope Changing the dedup strategy for live inserts (already handled by the explicit `duplicate_check_query` and the new `archive_id UNIQUE`). --- ## 8. OTR whitespace-tag stripping uses wrong `memmove` length **Labels:** otr · ui · pre-existing **Severity:** low (cosmetic, narrow trigger) ### Problem `src/otr/otr.c` `otr_on_message_recv` strips the OTR whitespace tag from an incoming message with ```c memmove(whitespace_base, whitespace_base + tag_length, tag_length); ``` The third argument is the *tag* length, not the length of the message tail that needs to shift left. Result: the tag area is overwritten with the first `tag_length` bytes of the rest of the message, but the rest itself is not shifted — the user sees `<first 24 bytes of body><full body>` duplication whenever the opportunistic/always OTR policy triggers a whitespace-tag handoff. Pre-existing in `origin/master` — PR #105 only tightened the `strstr`→`strncmp` precondition next to it, which is why we noticed. ### Acceptance criteria - [ ] Fix the length: `memmove(whitespace_base, whitespace_base + tag_length, strlen(whitespace_base + tag_length) + 1)`. - [ ] Add a unit test for `otr_on_message_recv` that feeds a message with a synthetic whitespace tag plus a known body and asserts the body is preserved verbatim after stripping. --- ## 9. `resource` pointer is read after ownership-transfer in presence handler **Labels:** xmpp · use-after-free risk · pre-existing **Severity:** low (no current UAF, fragile pattern) ### Problem In `src/xmpp/presence.c` (own-jid branch, around line 624): ```c connection_add_available_resource(resource); // hash_table_replace — takes ownership Resource* resource_for_roster = resource_copy(resource); … g_strcmp0(cur_resource, resource->name) // still deref resource->name, resource->priority, resource->presence, resource->status ``` Safe in the current control flow because nothing else does a `g_hash_table_replace` with the same key (`resource->name`) between the ownership transfer and the last read. Any future change that adds such a path silently introduces a use-after-free. Pre-existing pattern, not introduced by PR #105. ### Acceptance criteria - [ ] Capture the needed values into local variables *before* `connection_add_available_resource(resource)`, then deref the locals afterwards. - [ ] Or, document the lifetime invariant inline next to the call and add a brief assertion (e.g., a `g_hash_table_lookup` cross- check) that catches the bad case in debug builds. --- ## 10. `omemo_on_connect` discards `_omemo_finalize_identity_load` return **Labels:** omemo · error-handling **Severity:** low (no user-visible failure observed) Shall be closed by #128 ### Problem `src/omemo/omemo.c:263` calls `_omemo_finalize_identity_load(account)` and ignores the boolean return. The function returns `FALSE` only on `files_file_in_account_data_path` failure (out-of-disk / permission denied) — in which case `omemo_ctx.loaded` is left `FALSE` and the caller proceeds as if init succeeded. Subsequent OMEMO operations hit the `omemo_ctx.loaded` guards and silently no-op, so the user sees encryption "not available" with no log line pointing at the storage failure. Pre-existing pattern, surfaced during PR #105 review. ### Acceptance criteria - [ ] Propagate the return; if `FALSE`, log a user-visible message (`cons_show_error`) explaining that OMEMO state could not be loaded from disk. - [ ] Optionally bail out of `omemo_on_connect` early so we don't leave the partially-initialised `omemo_ctx.signal` / `omemo_ctx.store` lying around. --- ## 11. `_migrate_to_v3`: `INSERT … SELECT *` depends on positional column match **Labels:** database · tech-debt **Severity:** low ### Problem `src/database_sqlite.c` `_migrate_to_v3` copies rows with ```sql INSERT INTO ChatLogs_v3_migration SELECT * FROM ChatLogs … ``` This is safe today because the v2 schema has exactly the same column list / order as the new v3 table. Any out-of-band schema patch between v2 and v3 (e.g., a side branch that adds a column to ChatLogs) would silently mis-bind columns at migration time. ### Acceptance criteria - [ ] Replace `SELECT *` with an explicit column list matching the v3 destination table. Same change for the `WHERE archive_id IS NULL` and `GROUP BY archive_id` branches. - [ ] Add a comment near the schema definition noting that schema changes need a matching update to the migration column list.

jabber.developer2 referenced this issue

2026-04-28 14:37:43 +00:00

merge/upstream-full #105

jabber.developer2 referenced this issue

2026-04-28 14:39:13 +00:00

merge/upstream-full #105

jabber.developer2 referenced this issue

2026-04-28 14:45:58 +00:00

merge/upstream-full #105

jabber.developer2 referenced this issue

2026-04-28 14:46:54 +00:00

merge/upstream-full #105