From 2a15ee4ce4ad9390c883bf626cc1dbf81479c83f Mon Sep 17 00:00:00 2001 From: derrickmehaffy Date: Mon, 12 Oct 2026 18:55:20 -0700 Subject: [PATCH 1/3] fix: judge "Shared across callers" per route, as the boot warning does The boot warning and the dashboard badge exist to say the same thing: this content type caches authenticated responses under a key that does not identify the caller. Since #230 the warning has been computed per route, on the hitpass and keys each route resolved to. The badge was still computed from the content type's own hitpass and keys.useAuth, and those disagree with the resolved routes in both directions: - a content type that keeps the default hitpass, with a listed route that sets `hitpass: false` itself and no useAuth: the server warns, the dashboard showed nothing; - a content type with `hitpass: false` and no useAuth whose routes are all listed: those keep the strategy's bypass (listedRouteHitpass), so nothing authenticated is cached and the server does not warn, but the dashboard flagged it. The first is the one that matters. It is a real cross-caller exposure that the badge, which exists because nobody reads boot logs, did not show. Move the predicate into one helper, used by both the warning and the stats service, and expose its result as a new `unkeyedAuthenticatedRoutes` field on each stats row. The table flags a row when that list is non-empty, and falls back to the old expression only when the field is absent. `hitpass` and `keysAuthIdentity` are unchanged and still describe the content type, so anything else that reads them sees the same values. The warning's text and conditions are unchanged. --- docs/guide/admin/index.md | 10 +- docs/guide/caching/keys.md | 8 +- docs/guide/recipes/authenticated.md | 6 +- docs/guide/reference/routes.md | 8 ++ e2e/tests/shared-across-callers.spec.ts | 92 ++++++++++++ .../src/components/ContentTypeTable/index.tsx | 12 +- .../server/src/services/cacheStats.ts | 6 + .../plugin-rest-cache/server/src/types/api.ts | 12 ++ .../src/utils/config/resolveUserStrategy.ts | 13 +- .../config/unkeyedAuthenticatedRoutes.ts | 24 ++++ .../tests/auth-risk-flag-consistency.test.js | 134 ++++++++++++++++++ 11 files changed, 305 insertions(+), 20 deletions(-) create mode 100644 e2e/tests/shared-across-callers.spec.ts create mode 100644 packages/plugin-rest-cache/server/src/utils/config/unkeyedAuthenticatedRoutes.ts create mode 100644 shared/tests/auth-risk-flag-consistency.test.js diff --git a/docs/guide/admin/index.md b/docs/guide/admin/index.md index a4cb9b58..7d93b422 100644 --- a/docs/guide/admin/index.md +++ b/docs/guide/admin/index.md @@ -40,9 +40,13 @@ A row may carry a badge: - **Keyed per caller** — `keys.useAuth` is on, so entries are stored separately per authenticated caller. -- **Shared across callers** — `hitpass` is off but `keys.useAuth` is not set, - so one caller's response can be served to another. The server also warns - about this at boot; the badge is here because nobody reads boot logs. +- **Shared across callers** — at least one of the content type's GET routes + resolved to `hitpass: false` without `keys.useAuth`, so one caller's response + can be served to another. It is judged per route, on what each route + actually resolved to, exactly as the boot warning is. So a listed route that + turns `hitpass` off itself is flagged, and a content type whose listed routes + keep the strategy's bypass is not. The server also warns about this at boot; + the badge is here because nobody reads boot logs. The **Purge** button clears every entry for that content type, and for the content types related to it when `clearRelatedCache` is on. The counts update diff --git a/docs/guide/caching/keys.md b/docs/guide/caching/keys.md index 3f097073..e2e958b6 100644 --- a/docs/guide/caching/keys.md +++ b/docs/guide/caching/keys.md @@ -273,10 +273,10 @@ tokens carry different permissions. Cache that without `useAuth` and the first caller to miss decides what everyone else sees, until the entry expires. -The server logs a warning at boot when it finds a content type with -`hitpass: false` and no `keys.useAuth`, naming the content type. The admin -dashboard shows the same thing as a **Shared across callers** badge, because -nobody reads boot logs. +The server logs a warning at boot when it finds a GET route that resolved to +`hitpass: false` with no `keys.useAuth`, naming the content type and the routes. +The admin dashboard shows the same thing as a **Shared across callers** badge, +because nobody reads boot logs. :::: code-group diff --git a/docs/guide/recipes/authenticated.md b/docs/guide/recipes/authenticated.md index 0644eaa2..0b509b89 100644 --- a/docs/guide/recipes/authenticated.md +++ b/docs/guide/recipes/authenticated.md @@ -120,9 +120,9 @@ merely useless: `useAuth` adds a component to keys that are never written, because `hitpass` bypassed them. ::: warning The server tells you when you get this wrong -Strapi logs a warning at boot naming any content type with `hitpass: false` and -no `keys.useAuth`, and the admin dashboard shows it as a **Shared across -callers** badge on that content type's row. If you see either, treat it as a +Strapi logs a warning at boot naming any content type with a GET route that +resolved to `hitpass: false` and no `keys.useAuth`, and the admin dashboard +shows it as a **Shared across callers** badge on that content type's row. If you see either, treat it as a data-exposure bug rather than a configuration nit. ::: diff --git a/docs/guide/reference/routes.md b/docs/guide/reference/routes.md index b6e01cad..853dcbde 100644 --- a/docs/guide/reference/routes.md +++ b/docs/guide/reference/routes.md @@ -65,6 +65,7 @@ endpoint does not make someone an operator entitled to credentials. "maxAge": 3600000, "hitpass": true, "keysAuthIdentity": false, + "unkeyedAuthenticatedRoutes": [], "routes": ["/api/articles", "/api/articles/:id"], "relatedContentTypes": ["api::category.category"] } @@ -72,6 +73,13 @@ endpoint does not make someone an operator entitled to credentials. } ``` +`hitpass` and `keysAuthIdentity` describe the content type's own settings. +`unkeyedAuthenticatedRoutes` lists its GET routes that resolved to +`hitpass: false` without `keys.useAuth`. These are the routes the boot warning +names, and the list the dashboard's **Shared across callers** badge reads. A +route's resolved values can differ from its content type's, so the first two +fields do not answer that question on their own. + Counts are derived from the store's own key list rather than tracked separately, so they cannot drift from reality. That does mean a call costs one key enumeration — cheap on Redis, but not something to poll aggressively. diff --git a/e2e/tests/shared-across-callers.spec.ts b/e2e/tests/shared-across-callers.spec.ts new file mode 100644 index 00000000..7a92d030 --- /dev/null +++ b/e2e/tests/shared-across-callers.spec.ts @@ -0,0 +1,92 @@ +import { test, expect, type Page } from '@playwright/test'; + +const DASHBOARD = '/admin/settings/rest-cache'; +const BADGE = 'Shared across callers'; + +const rowFor = (page: Page, uid: string) => page.getByRole('row').filter({ hasText: uid }); + +interface StatsRow { + uid: string; + hitpass: boolean; + keysAuthIdentity: boolean; + unkeyedAuthenticatedRoutes?: string[]; +} + +/** + * Serve the real stats with some rows rewritten. + * + * The playground's strategy has no route that shares authenticated entries - + * every spec on this server relies on that - so the cases are staged in the + * response rather than in the configuration. The jest spec + * auth-risk-flag-consistency covers the server producing these values from a + * real strategy; this covers the dashboard reading them. + */ +async function stubStats(page: Page, rewrite: (row: StatsRow) => StatsRow) { + await page.route('**/rest-cache/stats', async (route) => { + const response = await route.fetch(); + const body = (await response.json()) as { contentTypes: StatsRow[] }; + + body.contentTypes = body.contentTypes.map(rewrite); + await route.fulfill({ response, json: body }); + }); +} + +test.describe('"Shared across callers" badge', () => { + /** + * The content type keeps the default hitpass, but a listed route caches + * authenticated responses unkeyed. The server warns about it at boot. + */ + test('flags a content type for a route that shares authenticated entries', async ({ + page, + }) => { + await stubStats(page, (row) => + row.uid === 'api::writer.editor' + ? { + ...row, + hitpass: true, + keysAuthIdentity: false, + unkeyedAuthenticatedRoutes: ['/api/editors/:id'], + } + : row + ); + + await page.goto(DASHBOARD); + + await expect(rowFor(page, 'api::writer.editor')).toContainText(BADGE); + }); + + /** + * hitpass: false on the content type, but every route keeps the strategy's + * bypass, so nothing authenticated is cached. The server does not warn. + */ + test('does not flag a content type whose routes all keep the bypass', async ({ page }) => { + await stubStats(page, (row) => + row.uid === 'api::page.page' + ? { ...row, hitpass: false, keysAuthIdentity: false, unkeyedAuthenticatedRoutes: [] } + : row + ); + + await page.goto(DASHBOARD); + + // Rendered at all, so the negative below is not vacuous. + await expect(rowFor(page, 'api::page.page')).toContainText('/api/pages'); + await expect(rowFor(page, 'api::page.page')).not.toContainText(BADGE); + }); + + test('falls back to the content type settings when the list is absent', async ({ page }) => { + await stubStats(page, (row) => { + const { unkeyedAuthenticatedRoutes: _omitted, ...rest } = row; + + if (row.uid === 'api::page.page') { + return { ...rest, hitpass: false, keysAuthIdentity: false }; + } + + return rest; + }); + + await page.goto(DASHBOARD); + + await expect(rowFor(page, 'api::page.page')).toContainText(BADGE); + await expect(rowFor(page, 'api::article.article')).not.toContainText(BADGE); + }); +}); diff --git a/packages/plugin-rest-cache/admin/src/components/ContentTypeTable/index.tsx b/packages/plugin-rest-cache/admin/src/components/ContentTypeTable/index.tsx index 8d9509c5..17883c92 100644 --- a/packages/plugin-rest-cache/admin/src/components/ContentTypeTable/index.tsx +++ b/packages/plugin-rest-cache/admin/src/components/ContentTypeTable/index.tsx @@ -119,8 +119,16 @@ const ContentTypeTable = ({ contentTypes, canPurge }: ContentTypeTableProps) => // two people authorised for the same route share one entry. The // server warns about this at boot; surface it where someone will // actually see it. - const sharesAuthenticatedEntries = - contentType.hitpass === false && !contentType.keysAuthIdentity; + // + // From the server's per-route list, which is what the boot warning + // reads. The content type's own hitpass and keys missed a listed + // route that sets hitpass: false itself, and flagged content types + // whose listed routes all keep the strategy's bypass (#230). The + // old expression stays only for a server that does not send the + // list. + const sharesAuthenticatedEntries = contentType.unkeyedAuthenticatedRoutes + ? contentType.unkeyedAuthenticatedRoutes.length > 0 + : contentType.hitpass === false && !contentType.keysAuthIdentity; return ( diff --git a/packages/plugin-rest-cache/server/src/services/cacheStats.ts b/packages/plugin-rest-cache/server/src/services/cacheStats.ts index 15ddf5f9..9a847dfe 100644 --- a/packages/plugin-rest-cache/server/src/services/cacheStats.ts +++ b/packages/plugin-rest-cache/server/src/services/cacheStats.ts @@ -3,6 +3,7 @@ import type { Core } from '@strapi/strapi'; import type { CachePluginHitpass } from '../types/common'; import type { CacheSummary, ContentTypeStats } from '../types/api'; import type { RestCachePluginConfig } from '../types/config'; +import { unkeyedAuthenticatedRoutes } from '../utils/config/unkeyedAuthenticatedRoutes'; export type { CacheSummary, ContentTypeStats }; @@ -55,6 +56,11 @@ export default function createCacheStatsService({ maxAge: conf?.maxAge, hitpass: (conf?.hitpass as CachePluginHitpass | boolean) !== false, keysAuthIdentity: conf?.keys?.useAuth === true, + // Per route, on resolved values - the same predicate as the boot + // warning. `hitpass` and `keysAuthIdentity` above describe the + // content type's own settings, which a route can differ from in + // both directions, so they cannot answer this on their own. + unkeyedAuthenticatedRoutes: unkeyedAuthenticatedRoutes(conf?.routes ?? []), routes: (conf?.routes ?? []) .filter((route) => route.method === 'GET') .map((route) => route.path as string), diff --git a/packages/plugin-rest-cache/server/src/types/api.ts b/packages/plugin-rest-cache/server/src/types/api.ts index 93a0dd17..0685dd8a 100644 --- a/packages/plugin-rest-cache/server/src/types/api.ts +++ b/packages/plugin-rest-cache/server/src/types/api.ts @@ -34,6 +34,18 @@ export interface ContentTypeStats { maxAge?: Milliseconds; hitpass: boolean; keysAuthIdentity: boolean; + /** + * GET routes that cache authenticated responses under a key that does not + * identify the caller: resolved `hitpass === false` without + * `keys.useAuth`. The same routes the boot warning names. + * + * This, not `hitpass && !keysAuthIdentity`, is what the dashboard's + * "Shared across callers" badge reads. Those two describe the content type, + * and a route's resolved values can differ from its content type's either + * way. Optional only so an admin bundle still renders against a server + * that predates the field; this server always sends it. + */ + unkeyedAuthenticatedRoutes?: string[]; routes: string[]; relatedContentTypes: string[]; } diff --git a/packages/plugin-rest-cache/server/src/utils/config/resolveUserStrategy.ts b/packages/plugin-rest-cache/server/src/utils/config/resolveUserStrategy.ts index 7c1c63d4..efd3dd06 100644 --- a/packages/plugin-rest-cache/server/src/utils/config/resolveUserStrategy.ts +++ b/packages/plugin-rest-cache/server/src/utils/config/resolveUserStrategy.ts @@ -3,6 +3,7 @@ import type { Core } from '@strapi/strapi'; import { getRelatedModelsUid } from './getRelatedModelsUid'; import { deepFreeze } from './deepFreeze'; import { shouldLookup } from '../middlewares/shouldLookup'; +import { unkeyedAuthenticatedRoutes } from './unkeyedAuthenticatedRoutes'; import { CachePluginStrategy, CacheRouteConfig, @@ -289,15 +290,11 @@ export const resolveUserStrategy = function ( // on the content type alone both missed that and warned for content types // whose own hitpass no route used. // See https://github.com/strapi-community/plugin-rest-cache/issues/230 + // + // The dashboard's "Shared across callers" badge uses the same predicate, so + // the two agree on which content types are at risk. for (const cacheConfig of cacheConfigs) { - const unkeyedPaths = cacheConfig.routes - .filter( - (route) => - route.method === 'GET' && - route.hitpass === false && - route.keys?.useAuth !== true - ) - .map((route) => route.path); + const unkeyedPaths = unkeyedAuthenticatedRoutes(cacheConfig.routes); if (unkeyedPaths.length) { strapi.log.warn( diff --git a/packages/plugin-rest-cache/server/src/utils/config/unkeyedAuthenticatedRoutes.ts b/packages/plugin-rest-cache/server/src/utils/config/unkeyedAuthenticatedRoutes.ts new file mode 100644 index 00000000..9faa75bc --- /dev/null +++ b/packages/plugin-rest-cache/server/src/utils/config/unkeyedAuthenticatedRoutes.ts @@ -0,0 +1,24 @@ +import type { CacheRouteConfig } from '../../types'; + +/** + * Whether a route caches authenticated responses under a key that does not + * identify the caller. + * + * Judged on the values the route actually resolved to, not on its content + * type's. A listed route can set `hitpass: false` itself under a content type + * that keeps the default, and a content type's `hitpass: false` is not + * inherited by listed routes whose keys do not identify the caller - they keep + * the strategy's bypass as a function (see listedRouteHitpass), which is never + * `=== false`. A check on the content type alone gets both of those wrong. + * See https://github.com/strapi-community/plugin-rest-cache/issues/230 + * + * The boot warning and the dashboard's "Shared across callers" badge both + * read this, so the two cannot disagree again. + */ +export const isUnkeyedAuthenticatedRoute = (route: CacheRouteConfig): boolean => + route.method === 'GET' && route.hitpass === false && route.keys?.useAuth !== true; + +/** The paths of the routes `isUnkeyedAuthenticatedRoute` holds for. */ +export const unkeyedAuthenticatedRoutes = ( + routes: readonly CacheRouteConfig[] +): string[] => routes.filter(isUnkeyedAuthenticatedRoute).map((route) => route.path); diff --git a/shared/tests/auth-risk-flag-consistency.test.js b/shared/tests/auth-risk-flag-consistency.test.js new file mode 100644 index 00000000..8f5c7552 --- /dev/null +++ b/shared/tests/auth-risk-flag-consistency.test.js @@ -0,0 +1,134 @@ +"use strict"; + +/** + * The admin dashboard's "Shared across callers" badge and the boot warning + * must agree on which content types cache authenticated responses under keys + * that do not identify the caller. + * + * The boot warning is computed per route, on the values each route actually + * resolved to (#230). The badge used to be computed from the content type's + * own `hitpass` and `keys.useAuth`, which a route's resolved values can differ + * from in both directions - so the dashboard missed a route the server warned + * about, and flagged a content type the server considered safe. + */ + +const { setup, teardown, adminAgent } = require("./helpers/strapi"); + +jest.setTimeout(90000); + +process.env.STRAPI_DISABLE_UPDATE_NOTIFICATION = true; +process.env.STRAPI_HIDE_STARTUP_MESSAGE = true; +process.env.STRAPI_TELEMETRY_DISABLED = true; + +// Strapi requires config/plugins.js, which requires ./cache-strategy, through +// jest's module registry - so the strategy can be extended here without +// touching the shared config every other spec uses. +jest.mock("../config/cache-strategy", () => { + const real = jest.requireActual("../config/cache-strategy"); + + return (args) => { + const strategy = real(args); + + strategy.contentTypes = [ + "api::article.article", + // (A) The content type keeps the default hitpass; one listed route sets + // hitpass: false itself, without keys.useAuth. The boot warning names + // the route; the content type's own hitpass says nothing is wrong. + { + contentType: "api::writer.editor", + routes: [ + { + path: "/api/editors/:id", + method: "GET", + hitpass: false, + keys: { useQueryParams: true, useHeaders: [] }, + }, + ], + }, + // (B) The content type has hitpass: false and no keys.useAuth, but has + // only listed routes. Those keep the strategy's bypass on top + // (listedRouteHitpass), so authenticated requests are never cached and + // the boot does not warn - yet the content type's own hitpass is false. + { + contentType: "api::page.page", + hitpass: false, + injectDefaultRoutes: false, + routes: ["/api/pages"], + }, + ]; + + return strategy; + }; +}); + +// What ContentTypeTable renders the badge from: the per-route list when the +// server sends it, the content type's own settings otherwise. +const dashboardFlags = (stats) => + stats.unkeyedAuthenticatedRoutes + ? stats.unkeyedAuthenticatedRoutes.length > 0 + : stats.hitpass === false && !stats.keysAuthIdentity; + +describe("Shared across callers agrees with the boot warning", () => { + let bootLog = ""; + + beforeAll(async () => { + process.env.KEYS_PREFIX = undefined; + process.env.STRAPI_LOG_LEVEL = "warn"; + + const write = process.stdout.write.bind(process.stdout); + const spy = jest + .spyOn(process.stdout, "write") + .mockImplementation((chunk, ...rest) => { + bootLog += String(chunk); + return write(chunk, ...rest); + }); + try { + await setup(); + } finally { + spy.mockRestore(); + } + }); + afterAll(async () => { + delete process.env.STRAPI_LOG_LEVEL; + await teardown(); + }); + + const statsFor = async (uid) => { + const res = await adminAgent().get("/rest-cache/stats"); + expect(res.status).toBe(200); + return res.body.contentTypes.find((c) => c.uid === uid); + }; + + const bootWarns = (uid) => + bootLog.includes(`"${uid}" has hitpass disabled but keys.useAuth is not set`); + + it("(A) flags a content type whose listed route shares authenticated entries", async () => { + // Precondition: the server considers this unsafe. + expect(bootWarns("api::writer.editor")).toBe(true); + + const stats = await statsFor("api::writer.editor"); + expect(dashboardFlags(stats)).toBe(true); + expect(stats.unkeyedAuthenticatedRoutes).toEqual(["/api/editors/:id"]); + }); + + it("(B) does not flag a content type whose routes all keep the strategy bypass", async () => { + // Precondition: the server considers this safe. + expect(bootWarns("api::page.page")).toBe(false); + + const stats = await statsFor("api::page.page"); + expect(dashboardFlags(stats)).toBe(false); + expect(stats.unkeyedAuthenticatedRoutes).toEqual([]); + }); + + it("keeps the content type's own fields as they were", async () => { + // The badge moved to the new field; these still describe the content + // type itself, for anything else reading them. + const page = await statsFor("api::page.page"); + expect(page.hitpass).toBe(false); + expect(page.keysAuthIdentity).toBe(false); + + const editor = await statsFor("api::writer.editor"); + expect(editor.hitpass).toBe(true); + expect(editor.keysAuthIdentity).toBe(false); + }); +}); From d7f0543852cb5250e881dad56699aea47c573578 Mon Sep 17 00:00:00 2001 From: derrickmehaffy Date: Mon, 12 Oct 2026 20:59:39 -0700 Subject: [PATCH 2/3] fix: judge a route's key by useAuth's truthiness, as the key generator does generateCacheKey adds the caller's identity whenever `keys.useAuth` is truthy. The shared-across-callers predicate asked `useAuth !== true`, so a route configured with `useAuth: "true"` - what env() returns in a JavaScript config - had its entries keyed per caller and was still named in the boot warning and flagged on the dashboard. The badge and warning exist to say a key does not identify the caller; they now ask the same question the key generator answers. Fixed in the predicate rather than by coercing useAuth when the config is resolved. The listed-route hitpass, the Cache-Control downgrade and the stats' keysAuthIdentity all compare `=== true`, so coercing would change what those configurations bypass and emit. Cache keys are untouched. The new jest case first proves the premise on the live cache - Alice misses then hits, Bob misses - and then asserts no warning and an empty route list. Seen red before the change: the boot warning named the route. --- .../config/unkeyedAuthenticatedRoutes.ts | 11 +++- .../tests/auth-risk-flag-consistency.test.js | 65 ++++++++++++++++++- 2 files changed, 73 insertions(+), 3 deletions(-) diff --git a/packages/plugin-rest-cache/server/src/utils/config/unkeyedAuthenticatedRoutes.ts b/packages/plugin-rest-cache/server/src/utils/config/unkeyedAuthenticatedRoutes.ts index 9faa75bc..204446ec 100644 --- a/packages/plugin-rest-cache/server/src/utils/config/unkeyedAuthenticatedRoutes.ts +++ b/packages/plugin-rest-cache/server/src/utils/config/unkeyedAuthenticatedRoutes.ts @@ -14,9 +14,18 @@ import type { CacheRouteConfig } from '../../types'; * * The boot warning and the dashboard's "Shared across callers" badge both * read this, so the two cannot disagree again. + * + * `useAuth` is tested for truthiness because that is what generateCacheKey + * does, and the question here is whether the key identifies the caller. The + * type says boolean, but a JavaScript config can pass anything - `env()` + * returns strings - and `=== true` flagged a route keyed by `"true"` as + * shared when its entries were already per caller. Nothing coerces useAuth + * at resolution, and nothing should here either: the hitpass, Cache-Control + * and stats checks compare `=== true`, and coercing would change what they + * bypass and emit for those configurations. */ export const isUnkeyedAuthenticatedRoute = (route: CacheRouteConfig): boolean => - route.method === 'GET' && route.hitpass === false && route.keys?.useAuth !== true; + route.method === 'GET' && route.hitpass === false && !route.keys?.useAuth; /** The paths of the routes `isUnkeyedAuthenticatedRoute` holds for. */ export const unkeyedAuthenticatedRoutes = ( diff --git a/shared/tests/auth-risk-flag-consistency.test.js b/shared/tests/auth-risk-flag-consistency.test.js index 8f5c7552..471b613a 100644 --- a/shared/tests/auth-risk-flag-consistency.test.js +++ b/shared/tests/auth-risk-flag-consistency.test.js @@ -12,7 +12,7 @@ * about, and flagged a content type the server considered safe. */ -const { setup, teardown, adminAgent } = require("./helpers/strapi"); +const { setup, teardown, agent, adminAgent } = require("./helpers/strapi"); jest.setTimeout(90000); @@ -30,7 +30,22 @@ jest.mock("../config/cache-strategy", () => { const strategy = real(args); strategy.contentTypes = [ - "api::article.article", + // (C) A route with hitpass: false whose useAuth is truthy but not the + // boolean true - what `env("USE_AUTH")` hands over, since env() returns + // strings. The key generator tests useAuth for truthiness, so these + // entries are keyed per caller and nothing is shared. + { + contentType: "api::article.article", + injectDefaultRoutes: false, + routes: [ + { + path: "/api/articles", + method: "GET", + hitpass: false, + keys: { useQueryParams: true, useHeaders: [], useAuth: "true" }, + }, + ], + }, // (A) The content type keeps the default hitpass; one listed route sets // hitpass: false itself, without keys.useAuth. The boot warning names // the route; the content type's own hitpass says nothing is wrong. @@ -87,6 +102,15 @@ describe("Shared across callers agrees with the boot warning", () => { } finally { spy.mockRestore(); } + + // (C) is checked against what the cache actually does, which needs + // authenticated callers the route lets through. + const role = await strapi + .query("plugin::users-permissions.role") + .findOne({ where: { type: "authenticated" } }); + await strapi.query("plugin::users-permissions.permission").create({ + data: { action: "api::article.article.find", role: role.id }, + }); }); afterAll(async () => { delete process.env.STRAPI_LOG_LEVEL; @@ -120,6 +144,43 @@ describe("Shared across callers agrees with the boot warning", () => { expect(stats.unkeyedAuthenticatedRoutes).toEqual([]); }); + it("(C) does not flag a route whose truthy non-boolean useAuth keys entries per caller", async () => { + const jwtFor = async (username) => { + const role = await strapi + .query("plugin::users-permissions.role") + .findOne({ where: { type: "authenticated" } }); + const user = await strapi.query("plugin::users-permissions.user").create({ + data: { + username, + email: `${username}@example.com`, + password: "Password123", + confirmed: true, + blocked: false, + role: role.id, + provider: "local", + }, + }); + return strapi.plugin("users-permissions").service("jwt").issue({ id: user.id }); + }; + const alice = await jwtFor("flag-alice"); + const bob = await jwtFor("flag-bob"); + const get = (jwt) => + agent().get("/api/articles").set("Authorization", `Bearer ${jwt}`); + + // Precondition: the key really does identify the caller. Alice's entry is + // cached and served back to her, and Bob does not get it. + await strapi.plugin("rest-cache").service("cacheStore").reset(); + expect((await get(alice)).get("x-cache")).toBe("MISS"); + expect((await get(alice)).get("x-cache")).toBe("HIT"); + expect((await get(bob)).get("x-cache")).toBe("MISS"); + + // So neither the boot warning nor the badge may claim it is shared. + expect(bootWarns("api::article.article")).toBe(false); + const stats = await statsFor("api::article.article"); + expect(stats.unkeyedAuthenticatedRoutes).toEqual([]); + expect(dashboardFlags(stats)).toBe(false); + }); + it("keeps the content type's own fields as they were", async () => { // The badge moved to the new field; these still describe the content // type itself, for anything else reading them. From da20dc9cfb306de7884fd6067ce5baeab9f4ea13 Mon Sep 17 00:00:00 2001 From: derrickmehaffy Date: Mon, 12 Oct 2026 21:06:45 -0700 Subject: [PATCH 3/3] fix(admin): name the routes in the "Shared across callers" tooltip The badge is now raised per route, but its tooltip still said "Hitpass is disabled but keys.useAuth is not set". For a content type that keeps its own hitpass and has a listed route that turns it off, that is false, and it points the admin at a content type setting that is already right. When the server sends the per-route list, the tooltip names those routes and says they cache authenticated requests without keys.useAuth, with en and fr strings under a new key. The old string stays for the fallback, where the badge comes from the content type's own hitpass and useAuth and the sentence is exactly true. Seen red before the change: the (A) case's tooltip read "Hitpass is disabled but keys.useAuth is not set, ..." with no route in it. --- docs/guide/admin/index.md | 5 +-- e2e/tests/shared-across-callers.spec.ts | 22 +++++++++++++ .../src/components/ContentTypeTable/index.tsx | 31 ++++++++++++++++--- .../admin/src/translations/en.json | 1 + .../admin/src/translations/fr.json | 1 + 5 files changed, 53 insertions(+), 7 deletions(-) diff --git a/docs/guide/admin/index.md b/docs/guide/admin/index.md index 7d93b422..6fac740b 100644 --- a/docs/guide/admin/index.md +++ b/docs/guide/admin/index.md @@ -45,8 +45,9 @@ A row may carry a badge: can be served to another. It is judged per route, on what each route actually resolved to, exactly as the boot warning is. So a listed route that turns `hitpass` off itself is flagged, and a content type whose listed routes - keep the strategy's bypass is not. The server also warns about this at boot; - the badge is here because nobody reads boot logs. + keep the strategy's bypass is not. Hover the badge for the routes at fault. + The server also warns about this at boot; the badge is here because nobody + reads boot logs. The **Purge** button clears every entry for that content type, and for the content types related to it when `clearRelatedCache` is on. The counts update diff --git a/e2e/tests/shared-across-callers.spec.ts b/e2e/tests/shared-across-callers.spec.ts index 7a92d030..d6d3ec2d 100644 --- a/e2e/tests/shared-across-callers.spec.ts +++ b/e2e/tests/shared-across-callers.spec.ts @@ -5,6 +5,15 @@ const BADGE = 'Shared across callers'; const rowFor = (page: Page, uid: string) => page.getByRole('row').filter({ hasText: uid }); +/** The badge's tooltip text for a row: what an admin reads to act on it. */ +async function tooltipFor(page: Page, uid: string) { + await rowFor(page, uid).getByText(BADGE).hover(); + const tooltip = page.getByRole('tooltip'); + await expect(tooltip).toBeVisible(); + + return tooltip; +} + interface StatsRow { uid: string; hitpass: boolean; @@ -53,6 +62,14 @@ test.describe('"Shared across callers" badge', () => { await page.goto(DASHBOARD); await expect(rowFor(page, 'api::writer.editor')).toContainText(BADGE); + + // This content type's own hitpass is enabled, so "Hitpass is disabled" + // would send the admin to a setting that is already right. The tooltip + // has to name the route that is actually at fault. + const tooltip = await tooltipFor(page, 'api::writer.editor'); + await expect(tooltip).toContainText('Route /api/editors/:id caches authenticated requests'); + await expect(tooltip).toContainText('keys.useAuth'); + await expect(tooltip).not.toContainText('Hitpass is disabled'); }); /** @@ -88,5 +105,10 @@ test.describe('"Shared across callers" badge', () => { await expect(rowFor(page, 'api::page.page')).toContainText(BADGE); await expect(rowFor(page, 'api::article.article')).not.toContainText(BADGE); + + // Without the list, the badge was raised from the content type's own + // settings, which this sentence describes exactly. + const tooltip = await tooltipFor(page, 'api::page.page'); + await expect(tooltip).toContainText('Hitpass is disabled but keys.useAuth is not set'); }); }); diff --git a/packages/plugin-rest-cache/admin/src/components/ContentTypeTable/index.tsx b/packages/plugin-rest-cache/admin/src/components/ContentTypeTable/index.tsx index 17883c92..0be21e4c 100644 --- a/packages/plugin-rest-cache/admin/src/components/ContentTypeTable/index.tsx +++ b/packages/plugin-rest-cache/admin/src/components/ContentTypeTable/index.tsx @@ -164,11 +164,32 @@ const ContentTypeTable = ({ contentTypes, canPurge }: ContentTypeTableProps) => ) : null} {sharesAuthenticatedEntries ? ( {/* Themed, not a hardcoded hex: the badge has to stay legible in dark mode and in every locale. Wrapped for diff --git a/packages/plugin-rest-cache/admin/src/translations/en.json b/packages/plugin-rest-cache/admin/src/translations/en.json index f92fd0ee..ed189a64 100644 --- a/packages/plugin-rest-cache/admin/src/translations/en.json +++ b/packages/plugin-rest-cache/admin/src/translations/en.json @@ -29,6 +29,7 @@ "contentTypes.authKeyed.hint": "Responses are cached separately for each authenticated caller.", "contentTypes.authRisk": "Shared across callers", "contentTypes.authRisk.hint": "Hitpass is disabled but keys.useAuth is not set, so one caller's response can be served to another.", + "contentTypes.authRisk.routesHint": "{count, plural, one {Route {routes} caches} other {Routes {routes} cache}} authenticated requests without keys.useAuth, so one caller's response can be served to another.", "contentTypes.related": "Also purges {count, plural, one {# related type} other {# related types}}", "panel.cached.title": "REST Cache", "panel.cached.body": "Responses for this entry are cached for up to {maxAge} after they are first requested.", diff --git a/packages/plugin-rest-cache/admin/src/translations/fr.json b/packages/plugin-rest-cache/admin/src/translations/fr.json index c5a99561..cf7f897d 100644 --- a/packages/plugin-rest-cache/admin/src/translations/fr.json +++ b/packages/plugin-rest-cache/admin/src/translations/fr.json @@ -29,6 +29,7 @@ "contentTypes.authKeyed.hint": "Les réponses sont mises en cache séparément pour chaque appelant authentifié.", "contentTypes.authRisk": "Partagé entre les appelants", "contentTypes.authRisk.hint": "Le hitpass est désactivé mais keys.useAuth n'est pas défini : la réponse d'un appelant peut donc être servie à un autre.", + "contentTypes.authRisk.routesHint": "{count, plural, one {La route {routes} met} other {Les routes {routes} mettent}} en cache des requêtes authentifiées sans keys.useAuth : la réponse d'un appelant peut donc être servie à un autre.", "contentTypes.related": "Vide également {count, plural, one {# type associé} other {# types associés}}", "panel.cached.title": "REST Cache", "panel.cached.body": "Les réponses pour cette entrée sont mises en cache pendant {maxAge} au maximum après leur première requête.",