The admin API has no application-layer authorization — it depends entirely on one proxy regex #63

New Issue

2026-08-19T11:06:01-05:00

bermudalamb commented

2026-08-19 11:06:01 -05:00

Found during a Microsoft/React/SonarQube best-practices review.

What is actually true

To be precise, because this is easy to overstate: the admin API is protected today. Nginx Proxy Manager wraps auth_request around location ~ ^/(admin|api/admin), so both the admin UI and the admin API sit behind authentik in the deployed environment. Nothing is publicly exposed right now.

The finding

There is no authorization check anywhere in the Express application. app.ts mounts five admin routers with no guard:

app.use('/api/admin/customers', adminCustomersRouter);
app.use('/api/admin/settings',  adminSettingsRouter);
app.use('/api/admin/categories', adminCategoriesRouter);
app.use('/api/admin/tags',      adminTagsRouter);
app.use('/api/admin',           adminRouter);

There is no requireAdmin middleware; the only auth middleware in the codebase is requireCustomer, which these routes do not use. Every integration test reaches /api/admin/items with no credentials, which is consistent — there is nothing to satisfy.

So the entire administrative surface — creating and deleting items, marking sold, reading the customer list with email addresses and spend history, disabling accounts, changing settings — is authorized by a single regex in a reverse-proxy configuration that lives outside this repository, is not covered by any test, and is not reviewed when this code changes.

The project context already names this as fragile: "don't accidentally widen or narrow that regex without checking both directions." That instruction exists because the consequence of getting it wrong is silent and total.

Concrete ways this bites without anybody making an obvious mistake

A route added under a path that does not match ^/(admin|api/admin) — say /api/reports or /api/internal/... — is unprotected the moment it is written, and nothing in the codebase indicates that.
Anything reaching the container directly rather than through NPM bypasses authentik entirely. The QA stack publishes port 32751 on the NAS; production publishes its own.
Locally, /admin and the whole admin API are wide open by design, so no developer ever sees the boundary being enforced.

Suggested approach

Defense in depth: a requireAdmin middleware in the application, so authorization does not rest solely on infrastructure outside the repo.

Worth deciding, because it is not obvious:

What the application should trust. The clean version is having NPM/authentik forward a signed header or JWT that the app verifies, rather than the app trusting a header any client could set. A naive "trust X-Forwarded-User" would be worse than the status quo, since it would look like a check while being trivially forgeable.
How local development and tests authenticate. Today they simply do not, and every integration and e2e test calls admin endpoints unauthenticated. Introducing a check means deciding what those get — a dev-only bypass gated on NODE_ENV, or a test credential. This has real reach: it touches most of the test suite.
Whether this is worth doing at all given the proxy does work. A reasonable answer is "no, but write down the dependency" — in which case the outcome should be an explicit note in the README next to the route table, rather than nothing.

Severity

Medium as it stands, because the proxy genuinely does protect it. The risk is entirely in the fragility of a single external, untested, invisible-from-the-code control — and in how quietly it fails.

Found during a Microsoft/React/SonarQube best-practices review. ## What is actually true To be precise, because this is easy to overstate: **the admin API is protected today.** Nginx Proxy Manager wraps `auth_request` around `location ~ ^/(admin|api/admin)`, so both the admin UI and the admin API sit behind authentik in the deployed environment. Nothing is publicly exposed right now. ## The finding There is **no authorization check anywhere in the Express application**. `app.ts` mounts five admin routers with no guard: ``` app.use('/api/admin/customers', adminCustomersRouter); app.use('/api/admin/settings', adminSettingsRouter); app.use('/api/admin/categories', adminCategoriesRouter); app.use('/api/admin/tags', adminTagsRouter); app.use('/api/admin', adminRouter); ``` There is no `requireAdmin` middleware; the only auth middleware in the codebase is `requireCustomer`, which these routes do not use. Every integration test reaches `/api/admin/items` with no credentials, which is consistent — there is nothing to satisfy. So the entire administrative surface — creating and deleting items, marking sold, reading the customer list with email addresses and spend history, disabling accounts, changing settings — is authorized by a single regex in a reverse-proxy configuration that **lives outside this repository**, is not covered by any test, and is not reviewed when this code changes. The project context already names this as fragile: *"don't accidentally widen or narrow that regex without checking both directions."* That instruction exists because the consequence of getting it wrong is silent and total. ## Concrete ways this bites without anybody making an obvious mistake - A route added under a path that does not match `^/(admin|api/admin)` — say `/api/reports` or `/api/internal/...` — is unprotected the moment it is written, and nothing in the codebase indicates that. - Anything reaching the container directly rather than through NPM bypasses authentik entirely. The QA stack publishes port 32751 on the NAS; production publishes its own. - Locally, `/admin` and the whole admin API are wide open by design, so no developer ever sees the boundary being enforced. ## Suggested approach Defense in depth: a `requireAdmin` middleware in the application, so authorization does not rest solely on infrastructure outside the repo. Worth deciding, because it is not obvious: - **What the application should trust.** The clean version is having NPM/authentik forward a signed header or JWT that the app verifies, rather than the app trusting a header any client could set. A naive "trust `X-Forwarded-User`" would be worse than the status quo, since it would look like a check while being trivially forgeable. - **How local development and tests authenticate.** Today they simply do not, and every integration and e2e test calls admin endpoints unauthenticated. Introducing a check means deciding what those get — a dev-only bypass gated on `NODE_ENV`, or a test credential. This has real reach: it touches most of the test suite. - **Whether this is worth doing at all** given the proxy does work. A reasonable answer is "no, but write down the dependency" — in which case the outcome should be an explicit note in the README next to the route table, rather than nothing. ## Severity Medium as it stands, because the proxy genuinely does protect it. The risk is entirely in the fragility of a single external, untested, invisible-from-the-code control — and in how quietly it fails.

