-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
On 08/02/2013 03:47 PM, Matthew Toseland wrote:
> On Thursday 01 Aug 2013 19:35:47 Martin Nyhus wrote:
>> I need to go though this is more detail, but some initial
>> thoughts:
>
> Any reason not to forward/CC this to devl?
Nope. Your original email wasn't sent there so I simply didn't think
about it. I'll send the stuff from your email that I didn't quote
separately.
>>
>> On 08/01/2013 02:47 PM, Matthew Toseland wrote:
>>> 7c9b4d08894f319d0e7022a02d35d96899261958 - not safe, surely? -
>>> headers can be encoded, can't they? is that implemented in
>>> Freemail? - this part only deals with body ... but elsewhere
>>> there must be support for quoted-printable etc in headers. in
>>> which case this is bogus. Do they use the same charset? It
>>> would be OK to pass through if the charset is only used for the
>>> body - but the charset for the headers MUST be known. (and we
>>> should test this). - it looks like there is support for encoded
>>> headers. but it seems to be completely separate from support
>>> for encoded body? SECURITY: quoted-printable etc in headers:
>>> MUST NOT fallback to raw content!
>>
>> That function is only used for displaying things on web pages, so
>> it is safe assuming the body is 7bit clean (as it should be).
>> Since we don't actually check the content before displaying or
>> when receiving we should either:
>
> If it's only displayed on a web page, then surely we add it with
> new HTMLNode("#", string)? If so, it's safe, period.
>
> Having said that, my main concern here is header filtering: From,
> To, etc. Stuff that might be meaningful to clients and cause them
> to do bad things too. We don't decode before filtering? Should we?
> Will an encoded From always be rejected?
With the exception of the spoofed From header check, we don't do *any*
filtering of incoming messages. When clients connect via IMAP they get
the raw content we store on disk, the IMAP server doesn't decode stuff
for them. A future filter for incoming messages must of course decode
messages and generally try to parse what the clients will actually parse.
>> 1. Fall back to a mode that displays the raw content, but filters
>> out anything that isn't ASCII printable characters 2. Throw
>> UnsupportedEncodingException and display an error of some kind in
>> the web ui.
>>
>> So, it might be unsafe at the moment.
>
> If it's only the web interface then it's safe, although it may look
> garbled. And from the call trace it looks like this is the case.
>>
>> Headers can be encoded and decoding is implemented in
>> MailMessage.decodeHeader(). It seems to lack proper test
>> coverage, but it supports both quoted printable[1] (Q) and base64
>> (B) encodings. Charset support depends on the JVM. Encoded
>> headers can have different encodings (even within one header),
>> and it is not related to the body encoding at all. We throw on
>> unknown charsets/encodings in the headers.
>>
>> [1] Note that quoted printable in the headers doesn't mean the
>> same thing as quoted printable in the body!
>>
>>
>>> 40141456bf5457d395b9afac49d1ef3c48c054c5 - i don't understand
>>> how writeBuffer can assert the buffer is only whitespace and
>>> then handle the non-whitespace case? And anyway this is
>>> definitely not always going to be true?
>>
>> Not 100% sure I understand what is going on (bad sign), so I'll
>> have a look at it later. Looks to me like we only put whitespace
>> into the buffer (which makes sense). Needs fixing anyway, if only
>> to make it easier to understand.
>
> Okay, so it's just a bit generous in what it accepts in the for()
> loop, and unclear. And over-complicated as a result. There can only
> be space, tab or carriage return, and we only need to writeEncoded
> if it's a carriage return? Also you should rename the buffer, or at
> least document it, to reflect the fact that it's always
> whitespace.
I don't remember all the details, but e.g. you can't end a line with
whitespace, and you need to be careful about \r\n since it suddenly
has two meanings depending on the context etc. I'll read the RFC again
and
fix/clarify/document the code.
>>
>>> 60aa04c71a778ffd403c032b01b994c2251355d6 - TODO: Fix the
>>> changelog given the versions released weren't as planned. Or
>>> were they close enough? I did the recent releases so I guess I
>>> can deal with this?
>>
>> Would be nice if you could deal with this, especially since you
>> know which versions you actually released into the wild. Not that
>> many commits anyway, so I can review whatever you have time to do
>> and pick up the rest.
>
> Ok.
>>
>>> 843d4ddef927270bf322e032aef01d6bcb30cca6 - Logger.debug()
>>> should be if(logDEBUG)??? Or don't we use this in Freemail?
>>> Should we? I think it still has its own logger implementation
>>> ... Does it still work on the command line?
>>
>> There is no if(logDEBUG) in Freemail, nor do I think it is
>> needed. The amount of logging is miniscule compared to Freenet.
>> The Freemail logger is still separate, but will always call the
>> Freenet log functions anyway. The command line version doesn't
>> work, but I'm not sure how far from working it is. For more
>> serious testing it would be nice if it did work, but I doubt it
>> is worth the effort right now.
>
> Ok.
>>
>>> 7d86e47d65e5bf300c3da9997a521135f7da4931 - No From header: we
>>> still know who sent it - shouldn't we synthesise one? isn't
>>> there a standard header for this even? IIRC we flag a bogus
>>> From header as spoofed, but do we add a real one?
>>
>> We should add it. We should have a filter that we pass received
>> messages through, and adding a From header if missing is one of
>> many things it should be doing. Related to bug #5835.
>
> Filed a separate bug:
> https://bugs.freenetproject.org/view.php?id=5955 This is not a
> regression so does not block releasing. It should however be fixed
> ASAP.
>>
>>> 2f93ef2ea150fcdc84ec929e04c86e68e6bed017 - shouldn't tools
>>> include junit? it seems to use sssertion errors - it should
>>> have assertions enabled - and later on it uses
>>> TextProtocolTester which again uses assertions.
>>
>> Not sure I follow here. AssertionError is in java.lang, same
>> thing that is thrown on assert(false). I can't see where anything
>> in tools/ uses TextProtocolTester?
>
> assert(false) is a no-op unless assertions are explicitly enabled
> at run-time with -enableassertions. Which is true for junit but not
> true for tools here? Oh, you don't actually use them in testing
> currently? Ok then.
Ok my attempt at explaining it was awful. I throw AssertionError in
the cases where my assumptions fail, mostly because I didn't want to
write a lot of error handling code. This has nothing to do with junit
or the unit tests in general. tools/send_receive_message runs a
testcase that sends an email via SMTP using Freemail account 1 then
receives it via IMAP using Freemail account 2. In other words, it
automates sending a real message over the real network for testing,
using the stuff in tools/src/.
>>> 6b1bb8e5b00a981f4e6b1f22a30a49530fdde958 - why is it a
>>> LinkedList<Locale[]> ? Why not LinkedList<Locale>?
>>
>> The @Parameter function should return a Collection<Object[]>,
>> where each item in the collection becomes a test case, and each
>> item in the array becomes an argument to the constructor.
>
> Ok.
>>
>>> MailHeaderFilter: should be ends with ".freemail" not
>>> "freemail" ??
>>
>> It should. We should probably just match it with the full domain
>> while we're at it.
>
> I have changed it to ".freemail".
>
> Okay so I can release this. I will need to deal with the changelogs
> first.
>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBCAAGBQJR++MaAAoJEL0Rn1Vb3j1/XbAQAKXZ1ZBHJlPkBBafVqjgnzC2
zqGDXrXTMcRaUxLFRsZgfmjXJiAqJvToDLUMHDHNhoy98EElsERp2JTarXASoSXp
osrFOl20LP/d2F5WXnzw1FKUCKFDb55/mk+8E6RL6ndDkHalcDktAMdFI/RSouih
Lf86p61wY+7ZhVy6tisFWUN/lC2Au2U5ZVM7YpR8UQxBo9w81gYeDk3UayQVrwjh
o2i1teTbZDHNBFvbXBE+4e0cdeztsfeZDl68SPrfh0ZGDDCz0XTJBtM9Pb/PBMVz
RgKW2nKCZapkGwlkcIx8gXMgvfpqwN+aZPhdOFAuT6Poj4KLi2Gfq8om3RHpD8Xy
oyDB4XoE8Iht1wihUDqv1vFw5fsXSygkiM6kWuuJQG6KMYKseR2v3k/+plymZg7G
myHo9SM3vrswxlduHzuW0nfcCKEi4gHqhF1e66Jd4Z7R1qi1YkpkuTiFUe6SVaVj
nz/6Ra/DGVptW6fbcD68IStY6hs4C36z5Lsi2/CjXtJNz1mInnfnk5eV8aju+Tno
x0YSHpMhqTjwPyDaf6OgY8Myf8NjtFiCagxbUl6njJXKgJwaztZJi9jxM7tQz4vC
Mb/eyRppeTrt1dNCyaI5S6a3Mq8ay9vR4RGdaAfNqvcRHBkZxGBUjOHzcwYdhSEG
XJDayYxKNwG79iwc5ygP
=JLdk
-----END PGP SIGNATURE-----
