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

Validate ByteCount ranges during deserialization #5743

Merged
merged 1 commit into from
May 13, 2024

Conversation

jgallagher
Copy link
Contributor

Fixes #5732.

I tried adding a custom JsonSchema too but hit a typify issue; left a link in the TODO comment.

@jgallagher jgallagher requested review from iliana and smklein May 13, 2024 15:03
Comment on lines +530 to +531
// TODO: custom JsonSchema impl to describe i64::MAX limit; this is blocked by
// https://github.com/oxidecomputer/typify/issues/589
Copy link
Collaborator

Choose a reason for hiding this comment

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

huh, that's surprising! This is still worth fixing, but to be clear, we'd rely on deserialize either way, so the behavior should be the "same" to clients afterwards, right? Would the only difference be "a more informative schema file"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this PR fixes the server-side problem. The custom JSON schema would be so (e.g.) the CLI could validate the value even before sending the request.

@jgallagher jgallagher merged commit d2ed452 into main May 13, 2024
20 checks passed
@jgallagher jgallagher deleted the john/panic-large-quota branch May 13, 2024 17:33
iliana added a commit that referenced this pull request May 14, 2024
iliana added a commit that referenced this pull request May 14, 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.

Large silo quota amount could crash nexus
3 participants