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

feat: Enforce no unexpected fields in events #355

Closed
wants to merge 5 commits into from

Conversation

stbrody
Copy link
Collaborator

@stbrody stbrody commented May 16, 2024

This PR temporarily removes the recently-added Builder structs, so as to focus first on getting the underlying Event types right and ensure all fields in the Events are specified explicitly. Once this is set we can add back the builders in a way that preserves type safety better and makes it impossible to build invalid events.

@stbrody stbrody self-assigned this May 16, 2024
@stbrody stbrody temporarily deployed to github-tests-2024 May 16, 2024 14:06 — with GitHub Actions Inactive
@stbrody stbrody force-pushed the disallow-unexpected-metadata branch from a626858 to 7efcd7d Compare May 16, 2024 14:16
@stbrody stbrody temporarily deployed to github-tests-2024 May 16, 2024 14:25 — with GitHub Actions Inactive
@stbrody stbrody marked this pull request as ready for review May 16, 2024 14:44
@stbrody stbrody requested review from dbcfd and nathanielc May 16, 2024 14:44
// Use self.len() here when we have cid@0.10
let buf = Vec::new();
let mut writer = std::io::BufWriter::new(buf);
self.write(&mut writer)?;
Ok(writer.into_inner()?)
// safe to unwrap because self.write should never fail.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nathanielc let me know if this is what you had in mind or if there's more I should do to handle this error.

@@ -6,6 +6,7 @@ use std::collections::BTreeMap;

/// A signed event payload.
#[derive(Debug, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the only one I'm not sure of. I believe this struct represents the JWS envelope and I don't actually know if it's safe to assume that that doesn't have extra fields. I think I'm leaning towards removing the deny_unknown_fields tag on this struct since the JWS envelope format isn't really a part of the Ceramic protocol spec so feels like maybe a place where we should be looser in what we accept. Curious for other's thoughts here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wait, actually I'm confused. There's also the SignedEvent struct that has a Jws struct:

@dbcfd - what's the relationship between this signed::Payload struct and the SignedEvent struct? Are they both used or can one be deleted?

Copy link
Contributor

Choose a reason for hiding this comment

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

SignedEvent is what js-ceramic requires, hence it is in ext. signed::Payload is what comes from js-ceramic when it is written to a car file.

@stbrody stbrody requested a review from dav1do May 16, 2024 15:08
@stbrody
Copy link
Collaborator Author

stbrody commented May 16, 2024

Note I have this test of sending extra fields in ceramic-tests: 3box/ceramic-tests#129. Not sure how to run that test against a version of rust-ceramic from this branch...

@stbrody
Copy link
Collaborator Author

stbrody commented May 17, 2024

closing. Event changes will be included in #357. Adding the serde directive to restrict unexpected fields will come in a follow-up PR

@stbrody stbrody closed this May 17, 2024
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.

2 participants