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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

validate content size a la ssb-validate #2

Merged
merged 5 commits into from Oct 11, 2022
Merged

validate content size a la ssb-validate #2

merged 5 commits into from Oct 11, 2022

Conversation

mixmix
Copy link
Member

@mixmix mixmix commented Oct 10, 2022

馃敟 馃悰

this module has been letting messages through that other peers consider invalid.
The reference implementation put a cap on the content size, see https://github.com/ssbc/ssb-validate/blob/main/index.js#L111

It appears that this module has not honoured that.
This means that there will likely be messages in the wild which are invalid for some peers and valid for others.

I think we need to consider how to handle that given it might have far reaching + feed-killing effects

@mixmix
Copy link
Member Author

mixmix commented Oct 10, 2022

Proposal - think we might want to change out validation to what planetary is doing:

  • don't allow writing > 8kb
  • allow reading any size

@staltz
Copy link
Member

staltz commented Oct 10, 2022

Thanks, well spotted. It seems that this validation was on my mind when I was writing this module, but for some reason I forgot it.

It appears that this module has not honoured that.

Gladly this module is not yet in production in Manyverse (it will be put this month into production!) so your PR came at the perfect time. I don't believe ssb-classic is used anywhere in production, maybe in Perihelion. Not sure.

The style changes were driving me crazy in this PR, but I realized that that's because we didn't have .prettierrc (we should) and that format-code is run automatically for every commit. I now added the prettierrc config and it should revert some of the code style changes.

validation.js Outdated Show resolved Hide resolved
validation.js Outdated Show resolved Hide resolved
@mixmix
Copy link
Member Author

mixmix commented Oct 10, 2022

Please take over and merge if it's patches

@staltz
Copy link
Member

staltz commented Oct 10, 2022

@mixmix There's still a test that needs to be moved from ssbc/ssb-db2#394 to here. I won't "take over" this PR because it wasn't my initiative. I had other plans for my day.

@mixmix mixmix requested a review from staltz October 11, 2022 04:24
@mixmix
Copy link
Member Author

mixmix commented Oct 11, 2022

@staltz added tests

@staltz staltz merged commit 85de088 into master Oct 11, 2022
@staltz
Copy link
Member

staltz commented Oct 11, 2022

Thanks!

@staltz staltz deleted the validate_size branch October 11, 2022 07:45
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