Read OWIDs without raising, and close unsigned construction - #2
Merged
Conversation
An OWID is read from whatever a caller was handed, which on a public endpoint means anything at all. Malformed data is therefore an ordinary outcome, not an exceptional one, and raising for it costs the construction and unwinding of an exception per bad input. Whoever is sending the data chooses how often that happens. tryFromBase64, tryFromByteArray and tryFromFrame answer instead, returning a result that says whether it worked, carries the OWID only then, and names the reason either way. The reader walks the buffer by index and checks each read against what is left, so a bad envelope is a comparison that fails. It does not call a raising reader and catch, because the exception would still be built and unwound. The four byte length is composed from its bytes rather than unpacked, so on a 32 bit runtime it is compared with the bytes present while still exact rather than after being narrowed to something allocatable. A caller can also no longer build an OWID. The constructor is private and the reading and creation code live inside the class, which PHP enforces itself, so an instance arrives only from a successful read or from Creator::create, which owns the version, domain, date and signature. The signature is calculated before the instance exists, so there is no moment at which an unsigned one could be held. An unsigned OWID is indistinguishable from a signed one to the code downstream of it, and the difference surfaces later, somewhere that is not looking. The fields are read only for the same reason. A read OWID's signature covers its fields as they arrived, so code that could rebind one would hold something whose signature no longer describes it. PHP strings are values rather than references, so what a caller does with a copy of the payload cannot reach the OWID it came from and no defensive copy is needed. Tests that tampered with a signed OWID in memory could not be written any more, so they now tamper with the serialised bytes and read them back, which is how tampering actually reaches a verifier. The status vocabulary is the cross-language one, so a failure means the same thing whichever language read the bytes. A separate signature vocabulary keeps "could not check" apart from "does not match", because a key that cannot be read leaves the signature unjudged and calling that invalid would report an outage as an attack. signatureStatus and Crypto::tryVerifyOnly report an unreadable key as InvalidKey. Reading no longer goes through the Io reader, so its read half is gone along with the exception factories that only it used. A framed read, which that reader also served, is now tryFromFrame, whose result reports how many bytes the envelope occupied so a caller can advance to the next one. The README examples are run by a test, because documentation naming a method that does not exist should fail the build rather than a reader. The workflow now runs the matrix on PHP 8.1, which composer.json supports and which the enums and read only properties need, as well as 8.3 and 8.4, and it runs the dependency free runner as well as PHPUnit. PHPUnit passes 102 tests and 8,395 assertions, 25 tests more than before, and the dependency free runner passes 130 checks against 113. The new ones cover that construction from outside is impossible, that no field can be rebound, that an empty payload and a one megabyte payload both read, that absent input, the wrong sort of input, invalid base 64, an unknown version, a trailing byte and a truncated envelope each report their own reason and hand nothing back, and that an identifier whose signature does not match reads and then fails verification.
A Windows checkout converts the README to carriage return and line feed, so the pattern that found the fenced examples matched nothing there and the test reported no examples to run rather than running them. The pattern now accepts either ending. The temporary file the test writes is removed along with the empty one tempnam creates for it, so a run leaves nothing behind.
The marker is a single zero byte standing for an optional OWID that is not there. It carries no domain, date, payload or signature, so it can never verify, and reading one from a whole buffer handed calling code an instance with no signature, which is the one thing the construction boundary exists to prevent. A whole buffer holding one is now refused as UnsupportedVersion, matching the Go, Rust, .NET and Python ports. That also removes the odd state where a value that had been read could not be written back, because the marker's version has no date encoding. Framed reading is unaffected. Inside a framed buffer the marker still says an optional OWID is absent, which a caller walking the frames has to be able to tell from a frame that is malformed, so tryFromFrame reports it and consumes its one byte. A buffer of no bytes is now MissingInput rather than UnexpectedEnd. Nothing was supplied, which is not the same as data that arrived and stopped part way through a field. The byte array surface already said so for an empty string; the case this reaches is base 64 that decodes to no bytes at all, such as a value that is only whitespace. Every member of both status vocabularies now has a test that produces it, or a comment on the member saying why this implementation cannot reach it. Two tests hold that: each walks its enum's cases and requires every one to appear either in a table of worked examples or in a short list of the unreachable, so a status cannot be added and left silently untested. Adding a case with neither makes them fail, which was checked. MalformedEnvelope joins the unreachable, since the marker was the last way to produce it, and its comment records that changing the byte count check makes four tests fail on it, so the guard is a live backstop rather than dead code. VerificationError is now reached by a test, using a framed marker as one of the others covered by a signature, which cannot be written into the data to check and so leaves the question unanswered rather than answered no. Crypto::verifySignatureStatus becomes Crypto::signatureStatus, which reads the same way as Owid::signatureStatus and drops a verb the return type already carries. PHPUnit passes 107 tests and 8,437 assertions, and the dependency free runner 131 checks.
The marker for an absent node, a single zero byte, is now ParseStatus::AbsentNode on both contracts. Refusing it as an unknown version was inaccurate, because version 0 is supported and meaningful. It simply is not an OWID, and a caller walking a run of frames has to be able to tell a node that is not there from a frame it cannot read. No OWID is handed back for it, on either contract. The marker carries no domain, date, payload or signature, so it can never verify, and nothing mistakable for an identifier reaches calling code. The result counts the one marker byte as consumed, so the same arithmetic that advances past an envelope advances past an absent node, and the frame after it reads. ParseResult gains absentNode for that shape, which is neither a value nor a fault: ok is false because there is no OWID, while the bytes are still counted because the frame was understood. A short frame already reported UnexpectedEnd and still does. Where a framed read finds the declared payload running past the bytes supplied, the bytes may still be arriving, and waiting for more is a different answer from giving up. ByteCountMismatch keeps its meaning on the whole buffer contract, where every byte is present by definition. A test now holds both halves of that distinction over the same three truncations. VerificationError moves to the statuses this implementation cannot reach, with the reason on the member. It was reached by passing the marker as one of the others a signature covers, since its version has no date encoding, and no OWID can hold that version now that the marker is never handed out. The two meta-tests still hold: every parse status is produced by an example or named unreachable, and AbsentNode is produced. The README gains the frame walk as a worked example, which the README test runs and checks, so the loop a caller needs in order to step over an absent node is one the build proves works. PHPUnit passes 109 tests and 8,456 assertions, and the dependency free runner 133 checks.
The payload limits section named ParseStatus::ByteCountMismatch as the answer to a declaration that disagrees with the bytes present, which is the whole buffer answer only. A frame short of its declared payload is ParseStatus::UnexpectedEnd, which the reading section further down already said, so the two sections now agree. phpunit passes 109 tests with 8456 assertions, and the plain runner passes 133 checks. Both include the test that extracts every php block from the README and runs them as one script.
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.
An OWID is read from whatever a caller was handed, which on a public end
point means anything at all, so malformed data is an ordinary outcome and not
an exceptional one. Reading now answers with a reason instead of raising, and
an OWID can no longer exist in an unsigned state.
What a caller sees
Reading
Before, reading raised, so every bad input from outside cost the construction
and unwinding of an exception and the reason had to be read out of a message.
After, reading answers with the same three facts every time, being whether it
worked, the OWID only when it did, and a named reason either way.
Owid::tryFromByteArrayreads raw bytes the same way.Owid::tryFromFramereplaces the old
Owid::fromReaderfor a buffer that carries more than oneenvelope, and its result reports
consumed, the number of bytes this envelopeoccupied, so a caller advances to the next one with it.
Creation
Before, a caller built an unsigned OWID and signed it afterwards, and could
hold or pass on the half made one.
After, creation makes a finished OWID in one call.
signStringandsignByteswere a pair only because other languagesdistinguish text from bytes. A PHP string is a byte array, so there is one
create.What else a caller will notice
Owid::fromBase64,Owid::fromByteArrayandOwid::fromReaderare gone.Nothing raising remains for reading, so there is no third way to obtain an
OWID.
Creator::sign,signWithOthers,signStringandsignBytesare gone.Signing an OWID that already exists is not offered, because with no way to
obtain an unsigned one there is nothing outside to sign, and re-signing one
would replace a signature its fields were read with.
Owidconstructor is private and its fields are read only. Reading afield is unchanged, so
$owid->payloadstill works, while assigning to onenow raises
Error.Iois gone, being the constructor,readByte,readBytes,readString,readUint32,readByteArray,readPayload,readSignature,readDateandremaining.Iokeeps the write helpers andthe base date. A caller framing OWIDs inside its own format uses
tryFromFrame.ParseResult,ParseStatus,SignatureStatus,Owid::signatureStatus,Owid::signatureStatusWithCrypto,Crypto::tryVerifyOnlyandCrypto::verifySignatureStatus.OwidExceptionis now raised only for a fault in the program, such as acreator configured with a domain that is too long, a key that cannot be used,
or fields that cannot be written.
The byte count rule
After the version, domain, date and four byte payload length are read, the
bytes actually present are computed as the bytes remaining after the length
field minus the signature length, signed and wide, so a short buffer gives a
negative number rather than wrapping. That is compared with the declaration
before anything is sized by it, and a disagreement is
ByteCountMismatchwhichever way the bytes fall short, including a truncated signature. Only after
the counts agree is anything sized by the declaration.
The four byte length is composed from its bytes rather than unpacked, so on a
32 bit runtime a value past the integer range becomes a float, which holds it
exactly, and it is compared with the bytes present before being narrowed to
anything allocatable. A declaration that survives the comparison and still
exceeds what an integer can hold would be
ImplementationCapacityExceeded.A node that is absent, and a frame that stops early
Two contracts, two answers, in both cases so that a caller can tell what to do
next.
ParseStatus::AbsentNodeis new in the shared vocabulary. The marker for a nodethat is not there, a single zero byte written by
Owid::emptyToBuffer, isreported as itself on both contracts. It is not an unknown version, because
version 0 is supported and meaningful, and it is not a malformed frame either.
date, payload or signature, so it can never verify, and reading one as an
identifier would be the single way an instance with no signature could reach
calling code.
advances past an envelope advances past an absent node and the next frame
reads. A caller walking a run of frames can therefore tell a node that is not
there from a frame it cannot read.
ParseResult::absentNodecarries that shape, which is neither a value nor afault:
okis false because there is no OWID, while the bytes are stillcounted because the frame was understood.
A framed read whose declared payload runs past the bytes supplied reports
UnexpectedEnd, which it already did and still does. There the bytes may stillbe arriving, and waiting for more is a different answer from giving up.
ByteCountMismatchkeeps its documented meaning on the whole buffer contract,where every byte is present by definition. A test holds both halves of that
distinction over the same three truncations.
A buffer of no bytes is
MissingInputrather thanUnexpectedEnd, becausenothing was supplied, which is not the same as data that arrived and stopped
part way through a field. The byte array surface already said so for an empty
string. The case this reaches is base 64 that decodes to no bytes at all, such
as a value that is only whitespace.
The README carries the frame walk as a worked example, and the README test runs
it and checks what it found, so the loop a caller needs is one the build proves
works.
Signature statuses
SignatureStatuskeeps "could not check" apart from "does not match". A keythat cannot be read is
InvalidKeyand neverSignatureInvalid, because on30 August 2026 the key end points served PEM a strict parser rejects and every
verification against it failed while the keys and the identifiers were both
fine. Reported as
InvalidKeythat reads as the operational fault it was.verifyWithPublicKeyandverifyWithCryptostill answer true or false, andsignatureStatusis for callers where the difference changes what they do.Every status is tested, or says why it cannot be
Two tests walk
ParseStatus::cases()andSignatureStatus::cases()and requireevery member to appear either in a table of worked examples or in a short list
of the unreachable. A status can therefore not be added and left silently
untested: adding a case with neither makes the tests fail, which was checked.
Reached by a test:
Parsed,AbsentNode,MissingInput,InvalidInputType,InvalidBase64,UnsupportedVersion,UnexpectedEnd,InvalidDomainEncodingand
ByteCountMismatch, andSignatureValid,SignatureInvalid,InvalidSignatureLengthandInvalidKey.Unreachable, with the reason on the enum member itself:
ParseStatus::ImplementationCapacityExceeded. On a 64 bit runtime every valuethe length field can hold fits in an integer. On a 32 bit one a declaration
past the integer range could only agree with the bytes present if a string
larger than that build can hold had already been read. The guard is kept so a
future change to the arithmetic has somewhere honest to report to.
ParseStatus::MalformedEnvelope. Every way an envelope can be wrong isalready named, and refusing the marker removed the last way to produce this.
The two places that report it are guards a correct reader never reaches. It is
not dead code: changing the byte count check to accept a longer buffer makes
four tests fail on this status, so it does catch an arithmetic mistake in the
check above it.
SignatureStatus::KeyUnavailableandSignatureStatus::ImplementationCapacityExceeded. This library never fetchesa key and verifies data already in memory, so obtaining a key is the caller's
job and the caller reports its own failure to do so.
SignatureStatus::VerificationError. It was reached by passing the absentnode marker as one of the others a signature covers, because its version has
no date encoding, and no OWID can hold that version now that the marker is
never handed out. The other way in is openssl reporting an error rather than
a verdict, which valid key material and a well formed signature do not
produce. It is kept because a caller of
Cryptosupplies its own data, andbecause a check that could not be made must have somewhere to go other than
SignatureInvalid.Naming
The public names were reviewed for reading as translations of the .NET API
rather than as PHP.
tryFromBase64andtryFromByteArrayare kept. PHP's own backed enums providetryFrom, meaning give me the value or null rather than throwing, which isexactly what these do, and the language already reads
Type::createFrom...as anamed constructor. The .NET shape is
bool TryParse(input, out value, out status), which has no PHP equivalent and is not what is here.tryFromFramefollows the same convention, and
tryFromBufferwas rejected becausetryFromByteArraytakes a buffer as well, so the name would not say which iswhich.
Creator::create,ParseResultwithok,owid,statusandconsumed, andParseResult::parsedandfailedas named constructors are all ordinary PHP.Crypto::tryVerifyOnlysits beside the existingnewVerifyOnlyandnewSignOnlyand carries the sametrymeaning as above.signatureStatusWithCryptomirrors this library's ownverifyWithCrypto.One rename:
Crypto::verifySignatureStatusis nowCrypto::signatureStatus,which reads the same way as
Owid::signatureStatusand drops a verb the returntype already carries.
Tests
PHPUnit goes from 77 tests and 5,195 assertions to 109 tests and 8,456
assertions. The dependency free runner in
tests/run.php, which the READMEoffers when composer is not available, goes from 113 checks to 133.
The new tests cover that a success reports all three facts, that an empty
payload and a one megabyte payload both read, that absent input, the wrong sort
of input, invalid base 64, an unknown version, a trailing byte and an envelope
that stops early each report their own reason and hand nothing back, that
construction from outside is impossible, that no field can be rebound, that
writing into a returned payload does not alter the OWID, that an identifier
whose signature does not match reads and then fails verification, and that a
key which cannot be read is not reported as a forgery.
The tests were neutralised to prove they measure something.
ParseContractTest::testConstructionFromOutsideIsImpossiblereadonlyremoved from the fieldsParseContractTest::testNoFieldCanBeRebound<instead of!=ParseContractTest::testTrailingByteIsByteCountMismatchand three tests inPayloadLengthTestsignStringReadmeTest::testReadmeExamplesRunUnsupportedVersionagainByteCountMismatchParseContractTest::testAShortFrameEndsEarlyRatherThanDisagreeingandPayloadLengthTest::testFramedReadOfATruncatedEnvelopeEndsEarlyUnexpectedEndagainParseContractTest::testAZeroLengthBufferIsMissingInputParseContractTest::testEveryParseStatusIsReachedOrNamedUnreachableThe README examples are now run by
ReadmeTest, which takes every fenced phpblock in order, runs them as one script and checks that the identifier the
first one creates reads back and verifies, so documentation naming a method
that does not exist fails the build.
Other changes
composer.jsonsupports and whichthe enums and read only properties need, as well as 8.3 and 8.4, on Ubuntu
and Windows, and it runs
tests/run.phpas well as PHPUnit. Only PHP 8.5.1was available where this was written, so the other versions are covered by
CI rather than locally, and all six jobs pass.
README arrives with carriage returns, which the pattern finding the fenced
examples did not allow for, so the new test reported no examples rather than
running them and the three Windows jobs failed while the three Ubuntu ones
passed. The second commit accepts either line ending.
OwidException::unexpectedEndOfBuffer,OwidException::base64,OwidException::payloadLengthMismatchandVersion::fromByte. The unusedDateTimeZoneimport inIowas already dead before this change.on
OwidException::domainTooLongsaying a read raises it, which a read nowreports as
InvalidDomainEncoding.Produced with AI assistance under James Rosewell's direction and needs human
review.