Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 95 additions & 0 deletions docs/architecture/adrs/0165-login-never-answers-404.es.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
# ADR-0165: El Login Nunca Responde 404

**Estado:** Aceptado
**Fecha:** 2026-08-10
**Responsable de Decisión:** Arquitectura
**Relacionado:** [ADR-0072](./0072-dynamic-auth-method-resolution.es.md) · G-053 (anti-enumeración en `/client/authenticate`)

---

## Contexto

`MapAuthError` traducía dos fallos del handler de autenticación a `404 Not Found`:

| Código | Significado | Estado anterior |
| --- | --- | --- |
| `AUTH_002` | El código de tenant no existe | 404 |
| `AUTH_004` | El usuario no existe en ese tenant | 404 |
| `AUTH_006` | Las credenciales no autentican | 401 |

Los mensajes de `AUTH_004` y `AUTH_006` son **idénticos palabra por palabra**: «No pudimos iniciar
sesión. Verifique sus credenciales.» Alguien los escribió así deliberadamente, para que quien
intenta entrar no pueda distinguir «ese usuario no existe» de «existe y te equivocaste». El estado
HTTP deshacía ese cuidado: bastaba mirar si la respuesta era 404 o 401 para saber cuál de las dos
cosas había pasado, sin leer el cuerpo.

Eso es un **oráculo de enumeración**: un atacante sin credenciales puede recorrer una lista de
correos contra `POST /api/v1/auth/login` y separar los que existen de los que no, a razón de una
petición por candidato. Lo que se obtiene no es acceso, pero sí la mitad del trabajo: una lista
depurada de cuentas reales sobre la que concentrar el resto del esfuerzo. Y la mitad cara, porque
es la que no se puede adivinar.

El desajuste salió a la luz por otro camino: la verificación de contrato Pact esperaba `400` para
un tenant inexistente y la API respondía `404`. El contrato tenía razón por accidente.

## Decisión

### §2.1 — Ninguna rama del login devuelve 404

`MapAuthError` no produce `404` bajo ninguna circunstancia. Un 404 en la ruta de autenticación
responde una pregunta que quien pregunta no ha demostrado tener derecho a hacer.

### §2.2 — Todo lo que huele a credencial responde 401

`AUTH_004` (usuario inexistente) pasa a `401`, el mismo estado que `AUTH_006` (credenciales
inválidas) y que `AUTH_005` (cuenta inactiva). Las tres respuestas son ahora indistinguibles desde
fuera salvo por el `code` del cuerpo, que es información que ya se entrega a quien la pide de
frente y que las pantallas necesitan para elegir el mensaje.

> El `code` sigue diferenciándolas. Eso es deliberado y no reabre el oráculo en la práctica: quien
> enumera lo hace a escala y automatizado, y el cuerpo ya viaja con el mensaje que lo delataría de
> todos modos. Lo que se cierra aquí es la señal **barata** —el estado, legible sin parsear nada—.
> Colapsar también el cuerpo es una decisión distinta, con coste real de usabilidad, y no se toma
> en este ADR.

### §2.3 — El código de tenant responde 400, y esa disclosure YA existía

`AUTH_002` pasa a `400 Bad Request`, no a 401. La razón no es que dé igual, sino que en este
endpoint **el cuerpo ya lo revela**: el mensaje es «Verifique el código del tenant», distinto del
de credenciales y elegido así para que quien teclea mal el código de su organización lo sepa.
Cambiar el estado a 401 no ocultaría nada que el cuerpo no siga diciendo. `400` describe con
precisión lo que pasó —la petición trae un código que no existe— y es lo que el contrato espera.

