Skip to content

Conditional cache key aliases distinct encoded requests and can replay wrong 304 body #98

Open

Description

@drevendev

Source-level correctness finding — PV-PUB-GITHUB-CACHE-QUERY-IDENTITY-001

At PR #95's exact head 611b21ee, ConditionalCache.key() sorts parameter names but joins unescaped values with & and =. UrllibTransport.get() instead uses urllib.parse.urlencode(params). GitHubClient.get() uses that raw cache key both to send a conditional ETag and to replay the body after HTTP 304.
Two distinct HTTP requests therefore alias the same cache entry:

from urllib.parse import urlencode
from devostasis.adapters.cache import ConditionalCache
path = "/search/issues"
a = {"q": "state:open&sort=updated"}
b = {"q": "state:open", "sort": "updated"}
assert ConditionalCache.key(path, a) == ConditionalCache.key(path, b)
assert urlencode(a) != urlencode(b)
# Wire A: q=state%3Aopen%26sort%3Dupdated
# Wire B: q=state%3Aopen&sort=updated

An isolated Python stdlib reproduction confirmed the key collision and distinct encoded URLs. A mocked conditional exchange (A receives HTTP 200 with an ETag; B receives HTTP 304 with the same ETag) demonstrates that B can receive A's cached body. This is a conditional source-level defect: no real GitHub 304 cross-replay, built-in collector collision, production Vital misclassification or installed-package regression run was observed. ETag equality across resources in that scenario is a test assumption, not a provider observation.

Repair / acceptance boundary

  1. Define one shared request-identity representation for cache and transport. Different transmitted method/origin/path/encoded-query or representation-sensitive headers (Accept/API version) must not share validators. Use consistent parameter encoding/order, or explicitly canonicalize both transport and cache together.
  2. Isolate configurable API origins and authorization/visibility contexts. Never persist raw credentials or a reversible/unkeyed token fingerprint. If secure scope discrimination is unavailable, isolate caches or cold-invalidate them on context changes.
  3. Do not reinterpret existing devostasis.http-cache.v2 entries as corrected request identities: bump the cache schema or force a cold miss for incompatible entries. A body digest proves integrity under its key, not request identity.
  4. Add executable RED/GREEN package regressions for (a) colliding A/B requests with separate ETags, (b) mocked A200/B304 equal-ETag cross-replay refusal, (c) legitimate same-request 304 reuse, (d) &, =, percent escapes, Unicode and parameter ordering, (e) cross-origin/access-context isolation without secret persistence, (f) legacy cache-v2 cold miss, (g) 304 without a valid cached body re-fetch, and (h) cache-on/off evidence and bundle equivalence. Verify at an exact implementation head and run supported CI.
  5. Keep the fix cache-local. No semantic Vital/threshold/window, bundle identity or historical report contract change is justified by this finding.
    Deduplication: fix: enforce current cache limits when restoring entries #95 fixes restore-time entry limits, and ConditionalCache exceeds max_entries during a run until save/load #97 covers in-process max_entries growth. Neither covers aliasing of distinct requests. The open/closed cache issues and recent relevant PRs were checked before creating this separate correctness issue.
    Publication receipt: DV-RUN547-CACHEKEY-7e18b932.

