Fix duplicate sticky events for encrypted events without a sticky key

[AurimasG12]

Fixes #5205

Changes

• _unstable_addStickyEvents in room.ts is now async and runs each sticky through decryptStickyEvent before calling the store, so encrypted stickies are decrypted (and get their msc4354_sticky_key) before insertion. RoomStickyEventsStore rejects unkeyed m.room.encrypted events so ciphertext cannot sit in unkeyedStickyEvents. sync.ts and embedded.ts now await that async add path.
• Tests were added in room-sticky-events.spec.ts and room.spec.ts for the store guard and the decrypt-then-add flow.

Cause

Encrypted sticky events arrive as m.room.encrypted with no msc4354_sticky_key in the ciphertext, so the SDK treated them as unkeyed stickies and stored them immediately. After decryption, the same logical event was added again (now with a sticky key or as a plain event), so the sticky store held two entries for one sticky.

Fix

_unstable_addStickyEvents now decrypts sticky events first via decryptStickyEvent, and only passes through events that are already keyed, plain unkeyed stickies, or successfully decrypted. As a backstop, RoomStickyEventsStore refuses to add unkeyed m.room.encrypted events, and sync/widget paths await the async add so decryption finishes before the store is updated.

Checklist

• Tests written for new code (and old code if feasible).
• New or updated public/exported symbols have accurate TSDoc documentation.
• Linter and other CI checks pass.
• Sign-off given on the changes (see CONTRIBUTING.md).

[toger5]

I added a couple of comments and looked into the code.

The more I look the more I see, that the JS sdk is already doing a good amount of checks.

especially decryptEventIfNeeded is rather powerful. It checks shouldAttemptDecryption and isBeingDecrypted.

So my current guess is that the cleanest solution would be to fix the encryption situation just in the addStickyEvent() method (not in addStickyEvents)
Since we do not have the encryption info in the type signature, I prefer changing addStickyEvent to work with both. encrypted and unencrypteed events.
so adding await decryptEventIfNeeded as the first line in addStickyEvent and making addStickyEvent async.
Some of the events (all from the sticky section should already be getting decrypted due to mapSyncEventsFormat in sync.ts. And the one from the timeline will be decrypted before checking the content.

I find the current decryptStickyEvent method confusing as it implies a lot of things need custom checks/solutions that the js-sdk already does/abstracts.

Combined this should:

• eliminate the need for: decryptStickyEvent
• change _unstable_addStickyEvents to:

 public async _unstable_addStickyEvents(
        events: MatrixEvent[],
    ): void {
        return void this.stickyEvents.addStickyEvents(decrypted);
    }

• and change addStickyEvents to:

/**
     * Add a series of sticky events, emitting `RoomEvent.StickyEvents` if any
     * changes were made.
     * @param events A set of new sticky events.
     */
    public async addStickyEvents(events: MatrixEvent[]): Promise<void> {
        const added: StickyMatrixEvent[] = [];
        const updated: { current: StickyMatrixEvent; previous: StickyMatrixEvent }[] = [];
        Promise.allSettled(
            events.map(async (event) => {
                try {
                    // We use allSettled (instead of a for loop) as we do not care about the order.
                    // addStickyEvent will ignore updates if it already has a newer event. (if they get processed in the "wrong" order)
                    const result = await this.addStickyEvent(event);
                    if (result.added) {
                        if (result.prevEvent) {
                            // e is validated as a StickyMatrixEvent by virtue of `addStickyEvent` returning added: true.
                            updated.push({ current: event as StickyMatrixEvent, previous: result.prevEvent });
                        } else {
                            added.push(event as StickyMatrixEvent);
                        }
                    }
                } catch (ex) {
                    logger.warn("ignored invalid sticky event", ex);
                }
            }),
        );
        if (added.length || updated.length) this.emit(RoomStickyEventsEvent.Update, added, updated, []);
        this.scheduleStickyTimer();
    }

This is possible due to addStickyEvents NOT returning the added event but relying on an emitter based approach.
So we can void their promises and make them async without needing to make _unstable_addStickyEvents async.
Does this sound sane?

[toger5]

public _unstable_addStickyEvents(events: MatrixEvent[]): ReturnType<RoomStickyEventsStore["addStickyEvents"]> {
        return this.stickyEvents.addStickyEvents(events);
    }

This should get an explicit void so we can see the promise gets dropped.

[toger5]

Suggested change

	await this.decryptEventIfNeeded(event);
	await this.decryptEventIfNeeded(event);
	if(event.getType() == EventType.RoomMessageEncrypted) throw Error("still encrypted after decryption attempt");

[toger5]

or similar error msg

[toger5]

Suggested change

	const type = event.getType();
	if (type === EventType.RoomMessageEncrypted) {
	logger.warn(
	"addStickyEvent called with encrypted event. Events are expected to be decrypted before adding!",
	);
	return { added: false };
	}

[toger5]

This case should never occur. It still feels saver to do the additional check however. But we should just add it after the await above.
Otherwise its a bit noisy

[toger5]

I am not following this change?
Was it there before?
What changed so the constructor can take a callback?

[toger5]

This test also needs to be investigated: should NOT filter sticky events before the retention period
Looks like it actually might point to a regression, but could also just be a test that needs adaption.