Skip to content

fix(cache): cache private redirects via a private column - #20

Draft
ronag wants to merge 2 commits into
mainfrom
fix/cache-private-redirects
Draft

fix(cache): cache private redirects via a private column#20
ronag wants to merge 2 commits into
mainfrom
fix/cache-private-redirects

Conversation

@ronag

@ronag ronag commented Jun 27, 2026

Copy link
Copy Markdown
Member

Summary

Caches responses marked Cache-Control: private (redirects) instead of dropping them, tracking the flag in a new private column on the SQLite store so they are bypassed on read rather than shared across clients (the cross-client propagation for redirect-following is left as a TODO).

While reviewing, the in-progress change was non-functional — it would not parse, construct, or survive a cache miss. This PR fixes those bugs so the feature actually works:

# Bug Fix
1 private is a strict-mode reserved word — used as a destructuring binding + variable in #flush, so the module failed to parse (SyntaxError) bind under a safe name (private: isPrivate)
2 Missing commas in CREATE TABLE, SELECT, and INSERT column lists added
3 node:sqlite rejects JS booleans as bind params (TypeError) persist the flag as 0/1, convert back with Boolean()
4 Schema changed but VERSION not bumped — an existing cacheInterceptorV10 table would lack the column bumped to 11
5 entry.private threw Cannot read properties of undefined on every cache miss guard with entry?.private
6 statusCode !== 30 typo; redirect set also missing 303 fixed to 302, added 303

Known limitation (not changed here)

onHeaders only admits 307/200/206 (existing behavior), so the private-redirect allow-list is currently only reachable for 307. Broadening which redirect codes are cacheable is a larger, separate behavioral change and out of scope for this fix.

Test plan

  • tap test/sqlite-cache-store.js — 154 assertions pass
  • tap test/cache.js test/cache-advanced.js — 88 assertions pass
  • tap test/review-bugfixes-cache.js test/review-bugfixes-cache-2.js test/review-bugfixes-store.js — 42 assertions pass
  • private-flag round-trip smoke test through the store
  • prettier + eslint clean

🤖 Generated with Claude Code

@ronag
ronag force-pushed the fix/cache-private-redirects branch from 487d25f to f0e0c10 Compare June 27, 2026 16:04
Store responses marked Cache-Control: private (redirects) instead of
dropping them, tracking the flag in a new `private` column so they are
bypassed on read rather than shared across clients.

Also fixes several bugs in the change so it actually parses/runs:
- 'private' is a strict-mode reserved word; bind it under a safe name in
  the flush destructuring/run() call
- add the missing commas in CREATE TABLE / SELECT / INSERT
- node:sqlite cannot bind a JS boolean, so persist the flag as 0/1
- bump store VERSION to 11 for the schema change
- guard the cache-miss case (entry?.private) to avoid a TypeError
- fix the redirect status-code allow-list (302 typo, add 303)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ronag
ronag force-pushed the fix/cache-private-redirects branch from f0e0c10 to 639407e Compare June 27, 2026 16:04
@ronag
ronag requested a review from Copilot June 27, 2026 16:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes and completes support for persisting Cache-Control: private redirect responses in the SQLite-backed cache by storing a per-entry “private” flag and ensuring those entries are bypassed on reads unless explicitly opted in. It aligns the implementation with the intended behavior (persist + round-trip the flag; don’t share private entries across callers by default).

Changes:

  • Bumped the SQLite cache schema version and added a private column, including SQL/query updates and 0/1 persistence for node:sqlite bindings.
  • Propagated Cache-Control: private into cached entries and adjusted caching logic to still allow caching specific redirect status codes even when private.
  • Added read-path bypass logic so cached private entries aren’t served unless explicitly requested.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/sqlite-cache-store.js Adds private column + version bump, persists the flag as 0/1, and round-trips it back into cache entries.
lib/interceptor/cache.js Adjusts cacheability rules for private redirects, writes the private flag into stored entries, and bypasses private entries on read unless opted in.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/interceptor/cache.js

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread lib/sqlite-cache-store.js
Comment thread lib/interceptor/cache.js Outdated
- Type the private cache surface in lib/index.d.ts: CacheOptions.private
  (read opt-in), CacheValue.private and CacheGetResult.private so the
  store API matches what it stores/returns.
- Correct the SqliteStoreValue JSDoc: the persisted/batched field is
  0 | 1 (node:sqlite cannot bind booleans), not boolean.
- Document why the private-redirect allow-list spells out the full
  redirect set even though only 307 passes the onHeaders admit guard
  today.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

Comment thread lib/interceptor/cache.js
Comment on lines +309 to +311
if (entry?.private && !opts.cache.private) {
entry = null
}
@ronag
ronag marked this pull request as draft July 2, 2026 07:58
@ronag

ronag commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

This should not merge in its current design.

The persisted private flag plus the caller-controlled cache.private boolean is not a private-cache partition. The entry is still written into the same shared SQLite store under the same URI/Vary key, and any later caller that sets cache.private can retrieve the most recently stored private redirect, regardless of which user caused it to be cached. A private Location can itself contain user-specific or capability-bearing data, so default-bypass does not remove the cross-user disclosure risk.

RFC 9111 section 5.2.2.7 permits a private cache dedicated to one user to store an unqualified private response; it requires a shared cache not to store it. A boolean read opt-in does not turn a shared store into a cache dedicated to one user.

Safe directions would be:

  1. Keep rejecting unqualified private responses in the shared cache.
  2. Require the application to supply a genuinely per-user CacheStore whose lifetime and namespace cannot cross users.
  3. Add an explicit, non-forgeable cache partition identity to both lookup and storage keys, and document that the resulting cache instance is private. A boolean flag alone is insufficient.

This is independent of the SQL/parser fixes already listed in the PR; those make the implementation run, but do not establish the isolation property the feature needs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants