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

Add grant parsing fuzz test #2534

Merged
merged 9 commits into from
Jan 6, 2023
Merged

Conversation

johanbrandhorst
Copy link
Collaborator

@johanbrandhorst johanbrandhorst commented Oct 11, 2022

Adds fuzz tests for the grant parser. Running this I found two issues:

  • We were not consistently handling invalid unicode. I fixed this by always replacing invalid unicode in inputs with the unicode replacement handler. This is what marshaling to JSON already does.
  • We were not handling empty output fields consistently. output_fields=, was allowed while output_fields= was not. Now the latter is allowed and the former is equal to the latter.

@johanbrandhorst
Copy link
Collaborator Author

Fuzzing did discover a fun one: output_fields=, was allowed.

@jefferai
Copy link
Member

Fuzzing did discover a fun one: output_fields=, was allowed.

Correct! This is allowed!

@johanbrandhorst
Copy link
Collaborator Author

johanbrandhorst commented Oct 14, 2022

Fuzzing did discover a fun one: output_fields=, was allowed.

Correct! This is allowed!

Interesting! The error happened because this input creates a canonical string of output_fields=, which when roundtripped back into the parser is not allowed. Should the canonical string of output_fields=, be output_fields=,?

@johanbrandhorst
Copy link
Collaborator Author

johanbrandhorst commented Oct 14, 2022

I removed the "fix" for output_fields=, and started allowing output_fields=, with the same behavior. This means that the input output_fields=, safely roundtrips to output_fields=, which presumably means "no output fields are allowed".

@jefferai
Copy link
Member

Yes, I think in general any trailing comma on a string should be stripped for parsing and for the canonical representation

@jefferai
Copy link
Member

Meant to follow up on this: yes, output_fields= means "no output fields allowed". While it doesn't have a lot of use cases -- and if it's confusing we could think about taking it out -- the idea is for things like services where you want to perform an action periodically and the only thing you need or want the service to know about is the HTTP status code, and don't want to divulge any other data.

@johanbrandhorst
Copy link
Collaborator Author

Meant to follow up on this: yes, output_fields= means "no output fields allowed". While it doesn't have a lot of use cases -- and if it's confusing we could think about taking it out -- the idea is for things like services where you want to perform an action periodically and the only thing you need or want the service to know about is the HTTP status code, and don't want to divulge any other data.

OK so it sounds like the only real change here (accepting output_fields=) is what we want, should we merge this then?

@johanbrandhorst johanbrandhorst modified the milestones: 0.11.x, 0.12.x Oct 31, 2022
internal/perms/grants.go Outdated Show resolved Hide resolved
@jefferai jefferai force-pushed the jbrandhorst-fuzz-grant-parser branch from d91348c to 21dc028 Compare January 6, 2023 18:22
@johanbrandhorst johanbrandhorst merged commit 61c90c5 into main Jan 6, 2023
@johanbrandhorst johanbrandhorst deleted the jbrandhorst-fuzz-grant-parser branch January 6, 2023 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants