Skip to content

fix(tenant)!: refuse a caller-named tenantId under a resolver - #152

Open
msalvatti wants to merge 2 commits into
mainfrom
fix/tenant-body-refusal
Open

fix(tenant)!: refuse a caller-named tenantId under a resolver#152
msalvatti wants to merge 2 commits into
mainfrom
fix/tenant-body-refusal

Conversation

@msalvatti

Copy link
Copy Markdown
Member

Summary

A deployment that configures a TenantIdResolver used to accept a caller-supplied tenantId and discard it. The request answered 201 while the account was created under the resolved tenant, so the caller's belief about which tenant it registered into diverged from server state with nothing in the response saying so — the shape a security audit of a derived backend found on POST /register. No privilege crossed, which is why it survived: accept-and-ignore is the worst of the three available answers precisely because it looks like success.

A request that names a tenantId under a configured resolver is now refused with 400 auth.validation on that field. This is the paired half of nest-auth's resolveTenantId (released in its 1.4.2), down to the message bytes, so the two backends answer the same request the same way.

Changes

  • bymax-auth-core — the refusal. resolve_tenant returns AuthError::Validation on tenantId when a resolver is configured and the caller named a tenant. The refusal is evaluated before the resolver runs, so a caller cannot tell a refusal apart from a deployment whose resolver would have failed — otherwise the pair of answers would reveal whether this deployment can scope the request. null and an absent field are still accepted: they assert nothing to contradict. The message lives in one BODY_TENANT_REFUSAL constant, shared verbatim with nest-auth.
  • Coverage of all nine tenant-scoped flows. Eight read the value from the request body (register, login, verify-email, resend-verification, forgot-password, reset-password, verify-otp, resend-otp); the ninth, GET /oauth/{provider}, reads it from the query string — so dropping a body field is not the change to make there. Each flow is now driven twice in the tests: once naming a tenant (must refuse) and once silent (must reach the resolver), because the two halves fail independently.
  • npm client (@bymax-one/rust-auth). tenantId is optional on LoginInput, RegisterInput and ResetPasswordInput, and useAuth() no longer substitutes 'default' for a caller that named none. The invented value was a 400 against a resolver-configured backend and a tenant nobody chose against one without a resolver. The field is now omitted when absent, which lets one client serve both deployment shapes.
  • crates/bymax-auth-core/tests/wire_contract_identity.rs. Pins the SHA-256 of conformance/wire-contract.json, the file held byte-identical with nest-auth. The suites only ever read values out of it, so a change to a part nothing happens to read yet — or a reformat that rewrites every line while changing no value — passed unnoticed.
  • Docs. README, the technical specification (§5.7, §7.1.1, §24 invariant 8) and the rustdoc on AuthConfig::tenant_id_resolver, the TenantIdResolver trait and every DTO carrying the field, all restated from "the body is ignored" to the refusal.

How to verify

cargo fmt --all --check
cargo test --workspace
cargo clippy --workspace --all-targets --all-features
cd packages/rust-auth && npm run typecheck && npm run lint && npm run test:cov

Behaviorally, against a deployment with a TenantIdResolver configured:

  • POST /auth/register with {"tenantId": "anything", …}400 auth.validation, field: "tenantId" (was 201, account under the resolved tenant).
  • The same body with tenantId omitted or null → unchanged; the resolved tenant scopes the request.
  • GET /auth/oauth/google?tenantId=anything400 auth.validation before any state is minted, so nothing is left behind for a callback to redeem.
  • A deployment without a resolver is unaffected: the field stays required, and its absence is still refused rather than defaulted.

Notes

Two breaking changes, both detailed in CHANGELOG.md:

  1. Backends configuring a TenantIdResolver must stop sending tenantId on the nine endpoints above. All nine previously answered 200/201 while discarding the value.
  2. npm consumers relying on the implicit 'default' from useAuth().login / forgotPassword must now pass tenantId: 'default' explicitly.

On the contract pin — it does not gate cross-implementation divergence, and calling it that would be false. nest-auth can change the contract and its own constant in one commit: its suite is green, nothing moved here, so this suite is green too — bytes divergent. Two independent local hashes cannot enforce agreement between two repositories, because neither reads the other. What the pin does catch is an unaccompanied byte change inside this repository: an edit that forgets to re-measure, a formatter rewriting the file, a merge resolving it a third way. Closing the cross-repo gap for real needs a comparison that crosses the boundary, which is proposed, unbuilt, and a maintainer call on both ends. The test, its constant's doc, and its failure message all state that limit rather than implying enforcement.

Review focus: the placement of the refusal ahead of the resolver call in resolve_tenant, and whether auth.validation surfacing through anti_enumerating_outcome discloses more than intended — it now signals which of the two deployment shapes this is, which is deliberate (a client cannot be told to stop sending a field it is never allowed to hear about) but is the one new thing an unauthenticated caller can learn.

- A request naming `tenantId` on a deployment that configures a
  `TenantIdResolver` is now refused with `400 auth.validation` on that
  field, instead of being accepted and silently discarded. The discarded
  value answered `201` while the account landed under the resolved tenant,
  so the caller's belief about which tenant it registered into diverged
  from server state with nothing in the response saying so — found by a
  security audit of a derived backend on `POST /register`.
