docs(review): add multi-tenancy review criteria - #130
Conversation
New conventions/review/multi-tenancy.md — horizontal-authorization bars that keep an authenticated request confined to its own tenant, distinct from security.md's vertical trust boundary. Diff-anchored criteria for tenant-scoped data access, tenant-context trust, cache and config isolation, async tenant propagation, per-tenant fairness, and offboarding-deletion completeness, plus a boundary section delimiting ownership against security/performance/concurrency/observability. A cross-tenant read or write defaults to Critical. Criteria verified against AWS SaaS Lens, OWASP (API1:2023 BOLA + Authorization Cheat Sheet), and Azure multitenant architecture guidance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1x1KNkkjSe2CjbuTb4Gis
|
Claude finished @kyle-sexton's task in 0s —— View job I'll analyze this and get back to you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1560bdaf1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two review-bot corrections: - The dedicated-infrastructure carve-out suppressed all bars; narrow it to the data-scoping predicate so lifecycle/offboarding stays in scope for silo tenants (a dedicated store still needs deletion coverage). - Enumerable-identifier severity hinged on id shape; make the Critical case the missing/bypassed tenant check (owned above) and treat id opacity as defense-in-depth (Suggestion when the scope check is present). Addresses the codex-connector findings on #130. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1x1KNkkjSe2CjbuTb4Gis
|
Claude finished @kyle-sexton's task in 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f45eb492f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… boundary Two review-bot corrections: - The client-supplied-tenant bar condemned any client-derived tenant that governs scoping, which would reject a legitimate multi-tenant user selecting a tenant per request. Reframe the finding as the missing membership check: validated selection (principal authorized for the tenant, then context set) is fine. - The Boundaries section claimed security.md owns same-tenant object-level authorization; it has no such bar. Stop misdirecting reviewers — state that general within-tenant object authorization is outside this file's scope, without asserting a false owner. Addresses the codex-connector findings on #130. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1x1KNkkjSe2CjbuTb4Gis
|
Claude finished @kyle-sexton's task in 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3f390b55f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The blanket "cross-tenant read/write is Critical" wording would reject a legitimate, explicitly-authorized platform-admin or support flow that is designed to span tenants. Add the carve-out for a path that carries its own cross-tenant authorization and audit controls. Addresses the codex-connector finding on #130. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1x1KNkkjSe2CjbuTb4Gis
|
Claude finished @kyle-sexton's task in 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6a99a1c76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The client-supplied-tenant and async-boundary bars both flagged a tenant value used only for telemetry (routing/logging, correlation) as an Important multi-tenancy finding — contradicting this file's own Boundaries section, which already assigns trace/log tenant correlation to observability.md and reserves this file for the scoping failure. - multi-tenancy.md — drop the telemetry-only case from both bars; point to observability.md instead of restating its concern. - observability.md — add the tenant-correlation-tag bar under Semantic conventions and correlation so the boundary this file already claims is actually backed by a criterion. Addresses the codex-connector finding on #130.
|
Claude finished @kyle-sexton's task in 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56c2b04bd5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The tenant-correlation-tag bar flagged metrics missing the tenant identifier alongside logs and traces, contradicting this same file's Bounded tag cardinality bar, which already carves unbounded identities (user identities, raw URLs, request identities) out of metric tags and into logs/traces. In a SaaS environment with many or unbounded tenants, a raw tenant id on a metric tag is exactly that unbounded- identity case and risks a cardinality explosion. - observability.md — scope the bar to logs/traces; metrics correlate through their log/trace context or a bounded tenant grouping (tier, plan, region) instead of the raw id. Addresses the chatgpt-codex-connector P2 finding on #130.
|
Claude finished @kyle-sexton's task in 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46afa4e80f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…alidation The client-supplied-tenant bar flagged any routing-only tenant hint as merely Important, even when the routing decision itself reaches tenant resources with no downstream authorization. A reverse-proxy or gateway hint used only to pick a backend before authentication has no authenticated principal to validate against at that ingress point — the safety property is that the routed application still authorizes the request for that tenant before touching data. Narrow the bar: Important only when the routed destination performs its own tenant check; Critical when a routing decision can reach tenant resources with no such downstream validation. Addresses the codex-connector finding on #130.
|
Claude finished @kyle-sexton's task in 0s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a1a12d24d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The prior wording flagged legitimate cross-tenant admin/support/ background paths that correctly decrypt with the data tenant's key rather than the requester's. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1x1KNkkjSe2CjbuTb4Gis
|
Claude finished @kyle-sexton's task in 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1112ab6cad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…dedicated The carve-out's `container` example made compute isolation alone sound sufficient. A per-tenant container or app stamp can still query a pooled database, cache, queue, or index; that shared store still needs the tenant predicate. Key the carve-out off the data store itself being tenant-dedicated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1x1KNkkjSe2CjbuTb4Gis
…ancy # Conflicts: # conventions/review/observability.md
|
Claude finished @kyle-sexton's task in 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83c5a00761
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ancy # Conflicts: # conventions/review/observability.md
…docs/review-multi-tenancy
|
Claude finished @kyle-sexton's task in 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 319c94357b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The client-supplied-tenant bar treated any `?? defaultTenant` fallback as the same defect as an unauthorized client-controlled tenant value, which would flag a legitimate pattern like `requestedTenant ?? principal.defaultTenant`. Narrow the criterion: only a fallback to a global or another client-controlled value is the same defect. A fallback to a tenant derived from the authenticated principal or session is safe once membership is confirmed before the default reaches data access — the missing check is the defect, not the default itself. Addresses the codex-connector finding on #130. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The provisioning-defaults bar capped onboarding that seeds a new tenant with global-scope or another tenant's access at Important, even though this file's own default treats a cross-tenant read or write as Critical. A copied default role that carries cross-tenant permissions is that same break, immediately exposing or letting the new tenant mutate other tenants' data. Keep Important for genuinely harmless copied defaults with no access implications; make an access- or data-bearing copied default Critical, matching the file's established severity convention. Addresses the codex-connector finding on #130. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 29s —— View job Review —
|
## Summary - Adds an **Object-level authorization** section to `security.md` covering the same-tenant BOLA/IDOR case: a request that carries an object id checked for well-formedness but not for the caller's authorization over that specific record. - Fills a gap surfaced during dogfood on #130: `multi-tenancy.md`'s own Boundaries section explicitly scopes out "one user reaching another's record inside the same tenant" as outside its file — `security.md` owned no object-level/ownership authorization check to pick it up. - Sourced from OWASP API1:2023 (Broken Object Level Authorization), the OWASP Authorization Cheat Sheet, and CWE-639; adds `security.md`'s first `## Sources` section. - Cross-references `multi-tenancy.md`'s "Fetch-by-id without ownership" bullet for the cross-tenant analog, using a code-span sibling ref (that PR, #130, has not yet merged to `main` as of this branch). ## Test plan - [x] `lefthook` pre-commit (typos, gitleaks, editorconfig, markdownlint) — passed locally. - [ ] CI green (markdown lint, lychee, review bot). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
New
conventions/review/multi-tenancy.md— diff-time review bars for a multi-tenant (SaaS) system, organized around horizontal authorization: keeping an already-authenticated request confined to its own tenant.security.mdowns the vertical boundary (authn, sanitization, injection); this file owns everything downstream of "we know who is calling."Criteria (all diff-anchored; a cross-tenant read/write defaults to Critical):
security.md/performance.md/concurrency.md/observability.md.Registered in the review README (placed by
security.md, away from the code-design row in #129 to avoid a README merge conflict between the two held PRs).Sourcing
Verified against AWS Well-Architected SaaS Lens (isolation mindset), OWASP (API1:2023 Broken Object Level Authorization + Authorization Cheat Sheet), and the Azure multitenant architecture guides. Dropped non-diff-reviewable candidates (tenancy-model choice, capacity planning, cost metering) by design; per-tenant encryption kept as an explicitly conditional bar.
Test plan
lefthookpre-commit — typos, editorconfig, gitleaks, markdownlint passlycheewill validate the source URLs🤖 Generated with Claude Code