Skip to content

Conversation

@TinkerPhu
Copy link

Summary

  • Security fix: Reports were previously filtered via ven_program (enrollment) JOIN, allowing VENs enrolled in the same program to see each other's reports
  • Adds ven_id column to the report table with a foreign key to ven(id), enabling direct ownership tracking
  • All report CRUD queries (create, retrieve, retrieve_all, update) now filter by r.ven_id = ANY($N) instead of joining through ven_program

Changes

  • Migration (20260208000000_report_add_ven_id.sql): Adds ven_id column, backfills from client_nameven.ven_name mapping
  • Report queries (postgres/report.rs): Replaces LEFT JOIN ven_program with direct ven_id filtering in retrieve, retrieve_all, and update; stores ven_id on create
  • SQLx offline cache: Updated query hashes for all modified queries

Test plan

  • All 36 existing integration test scenarios pass (197 steps)
  • 3 new VEN isolation scenarios verify:
    • VEN-1 and VEN-2 each see only their own reports in list queries
    • Cross-VEN report retrieval by ID returns 404
    • Business users can still see all reports
    • Each VEN sees only its own VEN record

TinkerPhu and others added 3 commits February 8, 2026 17:49
Add DISTINCT to report retrieve and retrieve_all queries to prevent
duplicate rows when a program has multiple VEN enrollments.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
VENs enrolled in the same program could previously see each other's
reports. Add a ven_id column to track report ownership and filter
all report queries by it, ensuring VENs only see their own reports.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Previous hashes were computed with wrong whitespace. Regenerated
from the actual Rust source file with correct LF line endings.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@pohlm01 pohlm01 self-requested a review February 10, 2026 07:59
@codecov
Copy link

codecov bot commented Feb 10, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.70%. Comparing base (606dfb2) to head (190de0b).

Files with missing lines Patch % Lines
openleadr-vtn/src/data_source/postgres/report.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #359      +/-   ##
==========================================
- Coverage   80.72%   80.70%   -0.02%     
==========================================
  Files          42       42              
  Lines        4824     4825       +1     
==========================================
  Hits         3894     3894              
- Misses        930      931       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pohlm01
Copy link
Member

pohlm01 commented Feb 10, 2026

@TinkerPhu thanks a lot for that contribution. You are indeed right that VENs should not be able to see reports of other VENs that are registered in the same program. Good catch!
I just ran the CI, and it found a couple of minor problems.

  • First, please make sure you are adding a signed-off-by to your commits, see also your README
  • Clippy warns that the ven_id is not used in PostgresReport, which I think is fair, as it's only used within the database. If you could remove it (or annotate it with a #[allow(unused)] in case you think it's actually required)
  • You can ignore the security warning about the time crate. That should be fixed by the latest updates on main. Just make sure you are rebasing or merging.

Other than that, the PR looks good to me, and I'm looking forward to merging it

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