123 f6 cross team mcp registration#7892
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7892 +/- ##
==========================================
+ Coverage 75.60% 75.65% +0.05%
==========================================
Files 432 432
Lines 23046 23109 +63
Branches 6111 6132 +21
==========================================
+ Hits 17424 17484 +60
- Misses 5622 5625 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
All handled in commit 4558d25 |
The owner check compared the URL :typeId against the token ownerId directly. For device tokens the URL carries the hashid while ownerId is the numeric id, so a device could never register or delete its own MCP entry. Compare the resolved target id instead. Also tidy the delete handler: drop the duplicated team check, guard for a missing device or instance with a 404, return 400 for an unknown target type, and scope the registration lookup to the resolved id and team.
The device branch resolved the target id to the numeric device id, which Postgres rejects when compared against the varchar targetId column. Use the string form so the lookup stays a varchar comparison.
There was a problem hiding this comment.
I don't like this. The pre-operation code in the POST and DELETE handlers is almost identical, the only real difference being that POST throws (which gets swallowed by the try/catch and returned as a generic 500) where it should be replying with a proper status code. A "device not found" or "unknown type" is a 404/400, not a 500, so the two handlers are out of alignment on how they report the same failures.
I believe we should refactor the pre-op checks (resolve device/instance, verify team ownership, and confirm the target matches the session owner) into a common function used by both POST and DELETE. That keeps the behaviour consistent and avoids this kind of misalignment creeping in again as the code changes.
Extract the device/instance resolution, team ownership, and owner match checks into a shared function used by both routes, so unknown device/instance/type now return 404/400 on POST instead of a generic 500 from the broad try/catch, matching what DELETE already returned.
|
Agreed, refactored the pre-op checks into a shared Updated the three POST tests that asserted the old 500 behavior to expect 404/404/400 to match DELETE. |
|
To be clear, the 500 was already there before your review, I just hadn't considered it worth changing. Agreed 400/404 is more useful for callers than a blanket 500, thanks for flagging it. |
3 participants
fixes FlowFuse/security#125
see FlowFuse/security#123 for details