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 >