privacy blocking iq should not stop stanzas sent by my server to me #2724
Conversation
|
8205.1 / Erlang 22.0 / small_tests / 544818e 8205.2 / Erlang 22.0 / internal_mnesia / 544818e 8205.4 / Erlang 22.0 / mysql_redis / 544818e 8205.3 / Erlang 22.0 / odbc_mssql_mnesia / 544818e 8205.5 / Erlang 22.0 / riak_mnesia / 544818e 8205.7 / Erlang 22.0 / elasticsearch_and_cassandra_mnesia / 544818e 8205.6 / Erlang 22.0 / ldap_mnesia / 544818e 8205.9 / Erlang 21.3 / pgsql_mnesia / 544818e |
Codecov Report
@@ Coverage Diff @@
## master #2724 +/- ##
==========================================
+ Coverage 76.32% 79.27% +2.94%
==========================================
Files 368 368
Lines 30510 30516 +6
==========================================
+ Hits 23287 24191 +904
+ Misses 7223 6325 -898
Continue to review full report at Codecov.
|
| #jid{lserver = S} = _To, | ||
| _) -> allow; | ||
| %% do not bother checking if privacy list is empty | ||
| basic_check(_, _, _, #privacy{lists = []}) -> allow; |
There was a problem hiding this comment.
I think this doesn't get a #privacy{} record, but rather a #userlists{} one. It's on the specs of the caller, and also codecov shows this line as never hit, with is a sign 🤔
There was a problem hiding this comment.
Also, #userlists{lists = []} is already ignored in the handler in mod_privacy:check_packet/, in case you missed it. I'm not saying it's bad to duplicate logic, but maaybe it's worth giving it a thought 🤔
There was a problem hiding this comment.
true, again. I'd put it here, though, if only for more visibility.
XEP-0199 is about Ping, the one for privacy lists is XEP-0016, which describes the IQ blocking feature in the following way:
As an entity is a user OR any service, I don't see a reason to exclude any IQ stanzas from the blocking feature. I think that if we want to change this, we would need to get an opinion on the XSF mailing list first. I am aware of the fact that Example 44 explicitly mentions "users", but the previous examples just mention "entities". Regarding the race condition, could we add a test for it first and make sure this test fails randomly to prove that it really exists? |
Of course, my mistake.
Yup, this is exactly why I expected a discussion. The XEP doesn't specifically state which IQs should be blocked and which shouldn't, so there is a need for interpretation. The purpose of privacy lists is to avoid unwanted communication, like malicious users or servers. Blocking IQs from my own server in effect breaks some other xeps (like mod_ping), but also RFC 6120 (which says that the server should reply to an IQ with a result or an error) and RFC 6121 (roster push). Even more, it breaks the same XEP, which specifically states that if you edit a privacy list you should receive a iq result. Having said that, I'm not opposed to consulting XSF if there is a need for it.
Not really, because the messages always arrived in the same order, so technically speaking it was not a race condition since one horse was winning it all the time. However, a minor change in code layout was sufficient to reverse the order, so the race must have been pretty tight. |
Let's have a discussion then, I think the best place for it would be a tech meeting, but the opinion on the XSF mailing list would be also very valuable. |
This comment was marked as spam.
This comment was marked as spam.
|
@chrzaszcz @NelsonVides Can we get back to this one? To me it seems still valid, though if you guys decide otherwise we can close this PR. |
Previously, blocking all incoming iqs also cut off communication with the server, also in legitimate cases - like receiving an iq result from my own request. This is not required by XEP-0199, and it broke some features, like mod_ping. There was also a race condition - when a user sets a privacy lists, his c2s process routes two messages - iq result and a broadcast, which also sets the list in the same process. Whether the result came back or not, depended on which of these two messages was handled first.
This PR skips privacy checks for iq stanzas sent to a user by his own server account or by the server itself.