bermudalamb added this to the Code Quality and Hardening project 2026-08-19 11:38:12 -05:00

bermudalamb commented

2026-08-21 10:43:02 -05:00

Implemented on `feature/63-admin-gate` — one commit, not pushed

Both halves of what this issue asked for: the defence-in-depth check, and the written-down dependency it offered as the alternative. The documentation is worth having regardless of the code, since the coupling is invisible from Express.

Decisions, and the option this issue raised that was rejected

The issue warned that a naive "trust X-Forwarded-User" would be worse than the status quo — it would look like a check while being trivially forgeable by anything reaching the container port. That is correct, and it rules out trusting authentik's identity headers directly. Verifying a signed JWT would fix it properly and identify which admin is acting, but needs authentik client configuration outside this repo, JWKS handling in the app, and an answer for what 113 test call sites present as a token. Recorded as the better long-term answer, not taken here.

What landed is a shared secret between the proxy and the app: it does not authenticate who, only that the request came through the gate.

`ADMIN_GATE_SECRET`	Behaviour
Unset	Every admin route reachable — exactly today. A `[admin-gate]` warning at boot names what is unprotected.
Set	`X-Admin-Gate` must match, or 403.

Why it is attached to the routers, not a path prefix

This is the part that makes it more than redundant with the proxy, and it answers this issue's first concrete concern directly.

An admin router added later at, say, /api/reports inherits the gate. The proxy only injects the header on paths its regex matches, so that router refuses on its first request rather than shipping quietly public. A 403 in that situation is the boundary reporting that it has drifted, which is the opposite of the current failure mode where nothing says anything at all.

Optional, and loud about being optional

Unset means today's behaviour, which is what keeps local development and all 113 existing admin call sites working untouched, and means shipping the image before configuring the proxy cannot take the admin panel down.

What it does not do is stay quiet. The server warns at boot:

[admin-gate] ADMIN_GATE_SECRET is not set — /api/admin is protected only by the reverse proxy.
Anything able to reach this container directly can administer the store.

Given how many times this project has met a control that reports success while doing nothing — #67, #60, #61, the new-code period in #79, the IPv6 key in #84 — an unconfigured gate should be a visible choice rather than an invisible one.

Two details that are the difference between a gate and the appearance of one. An empty value is treated as unset, because enforcing an empty secret would admit any caller sending an empty header. And comparison is timing-safe over SHA-256 digests of both sides: timingSafeEqual throws on buffers of unequal length, so comparing raw values would turn a short header into a 500 rather than a 403, and a length check first would leak the secret's length.

To turn it on

The secret must be set in two places at once:

ADMIN_GATE_SECRET in the stack environment.
proxy_set_header X-Admin-Gate "<secret>"; on the gated location in Nginx Proxy Manager — the ^/(admin|api/admin) location in production, and location / in QA where the whole site is gated.

Setting only one gives 403s until the other catches up. Loud and recoverable, unlike the failure it replaces — but worth doing in one sitting.

Verification

Over real HTTP, not only in tests:

Case	Result
Boot with no secret	warning logged, `GET /api/admin/items` → 200
Boot with secret, no header	403
Boot with secret, wrong header	403
Boot with secret, correct header	200
Public storefront throughout	200

Each refusal is logged distinguishably (no vs incorrect header) without echoing the value it was sent, because a 403 from behind a proxy is otherwise very hard to diagnose — it usually means the proxy config and the stack secret have drifted.

8 unit tests on the decision, 9 integration tests on the mounting — including one case per admin router, because these are five separate app.use calls and one of them missing the middleware would be a silent and easy mistake. A correct middleware nobody mounted would pass every unit test and protect nothing.

106 unit and 153 integration passing, lint 0 errors and 8 warnings unchanged, build clean.

README