> **Contraste deliberado con G-053.** El endpoint de máquina `POST /api/v1/client/authenticate`
> **sí** colapsa `AUTH_002` a 401 *y* a un mensaje genérico e indistinguible del de credenciales
> (`ClientAuthEndpoints.GetStatusCode` / `TraducirMensaje`). Allí no hay ningún humano que
> necesite el aviso, así que la enumeración se cierra a todos los niveles.
>
> Que `/auth/login` revele la existencia del tenant es una decisión de producto **anterior a este
> ADR**, que aquí no se cambia: este ADR retira la filtración por estado donde el cuerpo no
> filtraba ya (§2.2), y no toca la que el producto hace a propósito. Si se decidiera que el portal
> tampoco debe revelarlo, el cambio es de mensaje y de estado a la vez —extender G-053 a
> `/auth/login`— y merece su propio ADR y su propia conversación con producto. Queda señalado
> aquí para que la asimetría no se lea como olvido.

## Consecuencias

- El front no cambia: `auth.service.ts` decide el mensaje por el `code` del cuerpo, no por el
estado. La única rama que mira el estado es el respaldo para cuando no hay cuerpo, y sigue
tratando `401` como «verifique sus credenciales».
- El pacto `ums-web-app ↔ ums-api` se regenera: el cuerpo de error de esta API es
`LoginErrorResponse` —`{ code, message, supportReferenceId }`—, no `ProblemDetails`. El pacto
declaraba `{ status, title }`, la forma por defecto de ASP.NET, contra la que este endpoint
dejó de responder hace tiempo.
- Quien lea `MapAuthError` en el futuro verá 401 donde el instinto REST pediría 404. El comentario
del método explica por qué, y este ADR es la razón larga: **no es un descuido, es la decisión**.

## Alternativas descartadas

- **Dejar `AUTH_002` en 404 y mover sólo `AUTH_004`.** Cierra el oráculo de cuentas, que es el que
importa, pero deja el contrato Pact roto y obliga a explicar por qué un endpoint de
autenticación devuelve 404 para una cosa y 401 para otra sin que la diferencia sea la
sensibilidad del dato.
- **Colapsar `AUTH_002`, `AUTH_004` y `AUTH_006` en un único 401 sin `code`.** Es lo más hermético
y lo peor de usar: quien teclea mal el código de su organización merece que se lo digan. Se
descarta por coste de usabilidad frente a una ganancia que §2.2 ya obtiene en su mayor parte.
95 changes: 95 additions & 0 deletions docs/architecture/adrs/0165-login-never-answers-404.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
# ADR-0165: Login Never Answers 404

**Status:** Accepted
**Date:** 2026-08-10
**Decision Owner:** Architecture
**Related:** [ADR-0072](./0072-dynamic-auth-method-resolution.md) · G-053 (anti-enumeration on `/client/authenticate`)

---

## Context

`MapAuthError` translated two authentication-handler failures into `404 Not Found`:

| Code | Meaning | Previous status |
| --- | --- | --- |
| `AUTH_002` | The tenant code does not exist | 404 |
| `AUTH_004` | The user does not exist in that tenant | 404 |
| `AUTH_006` | The credentials do not authenticate | 401 |

The messages for `AUTH_004` and `AUTH_006` are **word-for-word identical**: "No pudimos iniciar
sesión. Verifique sus credenciales." Someone wrote them that way on purpose, so that whoever is
trying to sign in cannot tell "that user does not exist" from "it exists and you got it wrong."
The HTTP status undid that care: looking at whether the response was 404 or 401 was enough to know
which of the two had happened, without reading the body at all.

That is an **enumeration oracle**: an attacker with no credentials can run a list of email
addresses against `POST /api/v1/auth/login` and separate the ones that exist from the ones that do
not, one request per candidate. What that yields is not access, but it is half the work — and the
expensive half, because it is the half you cannot guess.

The mismatch surfaced from another direction: the Pact provider verification expected `400` for an
unknown tenant and the API answered `404`. The contract happened to be right.

## Decision

### §2.1 — No login branch returns 404

`MapAuthError` produces `404` under no circumstances. A 404 on the authentication path answers a
question the asker has not shown any right to ask.

### §2.2 — Anything credential-shaped answers 401