- The refusal is conditional and evaluated before the resolver runs:
  without a resolver the field is the only thing that can scope a request
  and stays required, and refusing ahead of resolution keeps a refusal
  indistinguishable from a resolver that could not scope the request.
  `null` and an absent field are still accepted — they assert nothing to
  contradict. Covers all nine tenant-scoped flows, including the OAuth
  initiate, which reads the value from the query string.
- Message held in one `BODY_TENANT_REFUSAL` constant and shared verbatim
  with nest-auth's `resolveTenantId`, so the two backends answer the same
  request with the same bytes.
- npm client: `tenantId` is optional on `LoginInput` / `RegisterInput` /
  `ResetPasswordInput`, and `useAuth()` no longer substitutes `'default'`
  for a caller that named no tenant — an invented value is a `400` against
  a resolver-configured backend and a tenant nobody chose against one
  without a resolver. A caller relying on the implicit default must now
  pass `tenantId: 'default'` explicitly.
- Pin the SHA-256 of `conformance/wire-contract.json`, so a byte change to
  the file shared with nest-auth has to be declared in the same commit.
  Two independent local hashes cannot detect the other repository moving —
  the test and its docs state that limit rather than implying enforcement.
Copilot AI balanced review requested due to automatic review settings August 14, 2026 20:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Aligns tenant resolution behavior across the Rust backend and npm client, refusing caller-supplied tenant IDs when a resolver is authoritative.

Changes:

  • Adds resolver-aware tenant refusal across scoped authentication flows.
  • Makes npm client tenant IDs optional and removes implicit defaults.
  • Updates contract checks, tests, and documentation.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
README.md Documents tenant refusal behavior.
packages/rust-auth/tests/react.test.tsx Tests omitted tenant IDs.
packages/rust-auth/src/react/index.ts Removes implicit default tenant.
packages/rust-auth/src/client/index.ts Makes tenant inputs optional.
docs/technical_specification.md Updates tenancy specification.
crates/bymax-auth-core/tests/wire_contract_identity.rs Pins contract-file bytes.
crates/bymax-auth-core/src/services/oauth.rs Applies refusal before OAuth state creation.
crates/bymax-auth-core/src/services/auth/register.rs Updates registration semantics documentation.
crates/bymax-auth-core/src/services/auth/password_reset.rs Documents refusal across reset flows.
crates/bymax-auth-core/src/services/auth/mod.rs Implements and tests tenant refusal.
crates/bymax-auth-core/src/services/auth/email_verification.rs Updates verification-flow semantics.
crates/bymax-auth-core/src/config/resolvers.rs Documents resolver behavior.
crates/bymax-auth-core/src/config/mod.rs Updates resolver configuration docs.
crates/bymax-auth-axum/tests/adapter.rs Adds password-reset contract tests.
crates/bymax-auth-axum/src/response.rs Documents validation disclosure behavior.
crates/bymax-auth-axum/src/dto.rs Updates tenant DTO documentation.
CHANGELOG.md Records breaking changes and contract pinning.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/bymax-auth-core/src/services/auth/mod.rs
Comment thread docs/technical_specification.md Outdated
- `register`, `login` and `reset_password` (confirm) now receive both a
  caller-named tenant and a silent one in the resolver-configured flow test,
  matching the five flows already covered there. The `resolve_tenant` helper
  test cannot stand in for them: each flow forwards its own
  `input.tenant_id`, so a call site that stopped forwarding it would leave
  the helper test green while that flow accepted a caller-named tenant
  again. `register` is where the audit found the divergence.
- Correct the tenant provenance in the specification. §6.2 stated the engine
  always passes the resolver's value "or the static default tenant" and
  never one read from the body; with no resolver configured it passes the
  caller's value verbatim and refuses its absence, and no static default
  exists anywhere in the engine. §5.7 carried the same claim one section
  earlier, which is where it originated. Both now separate the two
  deployment shapes instead of describing one.
Copilot AI review requested due to automatic review settings August 15, 2026 14:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (2)

crates/bymax-auth-core/src/services/auth/password_reset.rs:143

  • This behavior change leaves the public password-reset API docs contradictory: ForgotPasswordInput, VerifyResetOtpInput, and ResendResetOtpInput at lines 50-52, 99-101, and 112-114 still say a configured resolver “overrules” the caller value, and the helper docs at lines 161-164 and 302-306 say the same. Those values are now rejected before the resolver runs, so consumers can still be told to send a value that always fails. Update these rustdocs to describe the resolver-side refusal, consistent with the DTO docs changed in this PR.
        // when one is configured it is authoritative and a body that names a tenant is refused,
        // which is the whole anti-spoofing promise. Without it a caller on one tenant could drive

packages/rust-auth/src/client/index.ts:277

  • The central npm behavior promised here—omitting tenantId from the actual JSON body—is not covered by the client tests. Every createAuthClient test for login, register, reset-password, and forgot-password still supplies tenantId; the new React tests only exercise a fake client, so they cannot catch wire serialization regressions. Add requests without the field and assert that the captured JSON bodies have no tenantId key for these newly optional inputs.
  /** The tenant the account belongs to; omit under a resolver-configured backend. */
  tenantId?: string;

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants