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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
initial MMP protocol draft #899
base: main
Are you sure you want to change the base?
initial MMP protocol draft #899
Changes from 4 commits
92511f2
8f6136b
9882038
b257a2d
ecfeb82
4960bdd
3defc40
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is VCHAR the correct choice here? Should this be a smaller character set? Perhaps
pchar / "?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid parsing issues with URI libraries, it does make sense to keep this URI safe. However, because this needs maximum extensibility such that a version has the versatility it might need with various reserve characters I'm thinking it makes sense to make this
unreserved
/reserved
which I think would be correct here in saying that any URI safe character can be used as long as the total length is less than 512 characters.Does that sound good to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The important thing is that parsing ambiguities aren't introduced here. Also, since this isn't valid ABNF (to have two productions with different constraints) there should be some other mechanism used to express this constraint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, will make sure that this is complying with the URI spec.
I believe this would be valid ABNF under: https://datatracker.ietf.org/doc/html/rfc5234#section-3.3
The reason I used this method was to make it clear the maximum length of the entire URI is meant to be 512 characters. If there's a way to do that within a single rule, I'm happy to change it but with the variability of length for the
message-location
andquery-params
sections it wasn't clear to me how to do it other than this way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't define the
ttl
field that you show in your example; under the current ABNF, it would need to be part of theencrypt-key-param
production, which is required to appear first. Instead of defining an explicit initialencrypt-key-param
production that must appear before other query parameter pairs, permit an arbitrary order and then just require thekey
and potentiallyttl
(if intended) keys be present in prose.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: the key should appear in a fragment, not as a query parameter, as it should NOT be sent to the server (if processed by a naive parser.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The various options I considered here were:
:
delimited section after version, but before the message location that is the key, but this becomes weird if someone opts to not have a key all together in a version. This could be solved with an empty section that produces::
if we want and saves a few bytes.The reason I opted to go with the query parameter option is that I expected that the parsers would handle this fine, but as soon as someone passed this to a URL resolver it would fail in which case it wouldn't make a network call at all. The one edge case that I can think of here is the registration of custom scheme handlers in operating systems and some browsers which may end up passing the URI around locally on device, but would likely still fail to resolve them in which case I couldn't see a reason that the query parameter didn't make sense.
However, in looking into the behavior of fragments a bit more for different media types, it does look possible to define multiple such as the example for PDFs.
In this case, I'm convinced that using fragments is the better option here but will keep the syntax matching what's described (well with some modifications to the characters that can be used as the questions above around VCHAR apply here too).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I opted to just drop the ttl example from this spec. Instead it seems more relevant that this is defined in the actual v1 spec.