`AUTH_004` (user does not exist) becomes `401`, the same status as `AUTH_006` (invalid credentials)
and `AUTH_005` (inactive account). The three responses are now indistinguishable from outside
except by the body's `code`, which is information already handed to anyone who asks directly and
which the screens need in order to choose their message.

> The `code` still tells them apart. That is deliberate and does not reopen the oracle in practice:
> enumeration is done at scale and automated, and the body already carries the message that would
> give it away anyway. What closes here is the **cheap** signal — the status, readable without
> parsing anything. Collapsing the body as well is a different decision, with a real usability
> cost, and is not taken in this ADR.

### §2.3 — The tenant code answers 400, and that disclosure ALREADY existed

`AUTH_002` becomes `400 Bad Request`, not 401. The reason is not that it does not matter, but that
on this endpoint **the body already reveals it**: the message is "check the tenant code," distinct
from the credentials one and chosen that way so that someone who mistypes their organisation's code
finds out. Changing the status to 401 would hide nothing the body keeps saying. `400` describes
precisely what happened — the request carries a code that does not exist — and it is what the
contract expects.

> **Deliberate contrast with G-053.** The machine endpoint `POST /api/v1/client/authenticate`
> **does** collapse `AUTH_002` to 401 *and* to a generic message indistinguishable from the
> credentials one (`ClientAuthEndpoints.GetStatusCode` / `TraducirMensaje`). There is no human
> there who needs the hint, so enumeration is closed at every level.
>
> That `/auth/login` reveals tenant existence is a product decision **predating this ADR**, and it
> is not changed here: this ADR removes the status-level leak where the body was not already
> leaking (§2.2), and leaves alone the one the product makes on purpose. If it were decided that
> the portal should not reveal it either, the change is message and status together — extending
> G-053 to `/auth/login` — and deserves its own ADR and its own conversation with product. It is
> flagged here so the asymmetry does not read as an oversight.

## Consequences

- The front end does not change: `auth.service.ts` picks its message from the body's `code`, not
from the status. The only branch that reads the status is the fallback for when there is no
body, and it still treats `401` as "check your credentials."
- The `ums-web-app ↔ ums-api` pact is regenerated: this API's error body is `LoginErrorResponse` —
`{ code, message, supportReferenceId }` — not `ProblemDetails`. The pact declared
`{ status, title }`, ASP.NET's default shape, which this endpoint stopped producing long ago.
- Anyone reading `MapAuthError` in the future will see 401 where REST instinct would ask for 404.
The method's comment says why, and this ADR is the long form: **it is not an oversight, it is the
decision**.

## Alternatives rejected

- **Leave `AUTH_002` at 404 and move only `AUTH_004`.** Closes the account oracle, which is the one
that matters, but leaves the Pact contract broken and forces an explanation of why an
authentication endpoint returns 404 for one thing and 401 for another when the difference is not
the sensitivity of the data.
- **Collapse `AUTH_002`, `AUTH_004` and `AUTH_006` into a single 401 with no `code`.** The most
airtight and the worst to use: someone who mistypes their organisation's code deserves to be told
so. Rejected on usability cost against a gain that §2.2 already captures for the most part.
1 change: 1 addition & 0 deletions docs/architecture/adrs/index.es.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ UMS es un repositorio satelite de `evolith_arch32`. El repositorio padre define
| [ADR-0082](./0082-postgresql-authoritative-persistence-baseline.es.md) | Linea base autoritativa de persistencia PostgreSQL | Aceptado |
| [ADR-0090](./0090-recursive-menu-node-tree.es.md) | El Árbol Recursivo MenuNode Sustituye a Menu/SubMenu/Option | Aceptado |
| [ADR-0164](./0164-branch-closure-is-terminal.es.md) | Cerrar una Sucursal Es Terminal y Lógico | Aceptado |
| [ADR-0165](./0165-login-never-answers-404.es.md) | El Login Nunca Responde 404 | Aceptado |

