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

added serde to grant types #162

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Wicpar
Copy link

@Wicpar Wicpar commented Apr 11, 2023

This adds serde to the grant types

I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to chose either at their option.

Copy link
Owner

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a question in review.

Comment on lines +28 to +29
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
#[serde(tag = "type")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this type, there's a security concern when serializing Private since this may leak information rather unwittingly. I suppose it's okay overall but may be a paper cut when using the library. Just as a smoke-test, are there any alternatives that mitigate or avoid the risk?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed that is an issue. As a quick fix we can ignore it during serialization, but if we rely on serialization to store them on the server like with sqlx json it will work improperly and since that is my usecase for the serialization i kept it in.
Ideally it would serialize, but there would be a function to sanitize it before sending it to a user.

It's a double edged sword but i would err on the safer side with a serde skip indeed

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.

None yet

2 participants