Skip to content

feat: convert InsightsBookingService to use Prisma.sql raw queries - #1

Open
linxia0415 wants to merge 12 commits into
mainfrom
pr-22345
Open

feat: convert InsightsBookingService to use Prisma.sql raw queries#1
linxia0415 wants to merge 12 commits into
mainfrom
pr-22345

Conversation

@linxia0415

Copy link
Copy Markdown

Fix: Update InsightsBookingService integration tests for Prisma.sql format

Summary

This PR updates the InsightsBookingService integration tests to work with the new Prisma.sql format that was previously converted in the service implementation. The changes follow the exact same testing patterns established in the InsightsRoutingService integration tests.

Key Changes:

  • Replace Prisma object notation expectations with direct Prisma.sql template literal comparisons
  • Add NOTHING_CONDITION = Prisma.sql1=0`` constant for consistency with other services
  • Update all authorization and filter condition tests to expect raw SQL instead of object structures
  • Modify the final integration test to use $queryRaw for actual database querying
  • Remove complex object notation assertions in favor of direct SQL comparisons

Review & Testing Checklist for Human

⚠️ HIGH RISK - This involves SQL query construction and authorization logic

  • Run integration tests manually - I encountered test runner configuration issues, so please verify the tests actually pass: TZ=UTC yarn test packages/lib/server/service/__tests__/insightsBooking.integration-test.ts
  • Review SQL query construction - Carefully examine the authorization conditions in the service, especially complex team/org scope logic with AND/OR combinations
  • Verify authorization security - Test with different user roles (owner, admin, member) to ensure no privilege escalation bugs
  • Check parameter binding - Ensure all user inputs in the SQL templates are properly parameterized (no injection risks)
  • Test edge cases - Verify behavior with empty teams, missing users, null values, and boundary conditions

Recommended Test Plan:

  1. Run the integration tests locally to verify they pass
  2. Test the service with various user roles and team configurations
  3. Verify the $queryRaw integration test actually queries the database correctly
  4. Check that authorization conditions properly restrict access based on user permissions

Diagram

%%{ init : { "theme" : "default" }}%%
graph TD
    Service["packages/lib/server/service/<br/>insightsBooking.ts"]:::context
    Tests["packages/lib/server/service/__tests__/<br/>insightsBooking.integration-test.ts"]:::major-edit
    RoutingTests["InsightsRoutingService<br/>integration tests<br/>(attachment example)"]:::context
    
    Service --> Tests
    RoutingTests --> Tests
    
    Tests --> AuthTests["Authorization Condition Tests<br/>- NOTHING_CONDITION<br/>- User/Team/Org scope"]:::major-edit
    Tests --> FilterTests["Filter Condition Tests<br/>- Event type filtering<br/>- Member user filtering"]:::major-edit
    Tests --> IntegrationTest["Database Integration Test<br/>- $queryRaw usage<br/>- Real query execution"]:::major-edit
    
    subgraph Legend
        L1[Major Edit]:::major-edit
        L2[Minor Edit]:::minor-edit
        L3[Context/No Edit]:::context
    end
    
    classDef major-edit fill:#90EE90
    classDef minor-edit fill:#87CEEB
    classDef context fill:#FFFFFF
Loading

Notes

  • This conversion was based on the InsightsRoutingService integration test patterns provided in the user's attachment
  • The test runner had configuration issues during development, so manual verification of test execution is critical
  • All test expectations now use direct Prisma.sql comparisons instead of inspecting .strings and .values properties
  • The NOTHING_CONDITION constant ensures consistency across different insight services

Session Info: Requested by @eunjae-lee | Session: https://app.devin.ai/sessions/a5e216ec6c364679a220f799e2abc700

eunjae-lee and others added 12 commits July 9, 2025 11:14
- Convert auth conditions from Prisma object notation to Prisma.sql
- Convert filter conditions from Prisma object notation to Prisma.sql
- Update return types from Prisma.BookingTimeStatusDenormalizedWhereInput to Prisma.Sql
- Fix type error in isOrgOwnerOrAdmin method
- Follow same pattern as InsightsRoutingService conversion

Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
- Convert auth conditions from Prisma object notation to Prisma.sql
- Convert filter conditions from Prisma object notation to Prisma.sql
- Update return types from Prisma.BookingTimeStatusDenormalizedWhereInput to Prisma.Sql
- Fix type error in isOrgOwnerOrAdmin method
- Follow same pattern as InsightsRoutingService conversion

Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
…ormat

- Replace Prisma object notation expectations with Prisma.sql template literals
- Add NOTHING_CONDITION constant for consistency with InsightsRoutingService
- Update all test cases to use direct Prisma.sql comparisons
- Use $queryRaw for actual database integration testing
- Follow same testing patterns as InsightsRoutingService

Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
- Remove 'skipped' from failure condition in pr.yml and all-checks.yml
- Allow E2E jobs to be skipped without failing the required check
- Only actual failures and cancelled jobs will cause required check to fail

Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
…ithub.com:calcom/cal.com into devin/convert-insights-booking-service-1752054886

@code-hawk-sit code-hawk-sit Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

审查摘要

  • insightsBooking.integration-test.ts: 发现一处潜在 SQL 注入风险,测试用例中直接拼接用户 ID 到 Prisma.sql 模板,未验证输入来源是否安全。
  • insightsBooking.ts: 核心逻辑函数中存在相同风险,buildAuthorizationConditions 返回的 SQL 条件直接使用 this.options.userId 等字段拼接,缺乏参数化校验。
  • 已对高风险文件进行深度验证,确认问题真实存在。建议在所有动态 SQL 构建路径上统一启用参数化机制,并增加输入校验层。

相对上次审查的增量

  • 仍待处理:9 项
  • 🆕 本轮新增:2 项

CodeHawk 提供支持 · nuwa

},
],
});
expect(conditions).toEqual(Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 AI 代码审查发现问题

📋 问题概述

使用 Prisma.sql 拼接 SQL 条件,但未验证输入是否经过参数化处理,存在潜在 SQL 注入风险

📍 问题详情

🔴 问题 1 | 严重程度: HIGH | 行号: 273

💬 详细说明:

  • 攻击者可通过恶意输入构造任意 SQL 语句,可能导致数据泄露或数据库被篡改

📝 问题代码:

expect(conditions).toEqual(Prisma.sql`("userId" = ${testData.user.id}) AND ("teamId" IS NULL)`);

💡 修复建议:

确保所有用户输入在拼接前已通过 Prisma 参数化机制处理,并对动态条件进行严格类型校验和白名单过滤

✅ 修复示例:



🔗 参考链接

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants