Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughBoth bot commands in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bot/start.ts (1)
582-595:⚠️ Potential issue | 🟠 MajorAdd community scoping for non‑super admins in
checkorder/checkinvoice.By switching to
adminMiddlewarewithout adding the community checks used elsewhere, a community admin can query any order/invoice by ID across communities. That’s a permissions regression.🔧 Suggested fix (apply to both commands)
bot.command('checkorder', adminMiddleware, async (ctx: MainContext) => { try { const [orderId] = (await validateParams(ctx, 2, '\\<_order id_\\>'))!; if (!orderId) return; if (!(await validateObjectId(ctx, orderId))) return; const order = await Order.findOne({ _id: orderId }); if (order === null) return; + if (!ctx.admin.admin) { + if (!order.community_id) return await messages.notAuthorized(ctx); + if (order.community_id != ctx.admin.default_community_id) { + return await messages.notAuthorized(ctx); + } + } const buyer = await User.findOne({ _id: order.buyer_id }); const seller = await User.findOne({ _id: order.seller_id }); await messages.checkOrderMessage(ctx, order, buyer, seller); } catch (error) { logger.error(error); } });bot.command( 'checkinvoice', adminMiddleware, async (ctx: MainContext) => { try { const [orderId] = (await validateParams(ctx, 2, '\\<_order id_\\>'))!; if (!orderId) return; if (!(await validateObjectId(ctx, orderId))) return; const order = await Order.findOne({ _id: orderId }); if (order === null) return; + if (!ctx.admin.admin) { + if (!order.community_id) return await messages.notAuthorized(ctx); + if (order.community_id != ctx.admin.default_community_id) { + return await messages.notAuthorized(ctx); + } + } if (!order.hash) return; const invoice = await getInvoice({ hash: order.hash }); if (invoice === undefined) { throw new Error('invoice is undefined'); }Also applies to: 600-623
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bot/start.ts (1)
582-624:⚠️ Potential issue | 🟠 MajorScope community admins to their community before revealing order/invoice details.
With
adminMiddleware, community admins can now call these commands. Without a community check, any admin can query any order/invoice by ID, which leaks cross‑community data. Add the same non‑super‑admin guard used elsewhere in this file (e.g., cancelorder/settleorder).🔒 Proposed fix (apply to both handlers)
bot.command('checkorder', adminMiddleware, async (ctx: MainContext) => { try { const [orderId] = (await validateParams(ctx, 2, '\\<_order id_\\>'))!; if (!orderId) return; if (!(await validateObjectId(ctx, orderId))) return; const order = await Order.findOne({ _id: orderId }); if (order === null) return; + + // Restrict community admins to their community orders + if (!ctx.admin.admin) { + if (!order.community_id) return await messages.notAuthorized(ctx); + if (order.community_id != ctx.admin.default_community_id) { + return await messages.notAuthorized(ctx); + } + } const buyer = await User.findOne({ _id: order.buyer_id }); const seller = await User.findOne({ _id: order.seller_id }); await messages.checkOrderMessage(ctx, order, buyer, seller); } catch (error) { logger.error(error); } }); bot.command('checkinvoice', adminMiddleware, async (ctx: MainContext) => { try { const [orderId] = (await validateParams(ctx, 2, '\\<_order id_\\>'))!; if (!orderId) return; if (!(await validateObjectId(ctx, orderId))) return; const order = await Order.findOne({ _id: orderId }); if (order === null) return; + // Restrict community admins to their community orders + if (!ctx.admin.admin) { + if (!order.community_id) return await messages.notAuthorized(ctx); + if (order.community_id != ctx.admin.default_community_id) { + return await messages.notAuthorized(ctx); + } + } if (!order.hash) return; const invoice = await getInvoice({ hash: order.hash }); if (invoice === undefined) { throw new Error('invoice is undefined'); } await messages.checkInvoiceMessage( ctx, invoice.is_confirmed, invoice.is_canceled!, invoice.is_held!, ); } catch (error) { logger.error(error); } });
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Missing community scope check ❌
The original issue (#703) explicitly states:
"but only for orders created within their own communities"
This PR changes superAdminMiddleware → adminMiddleware, which allows any admin (including community admins and dispute solvers) to query any order in the system, not just orders from their own community.
validateAdmin() passes if user.admin === true OR isDisputeSolver(community, user). A dispute solver in community A could use checkorder to inspect orders from community B — that's an information leak.
Required fix
After fetching the order, verify the admin has jurisdiction:
const order = await Order.findOne({ _id: orderId });
if (order === null) return;
// If not a super admin, verify the order belongs to the admin's community
if (!ctx.admin.admin && order.community_id) {
if (String(order.community_id) !== String(ctx.admin.default_community_id)) {
return await messages.notAuthorized(ctx, ctx.admin.tg_id);
}
}Same check needed in both checkorder and checkinvoice.
Code quality ✅
The refactoring from callback-style to flat bot.command() for checkinvoice is a nice cleanup.
CI ✅
All checks passing.
Summary
The change in middleware is correct directionally, but without the community scope check it's a privilege escalation — community admins can see orders from other communities. Request changes until the scope check is added.
…voice commands that the order is in the community of the admin
There was a problem hiding this comment.
Changes addressed ✅
The community scope check has been implemented correctly in both checkorder and checkinvoice commands:
if (
!ctx.admin.admin &&
String(order.community_id) !== String(ctx.admin.default_community_id)
) {
return await messages.notAuthorized(ctx, ctx.admin.tg_id);
}This ensures:
- Super admins (
ctx.admin.admin === true) can check any order - Community admins can only check orders from their own community
- Unauthorized access returns
notAuthorizedmessage
CI ✅
All checks passing.
Summary
LGTM. The fix properly restricts community admins to their assigned communities while maintaining full access for super admins.
…ands (lnp2pBot#732) * Community administrators now can use checkorder and checkinvoice commands * Code formatting * Address mostronator comment, added a check for checkorder and checkinvoice commands that the order is in the community of the admin * Code formatting
2 participants
Before you needed to be superadmin in order been able to run those commands
Summary by CodeRabbit
New Features
Bug Fixes
Chores