Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(237)

Issue 29821558: Issue #6707 - Make the generated malware domain filter list encode domains as Punycode (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 2 weeks ago by Tudor Avram
Modified:
2 months ago
Visibility:
Public.

Description

Issue #6707 - Make the generated malware domain filter list encode domains as Punycode

Patch Set 1 #

Total comments: 4

Patch Set 2 : updated tests #

Total comments: 2

Patch Set 3 : Updated encoding format #

Total comments: 3

Patch Set 4 : Removed uncessary codecs call from updateMalwareDomains.py #

Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -7 lines) Patch
M .gitignore View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M sitescripts/subscriptions/bin/updateMalwareDomainsList.py View 1 2 3 3 chunks +5 lines, -4 lines 0 comments Download
M sitescripts/subscriptions/test/test_updateMalwareDomainsList.py View 1 2 3 2 chunks +5 lines, -2 lines 0 comments Download
M tox.ini View 1 2 3 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 16
Tudor Avram
Here is my first implementation for this issue.
2 months, 2 weeks ago (2018-07-03 15:52:39 UTC) #1
Vasily Kuznetsov
Hi Tudor, This looks pretty good. Just one comment about .gitignore and one about the ...
2 months, 2 weeks ago (2018-07-03 16:01:52 UTC) #2
Vasily Kuznetsov
LGTM Sebastian, could you please check that the output looks right (as in: it's what ...
2 months, 2 weeks ago (2018-07-03 16:42:22 UTC) #3
Vasily Kuznetsov
For reference, here's the whole patch combined (for some reason Rietveld doesn't let us download ...
2 months, 2 weeks ago (2018-07-05 13:45:32 UTC) #4
Vasily Kuznetsov
Argh. No, the previous comment doesn't work, after line wrapping by Rietveld. Sebastian, if you ...
2 months, 2 weeks ago (2018-07-05 13:48:07 UTC) #5
Vasily Kuznetsov
Argh. No, the previous comment doesn't work, after line wrapping by Rietveld. Sebastian, if you ...
2 months, 2 weeks ago (2018-07-05 13:48:07 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29821558/diff/29821560/sitescripts/subscriptions/bin/updateMalwareDomainsList.py File sitescripts/subscriptions/bin/updateMalwareDomainsList.py (right): https://codereview.adblockplus.org/29821558/diff/29821560/sitescripts/subscriptions/bin/updateMalwareDomainsList.py#newcode83 sitescripts/subscriptions/bin/updateMalwareDomainsList.py:83: print >>file, '||%s^' % domain.decode('idna').encode('punycode') This seems incorrect. Let's ...
2 months, 2 weeks ago (2018-07-05 14:55:28 UTC) #7
Tudor Avram
Amended the code. https://codereview.adblockplus.org/29821558/diff/29821559/.gitignore File .gitignore (right): https://codereview.adblockplus.org/29821558/diff/29821559/.gitignore#newcode9 .gitignore:9: env/ On 2018/07/03 16:01:52, Vasily Kuznetsov ...
2 months, 2 weeks ago (2018-07-06 11:53:17 UTC) #8
Sebastian Noack
https://codereview.adblockplus.org/29821558/diff/29824562/sitescripts/subscriptions/bin/updateMalwareDomainsList.py File sitescripts/subscriptions/bin/updateMalwareDomainsList.py (right): https://codereview.adblockplus.org/29821558/diff/29824562/sitescripts/subscriptions/bin/updateMalwareDomainsList.py#newcode63 sitescripts/subscriptions/bin/updateMalwareDomainsList.py:63: file = codecs.open(path, 'wb') codecs.open() without specifying an encoding ...
2 months, 2 weeks ago (2018-07-06 18:23:25 UTC) #9
Tudor Avram
On 2018/07/06 18:23:25, Sebastian Noack wrote: > https://codereview.adblockplus.org/29821558/diff/29824562/sitescripts/subscriptions/bin/updateMalwareDomainsList.py > File sitescripts/subscriptions/bin/updateMalwareDomainsList.py (right): > > https://codereview.adblockplus.org/29821558/diff/29824562/sitescripts/subscriptions/bin/updateMalwareDomainsList.py#newcode63 ...
2 months, 1 week ago (2018-07-09 09:15:40 UTC) #10
Sebastian Noack
https://codereview.adblockplus.org/29821558/diff/29824562/sitescripts/subscriptions/bin/updateMalwareDomainsList.py File sitescripts/subscriptions/bin/updateMalwareDomainsList.py (right): https://codereview.adblockplus.org/29821558/diff/29824562/sitescripts/subscriptions/bin/updateMalwareDomainsList.py#newcode63 sitescripts/subscriptions/bin/updateMalwareDomainsList.py:63: file = codecs.open(path, 'wb') On 2018/07/06 18:23:25, Sebastian Noack ...
2 months, 1 week ago (2018-07-09 11:59:29 UTC) #11
Tudor Avram
removed unnecessary `codecs` dependency. https://codereview.adblockplus.org/29821558/diff/29824562/sitescripts/subscriptions/bin/updateMalwareDomainsList.py File sitescripts/subscriptions/bin/updateMalwareDomainsList.py (right): https://codereview.adblockplus.org/29821558/diff/29824562/sitescripts/subscriptions/bin/updateMalwareDomainsList.py#newcode63 sitescripts/subscriptions/bin/updateMalwareDomainsList.py:63: file = codecs.open(path, 'wb') On ...
2 months, 1 week ago (2018-07-09 13:35:43 UTC) #12
Sebastian Noack
LGTM
2 months, 1 week ago (2018-07-09 13:39:40 UTC) #13
Vasily Kuznetsov
On 2018/07/09 13:39:40, Sebastian Noack wrote: > LGTM Sebastian, when should we push this change? ...
2 months, 1 week ago (2018-07-09 14:25:57 UTC) #14
Sebastian Noack
On 2018/07/09 14:25:57, Vasily Kuznetsov wrote: > On 2018/07/09 13:39:40, Sebastian Noack wrote: > > ...
2 months, 1 week ago (2018-07-09 14:39:59 UTC) #15
Vasily Kuznetsov
2 months, 1 week ago (2018-07-09 16:29:16 UTC) #16
On 2018/07/09 14:39:59, Sebastian Noack wrote:
> On 2018/07/09 14:25:57, Vasily Kuznetsov wrote:
> > On 2018/07/09 13:39:40, Sebastian Noack wrote:
> > > LGTM
> > 
> > Sebastian, when should we push this change? I suppose it would be good to
> > synchronize with corresponding release of ABP or should we just go ahead?
> 
> Yes, as discussed on the issue: Before this change lands, filters from the
> Malware Domains list targeting internationalized domains won't have any effect
> in Adblock Plus 3.2 (to be released on July 17th; and above). After this
change
> lands, the respective filters will be ignored in older versions of Adblock
Plus.
> However, given that the Malware Domains list is opt-in, and that only few
> filters in there actually target internationalized domains, I think the impact
> is small enough, that it doesn't matter to much when exactly this lands.

Thank you. Since there's not much activity currently in sitescripts, I think it
should be no problem to just push this on July 17th.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5