---

Expand Down
3 changes: 2 additions & 1 deletion docs/architecture/adrs/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,13 @@ UMS is a satellite repository of `evolith_arch32`. The parent repository defines
| [ADR-0082](./0082-postgresql-authoritative-persistence-baseline.md) | PostgreSQL Authoritative Persistence Baseline | Accepted |
| [ADR-0090](./0090-recursive-menu-node-tree.md) | Recursive MenuNode Tree Replaces Menu/SubMenu/Option | Accepted |
| [ADR-0164](./0164-branch-closure-is-terminal.md) | Branch Closure Is Terminal and Logical | Accepted |
| [ADR-0165](./0165-login-never-answers-404.md) | Login Never Answers 404 | Accepted |

---

## Bilingual Coverage (R-01 Compliance)

All ADRs (0050-0082) have Spanish translations, as do ADR-0090 and ADR-0164:
All ADRs (0050-0082) have Spanish translations, as do ADR-0090, ADR-0164 and ADR-0165:

| ADR | Spanish | ADR | Spanish |
|-----|---------|-----|---------|
Expand Down
25 changes: 25 additions & 0 deletions docs/architecture/technical-debt.md
Original file line number Diff line number Diff line change
Expand Up @@ -169,3 +169,28 @@ What the gate had been hiding — each of these is a live defect, not a type-ann
- **Impact**: the test runner and every plugin are compiled against a different major than the bundler that produces `dist/`. Nothing observable today; it is the kind of skew that produces an unreproducible failure later.
- **Suggested resolution**: converge on one major. `@vitejs/plugin-react@5.2.0` and `vitest@4.1.7` both accept 8, so raising the `app-web` pin is the smaller move — but it is a bundler major bump and deserves its own change, its own build verification and its own rollback story, not a corner of a type-cleanup branch.
- **Caveat**: this monorepo's `package-lock.json` must not be regenerated from scratch (it drops transitive deps); reconcile against the existing lock.

---

## [TD-009] The Login Pact Verified a Body the API Stopped Sending — and a 404 That Should Never Have Been There

- **Status**: **Resolved** (2026-08-10)
- **Severity**: High (the 404 half); Medium (the contract half)
- **Component**: `src/apps/ums.api/Ums.ContractTest/Consumers/AuthConsumerTests.cs`, `src/apps/ums.api/Ums.Presentation/Endpoints/Identity/Auth/AuthEndpoints.cs`
- **Description**: `Ums.ContractTest` failed 37/38 from the moment of the resync and stayed red on `main` (`c011e42a`). Two failures, one interaction each, both on `POST /api/v1/auth/login`:
1. The pact expected `{ status, title }` — ASP.NET's default `ProblemDetails` — and the API returns `LoginErrorResponse`: `{ code, message, supportReferenceId }`.
2. The pact expected `400` for an unknown tenant and the API answered `404`.
- **What the second one actually was**: not a contract nicety. `AUTH_002` (unknown tenant) and `AUTH_004` (unknown user) both returned `404`, while `AUTH_006` (bad credentials) returned `401`. The messages for `AUTH_004` and `AUTH_006` are word-for-word identical — deliberately, so that nobody can tell "that account does not exist" from "wrong password". The status code gave away exactly what the message was written to hide, which makes the login endpoint an **enumeration oracle**: one request per candidate email separates real accounts from invented ones.
- **Why it went unnoticed**: the contract suite had been red since the resync, so a genuine finding was sitting inside a check everyone had learned to expect red. The pact was right and nobody read it.
- **A second, quieter defect in the same file**: the `Content-Type` assertion used `Match.Type("application/problem+json")`. A type matcher on a string matches **any** string, so that line reported OK against a content type the API no longer sends. The check existed and checked nothing — the same shape as the GraphQL-key finding in TD-004.

### Resolution