A new "The admin authorization boundary" section beside the deployment notes records the regex, the three consequences that are invisible from the code, the two-place coupling, and what a 403 means when it appears.

Not pushed, per the usual arrangement.

## Implemented on `feature/63-admin-gate` — one commit, not pushed Both halves of what this issue asked for: the defence-in-depth check, and the written-down dependency it offered as the alternative. The documentation is worth having regardless of the code, since the coupling is invisible from Express. ## Decisions, and the option this issue raised that was rejected The issue warned that a naive "trust `X-Forwarded-User`" would be *worse* than the status quo — it would look like a check while being trivially forgeable by anything reaching the container port. That is correct, and it rules out trusting authentik's identity headers directly. Verifying a signed JWT would fix it properly and identify *which* admin is acting, but needs authentik client configuration outside this repo, JWKS handling in the app, and an answer for what 113 test call sites present as a token. Recorded as the better long-term answer, not taken here. What landed is a shared secret between the proxy and the app: it does not authenticate *who*, only that the request came through the gate. | `ADMIN_GATE_SECRET` | Behaviour | | --- | --- | | Unset | Every admin route reachable — exactly today. A `[admin-gate]` warning at boot names what is unprotected. | | Set | `X-Admin-Gate` must match, or 403. | ## Why it is attached to the routers, not a path prefix This is the part that makes it more than redundant with the proxy, and it answers this issue's first concrete concern directly. An admin router added later at, say, `/api/reports` inherits the gate. The proxy only injects the header on paths its regex matches, so that router refuses on its **first request** rather than shipping quietly public. A 403 in that situation is the boundary reporting that it has drifted, which is the opposite of the current failure mode where nothing says anything at all. ## Optional, and loud about being optional Unset means today's behaviour, which is what keeps local development and all 113 existing admin call sites working untouched, and means shipping the image before configuring the proxy cannot take the admin panel down. What it does not do is stay quiet. The server warns at boot: ``` [admin-gate] ADMIN_GATE_SECRET is not set — /api/admin is protected only by the reverse proxy. Anything able to reach this container directly can administer the store. ``` Given how many times this project has met a control that reports success while doing nothing — #67, #60, #61, the new-code period in #79, the IPv6 key in #84 — an unconfigured gate should be a visible choice rather than an invisible one. Two details that are the difference between a gate and the appearance of one. An **empty value is treated as unset**, because enforcing an empty secret would admit any caller sending an empty header. And comparison is **timing-safe over SHA-256 digests of both sides**: `timingSafeEqual` throws on buffers of unequal length, so comparing raw values would turn a short header into a 500 rather than a 403, and a length check first would leak the secret's length. ## To turn it on The secret must be set in **two places at once**: 1. `ADMIN_GATE_SECRET` in the stack environment. 2. `proxy_set_header X-Admin-Gate "<secret>";` on the gated location in Nginx Proxy Manager — the `^/(admin|api/admin)` location in production, and `location /` in QA where the whole site is gated. Setting only one gives 403s until the other catches up. Loud and recoverable, unlike the failure it replaces — but worth doing in one sitting. ## Verification Over real HTTP, not only in tests: | Case | Result | | --- | --- | | Boot with no secret | warning logged, `GET /api/admin/items` → **200** | | Boot with secret, no header | **403** | | Boot with secret, wrong header | **403** | | Boot with secret, correct header | **200** | | Public storefront throughout | **200** | Each refusal is logged distinguishably (`no` vs `incorrect` header) without echoing the value it was sent, because a 403 from behind a proxy is otherwise very hard to diagnose — it usually means the proxy config and the stack secret have drifted. **8 unit tests** on the decision, **9 integration tests** on the mounting — including one case per admin router, because these are five separate `app.use` calls and one of them missing the middleware would be a silent and easy mistake. A correct middleware nobody mounted would pass every unit test and protect nothing. **106 unit** and **153 integration** passing, lint 0 errors and 8 warnings unchanged, build clean. ## README A new "The admin authorization boundary" section beside the deployment notes records the regex, the three consequences that are invisible from the code, the two-place coupling, and what a 403 means when it appears. Not pushed, per the usual arrangement.

bermudalamb referenced this issue from a commit

2026-08-21 13:44:18 -05:00

feat(backend): add an application-layer gate to the admin API (#63)

bermudalamb moved this to Review in Code Quality and Hardening on 2026-08-21 13:45:40 -05:00

bermudalamb referenced this issue

2026-08-21 13:51:06 -05:00

The inventory image upload accepts any file type, in the picker and on the server #95

bermudalamb moved this to Ready for Release in Code Quality and Hardening on 2026-08-21 13:54:47 -05:00

bermudalamb closed this issue

2026-08-21 13:55:12 -05:00

bermudalamb referenced this issue

2026-08-21 15:13:11 -05:00