A write to a document that exists must say which state it was written against - #392
Merged
Merged
Conversation
… against. Every write in DocumentHierarchyGraphStoreImpl is a whole-graph read-modify-write: the graph is read, changed in memory and written back. Two writers that name no precondition therefore overwrite each other with nothing to show that anything was lost. The machinery to prevent that was already here and already worked - a stale If-Match answers 412 - but it was opt-in, and evaluatePreconditions() is vacuous for a request that carries no conditional header, so a client simply not sending one got last-write-wins. Measured in the constructor editor: one save's DELETE was correct in every detail, answered 204, and was reinstated by a second write that had read the graph before it landed. A write to an existing document without If-Match is now 428 Precondition Required. Creating a document is exempt: it has no state to have been written against, which is what If-None-Match rather than If-Match would express. The CLI writes conditionally to match, in LDHClient rather than at fifteen call sites, so every command inherits it. Two things the running stack taught, both of which would otherwise 412 every time: - an entity tag identifies a NEGOTIATED VARIANT, not the graph. The same document answers "5a4df28aea61e278" as RDF/XML and "5a4df28aec9db937" as Turtle - one model digest, two variant suffixes - so the conditional read has to send the same Accept as the write it is for. - delete() and post() defaulted to an empty accept list, which gets the HTML shell, whose tag no write will match. CLI writes now always negotiate RDF, which is what an RDF client wanted in any case. PushOutputTest asserted the old request sequence and its fixed indices; both now assert the conditional protocol, including that a failed write still stops the run before the next document. Verified against the running stack: no header 428, stale 412, correct 204, and ldh create -> patch -> delete end to end. /settings is a separate resource with no precondition check and is deliberately not covered here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…unts as one. Making the precondition mandatory left 40 tests writing unconditionally to documents that already exist, so they answered 428 where they expected 204, 400 or 422. Under `curl -f` that prints nothing and `set -e` aborts, so most of them failed with an empty log - including imports/import-internal-url-400.sh, where the 428 arrived before the SSRF check and the test stopped proving that a link-local ldh:file is rejected at all. A security assertion that fails for an unrelated reason is worse than one that fails loudly, which is what made this worth finishing rather than leaving the tests red. run.sh grows an exported `etag` helper. It reads the validator with the same Accept the write will send, because an entity tag identifies a NEGOTIATED VARIANT and not the graph: the same document answers "5a4df28aea61e278" as RDF/XML and "5a4df28aec9db937" as Turtle, and quoting the wrong one is refused with 412 rather than accepted. It says so on stderr when a document has no tag, because the alternative - handing back an empty string - produced exactly the unexplained 428 it was meant to prevent. Two writes stay unconditional, on purpose: - those asserting 401/403. A request that is going to be refused for authorization carries no precondition, because the agent cannot read the document to get one; requiring it would turn every "access is unauthorized" test into a 428 and stop testing authorization. - those creating a document. There is no state to have been written against, and the server exempts them. A proxied write reads its validator through the proxy (`?uri=`), not from the proxy's own base document, which is a different resource with a different tag. The server now treats a blank If-Match as absent. Measured while fixing the tests: `-H "If-Match: "` answered 204, because getHeaderString returns "" rather than null - so a client could opt out of the precondition entirely by sending the header empty, and a test whose validator lookup silently failed would have gone on writing unconditionally. Locally 140/73 ok/failed becomes 182/31. Of what remains, 15 need the test.localhost dataspace this working copy does not define, one needs SSRF protection that .env disables here (ALLOW_INTERNAL_URLS=true), and the rest are left to CI, which runs the suite in a clean environment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eight writes went out unconditionally, and against a server that now requires If-Match of any write to a document that already exists, each of them is a 428 the agent cannot act on. The Import ontology dialog is how it surfaced: its chain appends twice into the target, and with the first append refused the chain stops, so #add-data never closes and the UI test waits ninety seconds for a modal that will not go. The same shape as the constructor editor's Save, and the same cause - the close lives at the end of a chain, so any earlier failure is indistinguishable from a hang. The rest of the client already wrote conditionally, taking the validator from LinkedDataHub.contents. That only holds documents THIS browser fetched, which is why the gap falls where it does: every write that was unconditional targets a document the page never loaded, or one it loaded but whose write path predates the convention. - client.xsl: the document DELETE and the RDF file drop, both on the document being read, so the cached validator serves. - block.xsl: removing a saved block and reordering blocks, likewise on the current document. - memento.xsl: restoring a version, which overwrites the whole graph - without a validator it would discard anything written since the timemap was rendered. - modal.xsl: the import chain's two appends and the add/clone append. These target a document the agent NAMES in the form rather than one the browser loaded, so nothing is cached for it: the chain HEADs the target once, and the second append reuses the entity tag the first one's response carries, because a GSP write answers with the tag of the graph it just wrote (DocumentHierarchyGraphStoreImpl:392). One extra request for the whole flow. ldh:document-etag and ldh:conditional-headers in client/functions.xsl carry the two decisions that were being made ad hoc: where a validator comes from, and that an absent one sends NO If-Match rather than an empty one - the server reads blank as absent, so an empty value would have looked like a precondition that passed. Driven in a browser: ontology-import, which was failing, now passes, and the full UI suite is green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Requiring If-Match of every write quietly redefined acl:Write as acl:Write AND acl:Read. HEAD mapped to acl:Read, HEAD is the only way to learn an entity tag, and a conditional write has to quote one - so an agent granted write access and nothing else could no longer write at all. Measured in CI on nine admin/acl tests, each created with `ldh admin create authorization --write` and each failing with the helper saying why: "### etag: no entity tag for <https://localhost:4443/>". That coupling appears in no authorization document and no vocabulary; it was an emergent property of the precondition rule. The alternative considered and rejected was exempting agents who cannot read. It keeps them writing, but their writes are then unconditional - they overwrite a concurrent edit with nothing to show for it, which is the failure this branch exists to remove. Blind overwrite is not an acceptable price for the rule that was meant to prevent it. So HEAD is granted to any agent holding a mode on the document: it may be answered for acl:Read, acl:Append or acl:Write. GET is untouched and content still needs acl:Read; an agent with no mode at all - anonymous included - is refused as before. What that discloses, stated rather than left implicit: the entity tag is a hash of the graph (Core, GraphStoreBase:283), so an agent that may write but not read gains a way to CONFIRM a guess at the content - reproduce the graph exactly, hash it, compare. It cannot extract anything. Every graph carries a dct:modified stamped to the millisecond on each write, so the guess must include that too, which is why a non-reader's HEAD is trimmed here to the validator and its own modes: Last-Modified would otherwise narrow the search from milliseconds to seconds, and Content-Length describes a representation the agent may not have. Closing it completely means an opaque validator - a keyed or stored one - which is a change to Core and its own piece of work. Verified against the running stack: a reader's HEAD still carries ETag, Last-Modified and Content-Length; anonymous is still refused HEAD and GET alike; the UI suite is green. The nine acl tests cannot run in this working copy, which does not define the test.localhost dataspace they need - they die before reaching the write - so CI adjudicates those. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Requiring If-Match of every write broke every RDF import into a document that already existed. The import writer asks to create: PUT with If-None-Match: *, and on the 412 that says the document is already there, POST to append instead. Demanding If-Match of that PUT answered 428 - which is not 412, so the fallback never ran and the import failed with "could not be successfully created using PUT. Status code: 428". Measured on the running stack, and it is what CI was reporting as imports/import-rdf-no-query.sh. The rule was wrong, not the import. A write must be CONDITIONAL; If-Match is one way to say so and If-None-Match is the other, and they are mutually exclusive by construction - a request asking to create cannot quote the entity tag of a thing it is asserting does not exist. So a write carrying either satisfies the requirement. That was only the first of three links: - the 412 carried no entity tag, so the fallback had nothing to quote and its own POST went out unconditional - 428 again. A failed precondition now names the current validator, which is what lets a client retry without a second read, and is what a 304 is supposed to carry anyway. - the fallback append now quotes it. A 412 from the append would then mean the document changed between the two requests, which is the thing being guarded against and no longer silent. Three places use that idiom - twice in RDFGraphStoreOutput, once in CSVGraphStoreRowProcessor - and all three are fixed. Only one test caught it because the fallback only runs when the target document already exists: import-csv.sh and import-rdf.sh write to graphs they create, so their PUT succeeds and the 412 branch never executes. Verified against the running stack: PUT with If-None-Match: * on an existing document answers 412 carrying its tag, a write with no precondition at all still answers 428, and an RDF import into an existing document lands its triples. The UI suite is green and imports/import-rdf-no-query.sh passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch changed what a write must carry, what a HEAD may be answered for, and what a failed precondition says back - and leant on tests that touch none of it deliberately. It showed: the If-None-Match break was caught by imports/import-rdf-no-query.sh, which is about importing RDF and noticed only because its fallback keys on a status. Every other answer here was verified by hand at a terminal and then left unpinned. Three tests, one per thing that was actually wrong at some point while this was written: - document-hierarchy/PATCH-preconditions.sh: a write to an existing document with no precondition is 428, with a BLANK If-Match is 428 (an empty header is not a validator, and reading it as one let a client opt out of the rule entirely), with a stale one is 412, and with the current one succeeds. The last case reads the tag with the same Accept the write sends, because an entity tag identifies a negotiated variant rather than the graph. - document-hierarchy/PUT-if-none-match.sh: If-None-Match: * creates a document that is absent, and on one that exists answers 412 - not 428 - carrying the current entity tag, which the append that follows then quotes. That is the import writer's whole flow, and all three steps of it were broken in turn. - admin/acl/HEAD-write-without-read.sh: an agent granted acl:Write and nothing else is refused before the authorization exists, may then HEAD for the validator it has to quote, is still refused GET, and can complete the conditional write. Without this, making HEAD require acl:Read again would only show up as nine unrelated acl tests failing with an unexplained 428. The last of those found something while being written: Content-Length survives a non-reader's HEAD, because Jersey computes it when it serializes, after the response filter has run - so the line removing it was dead. It is gone, and the reason is written down rather than left as code that looks like it works. Nothing is lost by it: anyone able to confirm content by its hash knows its length. Last-Modified, which does matter - it narrows the millisecond dct:modified the hash is taken over - is removed and now asserted on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every write in
DocumentHierarchyGraphStoreImplis a whole-graph read-modify-write: the graph is read, changed in memory and written back. Two writers that name no precondition therefore overwrite each other with nothing to show that anything was lost. The machinery to prevent that was already here and already worked — a staleIf-Matchanswers 412 — but it was opt-in, andevaluatePreconditions()is vacuous for a request carrying no conditional header, so a client simply not sending one got last-write-wins.Measured in the constructor editor, which is what started this: one save's DELETE was correct in every detail, answered 204, and was reinstated by a second write that had read the graph before it landed.
What changed
The rule. A write to a document that already exists must be conditional.
If-Matchsays so, andIf-None-Matchsays so in the other direction — "only if this does not exist" — so either satisfies it. A blank header does not: an emptyIf-Matchis not a validator, and reading it as one let a client opt out entirely. Creating a document is exempt; it has no prior state to have been written against.The clients, so they can obey it. The
ldhCLI reads the validator before each write. Eight client-side writes that went out unconditionally now carry one — the document DELETE and RDF file drop, block removal and reorder, the memento restore PUT, and the appends in the import and add/clone chains.HEAD, so a writer can obtain a validator at all.HEADmapped toacl:Read, andHEADis the only way to learn an entity tag — so requiring a precondition quietly redefinedacl:Writeasacl:Writeandacl:Read, and an agent granted write access and nothing else could no longer write.HEADis now answered for any mode the agent holds.GETis untouched and content still needsacl:Read; an agent with no mode, anonymous included, is refused as before.Two things worth a reviewer's attention
An entity tag identifies a negotiated variant, not the graph. The same document answers
"5a4df28aea61e278"as RDF/XML and"5a4df28aec9db937"as Turtle. A conditional write must therefore send the sameAcceptas the read that produced its validator, or it is refused with 412. This bit the CLI twice and every test that was converted.A write-only agent gains a content-confirmation oracle. The tag is a hash of the graph (Core,
GraphStoreBase:283), so an agent that may write but not read can reproduce a guess, hash it and compare. It cannot extract anything, and every graph carries adct:modifiedstamped to the millisecond that the guess must also reproduce — which is why a non-reader'sHEADno longer returnsLast-Modified. Closing it completely means an opaque validator in Core; that is separate work, and worth doing only if write-without-read is deployed as a confidentiality boundary.Tests
Three pin the new behaviour directly, one per thing that was actually wrong while this was written:
document-hierarchy/PATCH-preconditions.sh— none → 428, blank → 428, stale → 412, current → succeeds.document-hierarchy/PUT-if-none-match.sh—If-None-Match: *creates; on an existing document answers 412 carrying the current tag, which the append that follows quotes. That is the import writer's whole flow, and all three steps of it broke in turn.admin/acl/HEAD-write-without-read.sh— an agent withacl:Writealone is refused before authorization, may then HEAD for its validator, is still refused GET, and completes the conditional write.40 existing HTTP tests were converted to write conditionally. Two categories deliberately stay unconditional: those asserting 401/403, because an agent that may not read cannot obtain a validator and requiring one would stop them testing authorization; and those creating documents, which the server exempts.
run.shgains an exportedetaghelper that reads a validator with the sameAcceptthe write will send, and says so on stderr when a document has none — the alternative, an empty string, produced exactly the unexplained 428 it was meant to prevent.Verification
CI green on the tip, and checked for hidden failures rather than trusting the tally: no test printed
failed. Driven locally in a browser as well — the full UI suite passes, includingontology-import, which this branch broke and then fixed.Depends on
fd7a18f07ondevelop: without it a failing test in theimportsgroup leaves the workflow green, which is how theIf-None-Matchbreak survived a CI run.