- The pact is regenerated against the envelope the API actually emits. `supportReferenceId` is deliberate — it is the only thing that lets a user's complaint be joined to a server trace — so the contract moves to the API, not the other way round.
- `Content-Type` is now asserted by exact value.
- `AUTH_002` → `400`; `AUTH_004` → `401`, indistinguishable from `AUTH_006`. **No login branch returns 404.** Recorded as [ADR-0165](./adrs/0165-login-never-answers-404.md), because 401-where-REST-would-say-404 is the kind of thing a future reader corrects back unless the reason is written down.
- The front end needed no change: `auth.service.ts` selects its message from the body's `code`, never from the status.

- **Verified**: `Ums.ContractTest` 38/38.
- **Still owed**: report upstream — `unimar-ums` has the same `MapAuthError`.

29 changes: 20 additions & 9 deletions src/apps/ums.api/Ums.ContractTest/Consumers/AuthConsumerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,18 @@ namespace Ums.ContractTest.Consumers;
/// El endpoint real de inicio de sesión es <c>POST /api/v1/auth/login</c> y recibe
/// <c>{ tenantCode, username, password }</c> (ADR-0096 / AuthenticateUserCommand). Aquí sólo se
/// contrasta la FORMA HTTP de las rutas de rechazo, que no requieren sembrar un grafo de
/// autorización completo: la API responde <c>400 application/problem+json</c> ante credenciales
/// inválidas o campos ausentes. El camino feliz (200 con el grafo + cookie de sesión) depende de
/// un grafo sembrado que excede el alcance de un contrato de forma y no se modela aquí.
/// autorización completo. El camino feliz (200 con el grafo + cookie de sesión) depende de un
/// grafo sembrado que excede el alcance de un contrato de forma y no se modela aquí.
///
/// <para>El cuerpo de error NO es ProblemDetails: es <c>LoginErrorResponse</c>
/// —<c>{ code, message, supportReferenceId }</c>—. El pacto declaraba <c>{ status, title }</c>, la
/// forma por defecto de ASP.NET, contra la que esta API dejó de responder; el `supportReferenceId`
/// es deliberado y es lo único que permite cruzar la queja de una persona con la traza del
/// servidor, así que el contrato se corrige hacia la API y no al revés.</para>
///
/// <para>El <c>Content-Type</c> se afirma por valor exacto. Con <c>Match.Type</c> —una plantilla de
/// cadena— la verificación pasaba con CUALQUIER content type, incluido el que la API dejó de
/// enviar: la comprobación existía y no comprobaba nada.</para>
/// </summary>
public sealed class AuthConsumerTests : IDisposable
{
Expand Down Expand Up @@ -56,11 +65,12 @@ public async Task PostLogin_WithMissingFields_Returns400()
})
.WillRespond()
.WithStatus(HttpStatusCode.BadRequest)
.WithHeader("Content-Type", Match.Type("application/problem+json"))
.WithHeader("Content-Type", "application/json; charset=utf-8")
.WithJsonBody(new
{
status = Match.Type(400),
title = Match.Type("Bad Request"),
code = "AUTH_001",
message = Match.Type("Tenant code, username and password are required."),
supportReferenceId = Match.Type("TX-2026-000001"),
});

await _pactBuilder.VerifyAsync(async ctx =>
Expand Down Expand Up @@ -100,11 +110,12 @@ public async Task PostLogin_WithUnauthenticableCredentials_Returns400()
})
.WillRespond()
.WithStatus(HttpStatusCode.BadRequest)
.WithHeader("Content-Type", Match.Type("application/problem+json"))
.WithHeader("Content-Type", "application/json; charset=utf-8")
.WithJsonBody(new
{
status = Match.Type(400),
title = Match.Type("Bad Request"),
code = "AUTH_002",
message = Match.Type("No pudimos iniciar sesión. Verifique el código del tenant."),
supportReferenceId = Match.Type("TX-2026-000001"),
});

await _pactBuilder.VerifyAsync(async ctx =>
Expand Down
Loading
Loading