-
Notifications
You must be signed in to change notification settings - Fork 23
fix(docs): add auth to root cache key #4655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: app
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -553,22 +553,58 @@ const getRoot = async ( | |||||
| authConfig: AuthEdgeConfig | undefined, | ||||||
| cacheConfig: Required<CacheConfig> | ||||||
| ) => { | ||||||
| const authCacheKey = authState.authed | ||||||
| ? `authed:${authState.user?.roles?.sort().join(",") || "noroles"}` | ||||||
| : "anon"; | ||||||
| console.log(`\n💾 [GET_ROOT] Cache MISS for getRoot - generating fresh root`); | ||||||
| console.log(` 🔑 Cache key: [${domainKey}, ${cacheConfig.cacheKeySuffix || '""'}, ${authCacheKey}]`); | ||||||
| console.log(` 👤 authState.authed: ${authState.authed}`); | ||||||
| console.log(` ⚙️ authConfig exists: ${!!authConfig}`); | ||||||
|
|
||||||
| console.log(`\n🌲 [GET_ROOT] Fetching unpruned root from unsafe_getRootCached`); | ||||||
| let root = await unsafe_getRootCached(cacheConfig)(domainKey); | ||||||
| console.log(` ✅ Got unpruned root with id: ${root.id}`); | ||||||
|
|
||||||
| if (authConfig) { | ||||||
| console.log(`\n🔀 [GET_ROOT] Calling pruneWithAuthState`); | ||||||
| console.log(` 👤 authState.authed: ${authState.authed}`); | ||||||
| console.log(` 👥 authState.user.roles: ${authState.authed ? JSON.stringify(authState.user?.roles || []) : 'N/A'}`); | ||||||
| console.log(` 📋 authConfig.allowlist: ${authConfig.allowlist?.length || 0}`); | ||||||
| console.log(` 📋 authConfig.denylist: ${authConfig.denylist?.length || 0}`); | ||||||
| console.log(` 📋 authConfig.anonymous: ${authConfig.anonymous?.length || 0}`); | ||||||
|
|
||||||
| root = pruneWithAuthState(authState, authConfig, root); | ||||||
|
|
||||||
| console.log(` ✅ [GET_ROOT] Finished pruning, root id: ${root.id}`); | ||||||
| } else { | ||||||
| console.log(`\n⚠️ [GET_ROOT] No authConfig - skipping pruneWithAuthState`); | ||||||
| } | ||||||
|
|
||||||
| FernNavigation.utils.mutableUpdatePointsTo(root); | ||||||
| console.log(`\n✅ [GET_ROOT] Returning pruned root\n`); | ||||||
| return root; | ||||||
| }; | ||||||
|
|
||||||
| const getRootCached = (cacheConfig: Required<CacheConfig>) => | ||||||
| cache(async (domainKey: string, authState: AuthState, authConfig: AuthEdgeConfig | undefined) => { | ||||||
| return await unstable_cache( | ||||||
| const authCacheKey = authState.authed | ||||||
| ? `authed:${authState.user?.roles?.sort().join(",") || "noroles"}` | ||||||
| : "anon"; | ||||||
|
|
||||||
| console.log(`\n🔍 [PRUNE_CACHE_CHECK] Checking cache for getRoot`); | ||||||
| console.log(` 🔑 Cache key: [${domainKey}, ${cacheConfig.cacheKeySuffix || '""'}, ${authCacheKey}]`); | ||||||
| console.log(` 👤 authState.authed: ${authState.authed}`); | ||||||
|
|
||||||
| const result = await unstable_cache( | ||||||
| (domainKey: string, authState: AuthState, authConfig: AuthEdgeConfig | undefined) => | ||||||
| getRoot(domainKey, authState, authConfig, cacheConfig), | ||||||
| [domainKey, cacheConfig.cacheKeySuffix], | ||||||
| { tags: [domainKey, "getRoot"] } | ||||||
| )(domainKey, authState, authConfig); | ||||||
|
|
||||||
| console.log(`\n✅ [PRUNE_CACHE_RETURN] Returning root (cache hit if no CACHE_MISS log above)\n`); | ||||||
|
|
||||||
| return result; | ||||||
| }); | ||||||
|
|
||||||
| const getNavigationNode = (cacheConfig: Required<CacheConfig>) => | ||||||
|
|
@@ -1111,25 +1147,50 @@ export const createCachedDocsLoader = async ( | |||||
| await clearKvCache(domainKey); | ||||||
| } | ||||||
|
|
||||||
| const authConfig = options?.skipAuth ? Promise.resolve(undefined) : getAuthConfig(domainKey); | ||||||
| const authConfig = options?.skipAuth | ||||||
| ? (console.log(`\n⚙️ [AUTH_CONFIG] Using skipAuth mode - no authConfig`), Promise.resolve(undefined)) | ||||||
| : (async () => { | ||||||
| console.log(`\n⚙️ [AUTH_CONFIG] Fetching authConfig for domainKey: ${domainKey}`); | ||||||
| const config = await getAuthConfig(domainKey); | ||||||
| console.log(` ✅ [AUTH_CONFIG] Fetched authConfig`); | ||||||
| console.log(` 📋 allowlist: ${config?.allowlist?.length || 0} paths`); | ||||||
| console.log(` 📋 denylist: ${config?.denylist?.length || 0} paths`); | ||||||
| console.log(` 📋 anonymous: ${config?.anonymous?.length || 0} paths`); | ||||||
| console.log(` 🔐 auth type: ${config?.type || 'none'}`); | ||||||
| return config; | ||||||
| })(); | ||||||
| const metadata = getMetadata(config)(withoutStaging(domainKey)); | ||||||
|
|
||||||
| const getAuthState = options?.skipAuth | ||||||
| ? async (_pathname?: string) => ({ | ||||||
| authed: true as const, | ||||||
| ok: true as const, | ||||||
| user: {}, | ||||||
| partner: "custom" as const | ||||||
| }) | ||||||
| ? async (_pathname?: string) => { | ||||||
| console.log(`\n🔓 [GET_AUTH_STATE] Using skipAuth mode - returning authed=true`); | ||||||
| return { | ||||||
| authed: true as const, | ||||||
| ok: true as const, | ||||||
| user: {}, | ||||||
| partner: "custom" as const | ||||||
| }; | ||||||
| } | ||||||
| : cache(async (pathname?: string) => { | ||||||
| console.log(`\n🔐 [GET_AUTH_STATE] Fetching auth state for domainKey: ${domainKey}`); | ||||||
| console.log(` 🎫 fern_token present: ${!!fern_token}`); | ||||||
| console.log(` 📍 pathname: ${pathname || 'undefined'}`); | ||||||
|
|
||||||
| const { getAuthState } = await createGetAuthState( | ||||||
| host, | ||||||
| domainKey, | ||||||
| fern_token, | ||||||
| await authConfig, | ||||||
| await metadata | ||||||
| ); | ||||||
| return await getAuthState(pathname); | ||||||
| const authState = await getAuthState(pathname); | ||||||
|
|
||||||
| console.log(` ✅ [GET_AUTH_STATE] Result - authed: ${authState.authed}`); | ||||||
| if (authState.authed) { | ||||||
| console.log(` 👥 roles: ${JSON.stringify(authState.user?.roles || [])}`); | ||||||
| } | ||||||
|
|
||||||
| return authState; | ||||||
| }); | ||||||
|
|
||||||
| return { | ||||||
|
|
@@ -1156,7 +1217,7 @@ export const createCachedDocsLoader = async ( | |||||
| { tags: [domainKey, "endpointByLocator"] } | ||||||
| ) | ||||||
| ), | ||||||
| getRoot: async () => getRootCached(config)(domainKey, await getAuthState(), await authConfig), | ||||||
| getRoot: async () => getRoot(domainKey, await getAuthState(), await authConfig, config), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The change from calling View DetailsAnalysisPerformance regression: RBAC pruning bypasses caching in getRoot() callWhat fails: The change from How to reproduce: // Multiple calls to the same domain with identical auth state
const loader = createCachedDocsLoader(config);
await loader.getRoot(); // First call - executes pruneWithAuthState()
await loader.getRoot(); // Second call - executes pruneWithAuthState() AGAIN (should be cached)Result: Expected: RBAC pruning should be cached per (domain + auth state) combination using both React's Technical details:
|
||||||
| getNavigationNode: async (id: string) => | ||||||
| getNavigationNode(config)(domainKey, id, await getAuthState(), await authConfig), | ||||||
| unsafe_getFullRoot: () => unsafe_getRootCached(config)(domainKey), | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,7 @@ import { GetObjectCommand, S3Client } from "@aws-sdk/client-s3"; | |||||||||||||||||||||||||||||||||||||
| import { getSignedUrl as getUncachedSignedUrl } from "@aws-sdk/s3-request-presigner"; | ||||||||||||||||||||||||||||||||||||||
| import type { FdrAPI } from "@fern-api/fdr-sdk"; | ||||||||||||||||||||||||||||||||||||||
| import { getS3KeyForV1DocsDefinition } from "@fern-api/fdr-sdk/docs"; | ||||||||||||||||||||||||||||||||||||||
| import { writeFile } from "fs/promises"; | ||||||||||||||||||||||||||||||||||||||
| import { cache } from "react"; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| import { isLocal } from "./isLocal"; | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -57,6 +58,17 @@ export const loadDocsDefinitionFromS3 = cache( | |||||||||||||||||||||||||||||||||||||
| if (response.ok) { | ||||||||||||||||||||||||||||||||||||||
| console.debug("Successfully loaded docs definition from S3: ", signedUrl); | ||||||||||||||||||||||||||||||||||||||
| const json = await response.json(); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Write to local file for debugging | ||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||
| const timestamp = new Date().toISOString().replace(/[:.]/g, '-'); | ||||||||||||||||||||||||||||||||||||||
| const filename = `/tmp/docs-definition-${cleanDomain.replace(/[^a-zA-Z0-9]/g, '-')}-${timestamp}.json`; | ||||||||||||||||||||||||||||||||||||||
| await writeFile(filename, JSON.stringify(json, null, 2)); | ||||||||||||||||||||||||||||||||||||||
| console.log(`💾 [DEBUG] Saved docs definition to: ${filename}`); | ||||||||||||||||||||||||||||||||||||||
| } catch (writeError) { | ||||||||||||||||||||||||||||||||||||||
| console.error("Failed to write docs definition to file:", writeError); | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+62
to
+69
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Debug code writes the full docs definition JSON to View DetailsAnalysisDebug code writes docs definition to /tmp/ in production without environment checkWhat fails: How to reproduce: # Call loadDocsDefinitionFromS3 in production environment
NODE_ENV=production node -e "
const fs = require('fs').promises;
// Simulate the exact debug code from lines 62-70
(async () => {
const json = {test: 'large docs data'};
const cleanDomain = 'example.com';
const timestamp = new Date().toISOString().replace(/[:.]/g, '-');
const filename = \`/tmp/docs-definition-\ Result: Creates files like Expected: Debug file writing should only occur in development environment, similar to other conditional debug code in the codebase (e.g. |
||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| return json as FdrAPI.docs.v2.read.LoadDocsForUrlResponse; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| throw new Error( | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code mutates the original
authState.user.rolesarray in-place by calling.sort()directly on it. This should create a copy before sorting to avoid mutating shared state.View Details
Analysis
Array mutation bug in cache key generation mutates authState.user.roles
What fails:
getRootCached()inpackages/commons/docs-loader/src/readonly-docs-loader.ts:567mutatesauthState.user.rolesarray when generating cache keys by calling.sort()directly on the original arrayHow to reproduce:
Result: The same mutated
authStateobject is passed topruneWithAuthState()for authorization decisions, causing authorization logic to see sorted roles instead of original JWT orderExpected: Authorization should use original role order from JWT; cache key generation should not mutate shared state
Fix: Create array copy before sorting:
authState.user?.roles?.slice().sort().join(",")