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

Delete this message

Reply to this message
Author: Alan
Date:  
To: tails-dev
Subject: Re: [Tails-dev] Please review and merge whisperback:bugfix/filter_more_serial_nos
Hi,

> >> > +    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.
>

Done.

> >> 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 :)
>

Done.

> >> > - 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).
>

We have:

    debug_command /usr/sbin/dmidecode -s system-version


> >> 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.
>

Done in that branch. But I can't push yet...

Cheers