Re: [Tails-dev] Please review and merge whisperback:bugfix/f…

Nachricht löschen

Nachricht beantworten
Autor: intrigeri
Datum:  
To: The Tails public development discussion list
Betreff: Re: [Tails-dev] Please review and merge whisperback:bugfix/filter_more_serial_nos
Hi,

Alan wrote (02 Mar 2014 10:09:59 GMT) :
> - I can't remove something that I don't have examples how it looks
> like;


There's plenty of documentation about IPv6 available, not mentioning
the RFCs :)

> - the proposed fix looks too complicated to me and I have no easy way
> to test it.


Agreed. There's most likely a way simpler solution.

> OK. Created :


Great!

>> > +    log_string = re.sub(r'[0-9a-fA-F]{4}(:[0-9a-fA-F]{4}){7}',
>> > +                        r'[IP6 REMOVED]',
>> > +                        log_string)

>>
>> Better make the regexp case insensitive, I think.
>>
> Why? The above regexp will match uppercase and lowercase.


/[a-fA-F]/ is the expanded version of /a-f/i, with the usual downcases
of expanding things that could be factored: longer, harder to read and
understand, more prone to typos. Also, I think it expresses the
meaning in a less clearer way. But that's certainly a bit subjective,
and I don't care that much.

>> Also, there's "IP6" written in a few places. Is this intentional?
>>
> Yes. What would you prefer?


Unless "IP6" has a well-defined meaning that I'm not aware of, let's
not invent a new name for it. If what you mean is "IPv6 addresses",
just write that :)

>> > - Whisperback should sanitize DMI
>> > (https://labs.riseup.net/code/issues/6797)
>>
>> I'm unsure about that one. The DMI string often contain useful info
>> (e.g. the version of the BIOS), so killing it entirely because it may
>> contain serial numbers does not look optimal to me.
>>
> I think we already have most of the information we want from dmidecode,
> which gives it more selectively. I fail to see how keep what could be
> useful to us while removing the serial number of the machine in this
> very vendor-specific string, so I preferred to be on the safe side.


Fair enough, assuming we already extract the Version field from the
dmidecode output (which I've not checked).

>> Also, I'm glad there is a test suite, but the way it works seems
>> strange to me: if I understand it correctly, it's testing that
>> "[$something REMOVED]" can be found after sanitizing, but it does not
>> test that the info that should be removed cannot be found anymore,
>> which is what we should really test, no?


> It would be better, yes. Want to create a ticket?


That's now #6806. I'd rather see it fixed as part of this pull
request, if we want to rely on this test suite to guarantee that the
code is working well, but I won't insist so I've refrained myself from
marking this ticket as blocking #6798.

Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc