Make UAuthority store binary IP addresses, update tests, and address bugs found along the way #73
Conversation
This file contains 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
|
Hmmm. Is there a way to have it so that on this PR we can review the range of commits you're going to add? Like... hmm. Seems to me that'd make it a lot easier to review. |
That's why it's in draft state right now. Either I make a PR into the 1.5.7 branch, which makes my PR easier to review but the 1.5.7 PR (#68) more complicated, or this is held until the 1.5.7 PR is merged. I lean towards the latter solution as it aims to keep PRs smaller and focused on specific changes. I could try to rebase this branch onto main get it committed first, but since the 1.5.7 PR contains so many formatting changes (e.g. removing |
gregmedd
force-pushed
the
bugfix/uauthority-binary-ip
branch
from
a4fbc53
to
252aef6
Compare
This was referenced Mar 28, 2024
Closed
The UAuthority proto file describes the IP field as holding "IPv4 or IPv6 Address in byte format". The intent was for IPv4 addresses to be represented in the native 32-bit binary representation (4 bytes) and for IPv6 to be represented in the native 128-bit (16 byte) format. Instead, BuildUAuthority is storing the human-readable string. This change revises the tests for BuildUAuthority to catch the issue.
Rather than having multiple separate implementations for parsing and converting IP addresses between representations, we should reuse the existing IpAddress class anywhere that conversions are done (e.g. BuildUAuthority). Moving the IpAddress.h file to uri/tools makes it clear this is an independent tool and not *specifically* part of the serializer, and makes the dependency graph between uri submodules clear.
Fixes the format conversion issues in BuildUAuthority by reusing the existing IpAddress class to perform the format conversion.
Change UAutority to UAuthority in BuildUAuthority's log messages.
Since this class is now being used in more places and is an important part of validating / converting / serializing / deserializing IP addresses, it is also worth while to have unit tests for it.
IpAddress had several issues: * It would take empty strings and report AddressType::Local, but this was never used anywhere, making checks for invalid IP addresses more difficult. Fix: report AddressType::Invalid instead. * Invalid address strings were stored and returned via getString(). Fix: return empty string when address is invalid. * When initializing an IpAddress from a byte vector, type is implicitly trusted, even if the vector isn't the right length for the given type. Fix: report AddressType::Invalid if validation of the address fails. * When initializing an IpAddress from a byte vector, the byte vector is assumed to be the correct length for the given type, potentially resulting in out-of-bounds memory access. Fix: Check size of byte vector length against expected value for the given type, and fail validation if they do not match
This streamlines several cases, including in the micro URI serializer, where an IP address is extracted from a UAuthority. This new constructor is a thin wrapper around the byte vector constructor. Includes new tests to cover the UAuthority constructor.
The IpAddress::AddressType enum had overloaded meanings that made checking the results of an IP address conversion more difficult by covering all types of micro UAuthority that could exist (local, IPv4, IPv6, ID, Invalid). However, a valid IpAddress can only have two of those values (IPv4 and IPv6) with all others being equivalent to Invalid. This change does three things: 1. Create a new AuthorityType enum inside MicroUriSerializer.h, using this instead when representing the type of a UAuthority. 2. Remove invalid types from IpAddress::AddressType. 3. Rename IpAddress::AddressType to IpAddress::Type for clarity.
This utilizes the IpAddress class to perform the conversion from string to byte string in the test. Since IpAddress now has unit tests, we *should* be able to trust it here.
Significant updates to the MicroUriSerializer test, including: * More coverage of boundary conditions * More coverage of address type mismatches * More coverage of ID authorities * More coverage of ID_LEN field in ID authorities * More coverage of deserialization for valid/invalid configurations * Adjusted assumptions around behaviors when invalid parameters provided (see testSerializeWithInvalid* tests) * Put slightly less faith in equality comparison between UUri's Also added an unexpected IP address corner case encountered while updating the MicroUriSerializer test to the IpAddress test suite.
Fixes the following test failures: * Fails to serialize IPv4 and IPv6 addresses from a UAuthority object. Caused by passing UAuthority::ip() to the IpAddress constructor, incorrectly calling the constructor for human-readable IP addresses (UAuthority stores a byte array in a std::string due to FlatBuffer implementation). Fix: Use UAuthority constructor in IpAddress instead. * BuildUAuthority ignores invalid IP addresses, resulting in the micro serializer producing a local URI instead of bailing with an empty byte vector. Fix: Update BuildUAuthority and BuildUUri to set their fields even if the isEmpty() check would return true, allowing for that emptiness to propogate to MicroUriSerializer::serialize(). * Correct ID size calculation error introduced by misreading of spec. * ID_LEN field was ignored by MicroUriSerializer::deserialize() when handling micro URIs with the AuthorityType::Id type, allowing for mismatch between reported and actual size. Fix: Add checking of ID_LEN field for ID authorities * checkMicroUriSize() checks the incoming byte vector size first, followed by the AuthorityType. This results in scenarios where mismatch between type and size are possible, allowing or blocking deserialization incorrectly. Fix: Pivot checkMicroUriSize() to check AuthorityType first, then check size (avoids issues around byte vectors having a known size, but not a matching type for that size). * BuildUAuthority incorrectly truncates IDs containing '\0' bytes. Fix: Remove unnecessary `.c_str()` in setId(). General quality-of-life improvements: * Log when exiting at start of MicroUriSerializer::serialize() (isEmpty or !isMicroForm). Helps find reason for test failures. * Log when exiting MicroUriSerializer::serialize() on an Invalid AuthorityType. Helps find reason for test failures. * Add more constexpr values to MicroUriSerializer, specifically for ID URIs. Makes many functions easier to follow. Cleans up some minor documentation and formatting issues: * Rewords some documentation for more consistency across files. * Reorders MicroUriSerializer() so that variables are assigned closer to where they are used in the serialization process. * Add / revise some logging statements for clearer indication of where and what failures have occurred. * Remove std::optional return type from getAuthorityType(). Use AuthorityType::Invalid for all invalid / unknown types. * Remove some functions that are very small and not reused after all the other changes made here. * Removes some trailing whitespace and unnecessary ';' at the end of function blocks.
gregmedd
force-pushed
the
bugfix/uauthority-binary-ip
branch
from
252aef6
to
562dc16
Compare
stevenhartley
approved these changes
Apr 4, 2024
umeirov
reviewed
Apr 4, 2024
umeirov
reviewed
Apr 4, 2024
umeirov
approved these changes
Apr 4, 2024
This was referenced Apr 4, 2024
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.
This PR addresses, among other things, #67 -
UAuthoritywas storing human readable string addresses instead of the "byte" representation required by the spec. It is based on the pending changes from #68, and will be blocked until that PR is merged.Along the way, I stumbled into several other bugs and deviations from spec. For full details of changes made, I recommend referring to the commit messages for my commits. The short summary of changes is:
IpAddressclass fromuri/serializertouri/toolsIpAddressclassMicroUriSerializerTest.cppfor better coverageNB: Because this PR is more extensive than initially planned, the commits have been staged such that the changes can be viewed in incremental steps, if needed. Each commit can build and run tests.