Skip to content

Re-authorize project scope after the QuickBooks lookup in resolveAmbiguousInvoiceCreateCore #462

Description

@Clarion1631

Deferred from PR #438 (Codex gate round 53, the only remaining finding — P1, no P0s).

The hole

resolveAmbiguousInvoiceCreateCore authorizes the caller against the invoice's project before it asks QuickBooks anything:

  • scope check: src/lib/qbo-ambiguous-create.ts:306 (canAccessProject)
  • then the network call: findQBInvoicesByDocNumber
  • then the locked write: writeUnderParentLocks, src/lib/qbo-ambiguous-create.ts:750

That locked transaction re-reads and re-validates the tax rate and the client mapping, but it never re-reads Invoice.projectId and never re-runs canAccessProject. A QuickBooks lookup is a real network round trip, so the window is not small.

If the invoice is reassigned to another project during that window, a project-scoped FINANCE user can still link or clear it — acting on an invoice that has moved outside their scope. Both outcomes are money-path writes: link-existing attaches a collectible QuickBooks invoice to the row, and confirmed-none releases the row to be sent again.

Why it was deferred rather than patched

It is not a line inside the existing lock block. The re-authorization has to happen after the lookup returns and before the adopt/clear, which means:

  1. threading the actor through writeUnderParentLocks (it currently takes no principal — it re-reads money state, not permissions), and
  2. deciding what a mid-resolve permission change should do to an operator's in-flight decision: refuse with a distinct refusal reason, or park. That is a product decision about a human-facing flow, not a mechanical fix, and it wants its own review round rather than being appended to a round that was already closing two P0s.

Acceptance criteria

  • The current ownership chain (Invoice.projectId, and the project's client) is re-read under the locks already taken by writeUnderParentLocks — not from the pre-lookup snapshot.
  • canAccessProject (or the equivalent scope predicate) is re-run against that freshly-read chain immediately before the child write, for both outcomes — link-existing and confirmed-none.
  • A scope change mid-resolve produces its own refusal reason, distinguishable from the existing mismatch (which means "the money moved"), so an operator is told the row left their scope rather than that it changed.
  • Regression test: a project-scoped FINANCE actor resolves an invoice that is reassigned to a project outside their scope during the QuickBooks lookup; the write is refused and nothing is linked or cleared. The interleaving must be staged in the real window — after the lookup returns, before the write — the way tests/qbo-ambiguous-create.test.ts already stages its customer-remap and tax-rate races with the onClientLocked / onClientRead seams.
  • Pre-fix mutation control: with the re-authorization removed, that test fails.
  • A control proving the guard is not simply always-on: an in-scope actor still resolves normally.

Notes

  • tests/qbo-ambiguous-create.test.ts already has the harness for this — its fake enforces lock ordering and fires seams at the moment each lock is granted, which is where the mid-resolve edit has to land.
  • The ADMIN branch is not proof of anything here: an ADMIN passes every scope check, so the test must run as a genuinely scoped FINANCE user (same lesson as test(estimates): prove the scoped-total labels in a real browser #350).

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions