security(HIGH): scope teCustId on POST /v1/timeentry to caller's company#364
Open
CryptoJones wants to merge 1 commit into
Open
security(HIGH): scope teCustId on POST /v1/timeentry to caller's company#364CryptoJones wants to merge 1 commit into
CryptoJones wants to merge 1 commit into
Conversation
`timeentrycontroller.create` accepted any integer-shaped teCustId from a
non-master caller without verifying the referenced Customer belongs to
the caller's company. Effect:
- Non-master scoped to company 7 sends `{ teCustId: 13 }`
- Customer 13 lives in company 99
- The new TimeEntry row gets teCompId=7 (forced server-side) AND
teCustId=13 (kept from body) — a permanent cross-tenant FK.
The list/get/update/delete handlers all filter by teCompId, so the
anomalous row stays invisible to the OTHER tenant on the API surface.
But:
- Any operator-side report or UI that joins TimeEntry -> Customer
(via the FK) surfaces company 99's customer inside company 7's view.
- It's a permanent integrity corruption that the API itself created.
Every other create handler with a cross-company FK (jobcontroller,
invoicecontroller, customerpaymentcontroller, productentrycontroller)
already does this check via `GetCompanyIdByCustomerId` / `GetCompanyIdByJobId`
/ `GetCompanyIdByPovId`. timeentry was the only outlier.
Fix follows the existing pattern: after teCustId integer validation
and before payload.teCompId assignment, non-master callers run
`GetCompanyIdByCustomerId(payload.teCustId)` and 403 if the
resolved company differs (or is -1, i.e. customer missing/archived).
Master keys still skip this check by design.
Tests added (4, all in tests/api/timeentry.test.js — a new
"tenant-scoped teCustId (cross-tenant FK)" describe):
- cross-tenant create returns 403, TimeEntry.create NOT called
- missing/archived customer returns 403, TimeEntry.create NOT called
- same-company create returns 201 (positive control)
- master keys bypass the check (create succeeds even cross-tenant)
The new tests drive the controller through the file-top db.config
vi.mock (mutating ApiKey/ApiMaster/Customer per scenario) rather than
vi.spyOn on the auth helpers — the existing spy-based tenant-defense
tests for invoice/etc. only pass because the empty `ApiKey: {}`
mock makes auth.getCompanyId() return -1, which short-circuits at
"Invalid Authorization Key." before the spy is ever consulted. The
db-mock approach actually exercises the cross-tenant comparison.
800 tests pass (was 796); lint clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
timeentrycontroller.createaccepted any integer-shaped teCustId from anon-master caller without verifying the referenced Customer belongs to
the caller's company. Effect:
{ teCustId: 13 }teCustId=13 (kept from body) — a permanent cross-tenant FK.
The list/get/update/delete handlers all filter by teCompId, so the
anomalous row stays invisible to the OTHER tenant on the API surface.
But:
(via the FK) surfaces company 99's customer inside company 7's view.
Every other create handler with a cross-company FK (jobcontroller,
invoicecontroller, customerpaymentcontroller, productentrycontroller)
already does this check via
GetCompanyIdByCustomerId/GetCompanyIdByJobId/
GetCompanyIdByPovId. timeentry was the only outlier.Fix follows the existing pattern: after teCustId integer validation
and before payload.teCompId assignment, non-master callers run
GetCompanyIdByCustomerId(payload.teCustId)and 403 if theresolved company differs (or is -1, i.e. customer missing/archived).
Master keys still skip this check by design.
Tests added (4, all in tests/api/timeentry.test.js — a new
"tenant-scoped teCustId (cross-tenant FK)" describe):
The new tests drive the controller through the file-top db.config
vi.mock (mutating ApiKey/ApiMaster/Customer per scenario) rather than
vi.spyOn on the auth helpers — the existing spy-based tenant-defense
tests for invoice/etc. only pass because the empty
ApiKey: {}mock makes auth.getCompanyId() return -1, which short-circuits at
"Invalid Authorization Key." before the spy is ever consulted. The
db-mock approach actually exercises the cross-tenant comparison.
800 tests pass (was 796); lint clean.
Self-review caveats (be aware before merging):
The existing 'tenant-enumeration defense' tests for invoice/etc. pass for the wrong reason — the empty
ApiKey: {}mock in those test files makesauth.getCompanyId()short-circuit to -1 (DB throw → catch → -1), so the controller returns 'Invalid Authorization Key.' before the cross-tenant comparison is ever reached. Thevi.spyOn(auth, 'isMaster')etc. in those tests doesn't take effect on the controller's call sites because the controller capturesconst IsMaster = auth.isMasterat module load (a value-not-reference capture, by Node module semantics). The new tests here mock the db layer directly (which the controller DOES reach via auth.getDb()), so they actually exercise the cross-tenant comparison. The other tenant-defense tests are still useful as routing/auth-fail smoke tests; they're just not what they look like.The fix doesn't add a unique constraint at the DB level to prevent the same class of bug from a direct INSERT (e.g. a future migration script that runs raw SQL). Defense-in-depth would be a CHECK constraint or trigger:
(teCompId, teCustId)must satisfyteCustId IN (SELECT custId FROM Customer WHERE custCompId = teCompId). Out of scope for this PR.Did NOT audit if existing data has cross-tenant teCustId rows from the time before this fix. Would need a one-shot SQL query:
SELECT te.* FROM TimeEntry te JOIN Customer c ON te.teCustId = c.custId WHERE te.teCompId != c.custCompId. Recommended as a follow-up.