fix: enforce tenant integrity and role-based authorization - #87
Open
rodrigobnogueira wants to merge 12 commits into
Open
fix: enforce tenant integrity and role-based authorization#87rodrigobnogueira wants to merge 12 commits into
rodrigobnogueira wants to merge 12 commits into
Conversation
createTask() inserted a task with a caller-supplied projectId without ever checking that the project lives in the current organization, so a task could be attached to another tenant's project; assignTask() scoped the task but not the assignee, so work could be handed to a user from another org. Both checks now run inside the same synchronous @transactional body, before any write, against the new MembershipsRepository.findByOrgAndUser() and the existing ProjectsRepository.findByIdInOrg(). Each refuses with exactly the error a nonexistent id gets, so the message is never a cross-tenant existence oracle.
Membership roles existed but authorized nothing: any authenticated caller could run users.invite — including minting another admin. @roles(...) declares the roles a procedure accepts and RolesGuard, composed after AuthGuard the ordinary Nest way (@UseGuards(AuthGuard, RolesGuard)), resolves the caller's membership in the ACTIVE organization from the database at request time. A missing membership (revoked) or a disallowed role is a ForbiddenException, so revocation lands on the next mutation instead of at token expiry. The policy stays small: users.invite is admin only, tasks create/assign/complete and projects.create accept admin or member, and reads declare no roles so they stay token-trusted.
The membership lookup had no ordering, so a user with more than one membership could land in a different tenant from one login to the next (whatever SQLite returned first). The oldest membership now wins, ordered by created_at with the id as the tiebreak.
The TTL was a raw Number.parseInt: a non-numeric value became NaN and signed tokens with exp: NaN (never verifiable), while zero or a negative value minted tokens that expire on arrival. It now goes through readIntFromEnv like every other positive-integer knob, so loadEnv() throws at boot.
The worker writes read-model rows the API process caches. With CACHE_SOCKET_PATH unset its tag invalidations never leave the worker, so API reads can stay stale until CACHE_TTL_MS lapses — a misdeploy that is silent today. One startup warning, no behaviour change.
…minism tenant-authz.spec.ts runs two organizations in one database: a cross-org projectId and a cross-org assignee must fail exactly like a missing one and commit nothing (no task row, no outbox event), and a user with two memberships must get the oldest one in the token on every login. roles-authz.spec.ts drives the real tRPC stack so the guard COMPOSITION is under test: a viewer cannot create/assign/complete tasks or create projects, a member cannot invite, an admin can invite another admin, and deleting a membership blocks that user's next mutation while their reads still pass. Both were verified by hand-mutation: dropping the checks, the ordering, or RolesGuard from the routers fails them.
…hash The README gains an auth/tenancy/roles section (the JWT snapshots one active organization; mutations re-check live membership, reads stay token-trusted until the TTL) plus an honest callout that password hashing is scryptSync and blocks the event loop under concurrent logins. The architecture tour gains an Authorization chapter next to Authentication and the matching lifecycle, layout, and test-table rows.
… input readAuthContext tried the tRPC context first and fell back to switchToHttp().getRequest(). That fallback is not HTTP-specific: getRequest() is getArgs()[0] whatever the transport is, and under tRPC args[0] is the caller's own input — so a procedure whose schema kept unknown keys would let a client hand AuthGuard (and now RolesGuard) an authContext of its choosing. Nothing is exploitable today: every procedure input is a z.object and zod strips unknown keys, and parser-less procedures get undefined. But an auth extractor must not rest on that, so it branches on context.getType() instead — args[1] for the 'rpc' contexts @nest-native/trpc creates, the request only for 'http'. auth-context.spec.ts pins both directions, including a forged authContext in args[0] resolving to undefined.
RolesGuard returned early for any procedure without @roles, so revocation only landed on writes. A deleted membership left the account a read-only insider for up to AUTH_TTL_SECONDS (default an hour): the member roster with everyone's email and role, every project, the activity feed, and POST /projects/:id/assistant, which streams an AI digest of that feed and bills tokens for it. The guard now resolves the membership whenever the token names an active organization and refuses when it is gone; @roles only narrows which roles may proceed. A caller with no organization at all still passes procedures that declare no roles (users.me), and auth.me keeps no RolesGuard because it just echoes the token back. The guard is added to the routers that were authenticated but unguarded — organizations, activity — and to the assistant controller. One indexed lookup per request, deliberately uncached. roles-authz.spec.ts now asserts the revoked member gets 403 from projects.list, users.list, activity.list and the assistant endpoint, and that logging in again still works. Verified by hand-mutation: restoring either the early return or the controller's old @UseGuards(AuthGuard) fails it.
…ccount users.invite upserted the invitee: an existing account was silently returned and given a membership in the caller's organization with the caller's chosen role. An org admin could therefore attach any account — including another tenant's admin — to their org without consent, and that account then satisfied every "is a member of this org" predicate, including the assignee check assignTask just gained. The invitee got no signal at all; the supplied initialPassword was quietly discarded. An invite now only ever creates a NEW account. An address that already has one is refused identically whether it is already a member here or belongs to another tenant, so the refusal never maps addresses to organizations. It does still reveal that an account exists — erasing that needs a pending-invitation row the invitee accepts, which the doc comment points at as the production shape. tenant-authz.spec.ts asserts the ACME admin cannot pull the RIVAL admin in, the two refusals are word-for-word identical, and nothing is written.
MembershipsRepository was registered through DrizzleModule.forFeature in five modules — auth, tasks, projects, users, onboarding — three of them added by this branch with a comment each explaining why the duplicate was there, while memberships.module.ts, whose whole job is to export that repository, sat unused and broken: it called forFeature() twice, and Nest 11 keys modules by object identity, so it exported a dynamic module the container never instantiated. That module now hoists the single forFeature() call into a constant reused by imports and exports, AuthModule imports and re-exports it, and the four duplicates go away. The guard's dependency travels with the guard: every module that already imports AuthModule for AuthGuard/RolesGuard gets the repository too, and there is now one instance of it in the app instead of five.
…ccount The auth section said reads stay token-trusted until the TTL; they no longer do, and the surface that changed is named: roster, projects, activity feed and the token-spending AI digest all go on the next request after a membership is revoked. Adds the invite rule (a new account only, so no admin can pull another tenant's user in), notes the residual account-exists signal, and records where the one MembershipsRepository registration lives and why forFeature() is hoisted into a constant. Lifecycle diagram, layout row and test table follow.
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
1 participant
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.
An external engineering evaluation of the portfolio (2026-08-13) flagged authorization/integrity gaps in this app. Every claim was verified against the code before fixing; this branch closes them.
Tenant relationship integrity
TasksService.createTask()inserted a task with a caller-suppliedprojectIdwithout proving the project belongs to the current org;assignTask()scoped the task but never the assignee. Both now check inside the same synchronous@Transactionalbody, before any write —ProjectsRepository.findByIdInOrg()and the newMembershipsRepository.findByOrgAndUser(). A refusal rolls back leaving no task row and no outbox event, and raises exactly the error a nonexistent id gets, so the message is never a cross-tenant existence oracle.users.invitecould attach any existing account — including another tenant's user — to the caller's org with a role of their choosing. An invite now only creates a new account (ConflictExceptionotherwise, same error whether the address is a member here or belongs to another tenant); attaching an existing account needs a consent flow, which a production app should model as a pending invitation.Role-based authorization
Roles existed but authorized nothing — any authenticated user could invite with
role: 'admin'. New@Roles(...)+RolesGuard(src/auth/) resolve the caller's membership in the active org from the database on every request:users.invite→admin;tasks.create/.assign/.completeandprojects.create→adminormember.@Rolesonly narrows which roles may proceed.readAuthContext()extractor branches on the transport and never falls back toswitchToHttp().getRequest()under tRPC — that argument is the caller's own procedure input, and an auth extractor must not read from it.No policy engine, no per-resource ACLs — three roles, composed the ordinary Nest way over tRPC procedures.
Smaller fixes
AUTH_TTL_SECONDSgoes throughreadIntFromEnv(a NaN TTL signed tokens withexp: NaN; zero/negative minted already-expired ones), with env spec cases.createdAt, thenid).start:workerwarns whenCACHE_SOCKET_PATHis unset: cross-process invalidation is off and app reads can stay stale up toCACHE_TTL_MS.scryptSyncblocks the event loop under concurrent logins.Tests
test/integration/tenant-authz.spec.ts(two-org fixtures: cross-org project id and cross-org assignee refused with the nonexistent-id error and no row/outbox write; login determinism),test/e2e/roles-authz.spec.ts(viewer/member/admin matrix over the real HTTP stack, revocation landing on reads including the assistant endpoint),test/integration/auth-context.spec.ts(a forgedauthContextplanted in procedure input resolves to nothing), plus the env cases. Each protection was kill-verified by hand-mutation: stubbing the check makes its test fail.Verification
npm test: 104 tests, 103 pass, 0 fail, 1 skipped (the live-Kafka spec self-skips without a broker)npm run infra:up+npm run test:full— base leg 103 pass / 1 skip, live-Kafka leg 1/1 pass (enqueue → Kafka → exactly one audit row after forced redelivery);infra:downcleannpm run typecheck,npm run build,npm run lint,npm run complexity:check: all clean