fix: atomic etag validation for document PUT/POST to fix TOCTOU race condition

[chomatdam]

Summary

Fixes a race condition where concurrent document writes could silently overwrite each other despite correct use of If-Match / If-None-Match concurrency headers.

Problem

The etag validation for document resources (state, agent profile, activity profile) happens in the etags-preproc interceptor — a separate stage that runs before the actual write in -set-document. Between the check and the write, another client can modify the document:

1. Client A: GET /state → etag "abc123"
2. Client B: PUT /state (writes new content, etag becomes "def456")
3. Client A: PUT /state with If-Match: "abc123"
   • etags-preproc reads document → etag is now "def456" → 412 ✓
   • Client A retries with GET → gets "def456"
   • Client A: PUT with If-Match: "def456"
   • etags-preproc validates → pass
   • Another write sneaks in between check and write
   • -set-document overwrites blindly → data loss

Fix

Move the etag validation into -set-document itself, inside the same jdbc/with-transaction block as the write:

1. Extract If-Match / If-None-Match headers from the Pedestal interceptor context (now passed through to -set-document)
2. Query the current document within the transaction
3. Compute the etag from the current document contents (SHA-1, matching the upstream com.yetanalytics.lrs.util.hash/sha-1 implementation)
4. Validate preconditions against the current etag
5. Only proceed with the write if validation passes
6. Return {:error ex-info} with :com.yetanalytics.lrs.xapi.document/precondition-failed type on failure, which the upstream route handler surfaces as HTTP 412

When no concurrency headers are present, the write proceeds directly with no overhead.

Changes

• src/main/lrsql/util/document.clj — New etag utility functions:
  • compute-etag: SHA-1 hash matching upstream implementation
  • parse-etag-header: Parse If-Match/If-None-Match header values
  • check-etag-precondition: Validate preconditions against current doc
• src/main/lrsql/system/lrs.clj — Modified -set-document:
  • Changed _ctx → ctx to access request headers
  • Atomic query + validate + write within single jdbc/with-transaction

Companion PR

Depends on yetanalytics/lrs#106 (fix/atomic-etag-validation) for 412 error handling in put-response and post-response.

Testing

Tested together with the companion lrs branch via {:local/root "../lrs"}:

• make test-sqlite: 88 tests, 923 assertions, 0 failures, 0 errors
• xAPI 1.0.3 conformance: 1365/1365 passed
• xAPI 2.0.0 conformance: 1435/1435 passed

New src/test/lrsql/etag_race_test.clj reproduces the TOCTOU race condition with 10 concurrent PUT/POST requests sharing the same If-Match etag:

• On main: 6 failures — all 10 writes go through, stale etags ignored, If-None-Match * doesn't prevent overwrites
• On fix/atomic-etag-validation: 0 failures — exactly 1 write succeeds, 9 rejected with precondition failure

[emmanuel]

Is there something we need to do for this PR to be considered/reviewed/accepted?

More broadly, are the project maintainers interested in outside contributions, or is this a "one-way OSS" project?

Please understand that there is no acrimony in the question. Your (project maintainer's) help calibrating expectations is appreciated.

[chomatdam]

@cliffcaseyyet we are running a hard fork with this race condition fix since 6 months, any chance we could make progress on this PR ? (goes with yetanalytics/lrs#106)

[cliffcaseyyet]

@cliffcaseyyet we are running a hard fork with this race condition fix since 6 months, any chance we could make progress on this PR ? (goes with yetanalytics/lrs#106)

@chomatdam Yes, apologies we fell behind on some of these but we appreciate the contribution and are catching up today. I should have an answer for you soon.

[cliffcaseyyet]

Is there something we need to do for this PR to be considered/reviewed/accepted?

More broadly, are the project maintainers interested in outside contributions, or is this a "one-way OSS" project?

Please understand that there is no acrimony in the question. Your (project maintainer's) help calibrating expectations is appreciated.

@emmanuel Hi, totally fair question. We fell behind on these contributions and we are working to catch up right now, this one we should have feedback or approval on shortly. We do appreciate outside contributions and will be more responsive in the future.

[cliffcaseyyet]

@chomatdam
Thanks again for your patience. This is a valid issue and we're appreciative that you raised it and did all this work to make the issue clearer. After spending some time with this issue we realized a few things:

• Wrapping the check in the transaction helps in many cases, mainly SQLite, because it is processing the requests sequentially at the database layer. When we apply the fix to concurrency-enabled DBMSs like Postgres, however, we still see the potential for conflicts because of the transaction isolation mode we are running those DBs in.

• We also realize there are a number of other resources which may have this issue, so we see the need to expand a fix to those areas as well.

On our end we are planning to implement an in-db fix which involves a conditional update so that the SQL query itself is protecting against bad updates. We will tag you on the PRs as we make the appropriate changes in the LRS project and this one.

[cliffcaseyyet]

@chomatdam if you didn't see it already, this is our approach to these issues. We will be merging and cutting a release soon as it goes through review. #520