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

fix!: pss target length verification #384

Merged
merged 5 commits into from
Sep 2, 2021
Merged

Conversation

AuHau
Copy link
Contributor

@AuHau AuHau commented Aug 2, 2021

Closes #380

Most of the validation was already in place from my earlier validation work, I was just asserting wrong values 😅

Breaking changes

  • bee.pssSend() now throws error if the specified target exceeds maximal value. Use Utils.makeMaxTarget() that will give you the max target that Bee accepts.

@AuHau AuHau requested a review from nugaon as a code owner August 2, 2021 13:31
@AuHau AuHau requested review from a team and Cafe137 August 2, 2021 13:31
@AuHau AuHau force-pushed the fix/pss-input-validations branch 2 times, most recently from 3d6b884 to 7e07ecd Compare August 2, 2021 15:21
Copy link
Collaborator

@Cafe137 Cafe137 left a comment

Choose a reason for hiding this comment

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

Great, thanks :)

@AuHau AuHau changed the title fix: pss target length verification fix!: pss target length verification Aug 3, 2021
@AuHau
Copy link
Contributor Author

AuHau commented Aug 3, 2021

The only downside about this fix is that it is breaking 😒 But honestly the other work (directory streaming and fetch refactor) are also breaking so I guess we should just go with 2.0 release.

@AuHau AuHau added the status:blocked Unable to be worked further until needs are met (see 'need:*' category) label Aug 4, 2021
@AuHau AuHau removed the status:blocked Unable to be worked further until needs are met (see 'need:*' category) label Aug 31, 2021
Copy link
Member

@agazso agazso left a comment

Choose a reason for hiding this comment

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

Using makeMaxTarget in the test may make the PSS tests to run longer and use more CPU than necessary. In general for testing PSS it would be enough to assume the shortest possible target length.

@AuHau
Copy link
Contributor Author

AuHau commented Aug 31, 2021

You are right, but that is not the scope of this PR. I was only reproducing the current state which was passing the whole address which then got truncated with the original slice().

I have created an issue about this improvement and I will work on it in the future.

@agazso
Copy link
Member

agazso commented Aug 31, 2021

You are right, but that is not the scope of this PR. I was only reproducing the current state which was passing the whole address which then got truncated with the original slice().

Okay, I see.

I have created an issue about this improvement and I will work on it in the future.

I also opened one long time ago, you may want to close either as duplicate.

@AuHau
Copy link
Contributor Author

AuHau commented Aug 31, 2021

Thanks for the info!

Copy link
Member

@nugaon nugaon left a comment

Choose a reason for hiding this comment

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

I don't completely get the API allows any length of target, but the BeeJS restricts it to 4-length hex string. Because we don't expose/support the direct use of modules as we discussed, by design you even cannot get around this limitation in this form, meanwhile you bring a utility function usage to the workflow. it seems more unconvenient for me with zero yield compare to the previous version.

src/utils/pss.ts Outdated Show resolved Hide resolved

/**
* Utility function that for given strings/reference takes the most specific
* target that Bee node will except.
Copy link
Member

Choose a reason for hiding this comment

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

does not the bee accept any length of target? then why we allow to pass any length address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. You might got this impression from Bee-js usage of PSS before this patch, which silently sliced the target to max. allowed length without user's knowledge. This patch impose correct validation and tools for the user to have predictable behavior.

src/utils/pss.ts Show resolved Hide resolved
@AuHau
Copy link
Contributor Author

AuHau commented Sep 1, 2021

@nugaon Bee API also has the same restrictions. It will return Bad Request for target exceeding the limit. See https://github.com/ethersphere/bee/blob/master/pkg/api/pss.go#L29 (btw. there the limit is 3 bytes = 6 hex chars, but this was bumped only after Bee 1.1.0 release, so it is not propagated to Bee-js for now and I will change it once release cycle will begin).

@AuHau AuHau requested a review from nugaon September 1, 2021 08:00
Copy link
Member

@nugaon nugaon left a comment

Choose a reason for hiding this comment

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

as everybody is satisfied with this then we can roll it out, though from my side a jsdoc about this splitting would have been enough

@AuHau AuHau merged commit fd032a8 into master Sep 2, 2021
@AuHau AuHau deleted the fix/pss-input-validations branch September 2, 2021 07:59
@bee-worker bee-worker mentioned this pull request Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PSS Send: target is truncated to 4 characters
4 participants