chore: filter v2 tests coverage improvement

[romanzac]

Description
Batch of tests for Waku Filter v2 to improve coverage

Changes
Tests available to run:
cd go-waku/waku/v2/protocol/filter
go test

[status-im-auto]

Jenkins Builds

Click to see older builds (7)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	49435a8	#1	2023-11-28 15:08:09	~4 min	nix-flake	📄log
✔️	d0ef90e	#2	2023-11-29 07:19:25	~2 min	nix-flake	📄log
✔️	c631963	#3	2023-11-29 07:39:16	~1 min	nix-flake	📄log
✔️	28456d3	#4	2023-11-29 08:06:44	~1 min	nix-flake	📄log
✔️	e7adf54	#5	2023-11-29 12:36:45	~1 min	nix-flake	📄log
✔️	9b7bce4	#6	2023-11-29 12:38:46	~1 min	nix-flake	📄log
✔️	bfbfa95	#7	2023-11-30 05:46:01	~2 min	nix-flake	📄log

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	451a715	#8	2023-12-01 00:12:55	~1 min	nix-flake	📄log
✔️	81a7048	#9	2023-12-01 02:06:33	~1 min	nix-flake	📄log

[romanzac]

@chaitanyaprem I would like to wrap up filter tests with covering certain error cases. I wonder if you see the same value in 49435a8 I'd say integrator might appreciate to receive error when sending UnsubscribeAll with wrong peer.ID (host.ID) by mistake. Being an attacker would probably give up looking at peer ID length. So I would say benefit >> security risk to provide error?

[chaitanyaprem]

@chaitanyaprem I would like to wrap up filter tests with covering certain error cases. I wonder if you see the same value in 49435a8 I'd say integrator might appreciate to receive error when sending UnsubscribeAll with wrong peer.ID (host.ID) by mistake. Being an attacker would probably give up looking at peer ID length. So I would say benefit >> security risk to provide error?

Good one, looks like as of now the result will just be an empty instead of an error indicating no subscription found for the peer which is not a great dev ex.
This could be addressed so that the API returns appropriate error, but I don't understand how it is some secuirty risk.

[romanzac]

@chaitanyaprem I would like to wrap up filter tests with covering certain error cases. I wonder if you see the same value in 49435a8 I'd say integrator might appreciate to receive error when sending UnsubscribeAll with wrong peer.ID (host.ID) by mistake. Being an attacker would probably give up looking at peer ID length. So I would say benefit >> security risk to provide error?

Good one, looks like as of now the result will just be an empty instead of an error indicating no subscription found for the peer which is not a great dev ex. This could be addressed so that the API returns appropriate error, but I don't understand how it is some secuirty risk.

It is not a security risk, or it is just a negligible one. To compare, there is also no error when we unsubscribe from non existent topic. Here, topic could be very short string, and could be potentially guessed by executing enough unsubscribe calls. Attacker could then listen to conversation or spam (up to rate the limit). It is good we don't return any error code at that case.

I've created improvement #933

[chaitanyaprem]

@chaitanyaprem I would like to wrap up filter tests with covering certain error cases. I wonder if you see the same value in 49435a8 I'd say integrator might appreciate to receive error when sending UnsubscribeAll with wrong peer.ID (host.ID) by mistake. Being an attacker would probably give up looking at peer ID length. So I would say benefit >> security risk to provide error?

Good one, looks like as of now the result will just be an empty instead of an error indicating no subscription found for the peer which is not a great dev ex. This could be addressed so that the API returns appropriate error, but I don't understand how it is some secuirty risk.

It is not a security risk, or it is just a negligible one. To compare, there is also no error when we unsubscribe from non existent topic. Here, topic could be very short string, and could be potentially guessed by executing enough unsubscribe calls. Attacker could then listen to conversation or spam (up to rate the limit). It is good we don't return any error code at that case.

I've created improvement #933

We need to return the proper errors so that dev ex is better.
Wrt an attacker listening to conversation, they can listen to any messages as long as they are subscribed to a pubsubTopic using relay (since this is a gossipsub network), which is why it is recommended for applications to encrypt their data so that 3rd parties cannot read the actual content.
Also note that there is no rate limit when it comes to listening/receiving messages, the only rate-limit is for publishing messages to the network (in order to avoid spam).

[chaitanyaprem]

LGTM

[romanzac]

@chaitanyaprem I would like to wrap up filter tests with covering certain error cases. I wonder if you see the same value in 49435a8 I'd say integrator might appreciate to receive error when sending UnsubscribeAll with wrong peer.ID (host.ID) by mistake. Being an attacker would probably give up looking at peer ID length. So I would say benefit >> security risk to provide error?

Good one, looks like as of now the result will just be an empty instead of an error indicating no subscription found for the peer which is not a great dev ex. This could be addressed so that the API returns appropriate error, but I don't understand how it is some secuirty risk.

It is not a security risk, or it is just a negligible one. To compare, there is also no error when we unsubscribe from non existent topic. Here, topic could be very short string, and could be potentially guessed by executing enough unsubscribe calls. Attacker could then listen to conversation or spam (up to rate the limit). It is good we don't return any error code at that case.
I've created improvement #933

We need to return the proper errors so that dev ex is better. Wrt an attacker listening to conversation, they can listen to any messages as long as they are subscribed to a pubsubTopic using relay (since this is a gossipsub network), which is why it is recommended for applications to encrypt their data so that 3rd parties cannot read the actual content. Also note that there is no rate limit when it comes to listening/receiving messages, the only rate-limit is for publishing messages to the network (in order to avoid spam).

@chaitanyaprem Would you like to fix #933 some time soon ? This PR has a test which depends on fixing #933, so it won't pass required checks before fixing.

[chaitanyaprem]

@chaitanyaprem I would like to wrap up filter tests with covering certain error cases. I wonder if you see the same value in 49435a8 I'd say integrator might appreciate to receive error when sending UnsubscribeAll with wrong peer.ID (host.ID) by mistake. Being an attacker would probably give up looking at peer ID length. So I would say benefit >> security risk to provide error?

Good one, looks like as of now the result will just be an empty instead of an error indicating no subscription found for the peer which is not a great dev ex. This could be addressed so that the API returns appropriate error, but I don't understand how it is some secuirty risk.

It is not a security risk, or it is just a negligible one. To compare, there is also no error when we unsubscribe from non existent topic. Here, topic could be very short string, and could be potentially guessed by executing enough unsubscribe calls. Attacker could then listen to conversation or spam (up to rate the limit). It is good we don't return any error code at that case.
I've created improvement #933

We need to return the proper errors so that dev ex is better. Wrt an attacker listening to conversation, they can listen to any messages as long as they are subscribed to a pubsubTopic using relay (since this is a gossipsub network), which is why it is recommended for applications to encrypt their data so that 3rd parties cannot read the actual content. Also note that there is no rate limit when it comes to listening/receiving messages, the only rate-limit is for publishing messages to the network (in order to avoid spam).

@chaitanyaprem Would you like to fix #933 some time soon ? This PR has a test which depends on fixing #933, so it won't pass required checks before fixing.

Just merged the fix on to master. If you update your branch, you should have the fix in it and ci would pass :)