Aug 17 - Fix bug, cross-customer form-name leak

app/routes/inspections.py

@@ -489,10 +489,30 @@ def templates_for_project(project_id):
     """Forms usable on this contract — shared ones plus any attached to it.
 
     Powers the Contract -> Form cascade on the start-inspection page, the same
-    way facilities_for_project powers Contract -> Facility. Read-only, and the
+    way facilities_for_project powers Contract -> Facility.
-    real gate is still the POST validation in start(); this only keeps the
+
-    picker honest as the contract changes.
+    **Scoped to the caller's own contracts.** The POST validation in start() is
+    what stops a form being *used* across contracts, but this endpoint would
+    otherwise happily list one customer's bespoke form NAMES to another
+    customer's inspector who simply asked for a contract id — the same leak
+    that rule 96 covers on the mobile API. Empty list rather than 403, so it
+    does not confirm whether the contract exists either.
     """
+    if current_user.is_inspector:
+        fids = get_inspector_scope(current_user) or []
+        allowed = {
+            f.project_id
+            for f in Facility.query.filter(Facility.id.in_(fids)).all()
+        } if fids else set()
+        if project_id not in allowed:
+            logger_msg = ('TEMPLATES_FOR_PROJECT | out-of-scope request | '
+                          'user=%s | project_id=%s')
+            current_app.logger.warning(logger_msg, current_user.username, project_id)
+            return jsonify([])
+    elif current_user.role == 'customer':
+        # Customers never start inspections; nothing here is theirs to see.
+        return jsonify([])
+
     templates = InspectionTemplate.available_query(project_id).all()
     return jsonify([
         {'id': t.id, 'name': t.name, 'shared': t.is_shared}
@@ -1480,15 +1500,25 @@ def bulk_action():
     if action == 'delete':
         from app.utils import storage
         photo_paths = []
+        # Snapshot (id, label) BEFORE deleting: the objects are expired after
+        # the commit, and the audit pass must run after it. log_action()
+        # commits internally (rule 41), so auditing inside this loop would
+        # commit the deletes one at a time — and a mid-loop failure would
+        # leave rows gone with the photo cleanup below never reached.
+        deleted = []
         for insp in inspections:
             photo_paths.extend(_collect_inspection_photos(insp))
-            log_action(ACTION_DELETE, 'Inspection', insp.id,
+            deleted.append((
-                       f'{insp.template.name if insp.template else "—"} @ '
+                insp.id,
-                       f'{insp.facility.name if insp.facility else "—"}',
+                f'{insp.template.name if insp.template else "—"} @ '
-                       f'bulk deleted by {current_user.username}')
+                f'{insp.facility.name if insp.facility else "—"}',
+            ))
             db.session.delete(insp)
             changed += 1
         db.session.commit()
+        for insp_id, label in deleted:
+            log_action(ACTION_DELETE, 'Inspection', insp_id, label,
+                       f'bulk deleted by {current_user.username}')
         # Files only after the rows are gone — an orphaned file is recoverable,
         # a deleted file belonging to a surviving row is not.
         for rel_path in photo_paths:
@@ -1498,6 +1528,11 @@ def bulk_action():
     # ── Request follow-up ────────────────────────────────────────────────
     elif action == 'flag_followup':
         note = request.form.get('follow_up_note', '').strip() or None
+        # Only the rows this run actually flagged. Re-deriving it afterwards
+        # from `follow_up_requested_by == current_user.id` would also match
+        # inspections this same user flagged on an EARLIER run and that were
+        # skipped here as already-flagged — re-notifying their inspectors.
+        flagged = []
         for insp in inspections:
             # Same two guards as the single-inspection route: nothing to follow
             # up on before submission, and a repeat request must not overwrite
@@ -1509,12 +1544,11 @@ def bulk_action():
             insp.follow_up_note         = note
             insp.follow_up_requested_by = current_user.id
             insp.follow_up_requested_at = now_eastern()
+            flagged.append(insp)
             changed += 1
         db.session.commit()
 
-        for insp in inspections:
+        for insp in flagged:
-            if insp.follow_up_requested_by != current_user.id or not insp.follow_up_required:
-                continue
             body = (f'{current_user.display_name} has requested a follow-up '
                     f're-inspection of "{insp.template.name if insp.template else "—"}" '
                     f'at {insp.facility.name if insp.facility else "—"}.'
@@ -1543,15 +1577,18 @@ def bulk_action():
                 exclude_user_ids = {current_user.id,
                                     inspector.id if inspector else None} - {None},
             )
+        db.session.commit()   # notify() does not commit — rule 70
+        # Audited after the commit (rule 41) — log_action commits internally.
+        for insp in flagged:
             log_action(ACTION_UPDATE, 'Inspection', insp.id,
                        f'{insp.template.name if insp.template else "—"}',
                        f'bulk follow_up_required=True by {current_user.username}')
-        db.session.commit()   # notify() does not commit — rule 70
         _flash_bulk(changed, skipped, 'flagged for follow-up',
                     skip_reason='not submitted, or already flagged')
 
     # ── Clear follow-up ──────────────────────────────────────────────────
     elif action == 'clear_followup':
+        cleared = []
         for insp in inspections:
             if not insp.follow_up_required:
                 skipped += 1
@@ -1560,11 +1597,13 @@ def bulk_action():
             insp.follow_up_note         = None
             insp.follow_up_requested_by = None
             insp.follow_up_requested_at = None
+            cleared.append(insp)
             changed += 1
+        db.session.commit()
+        for insp in cleared:            # after the commit — rule 41
             log_action(ACTION_UPDATE, 'Inspection', insp.id,
                        f'{insp.template.name if insp.template else "—"}',
                        f'bulk follow_up cleared by {current_user.username}')
-        db.session.commit()
         _flash_bulk(changed, skipped, 'cleared of the follow-up flag',
                     skip_reason='not flagged')
 

app/routes/issues.py

@@ -1033,16 +1033,23 @@ def bulk_action():
                 flash('That user no longer exists.', 'danger')
                 return redirect(back)
 
+        # Track what actually moved. Re-deriving this after the commit by
+        # testing `issue.assigned_to == user.id` would also match the issues
+        # that were ALREADY assigned to that person — they were counted as
+        # skipped, but would still be emailed "assigned to you" every time
+        # anyone ran a bulk assign over them.
+        newly_assigned = []
         for issue in issues:
             if issue.assigned_to == (user.id if user else None):
                 skipped += 1
                 continue
             issue.assigned_to = user.id if user else None
+            newly_assigned.append(issue)
             changed += 1
         db.session.commit()
 
-        for issue in issues:
+        if user:
-            if issue.assigned_to == (user.id if user else None) and user:
+            for issue in newly_assigned:
                 notify(
                     recipient  = user,
                     title      = f'Issue #{issue.id} assigned to you',
@@ -1054,7 +1061,7 @@ def bulk_action():
                     event_type = EVENT_ISSUE_ASSIGNED,
                     send_email = True,
                 )
-        db.session.commit()   # notify() does not commit — rule 70
+            db.session.commit()   # notify() does not commit — rule 70
 
         label = user.display_name if user else 'Unassigned'
         log_action(ACTION_UPDATE, 'Issue', None, f'bulk assign → {label}',
@@ -1068,11 +1075,17 @@ def bulk_action():
             flash('Please choose a status to set.', 'warning')
             return redirect(back)
 
+        # (issue, old_status) for the audit pass, which must run AFTER the
+        # commit — log_action() commits internally (rule 41), so calling it
+        # inside this loop would commit each row separately and lose the
+        # batch's atomicity.
+        moved = []
         for issue in issues:
             if issue.status == new_status:
                 skipped += 1
                 continue
             old = issue.status
+            moved.append((issue, old))
             issue.status = new_status
             # Keep resolved_at consistent with the status, the same way the
             # single-issue update does — a resolved issue with no resolved_at
@@ -1082,14 +1095,16 @@ def bulk_action():
             elif new_status in ('open', 'in_progress'):
                 issue.resolved_at = None
             changed += 1
+        db.session.commit()
+        for issue, old in moved:
             log_action(ACTION_UPDATE, 'Issue', issue.id, f'#{issue.id}',
                        f'bulk status {old} → {new_status} by {current_user.username}')
-        db.session.commit()
         _flash_bulk(changed, skipped,
                     f'set to {new_status.replace("_", " ").title()}')
 
     # ── Verify & close ───────────────────────────────────────────────────
     elif action == 'verify':
+        verified = []
         for issue in issues:
             if issue.status not in ('resolved', 'pending_verification'):
                 skipped += 1
@@ -1099,10 +1114,12 @@ def bulk_action():
             issue.verified_at = now_eastern()
             if not issue.resolved_at:
                 issue.resolved_at = now_eastern()
+            verified.append(issue)
             changed += 1
+        db.session.commit()
+        for issue in verified:          # after the commit — rule 41
             log_action(ACTION_UPDATE, 'Issue', issue.id, f'#{issue.id}',
                        f'bulk_verified_by={current_user.username}')
-        db.session.commit()
         _flash_bulk(changed, skipped, 'verified and closed',
                     skip_reason='not awaiting verification')
 
@@ -1110,17 +1127,25 @@ def bulk_action():
     elif action == 'delete':
         from app.utils import storage
         photo_paths = []
+        # Snapshot the ids BEFORE deleting — the objects are expired after the
+        # commit, and the audit pass has to run after it (rule 41: log_action
+        # commits internally, so auditing inside this loop would commit the
+        # deletes one at a time and, on a mid-loop failure, leave rows gone
+        # with the photo cleanup below never reached).
+        deleted_ids = []
         for issue in issues:
             if issue.photo_path:
                 photo_paths.append(issue.photo_path)
             for lst in (issue.mobile_photo_paths, issue.result_photos):
                 if lst:
                     photo_paths.extend(lst)
-            log_action(ACTION_DELETE, 'Issue', issue.id, f'#{issue.id}',
+            deleted_ids.append(issue.id)
-                       f'bulk deleted by {current_user.username}')
             db.session.delete(issue)
             changed += 1
         db.session.commit()
+        for issue_id in deleted_ids:
+            log_action(ACTION_DELETE, 'Issue', issue_id, f'#{issue_id}',
+                       f'bulk deleted by {current_user.username}')
         # Files go only after the rows are safely gone — a failure here leaves
         # an orphaned file, which is recoverable; the reverse is not.
         for rel_path in photo_paths: