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

penumbra: review user supplier string fields in messages #3620

Closed
erwanor opened this issue Jan 17, 2024 · 3 comments · Fixed by #4567
Closed

penumbra: review user supplier string fields in messages #3620

erwanor opened this issue Jan 17, 2024 · 3 comments · Fixed by #4567
Assignees
Labels
_P-low Priority: low _P-V1 Priority: slated for V1 release
Milestone

Comments

@erwanor
Copy link
Member

erwanor commented Jan 17, 2024

Is your feature request related to a problem? Please describe.
In #2066, we discovered that validator definition string fields were not length-limited. We should review governance action handlers (among others) to make sure that we check those fields.

Assigning myself to keep track of this issue, however it's a good first issue and I would be happy to have a contributor take it on and play assist.

@erwanor erwanor added help wanted Help on this issue is welcomed! good first issue A good issue for people without existing context on the project. _P-V1 Priority: slated for V1 release labels Jan 17, 2024
@erwanor erwanor self-assigned this Jan 17, 2024
@github-project-automation github-project-automation bot moved this to 🗄️ Backlog in Penumbra Jan 17, 2024
@erwanor erwanor moved this to Future in Testnets Jan 17, 2024
@aubrika aubrika removed this from Testnets Feb 8, 2024
@hdevalence hdevalence added the _P-low Priority: low label Feb 9, 2024
@cronokirby
Copy link
Contributor

cronokirby commented Mar 22, 2024

Idea: create a SmallString<N> type which will automatically validate upon construction that the string is less than N characters. Then you just put it in the action struct, and it becomes impossible to not validate this in the resulting app logic.

@conorsch conorsch modified the milestone: v1 Mar 25, 2024
@erwanor erwanor removed help wanted Help on this issue is welcomed! good first issue A good issue for people without existing context on the project. labels Apr 13, 2024
@hdevalence
Copy link
Member

I think it would be simpler and more ergonomic to put the validation in the TryFrom domain type conversion rather than introduce our own custom string type that would have feature parity with the stdlib's string type. Otherwise we put ourselves through worse ergonomics forever.

@aubrika aubrika added this to the Sprint 8 milestone Jun 3, 2024
@aubrika aubrika moved this from Backlog to Todo in Penumbra Jun 3, 2024
@zbuc zbuc self-assigned this Jun 6, 2024
@zbuc zbuc moved this from Todo to In progress in Penumbra Jun 6, 2024
@zbuc
Copy link
Contributor

zbuc commented Jun 6, 2024

I think it would be simpler and more ergonomic to put the validation in the TryFrom domain type conversion rather than introduce our own custom string type that would have feature parity with the stdlib's string type. Otherwise we put ourselves through worse ergonomics forever.

One downside of this is duplication, for example Validator has three different TryFrom implementations:

impl TryFrom<ValidatorToml> for Validator {
impl TryFrom<pb::Validator> for Validator {
impl TryFrom<&TestnetValidator> for Validator {

Most types will probably only deserialize from the proto type however, validators are likely an outlier.

@github-project-automation github-project-automation bot moved this from In progress to Done in Penumbra Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
_P-low Priority: low _P-V1 Priority: slated for V1 release
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants