Repository navigation
Update appwrite push ledger name - #13848
Conversation
…tions and improve topic name resolution
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| foreach ($subscribe->filters() as $filter) { | ||
| Span::add('mqtt.topic', $filter->topic); | ||
|
|
||
| $document = $topicsById[$filter->topic] ?? null; | ||
| if (!isset($allowed[$filter->topic]) || $document === null) { | ||
| $suback->deny(); | ||
| continue; | ||
| } | ||
| $document = $this->isWildcard($filter->topic) ? null : ($topicsByName[$filter->topic] ?? null); | ||
|
|
||
| if (!$this->authorizedForTopic($document->getAttribute('subscribe', []) ?? [], $roles)) { | ||
| $suback->deny(); | ||
| continue; | ||
| } | ||
|
|
||
| $topicQos = $document->getAttribute('qos'); | ||
| $topicQos = $document?->getAttribute('qos'); | ||
| $grantedQos = min($filter->qos, $topicQos === null ? Packet::QOS_1 : (int) $topicQos); | ||
|
|
||
| $suback->grant($grantedQos); |
There was a problem hiding this comment.
The handler now grants every subscription without checking the topic's configured subscribe roles. Any authenticated project user can therefore subscribe to a restricted topic by name or wildcard. No later delivery layer checks those roles, so unauthorized users can receive live messages and, for exact topics, backlog replays.
How this was verified: Topic subscribe roles are stored by the public topic API, while the changed handler grants every filter and delivery subsequently fans messages directly to matching subscribers without another authorization check.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/Mqtt/Handler.php
Line: 175-182
Comment:
**Topic Authorization Is Removed**
The handler now grants every subscription without checking the topic's configured `subscribe` roles. Any authenticated project user can therefore subscribe to a restricted topic by name or wildcard. No later delivery layer checks those roles, so unauthorized users can receive live messages and, for exact topics, backlog replays.
**How this was verified:** Topic subscribe roles are stored by the public topic API, while the changed handler grants every filter and delivery subsequently fans messages directly to matching subscribers without another authorization check.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
@greptile it is intentional
There was a problem hiding this comment.
Understood. If topic subscriptions are intentionally open to every authenticated project connection, then the authorization-removal concern is not actionable for this PR. The updated comment in onSubscribe() documents that policy; no change is needed from my side.
| $documents = $authorization->skip(fn () => $projectDB->find('topics', [ | ||
| Query::equal('$id', array_keys($allowed)), | ||
| Query::limit(\count($allowed)), | ||
| Query::equal('name', array_keys($names)), | ||
| Query::limit(\count($names)), | ||
| ])); | ||
| foreach ($documents as $document) { | ||
| $topicsById[$document->getId()] = $document; | ||
| $topicsByName[$document->getAttribute('name')] ??= $document; |
There was a problem hiding this comment.
Topic names are not unique, but this lookup treats each name as a unique channel and keeps only the first matching document. Two supported topics with the same name will have their live publications merged onto one MQTT channel, while QoS and replay use an arbitrary topic document. Subscribers can consequently receive another topic's messages or the wrong backlog.
How this was verified: The topic schema and API permit duplicate names, while publishing maps IDs to names and subscription lookup retains only the first document for each name.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/Mqtt/Handler.php
Line: 164-169
Comment:
**Duplicate Names Merge Topics**
Topic names are not unique, but this lookup treats each name as a unique channel and keeps only the first matching document. Two supported topics with the same name will have their live publications merged onto one MQTT channel, while QoS and replay use an arbitrary topic document. Subscribers can consequently receive another topic's messages or the wrong backlog.
**How this was verified:** The topic schema and API permit duplicate names, while publishing maps IDs to names and subscription lookup retains only the first document for each name.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
@greptile dont you think it make sense. A project can't have same topic name. If they are that is wrong. It should be merged to one only as the subscriber listening to topic name and not the topic id
There was a problem hiding this comment.
That makes sense as the intended product invariant: MQTT subscribers address the channel by name, not the topic document ID, so duplicate names should represent one logical channel.
My concern is that the current schema/API does not enforce this invariant. Create only treats the document ID as unique, and V24.php explicitly removes the existing _key_name index, so duplicate-name documents can currently exist.
If duplicate names are invalid, uniqueness should be enforced at the data layer/API, with existing duplicates cleaned up or otherwise handled. Until then, ??= remains lossy: it selects one arbitrary document for QoS and replay, so messages stored under another duplicate document ID will not be replayed.
| private const BROKER_PORT = 1883; | ||
| private const BROKER_WS_PORT = 8083; | ||
|
|
||
| // The CONNACK/DISCONNECT Reason String the broker sends on an unauthorized connection — the |
There was a problem hiding this comment.
REASON_UNAUTHORIZED copies the production exception text verbatim, and the new assertions require that exact wording. This violates the repository directive to test observable behavior rather than mirror source strings, and it makes harmless copy changes break an end-to-end protocol test. This repository requirement must be satisfied before merging; test the rejection separately and assert only the reason's observable semantic properties.
Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/Services/Mqtt/MqttServerTest.php
Line: 46
Comment:
**Test Mirrors Production Message**
`REASON_UNAUTHORIZED` copies the production exception text verbatim, and the new assertions require that exact wording. This violates the repository directive to test observable behavior rather than mirror source strings, and it makes harmless copy changes break an end-to-end protocol test. This repository requirement must be satisfied before merging; test the rejection separately and assert only the reason's observable semantic properties.
**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after) |
… schema and assertions
2 participants
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist