diff --git a/docs/guide/admin/index.md b/docs/guide/admin/index.md index eaea88fc..45ec2ef6 100644 --- a/docs/guide/admin/index.md +++ b/docs/guide/admin/index.md @@ -40,9 +40,14 @@ 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. 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/docs/guide/caching/keys.md b/docs/guide/caching/keys.md index f4f1384d..c6680777 100644 --- a/docs/guide/caching/keys.md +++ b/docs/guide/caching/keys.md @@ -300,10 +300,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 bf1e1ea9..8ad80bfe 100644 --- a/docs/guide/reference/routes.md +++ b/docs/guide/reference/routes.md @@ -66,6 +66,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"] } @@ -73,6 +74,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..d6d3ec2d --- /dev/null +++ b/e2e/tests/shared-across-callers.spec.ts @@ -0,0 +1,114 @@ +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 }); + +/** 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; + 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); + + // 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'); + }); + + /** + * 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); + + // 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 8d9509c5..0be21e4c 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 ( @@ -156,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.", diff --git a/packages/plugin-rest-cache/server/src/services/cacheStats.ts b/packages/plugin-rest-cache/server/src/services/cacheStats.ts index eb963bea..66f182df 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 }; @@ -63,6 +64,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 79572c1e..fc9ee531 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 b2a944dc..12cc8c17 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, @@ -307,15 +308,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..204446ec --- /dev/null +++ b/packages/plugin-rest-cache/server/src/utils/config/unkeyedAuthenticatedRoutes.ts @@ -0,0 +1,33 @@ +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. + * + * `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; + +/** 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..471b613a --- /dev/null +++ b/shared/tests/auth-risk-flag-consistency.test.js @@ -0,0 +1,195 @@ +"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, agent, 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 = [ + // (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. + { + 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(); + } + + // (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; + 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("(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. + 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); + }); +});