Skip to content

security: send lock_id via X-CacheKit-Lock-Id header, not query string (#131)#179

Merged
27Bslash6 merged 1 commit into
mainfrom
fix/131-lock-id-header
Jun 11, 2026
Merged

security: send lock_id via X-CacheKit-Lock-Id header, not query string (#131)#179
27Bslash6 merged 1 commit into
mainfrom
fix/131-lock-id-header

Conversation

@27Bslash6

@27Bslash6 27Bslash6 commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Closes #131.

The distributed-lock capability token was sent as ?lock_id= on the unlock DELETE. Query strings are routinely captured by access/proxy logs and OpenTelemetry http.url spans (CWE-532), letting anyone with log access replay the token to release a lock within its ~30s TTL.

Change

  • CachekitIOBackend._release_lock now sends the token in the X-CacheKit-Lock-Id request header and drops it from the URL. New module constant LOCK_ID_HEADER.
  • The token is server-issued and sent verbatim (headers aren't URL-encoded; the server compares the raw value).

Wire compatibility

The SaaS unlock handler dual-reads the header + legacy ?lock_id= query (prefers header), shipped in saas#140 / deployed in 0.1.7. So this flip is compatible with live api.cachekit.io. Ratified in protocol spec/saas-api.md (the query param is removed in protocol 2.0).

Tests

  • The URL-injection cases now assert the token is isolated in the header and never touches the URL (no query smuggling possible by construction).
  • Added a positive header-transport assertion (X-CacheKit-Lock-Id carries the exact token).
  • SECURITY.md documents the transport.

Part of the cross-SDK #131 rollout (ts#63 / rs#24 to follow).

Summary by CodeRabbit

Release Notes

  • Security Fixes

    • Lock tokens for distributed lock operations are now transmitted via the X-CacheKit-Lock-Id request header instead of URL query strings.
  • Documentation

    • Updated security documentation detailing lock token transmission protocols and CWE-532 compliance guidance.

#131)

The distributed-lock capability token was sent as ?lock_id= on the unlock
DELETE. Query strings are captured by access/proxy logs and OpenTelemetry
http.url spans (CWE-532), letting anyone with log access replay the token to
release a lock within its ~30s TTL.

_release_lock now sends the token in the X-CacheKit-Lock-Id request header and
drops it from the URL. The SaaS handler dual-reads header + legacy ?lock_id=
(deployed in saas 0.1.7), so this is wire-compatible with prod. Ratified in
protocol spec/saas-api.md.

Tests updated: the URL-injection cases now assert the token is isolated in the
header and never touches the URL; added a positive header-transport assertion.
SECURITY.md documents the transport.
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5735ae72-a6a7-435e-a325-2047aa577fb2

📥 Commits

Reviewing files that changed from the base of the PR and between 07209aa and 72a867d.

📒 Files selected for processing (3)
  • SECURITY.md
  • src/cachekit/backends/cachekitio/backend.py
  • tests/unit/backends/test_cachekitio_lock_protocol.py

Walkthrough

This pull request relocates the distributed-lock token (lock_id) from the DELETE request URL query parameter to the X-CacheKit-Lock-Id HTTP request header to close CWE-532 credential leakage via debug logs and proxy access records. The backend constant, implementation, tests, and security documentation are all updated in sync.

Changes

Lock Token Header Protocol

Layer / File(s) Summary
Lock token header protocol definition
src/cachekit/backends/cachekitio/backend.py, SECURITY.md
Introduces LOCK_ID_HEADER = "X-CacheKit-Lock-Id" module constant encoding the protocol rule. SECURITY.md documents the wire-format change and CWE-532 leak closure, and adds the corresponding reference link.
Backend lock release implementation
src/cachekit/backends/cachekitio/backend.py
Lock release now sends the server-issued lock_id in the X-CacheKit-Lock-Id request header instead of the ?lock_id=... query parameter, whilst maintaining percent-encoding for the caller-controlled lock_key path segment.
Lock protocol conformance tests
tests/unit/backends/test_cachekitio_lock_protocol.py
Validates that DELETE requests carry the lock token in the X-CacheKit-Lock-Id header and assert its absence from the DELETE URL. Updated test docstring, endpoint path assertion (no query string), new header presence assertion, and rewritten injection/smuggling tests confirm the token does not leak via URL.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Possibly related issues

  • #131: Directly implements the main acceptance criteria—sending lock_id via X-CacheKit-Lock-Id request header instead of the URL query string to close CWE-532 capability-token-in-URL leak.

Possibly related PRs

  • cachekit-io/cachekit-py#130: Predecessor PR that hardened query-string encoding in _release_lock(); this PR completes the security fix by moving the token out of the URL entirely.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main security change: moving lock_id transmission from URL query string to X-CacheKit-Lock-Id header, directly addressing CWE-532.
Description check ✅ Passed The PR description provides thorough context: motivation (CWE-532 leak via logs), implementation details (header constant and wire protocol), backward compatibility notes, and test coverage, though the security checklist template was not formally filled.
Linked Issues check ✅ Passed The PR satisfies all acceptance criteria from #131: header-based token transport implemented, tests assert token isolation in header with no URL query leakage, SECURITY.md and wire protocol documented, and cross-SDK coordination noted.
Out of Scope Changes check ✅ Passed All changes are tightly scoped to the lock_id header transport requirement: backend implementation, test updates, and documentation are all directly tied to issue #131.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/131-lock-id-header

Comment @coderabbitai help to get the list of available commands and usage tips.

@27Bslash6 27Bslash6 merged commit 4cb00df into main Jun 11, 2026
14 of 15 checks passed
@27Bslash6 27Bslash6 deleted the fix/131-lock-id-header branch June 11, 2026 11:28
@codecov

codecov Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

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.

lock_id leaks via URL query string (CWE-532) — move to request header

1 participant