feat!: input validation #343

AuHau · 2021-06-08T09:20:13Z

Closes #202 and closes #248

src/bee.ts

agazso · 2021-06-08T09:36:06Z

src/chunk/signer.ts

@@ -89,7 +89,7 @@ export function makePrivateKeySigner(privateKey: PrivateKeyBytes): Signer {
 }

 export function assertSigner(signer: unknown): asserts signer is Signer {
-  if (typeof signer !== 'object' || signer === null) {
+  if (typeof signer !== 'object' || signer === null || Array.isArray(signer)) {


Can you please explain the use-case when the signer is an array?

This is an assertion that Signer is not an array ;-) Because arrays are objects, so simple typeof signer !== 'object' won't catch that.

It is strange for me why you check it shouldn't be array, you could check also then it is not a number, boolean, etc. but what if it is an array that has the below checked functions that we call only? Will the code not run successfully?

Ehm, well technically you are right, but are you really suggesting that we should support this type of object?

const signer = [] signer.sign = () => {...} signer.address = '...'

Honestly, this looks evil to me, and that is the reason why Arrays (even though yes, they are technically object) are not allowed.

we shouldn't support but I don't see why it is necessary to check, because it does not cause any failure

I also don't think we need to support such array definition. That being said, the fact that we have such discussion indicates the code is confusing. Maybe this should be a different condition checking what the input SHOULD be and not what it SHOULD NOT be. So something like:

if (!( typeof singer === `object` typeof signer.sign === 'function' && typeof signer.address === 'string' && ... )) throw ...

src/utils/collection.ts

Co-authored-by: Attila Gazso <agazso@gmail.com>

src/bee-debug.ts

src/bee.ts

src/utils/type.ts

agazso

I think it would be better if you could change the TS project settings of your IDE and it would not be necessary to add the control comments in the committed file.

AuHau · 2021-06-08T15:06:49Z

I think it would be better if you could change the TS project settings of your IDE and it would not be necessary to add the control comments in the committed file.

Well, then can I remove the .vscode folder as well? 🤪

nugaon · 2021-06-09T08:02:35Z

src/chunk/signer.ts

@@ -89,7 +89,7 @@ export function makePrivateKeySigner(privateKey: PrivateKeyBytes): Signer {
 }

 export function assertSigner(signer: unknown): asserts signer is Signer {
-  if (typeof signer !== 'object' || signer === null) {
+  if (typeof signer !== 'object' || signer === null || Array.isArray(signer)) {


It is strange for me why you check it shouldn't be array, you could check also then it is not a number, boolean, etc. but what if it is an array that has the below checked functions that we call only? Will the code not run successfully?

src/utils/type.ts

nugaon · 2021-06-09T08:36:27Z

src/utils/type.ts

+    if (typeof options.tag !== 'number') {
+      throw new TypeError(`options.tag property in ${name} has to be number or undefined!`)
+    }
+
+    assertNonNegativeInteger(options.tag, 'options.tag')


the assertNonNegativeInteger already checks the value is whether a number so this condition runs twice. If we need some integers shouldn't be bigint, then I suggest that you should refactor the assertNonNegativeInteger that could only accept number as integer if its bigint: boolean parameter is false, or even better if you make a separate function for only number types which does the same.

src/utils/type.ts

nugaon · 2021-06-09T09:51:34Z

src/utils/type.ts

+
+export function assertPssMessageHandler(value: unknown): asserts value is PssMessageHandler {
+  if (typeof value !== 'object' || value === null || Array.isArray(value)) {
+    throw new TypeError('PssMessageHandler has to be object!')


an array is also an object.

src/bee.ts

nugaon · 2021-06-09T10:04:06Z

src/bee.ts

+    if (typeof topic !== 'string') {
+      throw new TypeError('topic has to be an string!')
+    }


Here the topic should be a 32 lengthed hex string without prefix, shouldn't it?

I don't think so. This is a PSS topic which I think is different from Feed's topic.

In what points are different? and why we don't distinguish feed topic from pss topic then?

Honestly, I am not sure, but this is already confusing on Bee's side. Feed topic is advertised as hex string as per API reference while PSS's topic is advertised as generic string.

I am all in to clear this up, but I don't think this is the time for that. I would start from Bee side to clarify what is expected, update API reference and then we can tackle that on bee-js side.

that's why I'm interested, because in swarm-cli we handle the PSS topics in the same way like the Feed topics.

I agree that this is confusing on the Bee side and I opened an issue about it. It seems that the way how we handle PSS topics in swarm-cli is incorrect at the moment (opened and issue about it as well).

src/bee.ts

nugaon · 2021-06-09T10:04:41Z

src/bee.ts

+    if (typeof topic !== 'string') {
+      throw new TypeError('topic has to be an string!')
+    }


Here the topic should be a 32 lengthed hex string without prefix, shouldn't it?

PSS not Feed.

Co-authored-by: nugaon <50576770+nugaon@users.noreply.github.com>

This reverts commit 9fa2189.

AuHau requested a review from nugaon as a code owner June 8, 2021 09:20

bee-runner bot added the type:pull-request label Jun 8, 2021

AuHau requested a review from a team June 8, 2021 09:20

agazso reviewed Jun 8, 2021

View reviewed changes

src/bee.ts Outdated Show resolved Hide resolved

agazso reviewed Jun 8, 2021

View reviewed changes

src/bee.ts Outdated Show resolved Hide resolved

agazso reviewed Jun 8, 2021

View reviewed changes

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

AuHau and others added 3 commits June 8, 2021 15:59

feat!: input validation

195663a

chore: feedback

076371b

Co-authored-by: Attila Gazso <agazso@gmail.com>

test: fixing tests

846a9a3

AuHau force-pushed the feat/input-validation branch from dc20db7 to 846a9a3 Compare June 8, 2021 14:00

AuHau requested a review from agazso June 8, 2021 14:08

agazso reviewed Jun 8, 2021

View reviewed changes

src/bee-debug.ts Outdated Show resolved Hide resolved

agazso reviewed Jun 8, 2021

View reviewed changes

src/bee.ts Outdated Show resolved Hide resolved

agazso reviewed Jun 8, 2021

View reviewed changes

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

agazso approved these changes Jun 8, 2021

View reviewed changes

chore: feedback

1b8e796

nugaon reviewed Jun 9, 2021

View reviewed changes

AuHau and others added 3 commits June 9, 2021 15:31

chore: feedback

1ff1cb3

Co-authored-by: nugaon <50576770+nugaon@users.noreply.github.com>

chore: feedback

9fa2189

revert: feedback

265ca79

This reverts commit 9fa2189.

AuHau merged commit 1a33932 into master Jun 9, 2021

AuHau deleted the feat/input-validation branch June 9, 2021 13:54

bee-worker mentioned this pull request Jun 9, 2021

chore: release 0.11.0 #344

Merged

nugaon mentioned this pull request Jun 11, 2021

feat: add number separators, units, conditional required, hex strings ethersphere/swarm-cli#137

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: input validation #343

feat!: input validation #343

AuHau commented Jun 8, 2021

agazso Jun 8, 2021

AuHau Jun 8, 2021

nugaon Jun 9, 2021

AuHau Jun 9, 2021 •

edited

Loading

nugaon Jun 9, 2021

vojtechsimetka Jun 9, 2021 •

edited

Loading

agazso left a comment

AuHau commented Jun 8, 2021 •

edited

Loading

nugaon Jun 9, 2021

nugaon Jun 9, 2021

nugaon Jun 9, 2021

nugaon Jun 9, 2021

AuHau Jun 9, 2021

nugaon Jun 9, 2021

AuHau Jun 9, 2021

nugaon Jun 9, 2021 •

edited

Loading

agazso Jun 9, 2021 •

edited

Loading

nugaon Jun 9, 2021

AuHau Jun 9, 2021

feat!: input validation #343

feat!: input validation #343

Conversation

AuHau commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AuHau Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechsimetka Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

agazso left a comment

Choose a reason for hiding this comment

AuHau commented Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nugaon Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

agazso Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AuHau Jun 9, 2021 •

edited

Loading

vojtechsimetka Jun 9, 2021 •

edited

Loading

AuHau commented Jun 8, 2021 •

edited

Loading

nugaon Jun 9, 2021 •

edited

Loading

agazso Jun 9, 2021 •

edited

Loading