This is the reason why we are doing code review... > On 2 Oct 2022, at 12:04, Peter Müller wrote: > > Hello Michael, > >> Hello, >> >>> On 26 Sep 2022, at 19:26, Peter Müller wrote: >>> >>> Hello Michael, >>> >>> better late than never: Thanks for your reply and the comments. >>> >>>> Hello Peter, >>>> >>>>> On 18 Aug 2022, at 22:42, Peter Müller wrote: >>>>> >>>>> Previously, the lack of distinction between different DROP lists caused >>>>> only the last one to be persisted. >>>>> >>>>> Signed-off-by: Peter Müller >>>>> --- >>>>> src/scripts/location-importer.in | 58 +++++++++++++++++++++----------- >>>>> 1 file changed, 39 insertions(+), 19 deletions(-) >>>>> >>>>> diff --git a/src/scripts/location-importer.in b/src/scripts/location-importer.in >>>>> index 8d47497..e4a9ab0 100644 >>>>> --- a/src/scripts/location-importer.in >>>>> +++ b/src/scripts/location-importer.in >>>>> @@ -1427,18 +1427,21 @@ class CLI(object): >>>>> def _update_overrides_for_spamhaus_drop(self): >>>>> downloader = location.importer.Downloader() >>>>> >>>>> - ip_urls = [ >>>>> - "https://www.spamhaus.org/drop/drop.txt", >>>>> - "https://www.spamhaus.org/drop/edrop.txt", >>>>> - "https://www.spamhaus.org/drop/dropv6.txt" >>>>> + ip_lists = [ >>>>> + ("Spamhaus DROP list", "https://www.spamhaus.org/drop/drop.txt"), >>>>> + ("Spamhaus EDROP list", "https://www.spamhaus.org/drop/edrop.txt"), >>>>> + ("Spamhaus DROPv6 list", "https://www.spamhaus.org/drop/dropv6.txt") >>>>> ] >>>> >>>> Would it not be better to have this as a dict? >>>> >>>> Or if you prefer something to iterate over, a tuple? >>>> >>>> Would it not be better/easier if we would use a handle like “SPAMHAUS-DROP”, “SPAMHAUS-EDROP”, … instead of a string with spaces? It does not matter that much to me, but since this is a machine-readable string, should it not be less human formatted? >>> >>> Hm, good point. I have changed that in the upcoming second version of the patch. >>> >>>> >>>>> >>>>> - asn_urls = [ >>>>> - "https://www.spamhaus.org/drop/asndrop.txt" >>>>> + asn_lists = [ >>>>> + ("Spamhaus ASN-DROP list", "https://www.spamhaus.org/drop/asndrop.txt") >>>>> ] >>>>> >>>>> - for url in ip_urls: >>>>> - # Fetch IP list >>>>> + for list in ip_lists: >>>>> + name = list[0] >>>>> + url = list[1] >>>> >>>> It would be more Pythonic if you wrote it like this: >>>> >>>> for name, url in ip_lists: >>>> ... >>>> >>>>> + >>>>> + # Fetch IP list from given URL >>>>> f = downloader.retrieve(url) >>>>> >>>>> # Split into lines >>>>> @@ -1448,11 +1451,11 @@ class CLI(object): >>>>> # downloads. >>>>> if len(fcontent) > 10: >>>>> self.db.execute(""" >>>>> - DELETE FROM autnum_overrides WHERE source = 'Spamhaus ASN-DROP list'; >>>>> - DELETE FROM network_overrides WHERE source = 'Spamhaus DROP lists'; >>>>> - """) >>>>> + DELETE FROM network_overrides WHERE source = '%s'; >>>> >>>> No need for the ‘’ around the %s. >>> >>> Yes there is, the SQL command results in an exception otherwise. >> >> Err, you are allowing some SQL command injection here. >> >> You do not want to use “%s” as a string format parameter for Python strings. It is more of a placeholder for the value that has to be escaped first - SQLite is using ? which is probably a better choice for Python. >> >> The database binding is doing this automatically, but the arguments need to be passed after the query. >> >> Under no circumstances whatsoever you want to chance the query dynamically. Never ever. >> >> I fixed this here: https://git.ipfire.org/?p=location/libloc.git;a=commitdiff;h=b56bf4bf065130b38968b580e0bea6db809783d8 > > better late than never: Many thanks for spotting and fixing this before it went into > anything stable or productive! Guess I'm not a good programmer after all... > > All the best, > Peter Müller > >> >> Best, >> -Michael >> >>> >>> Second version of the patch is coming right up. >>> >>> Thanks, and best regards, >>> Peter Müller >>> >>>> >>>>> + """ % name, >>>>> + ) >>>>> else: >>>>> - log.error("Spamhaus DROP URL %s returned likely bogus file, ignored" % url) >>>>> + log.error("%s (%s) returned likely bogus file, ignored" % (name, url)) >>>>> continue >>>>> >>>>> # Iterate through every line, filter comments and add remaining networks to >>>>> @@ -1475,8 +1478,8 @@ class CLI(object): >>>>> >>>>> # Sanitize parsed networks... >>>>> if not self._check_parsed_network(network): >>>>> - log.warning("Skipping bogus network found in Spamhaus DROP URL %s: %s" % \ >>>>> - (url, network)) >>>>> + log.warning("Skipping bogus network found in %s (%s): %s" % \ >>>>> + (name, url, network)) >>>>> continue >>>>> >>>>> # Conduct SQL statement... >>>>> @@ -1488,14 +1491,31 @@ class CLI(object): >>>>> ) VALUES (%s, %s, %s) >>>>> ON CONFLICT (network) DO UPDATE SET is_drop = True""", >>>>> "%s" % network, >>>>> - "Spamhaus DROP lists", >>>>> + name, >>>>> True >>>>> ) >>>>> >>>>> - for url in asn_urls: >>>>> + for list in asn_lists: >>>>> + name = list[0] >>>>> + url = list[1] >>>>> + >>>> >>>> See above. >>>> >>>>> # Fetch URL >>>>> f = downloader.retrieve(url) >>>>> >>>>> + # Split into lines >>>>> + fcontent = f.readlines() >>>>> + >>>>> + # Conduct a very basic sanity check to rule out CDN issues causing bogus DROP >>>>> + # downloads. >>>>> + if len(fcontent) > 10: >>>>> + self.db.execute(""" >>>>> + DELETE FROM autnum_overrides WHERE source = '%s'; >>>>> + """ % name, >>>>> + ) >>>>> + else: >>>>> + log.error("%s (%s) returned likely bogus file, ignored" % (name, url)) >>>>> + continue >>>> >>>> Is there a reason why this DELETE statement is not part of the transaction? If something goes wrong later on, you would have lost the previous data. >>>> >>>> Would it also not be programmatically better to start the import and then count how many lines you are importing? >>>> >>>> The “f.readlines()” call is something that I would consider a bit ugly. >>>> >>>>> + >>>>> # Iterate through every line, filter comments and add remaining ASNs to >>>>> # the override table in case they are valid... >>>>> with self.db.transaction(): >>>>> @@ -1518,8 +1538,8 @@ class CLI(object): >>>>> >>>>> # Filter invalid ASNs... >>>>> if not self._check_parsed_asn(asn): >>>>> - log.warning("Skipping bogus ASN found in Spamhaus DROP URL %s: %s" % \ >>>>> - (url, asn)) >>>>> + log.warning("Skipping bogus ASN found in %s (%s): %s" % \ >>>>> + (name, url, asn)) >>>>> continue >>>>> >>>>> # Conduct SQL statement... >>>>> @@ -1531,7 +1551,7 @@ class CLI(object): >>>>> ) VALUES (%s, %s, %s) >>>>> ON CONFLICT (number) DO UPDATE SET is_drop = True""", >>>>> "%s" % asn, >>>>> - "Spamhaus ASN-DROP list", >>>>> + name, >>>>> True >>>>> ) >>>>> >>>>> -- >>>>> 2.35.3