Test if target is a valid host #10877

Activity

1. J0WI

   added

   ruleset-testing

   on Jul 5, 2017
2. cschanaj commented on Jul 6, 2017

   

   cschanaj

   on Jul 6, 2017

   Collaborator

   More actions

   @J0WI thank you for opening this issue. I would like to add some details here.

   In #10869 I checked if a target is valid by matching it against the Public Suffix List. While this test does not guarantee each target has a working DNS record, it has the advantage that no network request is required to perform the checking.

   It was found that AM-Best-Company.xml#L11 with a invalid target passed the comprehensive fetch test and got re-activated in #10538. Despite @Bisaloo's assertion, I would like to hear from @Hainish for an explaination.
3. cschanaj commented on Jul 16, 2017

   

   cschanaj

   on Jul 16, 2017

   Last edited by cschanaj

   Collaborator

   More actions

   @Hainish @J0WI I have implemented the logic which check each target is having a valid gTLD/ ccTLD in the rule-test in #11187. Besides, I noted that the following type of rewrite will pass the fetch-test without any Travis complaint

   <ruleset name="cschanaj">
     <target host="dns-record-does-not-exist.example.com" />
     
     <rule from="^http://dns-record-does-not-exist\.example\.com/"
             to="https://example.com/" />
   </ruleset>

   Is this an intended behavior? Should we check each target for an existing DNS record? See cschanaj/https-everywhere/pull/6

   Besides, should we allow IP target as in Xxxbunker.com.xml#L44?

   Please be noted that the code does not compile currently.

   👍React with 👍1Reacted by Hugo Gruson
4. cschanaj commented on Jul 19, 2017

   

   cschanaj

   on Jul 19, 2017

   Collaborator

   More actions

   @Hainish can you have a look on #10877 (comment) and #11187 ?
5. Hainish commented on Jul 20, 2017

   

   Hainish

   on Jul 20, 2017

   Member

   More actions

   Not quite sure how AM-Best-Company.xml#L11 got re-enabled. Checking targets against the Public Suffix List seems reasonable.
6. cschanaj commented on Jul 20, 2017

   

   cschanaj

   on Jul 20, 2017

   Last edited by cschanaj

   Collaborator

   More actions

   @Hainish For any target if HTTPS connection works, the fetch test will ignore any result for the HTTP connection, see check_rules.py#L131. In particular, it will ignore the could not resolve host error. As a result, any rewrite in the form of #10877 (comment) will always pass the fetch test.

   It seems to be a bug to me. A quick fix is to throw an error if HTTP cannot be resolved. Can you confirm this? Besides, what should be done for ip target as in Xxxbunker.com.xml#L44?
7. cschanaj commented on Jul 21, 2017

   

   cschanaj

   on Jul 21, 2017

   Last edited by cschanaj

   Collaborator

   More actions

   All ruleset in the code base will pass the proposed rule-test once #11263 is merged, it will be also safe to merge #11187 (hopefully before next release). See also #11264.

   Updated #11187 to include the changes in #11264. thanks!
8. Hainish

   closed this as completedin #11187on Aug 22, 2017
9. Hainish

   added a commit that references this issue on Aug 22, 2017

   Update https_everywhere_checker/check_rules.py, Fix #10877 (#11187)

   This issue will close once commit 4964511 is merged into the 'master' branch.

   4964511