fix(vault): refuse a product group that contradicts its payload - #368
Merged
Merged
Conversation
Merged
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
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.
Closes #360.
A create body could carry an explicit
productGroupand aproductGroupDatawhose internal tag said something else, and nothing compared them. The two are read by different things — the explicit value becomes the storedproduct_groupand picksschemaVersion, the payload is schema-validated against its own tag — so both halves passed independently.Why it is a disclosure defect rather than a tidiness one
publishfilters the public view throughProductGroupAccessPolicy::for_schema_version(passport.product_group, …)— the label. Handed a coherent-but-wrong label it resolves perfectly, soaudience_view's fail-closed backstop never fires: that guard keys on the policy failing to resolve, and a real product group at its real schema version is not that case.Every payload field the label's table does not name then falls to
default_disclosure, which isPublic. Battery's table names neithersvhcSubstancesnordisassemblyInstructions; textile's marks bothrestricted. So textile data stored under a battery label publishes the REACH Art. 33 substance declarations into the public view — and signs them intopublicJwsSignature, where a published passport cannot be edited, only superseded.🚨 There were two doors, not one
The issue listed the update route as a question to check. It is a real second door, and a wider one.
PATCHABLE_FIELDScontainsproductGroupDataand does not containproductGroup. SoPUT /dpp/{id}on a draft could swap in another product group's payload while the protected label stayed exactly as it was — reaching the same mislabelled state by a route that never names a product group at all. A create-side check alone would have closed the front door and left this one open.Both are closed here:
validate_create_requestrefuses when both declarations are present and disagree, naming each so the caller can see which to change. Refused rather than reconciled: either choice silently discards a declaration the caller made, and this is a create, so no existing record's meaning changes by rejecting.apply_patchrefuses when a patched payload's group differs from the passport's stored group. Refusing rather than re-labelling, becauseproduct_groupis protected precisely so it is not a caller's to change after creation — quietly moving it here would route around that protection on the one path that does not mention it.Tests
Four on create, one on the patch door.
The create four pin that the rule is about a contradiction, not about requiring both fields: a battery label over textile data is refused; agreement is accepted; payload-only is accepted (the label is derived from it, which is the documented way); label-only is accepted.
a_patch_may_not_change_the_product_group_under_the_labeldrivesapply_patchdirectly and also asserts the label is untouched afterwards — a refusal that half-applied would be its own defect.Not in this branch
The issue also asks for a query over
odal.passportcomparing the column againstdoc->'productGroupData'->>'productGroup', to find any record already in this state. That is an operational check rather than a code change, and with no deployments holding data there is nothing to sweep yet; worth running before there is.just checkgreen.