Hi,
I had a quick look to help a bit anonym.
Alan wrote (01 Mar 2014 23:28:20 GMT) :
> - Sanitize IPv6 addresses in WhisperBack
> (https://labs.riseup.net/code/issues/6571)
I'm not following you on the "I don't see why to use such a complex
solution while addresses in the exemple are already expanded"
position. The fact the provided example contains expanded IPv6
addresses does not guarantee that "compressed" (or whatever it's
called) IPv6 addresses never appear in logs.
To clarify: I'm (almost, see below) fine with seeing your proposed
changes merged, because it improves the current situation, but IMO
what it does is resolve a (non-existing yet) "Sanitize expanded IPv6
addresses in WhisperBack reports" subtask, but clearly *not* #6571
entirely.
> + 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.
Also, there's "IP6" written in a few places. Is this intentional?
> - 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.
> - Whisperback should sanitize more serial numbers
> (https://labs.riseup.net/code/issues/6798)
The regexp could certainly use some refactoring at some point,
but that's no blocker yet.
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?
Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
