pages#template ignores its :version URL param, allowing wrong-app-type content to be permanently cached by the browser

[philayres]

Summary

GET /pages/:version/template (PagesController#template) never reads the :version URL token. It renders content from the user's current state at the moment the AJAX request arrives, and stamps the response Cache-Control: private, max-age=604800, immutable. The browser caches by URL for 7 days without revalidation. If the user's app type changes between the page render (which embeds the token) and the deferred template AJAX, the wrong app type's templates are permanently bound to the URL for the correct app type.

Follow-up to #1279 / PR #1283 (which fixed the compiled-file cache-key scoping) and the viewable_tables cache-key fix. This is a narrower race, but produces the identical user-visible symptom: persistently missing master panel tabs / incomplete templates.

Mechanism

1. Each full page load embeds _fpa.state.template_version = '<digest>' (app/views/layouts/_setup_app.html.erb). The digest (ApplicationHelper#template_version -> partial_cache_key(:loaded)) includes the user's app_type_id at page-render time.
2. Later, asynchronously, _fpa.load_template_version fetches GET /pages/<token>/template with cache: true.
3. PagesController#template ignores params[:id], computes the etag and renders masters/cache_search_results_template from the user's app type at AJAX time, and sets set_browser_cache(max_age: 604_800, immutable: true).
4. The browser stores the response keyed by URL, and immutable prevents any revalidation for a week.

Concrete failure sequence (multi-tab)

• t0: Tab 1 loads /masters/search in app type A. HTML embeds token V_A. The heavy template AJAX has not fired yet.
• t1: Tab 2 switches app type via the navbar (/pages/home?use_app_type=B). AppTypeChange saves user.app_type_id = B (shared by all tabs).
• t2: Tab 1's deferred AJAX fires: GET /pages/V_A/template. Server ignores V_A, sees app type B, returns app B's templates with max-age=604800, immutable. Browser caches B-content under the A URL.
• t3: User switches back to app A. The new page embeds V_A again (same sign-in, same timestamps -> same digest). Browser serves the cached app B templates without contacting the server. Master panel shows app B's tabs; app A's tabs are missing.

The poisoning is durable within the session:

• Revalidation cannot repair it: the etag in pages#template is also computed from current state, so when the user is back on app A the server's etag matches what the content should be and returns 304, confirming the browser's wrong body.
• immutable means normal reloads never revalidate anyway; only a hard refresh or cache eviction clears it.

The same race can fire without a second tab: clicking the app selector while the first page is still loading its deferred template AJAX, or a logo_link/home redirect that embeds use_app_type and changes app type between page render and AJAX arrival.

Why this was previously hidden

Before #1270 (in 9.46.2 and earlier), partial_cache_key included user.updated_at, and every User save (including the app type switch itself) cleared the whole Rails cache and rotated server_cache_version. Switching A->B->A produced a brand-new URL every time, so a poisoned URL was never requested again. 9.46.3's stable tokens make URL reuse - and therefore permanent poisoning - possible.

Suggested fix

Make the server honor the URL/content contract in PagesController#template:

• Compute the current template_version digest and compare with params[:id].
• If they match: serve as now, with private, max-age=604800, immutable.
• If they do not match: serve with Cache-Control: no-store (or redirect to the current-token URL) so mismatched content can never be cached under a long-lived immutable header.

A system spec should reproduce the race deterministically, e.g. render a page for app A, change the user's app type server-side before triggering the template fetch, and assert that the mismatched response is not cached / the correct tabs are shown after switching back.

Related

• Handlebars master_tabs partial cached with app-type-agnostic key hides per-app-type panels (analysis_plans/grant_aims specs) #1279, PR Fixed handlebars_cache_key to be app_type-aware, preventing cross-app_type cache poisoning - fixes #1279 #1283 (handlebars cache-key scoping)
• User/Admin saves clear entire Rails cache, causing template cache misses #1270, PR Fixed whole-cache clear on routine User/Admin saves - fixes #1270 #1271 (removal of whole-cache clears on routine User/Admin saves, which exposed this class of issue)
• viewable_tables cache-key fix (branch viewable-tables-cache-scope-1279)

[philayres]

I reviewed the follow-up change for the missing master panel tabs after PR #1283.

Findings:

• The root cause appears to be upstream of the Handlebars compiled-file cache. Admin::UserAccessControl.viewable_tables cached results by user/session, but not by the user's effective app_type_id. When a user switched app type within the same session, the cache could return the previous app type's viewable table set.
• That stale result feeds master_viewables, which filters panels in app/views/masters/_search_results_master_tabs.html.erb. The result is incomplete master_tabs content, which then gets compiled into the Handlebars template artifacts.
• This explains why rolling back to 9.46.2 hid the issue: before the cache-clear scope change, routine user saves/sign-ins/app-type switches cleared the whole Rails cache, masking the under-scoped key.
• The proposed fix to include the effective app type in the viewable_tables cache key is well aligned with the actual data dependency.
• The additional preloadables cache key change is also appropriate, since that list is user/app-type/access-control dependent.
• The access_for_list? fix from Rails.cache.write(cache_key, v) to Rails.cache.write(ck, v) looks correct; the previous code was not writing the per-resource entries under the intended key.

Test coverage looks strong:

• A focused model spec primes the cache in app type A, switches the user to app type B without changing sign-in/session inputs, and verifies the viewable tables are recalculated correctly.
• A system spec reproduces the browser-visible failure by switching app types within one example and checking the expected master panel tabs.
• A second system spec switches users within one example, checking that user-specific tab visibility does not leak between users.
• The app-type switch system spec was verified to fail without the implementation fix, which is the right shape of regression test for this issue.

Residual follow-up risks:

• Admin::AppType.all_ids_available_to still has a cache key scoped only by user id. It is outside this fix, but it has the same under-scoping smell and should probably get a follow-up audit.
• PagesController#template serves template content based on current state while the URL contains a version token and long-lived immutable caching. That remains a possible multi-tab/app-switch race and may deserve a separate issue.
• remake_from_attributes mutates cached hashes by deleting admin_id. This was pre-existing, but now that per-resource cache writes are effective, it may be worth making that method duplicate the hash before mutation.

Overall, the fix is well targeted and the rationale matches the observed production behavior.

[philayres]

Follow-up on the review observations, addressed on branch viewable-tables-cache-scope-1279:

Addressed (commit a9bc73d81):

1. Admin::AppType.all_ids_available_to under-scoped cache key — the key now includes Admin::UserAccessControl.latest_update, Admin::UserRole.latest_update, Admin::AppType.latest_update and Settings::OnlyLoadAppTypes alongside the user id. Today the under-scoping is masked because UAC/role/app-type saves still clear the whole Rails cache, but User/Admin saves clear entire Rails cache, causing template cache misses #1270 established the direction of removing whole-cache clears, so the key is hardened now rather than becoming the next latent instance of this defect class. Covered by a new spec (spec/models/admin/app_type_available_ids_cache_spec.rb) that stubs clear_rails_cache_on_save? to false to simulate the future no-clear state; verified to fail without the key change.
2. remake_from_attributes mutating cached hashes — now uses attribs.except('admin_id') instead of delete, so shared cached attribute hashes are never mutated. Relevant now that the per-resource cache writes in access_for_list? are effective.
3. pages#template version-token race — tracked by this issue itself; not changed on this branch.

Two platform caveats surfaced while writing the new spec:

• latest_update values are interpolated into cache keys via Time#to_s, i.e. second granularity — two access-control changes within the same second do not rotate dependent cache keys. Pre-existing behaviour across cache_key_for_access_for and related keys; harmless in normal operation but worth knowing when reasoning about cache invalidation (and when writing specs — the new spec uses an explicit updated_at offset to avoid same-second collisions).
• Spec-support setup_access/enable_user_app_access call auto_admin, which may create an Admin whose save clears the whole Rails cache — this silently masks cache-persistence bugs in specs. Tests probing cache behaviour should create access controls directly.

Prior commit e2361d6c4 on the same branch contains the primary fix (viewable_tables app_type scoping, preloadables key, access_for_list? per-item write) with model + system specs, all verified to fail without the fixes. Full regression set green.