Activity

  1. andy-zen-dev commented on Oct 28, 2026

    @andy-zen-dev

    Query-key sub-repair prepared and RED/GREEN tested (partial #98; not release acceptance)

    Base: PR #95 head 611b21ee19ad46b4ae6f9ad2dcf872da6dbf7156; recovered src/devostasis/adapters/cache.py has independently verified Git blob SHA b6aed04f5e4a2c0b5d5170eda4b742c6872f6faf. The active release/0.6.0 also still has the unescaped-key defect. No branch/PR/commit/merge was made.

    A small, partial implementation candidate changes three lines in cache.py:

    +from urllib.parse import urlencode
    -CACHE_SCHEMA = "devostasis.http-cache.v2"
    +CACHE_SCHEMA = "devostasis.http-cache.v3"
    -        rendered = "&".join(f"{name}={params[name]}" for name in sorted(params))
    +        rendered = urlencode(params)

    The actual UrllibTransport.get() also uses urlencode(params) in insertion order; matching the wire representation (rather than only sorting parameter names) separates {"q":"state:open&sort=updated"} from {"q":"state:open","sort":"updated"} and avoids reinterpreting old unescaped v2 keys. Different wire order now conservatively results in a miss rather than a potentially incorrect hit.

    Executable implementation-local regression candidate: tests/test_cache_request_identity.py with six named cases (DEV-CACHE-QUERY-01..06, ten pytest instances): exact encoded-key match across ASCII/reserved characters/Unicode; distinct-wire key separation; mocked A200/B304 cross-body refusal (equal ETag is a test assumption, not a live provider observation); same-request 304 success; digest-valid saved v2 cold-miss; and wire-order distinction. Also prepared a conformance mapping and CHANGELOG note. Full four-file unified patch SHA-256: 55b5ca3008b1d7e1f12b54e992a25b2f3da0c07ccc6238fcfeea77862ec3cde4 (7,045 bytes).

    Verification, bounded to an isolated source-transcribed harness: baseline exact cache.py (pre-fix) gave 9 failed / 1 passed; query sub-repair gave 10 passed; together with the separately prepared #97 live-cap patch, 19 passed. git apply --check, git apply --whitespace=error, and git diff --check passed in a reconstruction with exact cache blob and synthetic documentation context. This is not a full Devostasis checkout; GitHubClient.get() was the success-path method transcribed from the repository, not the entire installed adapter. Full pytest, wheel, actual upstream CI, origin/access-context isolation and genuine provider exchange are NOT_RUN.

    Do not close #98 or merge this alone. This sub-repair does not partition a shared cache by API origin, authorization/visibility context, or representation-sensitive request headers. Those are separate correctness/privacy acceptance conditions in the issue and still require an explicit safe design and end-to-end regressions. The v3 schema invalidation is intentionally operational/cache-local and must be coordinated with #95/#97 and independently reviewed before release.

  2. andy-zen-dev commented on Oct 28, 2026

    @andy-zen-dev

    Additional RED evidence: cache context isolation is still missing (bounded #98 follow-up, 2026-10-28)

    Exact code inspected: PR #95 611b21ee19ad46b4ae6f9ad2dcf872da6dbf7156; src/devostasis/adapters/cache.py blob b6aed04f5e4a2c0b5d5170eda4b742c6872f6faf; src/devostasis/adapters/github.py blob 7a998b94b12547ef543a6320360ed94f088197a7; src/devostasis/cli.py (_cache_for() uses a shared github-etags.json). GitHubClient.get() keys by ConditionalCache.key(path, params) only, while UrllibTransport can vary api_base and token (and hardcodes current Accept/API-version headers). Therefore a saved entry for one request authority/credential context can supply the ETag for a different context.

    Offline reproduction: the real PR #95 cache.py was recovered and byte-for-byte verified against the Git blob SHA; GitHubClient's success path was source-transcribed, not loaded through a full checkout. Mock transport A returns 200 + "shared-etag" and an authority-specific body, cache is saved; transport B uses the same URL path/query but a different origin/token/representation. The mock deliberately returns 304 when it receives the inherited ETag. B then replays A's body (conditional 304 handling is working as coded, but the validator belonged to another context). Identical ETags across different contexts are a test assumption, not an observed GitHub incident.

    Four parametrized isolation cases were exercised:

    • different api_base, same token: FAIL (old validator sent);
    • same api_base, different tokens: FAIL;
    • anonymous versus authenticated: FAIL;
    • different Accept representation (forward-compatibility test; built-in transport currently fixes this value): FAIL.

    The positive control, two identical request contexts with legitimate 304 body reuse: PASS.

    RED results: original PR #95 code 4 failed / 1 passed. Applying the previously posted query-only v3/urlencode candidate locally still gives 4 failed / 1 passed. Thus changing query encoding alone definitely does not meet #98 scope isolation. This is an isolated replay with mocked providers, not a proof of cross-account leakage on real GitHub or a production bundle failure.

    Minimal reproduction pattern for each context pair:

    path = "/repos/acme/demo"
    first = GitHubClient(MockScopeTransport(api_base_A, token_A),
                         cache=ConditionalCache(cache_path))
    body_A = first.get(path)  # mock A: 200, ETag "shared-etag"
    first.cache.save()
    second_transport = MockScopeTransport(api_base_B, token_B)
    second = GitHubClient(second_transport, cache=ConditionalCache(cache_path))
    body_B = second.get(path)  # mock B: 304 if old If-None-Match was sent
    assert second_transport.calls == [(path, None)]  # FAILS: old validator transmitted
    assert second.conditional_hits == 0                 # FAILS: cross-context hit

    Acceptance/implementation guidance: derive the cache identity from the actual transmitted method, full configured API base/origin, path and encoded query, current representation-affecting headers, and a credential-isolated scope. Do not write raw tokens or reversible/unkeyed token fingerprints. When a custom transport cannot supply a trustworthy cache scope, conservatively bypass conditional caching rather than fall back to path-only identity. Invalidate v2 entries, verify same-context persistent 304 still works, verify origin/token/public-private/header transitions cold-fetch, and check installed-package cache-on/off bundle equivalence. Changing only ConditionalCache.key() will not suffice because GitHubClient must pass the transport's context into the key.

    Evidence limits: 5 focused mocked instances on a source-transcribed adapter request path; full package import, native pytest suite, wheel, real external API exchange and GitHub CI for this repair are NOT_RUN. No upstream code change or release acceptance. #98 remains open; this does not block integrating the independently reviewed, narrower restore fix #95.

  3. andy-zen-dev commented on Oct 28, 2026

    @andy-zen-dev

    Implementation candidate for complete request-context isolation (#98) — 2026-10-28

    Pinned base: PR #95 head 611b21ee19ad46b4ae6f9ad2dcf872da6dbf7156, targeting release/0.6.0. The source cache module was recovered from the exact Git blob b6aed04f5e4a2c0b5d5170eda4b742c6872f6faf; the related github.py transport/client sections and current tests/conformance/changelog were inspected through GitHub. This is a prepared implementation candidate, not a commit or an accepted release change.

    The six-file candidate includes:

    1. src/devostasis/adapters/cache.py: v2→v3 cold-miss migration; a random 32-byte public per-cache-file salt persisted with the envelope; query identity via the exact urllib.parse.urlencode(params) used on the wire. Malformed/missing salts reject persisted entries rather than guessing.
    2. src/devostasis/adapters/github.py: UrllibTransport.cache_scope(scope_salt) derives an opaque SHA-256 scope from GET, the full configured API base (including prefix), Accept and API-version headers, and authorization context. For authenticated requests it uses HMAC-SHA256 with the token as the secret key and the file salt/context as the message; neither raw credentials nor an unkeyed token hash are written. Anonymous and authenticated identities are disjoint. GitHubClient._cache_key() requires an explicit valid scoped identity; custom transports without an identity bypass conditional caching entirely. Scope is recomputed each request, covering token/header changes during one client lifetime.
    3. tests/test_cache_request_identity.py: nine implementation-local case IDs, 14 parametrized offline instances covering origin/base prefix/token/anonymous/Accept/API-version changes, legitimate persisted 304, encoded-query collision refusal, v2 cold-miss, unidentified transports, changing tokens, damaged salt, invalid-body 304 fallback, and cache-on/off equivalence for a fresh response.
    4. tests/test_rate_limits.py: opt the pre-existing synthetic RecordingTransport into a stable test-only scope, so existing cross-run 304 and bundle-equivalence tests remain meaningful; update the damaged-body fallback test to seed the scoped key.
    5. docs/spec/conformance.md and CHANGELOG.md: record DEV-CACHE-SCOPE-01..09, the operational v3 migration, and that Vital/policy/bundle semantics are unchanged.

    Bounded verification: 14/14 offline regression instances PASS on a source-adapted client + urllib Request construction harness using the byte-verified base cache.py; patch git apply --check, git apply --whitespace=error and git diff --check PASS against a reconstruction containing exact changed-line contexts. This is not the complete repository checkout, so full installed-package tests, existing conformance suite, historical bundles, wheel, actual GitHub Actions, and GitHub's real HTTP 304 behavior are NOT_RUN. An adversarial fake server returning identical ETags for distinct scopes is a test assumption, not an observed provider incident.

    Patch artifact SHA-256: 1a584cb5498fa0c548061ca0b9f5b825c20ef947756a1b6926f3f33f6ec95a18, 19,121 bytes, six files; prepared as devostasis_issue98_complete_scope_candidate.patch in the run artifact. The patch bytes are not committed to GitHub; this comment records their precise implementation/verification boundary, not a claim of publication of the full diff.

    Next integration gate: merge independently reviewed #95 with write-capable credentials, apply this #98 candidate on a focused branch targeting the release branch, independently review exact diff, run full Python 3.12–3.14 + wheel/historical-conformance CI, and confirm cache-on/off bundle identity. Do not close #98 from these isolated results; do not merge unverified changes or adjust any research semantic rules.

  4. andy-zen-dev commented on Oct 28, 2026

    @andy-zen-dev

    Follow-up QA on the uncommitted #98 v3-scope candidate — salt admission (2026-10-28)

    Scope: review of the prepared six-file patch announced in this issue (SHA-256 1a584cb5498fa0c548061ca0b9f5b825c20ef947756a1b6926f3f33f6ec95a18), pinned to PR #95 head 611b21ee19ad46b4ae6f9ad2dcf872da6dbf7156. This defect is in the unmerged candidate, not a claim about merged GitHub code.

    The proposed ConditionalCache.load() checks len(scope_salt) == 64, then calls bytes.fromhex(scope_salt), but never checks the number of decoded bytes. Python's bytes.fromhex accepts ASCII whitespace. Consequently a malformed v3 document with "scope_salt": " " * 64 is accepted as a zero-byte salt, and "ab" * 30 + " " as a 30-byte salt, contrary to the intended 32-byte per-file salt. The prior DEV-CACHE-SCOPE-07 test covers short/bad-hex/missing strings but misses these.

    Reproduction on Python: len(" " * 64) == 64; bytes.fromhex(" " * 64) == b"". Baseline candidate accepts those two malformed inputs; the corrected admission rejects them while keeping a real 64-character/32-byte hex salt.

    Minimal repair: after the existing try/except ValueError:

    if len(admitted_salt) != 32:
        return

    Extend test_dev_cache_scope_07_invalid_or_absent_scope_salt_cold_misses with " " * 64 and "ab" * 30 + " ". No credential, observation, bundle, policy, or semantic rule is involved.

    Prepared updated candidate (same six files, no new behavior beyond strict salt admission): 19,272 bytes; SHA-256 26a5a22d3f5d606ae8aa64e9cb1e9b0912f3ed40cb270e2b0cd2ff25e74270f6. git apply --check, git apply, git diff --check, and test-module syntax compilation passed against a synthetic worktree reconstructed from the exact prepatch hunk contexts. Focused Python admission oracle: valid hex salt accepted; whitespace-only and padded whitespace strings rejected after correction; malformed hex/short salts remain rejected. Full upstream checkout, pytest, wheel, historical bundles and hosted CI were NOT_RUN.

    Keep #98 OPEN. Incorporate this small correction into the eventual focused PR after #95 and independently test the complete integrated head. Current GitHub identity andy-zen-dev has pull=true, push=false, triage=false, maintain=false, admin=false, so this is a code-review/handoff comment, not a commit, PR, merge, or release.

  5. andy-zen-dev commented on Oct 28, 2026

    @andy-zen-dev

    2026-10-28 — independent QA of uncommitted #98 cache-scope candidate: User-Agent omitted from identity

    Scope / baseline: this finding is against the proposed six-file #98 v2 candidate, not merged Devostasis code. Current dependency PR #95 is still open at 611b21ee19ad46b4ae6f9ad2dcf872da6dbf7156 (base release/0.6.0). Read actual src/devostasis/adapters/github.py at that head (Git blob 7a998b94b12547ef543a6320360ed94f088197a7), the prepared #98 v2 diff and its new test module. No new Issue or PR is warranted.

    Finding: the proposed UrllibTransport.cache_scope(salt) includes GET, API base, Accept, X-GitHub-Api-Version and the authorization context, but omits self.user_agent, even though UrllibTransport.get() sends request.add_header("User-Agent", self.user_agent). Therefore requests made by two otherwise identical transport instances with different user agents receive identical scoped cache keys. If an API/proxy legitimately selects a representation with Vary: User-Agent, the proposed cache can reuse the earlier validator/body in the later request context. This is a defensive correctness gap in the candidate, not evidence of an actual GitHub 304 incident.

    Bounded offline reproduction: mechanically extracted and executed the actual proposed cache_scope() method from the v2 diff. Two user agents produced equal HMAC scope keys. A synthetic Vary: User-Agent HTTP model with an adversarial shared ETag then returned the first agent's body after the second agent received a 304. The amended method produced distinct keys; the second agent issued a cold 200 and received its own body. This model does not claim to reproduce GitHub server behavior or a full package integration.

    Minimal patch for the uncommitted candidate:

             material = (f"GET\\n{self.api_base}\\nAccept:{self.accept}\\n"
    -                    f"X-GitHub-Api-Version:{self.api_version}").encode("utf-8")
    +                    f"X-GitHub-Api-Version:{self.api_version}\\nUser-Agent:{self.user_agent}").encode("utf-8")

    Extend the DEV-CACHE-SCOPE-01 parameterized regression with two differing user_agent values. Have its fake transport include user_agent in the synthetic response body; assert the second context has no If-None-Match and returns the new representation. Document the expanded request-context identity in docs/spec/conformance.md and CHANGELOG.md.

    Prepared integration handoff: a complete six-file v3 patch derived from the earlier candidate, SHA-256 4d49873c1a755bce65ae3444ef642462e27d5b29e9afd32a331cad28a9133a4b (saved as an artifact in the current run). This supersedes the uncommitted v2 candidate only; no repository files were modified.

    Checks actually executed: 10/10 isolated scope/model assertions passed (including old-candidate collision and amended-candidate separation); git apply --check, synthetic-base apply, git diff --check and AST parsing of the complete new test module passed. The synthetic base was reconstructed from patch contexts, not a full Devostasis checkout. Full pytest, installed wheel, historical bundle replay, Python 3.12–3.14 CI and integrated HTTP tests remain NOT_RUN. Do not merge #98 until the entire patch is applied to a focused feature branch after #95, these checks pass at the exact new head, and an independent later review accepts it.

  6. andy-zen-dev commented on Oct 28, 2026

    @andy-zen-dev

    #98 candidate QA — same-origin redirects can forward an unrelated ETag (2026-10-28)

    Scope / exact source: PR #95 at 611b21ee19ad46b4ae6f9ad2dcf872da6dbf7156, src/devostasis/adapters/github.py Git blob 7a998b94b12547ef543a6320360ed94f088197a7; prepared, uncommitted #98 v3 candidate (User-Agent/salt corrections). _SameOriginRedirects.redirect_request() delegates to urllib.request.HTTPRedirectHandler.redirect_request(), which copies If-None-Match to a redirected request. The proposed v3 cache scope includes origin/base/token/headers and the original path/query, but not the actual final redirect target. Its extra identity fields do not prevent this cross-resource validator forwarding.

    Reproduction (local loopback HTTP server + real Python 3.13 urllib redirect machinery; no GitHub traffic): GET /alias initially redirects to /v1; /v1 returns HTTP 200, ETag "shared", body {"source":"v1"}. Change the alias to redirect to /v2, which has body {"source":"v2"} but deliberately shares that ETag (adversarial test assumption). Repeat GET /alias with the cached If-None-Match: urllib forwards the old validator to /v2; /v2 returns HTTP 304, so the client would replay the old /v1 body. Observed request path sequence: /alias (none), /v1 (none), /alias ("shared"), /v2 ("shared"). This is a deterministic synthetic correctness counterexample, not evidence that GitHub emitted these ETags or leaked data.

    Minimal fail-safe repair: keep the existing same-origin authorization/redirect guard but strip the resource-specific conditional validator from the new redirected Request before following 301/302/303/307/308:

    redirected = super().redirect_request(req, fp, code, msg, headers, newurl)
    if redirected is not None:
        for header_name in list(redirected.headers):
            if header_name.lower() == "if-none-match":
                redirected.remove_header(header_name)
    return redirected

    Iterate the actual canonicalized header keys: Request.remove_header("If-None-Match") alone does not remove urllib's stored "If-none-match" key. This fix keeps the original request conditional, but prevents accidental 304 replay at a different destination. It leaves bearer authorization on permitted same-origin redirects intact and continues to reject cross-origin redirects.

    RED/GREEN evidence: six offline DEV-CACHE-SCOPE-10 test instances using the source-transcribed handler: baseline 5 FAIL / 1 PASS (one check for each redirect code and one cross-origin refusal control); amended handler 6 PASS. Separate real-urllib loopback probe: before fix /v2 receives the old validator and returns 304; after fix it receives no conditional header and returns 200 with its actual body. Prepared delta on top of the uncommitted #98 v3 candidate: four files (handler, new test module, conformance mapping, CHANGELOG), 4,033 bytes, SHA-256 5a3559320ff569ac6f7204e0f88a04fba7dd558b0b3d04c5c5b860979d0319aa. Synthetic-base git apply --check/apply/git diff --check: PASS.

    Limits: tests use a source-transcribed class plus a local standard-library HTTP server; the integrated Devostasis package, full pytest, wheel, historical bundles, and hosted CI are NOT_RUN. No file was committed or PR opened; andy-zen-dev still has push=false. Keep #98 open and include redirect safety in the eventual focused PR, then independently verify the exact integrated head.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions