Skip to content

fix: parse AUDIT_REPLACE properly #14

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vthib
Copy link

@vthib vthib commented Aug 7, 2023

The AUDIT_REPLACE message payload is a uint32 number, which is not an ascii string. This causes parsing issues when such a message is parsed, and one of the byte is >= 0x7F

There is two solutions for this afaict:

  • Parse the AUDIT_REPLACE message on its own, making minimal changes.
  • Change the AuditMessage Event/Other enums to store a Vec instead of a String.

I picked the first one, to have minimal impact on existing code. However, the second solution imho would be a much better solution. This crate should not expect every message to a utf-8, which is a weird assumption. Its much better to let the caller decide how to parse the payloads, and not have errors when messages have non utf-8 compatible bytes.

The AUDIT_REPLACE message payload is a uint32 number, which is not an
ascii string. This causes parsing issues when such a message is parsed.

There is two solutions for this afaict:

- Parse the AUDIT_REPLACE message on its own, making minimal changes.
- Change the AuditMessage Event/Other enums to store a Vec<u8> instead
  of a String.

I picked the first one, to have minimal impact on existing code.
However, the second solution imho would be a much better solution.
This crate should not expect every message to a utf-8, which is a weird
assumption. Its much better to let the caller decide how to parse the
payloads, and not have errors when messages have non utf-8 compatible
bytes.
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

Successfully merging this pull request may close these issues.

1 participant