lizhimins commented on PR #4503: URL: https://github.com/apache/rocketmq-dashboard/pull/4503#issuecomment-5761154024
Thanks — the hardening goal here is correct and your write-up of #4502 is what made it precise. `GET /api/clients` is reader-accessible (`AuthInterceptor` lets every GET through, and `/api/clients` is not in the admin-only list), `ClientService` only trimmed the incoming `namesrvAddr` before handing it to the provider, and `MqAdminExtFactory` caches one started `DefaultMQAdminExt` per address in an unbounded map that is only reclaimed on explicit `release` or `@PreDestroy`. So an unregistered address really is both an outbound-connection surface and a client-cache growth surface, and closing it in the backend is the right call — the frontend only ever submits registered endpoints, so it cannot enforce this. We are closing this as a duplicate of #4534, which fixes the same defect at the same entry point and has now been merged. Two reasons we preferred that implementation: - The check lives in `NameserverRegistryService.requireRegisteredAddress(...)`, i.e. in the component that owns the registry, rather than inlined in `ClientService`. That is where the next caller will look for it. - It answers the question with a conditional count query on `namesrv_addr`, whereas this PR streams `registryService.list()` — a full `selectList` plus a `toVO` mapping per row — on every `GET /api/clients`. - It also covers the malformed-address path, where `NamesrvAddrParser.normalize` throws 400 before any database lookup. That behaviour is untested here. You filed this about six hours before #4534, so this is not a first-come decision — it is a "which implementation do we want to own" one, and we took the better-encapsulated version. Sorry for the wasted round. One gap that exists in both PRs and is worth carrying over as a follow-up: the comparison is normalized-request against raw-stored value. Registry rows written before #2510 (which started normalizing on create/update) can still hold a form like `NS1:9876;ns2:9876`, and the frontend submits exactly that raw string, so a legitimate endpoint can 404 after normalization. Normalizing both sides, or a small data migration, would close it. If you would like to pick that up, it is a genuinely open problem and we would review it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
