Skip to content
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

MSG isn't validated #18

Closed
dra27 opened this issue Aug 11, 2018 · 4 comments
Closed

MSG isn't validated #18

dra27 opened this issue Aug 11, 2018 · 4 comments

Comments

@dra27
Copy link
Contributor

dra27 commented Aug 11, 2018

RFC3164 4.1.3 describes the two fields of the MSG portion, but this isn't validated anywhere. In principle, the encode function should probably raise if there is no discernable alphanumeric TAG of 32 chars or fewer delimited by a non-alphanumeric character in message.

@verbosemode
Copy link
Owner

We could also replace the message field in type t by a tuple, record or even a new type to accommodate for the tag and content field. This would break the API, but afaik the only consumer of this library is logs-syslog at the moment. If @hannesm is fine with it, this might be an option to raising an exception.

@Leonidas-from-XIV
Copy link
Collaborator

I think this would be preferable, since exceptions tend to be rather surprising and break your code on runtime. The other possibility is to use the result type with polymorphic variants, I've also made good experiences with that.

@hannesm
Copy link
Collaborator

hannesm commented Aug 13, 2018

I'm happy with a (breaking) API change :)
FWIW: I like to use the result type for decode style functions (i.e. input from the network), but encode (i.e. input from trusted source, such as hardcoded parameters) I'm happy with Invalid_argument exceptions.

and I don't expect that this tag comes dynamically from untrusted sources..

hannesm added a commit to hannesm/syslog-message that referenced this issue Sep 8, 2018
@hannesm
Copy link
Collaborator

hannesm commented Sep 8, 2018

see #20

verbosemode added a commit that referenced this issue Sep 17, 2018
extend type t to contain tag and message (both type string), fixes #18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants