fix(FAR-823) [partial]: security — analytics team isolation, license TOCTOU (#1795 #1798); #1796 verified already fixed

[farnalabs]

Security fixes (archived farnalabs/modulo-bak findings)

Each was re-verified against current main before fixing.

• #1795 analytics team-isolation gap � a team boundary is now threaded from the analytics routes (resolved from the caller's own TeamMembership rows in an RLS-pinned session) through the query builder/service to the guardrails scorecard and export; scoped callers see own-team + org-level rows only; empty membership ? fail-closed; resolver DB failure ? 503 fail-closed. Corrections/scorecard owner is resolved through the pipeline. The budget-exhausted count is documented as an accepted org-wide residual (no run reference exists to scope it).
• #1798 admin_orgs license settings_json read-modify-write TOCTOU � both license handlers now do one locked read-modify-write (for_update on get_organisation).
• #1796 SSRF on admin-gated webhook & error-forwarder URLs � verified already remediated on current main (webhook POSTs go through pinned_async_client, which rejects private/metadata IPs; config-time URL validation present). No code change; recorded for the audit trail.

Closes FAR-823.

Tests

220 tests (analytics team-scope, admin orgs, analytics builder/guardrails/service), architecture test-style suite, ruff/mypy clean.

[modulo-reviewbot]

Review Decision: CHANGES_REQUESTED

Head SHA: 81c52f9b6ddf2f1e82d90931dd41bcab7a111b7a

Blocking finding

backend/src/modulo/api/routes/admin_orgs.py — MAJOR. _verify_license_key was moved before the org fetch in admin_set_org_license, so a nonexistent org with an invalid license key now returns 422 instead of 404. This broke the existing test test_set_org_license_missing_org_404 (CI Test (Backend) failure: asserts 404, gets 422). Fix: inside the locked transaction, fetch the org FOR UPDATE and raise 404 if missing, then verify the key, then write.

Required changes

1. Reorder admin_set_org_license so the org-existence check (404) happens inside the locked transaction before license-key verification, restoring the previous API contract.

Non-blocking findings (carry through)

• backend/src/modulo/core/analytics/service.py (MINOR): export_facts default offset silently changed from 0 to _EXPORT_DEFAULT_LIMIT (500). The route always passes offset explicitly, so no runtime impact today, but future callers omitting it would silently skip the first 500 rows. Recommend reverting to 0; unrelated to the security fix.
• backend/src/modulo/core/analytics/guardrails.py (NOTE): budget-exhausted count intentionally stays org-wide for team-scoped callers (documented residual); ensure #1795 scope covers it or file a follow-up.
• backend/src/modulo/api/mcp_server.py (context, unchanged; NOTE): MCP query_analytics calls run_analytics_query without team_ids, so non-admin MCP callers remain org-wide; if #1795 is meant to cover MCP too, this needs a follow-up.

Acknowledged as correct (context)

• backend/src/modulo/api/routes/analytics.py — _resolve_scoped_team_ids is fail-closed.
• backend/src/modulo/core/analytics/builder.py — bound team ids via expanding bindparam; empty boundary degrades to org-level; legacy single-team_id branch preserved.
• backend/src/modulo/db/crud/organisation.py — for_update=True adds with_for_update() (no-op on SQLite), closing the #1798 TOCTOU window.
• Test updates across test_analytics_team_scope.py, test_admin_orgs.py, test_analytics_builder.py, test_analytics_guardrails.py, test_analytics_service_execution.py.

The analytics team-boundary (#1795) and license TOCTOU (#1798) fixes themselves look correct and fail-closed; the contract regression above must be fixed before merge.

[farnalabs]

Automated fix: restored 404-before-422 contract in admin_set_org_license

Addresses: CHANGES_REQUESTED blocking finding on PR #455 (FAR #1798 regression — test_set_org_license_missing_org_404 failing with 422 instead of 404).

What changed (commit a79e48067, backend/src/modulo/api/routes/admin_orgs.py):

• In admin_set_org_license, _verify_license_key(req) was being called before the org fetch, so a nonexistent org with an invalid license key hit the 422 verify path before the 404 existence check.
• Moved the verification inside the locked session.begin() transaction, after the org-existence check. The org is now fetched FOR UPDATE and a missing org raises 404 first; only then is the license key verified (422) and written.
• The TOCTOU fix (#1798) is preserved: the license-key read-modify-write still happens inside the same row-locked transaction.

Why this option: keeps the prior API contract (404 for missing org, 422 for invalid key) and leaves the #1798 concurrency hardening intact — minimal, single-call-site change.

Verification: ruff, ruff-format, bandit, semgrep, gitleaks, import-linter all pass; the *_license* subset of tests/unit/api/test_admin_orgs_coverage_gaps.py passes (incl. test_set_org_license_missing_org_404).

[farnalabs]

Re-review of head a79e480 (fix push after the 12:59Z CHANGES_REQUESTED).

Blocking finding RESOLVED: _verify_license_key is now invoked only AFTER the org-existence (404) check inside the locked transaction in admin_set_org_license (and admin_remove_org_license keeps the same structure), so a nonexistent org with an invalid key returns 404 again — the main-side regression test test_set_org_license_missing_org_404 (backend/tests/unit/api/test_admin_orgs_coverage_gaps.py:474, key "k" against the real validator) fails without the reorder and passes with it. Verdict: APPROVE.

Non-blocking (carry-through, not addressed on this head):

1. backend/src/modulo/core/analytics/service.py (MINOR): export_facts default offset is still _EXPORT_DEFAULT_LIMIT (500) instead of 0. The route passes offset=0 explicitly so no runtime impact today, but a future caller omitting it would silently skip the first 500 rows.
2. backend/src/modulo/core/analytics/guardrails.py (NOTE): budget-exhausted count remains org-wide for team-scoped callers (documented as a deliberate residual — needs #1795 scope decision or a follow-up ticket).
3. MCP query_analytics still calls run_analytics_query without team_ids, so non-admin MCP callers remain org-wide (out of this PR's scope; follow-up suggested).

[modulo-reviewbot]

Approving PR #455 (head a79e480). The #1795 analytics team-boundary fix and #1798 license TOCTOU fix are both fail-closed and correct. The prior blocking finding is resolved: _verify_license_key now runs only after the org-existence 404 check inside the locked read-modify-write transaction, restoring the main-side contract (404 on missing org with invalid key). New unit coverage added for team-scope, admin org license ordering, builder, guardrails, and service execution; no test deletions. Non-blocking carry-through notes: export_facts default offset is 500 instead of 0 (latent footgun), guardrail budget-exhausted count stays org-wide for team-scoped callers, and MCP query_analytics still omits team_ids (follow-up). CI was pending on parallel checks at review time; mergeable=true.

[sonarqubecloud]

Quality Gate failed

Failed conditions
9.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud