Subject: Re: [sup-devel] [PATCH] Implement inline GPG (updated)

From: Michael Stapelberg <[email-masked]>
Date: Wed, 10 Mar 2010 22:23:26 +0100

Hi,

Excerpts from Michael Stapelberg's message of Di Mär 09 17:43:03 +0100 2010:
> Alright, tested it and reworked my patch. The latest version is attached.
Updated it once again after testing with a user on sup-talk. See attachment
and please merge it now :).

Best regards,
Michael
_______________________________________________
Sup-devel mailing list
[email-masked]
http://rubyforge.org/mailman/listinfo/sup-devel

---

From: Rich Lane <[email-masked]>
Date: Fri, 26 Feb 2010 16:24:08 -0500

Excerpts from Michael Stapelberg's message of 2010-02-18 06:40:45 -0500:
> Hi,
> 
> as my previous patch was not merged, I have updated the patch to apply against
> the current code. Furthermore, it now correctly handles character sets for the
> GPG encrypted part.
> 
> The patch has been tested by me and another user and seems to work fine.
> 
> Please merge it for the next release.
> 
> Best regards,
> Michael

Not a full review yet, just a bug that bit me:

message.rb:531 assumes the text will be on its own line, while the
regex on 530 does not. (check my edge branch for line numbers)
_______________________________________________
Sup-devel mailing list
[email-masked]
http://rubyforge.org/mailman/listinfo/sup-devel

---

From: Rich Lane <[email-masked]>
Date: Thu, 11 Mar 2010 23:43:38 -0500

lib/sup/message.rb:

Since the regexes only match whole lines, why not just do string
comparisons? I'd also like those strings to be constants but I won't
insist on that. 

The body assignment should be a ternary.

I really dislike the flip-flop operator but it looks like the best way
to do this. Please package those selects into a commented Enumerable
method.

Please factor your two cases in message_to_chunks into very well
documented methods. message_to_chunks is already too complicated.
_______________________________________________
Sup-devel mailing list
[email-masked]
http://rubyforge.org/mailman/listinfo/sup-devel

---

From: Rich Lane <[email-masked]>
Date: Mon, 15 Mar 2010 01:19:59 -0400

Branch inline-gpg, merged to next.
_______________________________________________
Sup-devel mailing list
[email-masked]
http://rubyforge.org/mailman/listinfo/sup-devel

---

From: Christian Dietrich <[email-masked]>
Date: Mon, 01 Mar 2010 18:46:52 +0100

Excerpts from Michael Stapelberg's message of Mo M�r 01 17:49:30 +0100 2010:
> Hi didi,
> 
> Excerpts from Christian Dietrich's message of Mo Mär 01 15:36:10 +0100 2010:
> > tried this, but broke on this specific message, cause the string was
> > included but there was no signature.
> Can you forward this message so that I can have a look, please?

It was your message with the patch.

<[email-masked]>

> 
> > 517         ## Check for inline-PGP
> > 518         if body =~ /^-----BEGIN PGP SIGNED MESSAGE-----/
> > 519             gpg_start = "^-----BEGIN PGP SIGNED MESSAGE-----"
> > 520             gpg_signature = "^-----BEGIN PGP SIGNATURE-----"
> > 521             gpg_end = "^-----END PGP SIGNED MESSAGE-----"
> Why do you use ^ in the beginning but not $ in the end?

You never know what kind of crude mailers are out there.

greetz didi
-- 
No documentation is better than bad documentation
-- Das Ausdrucken dieser Mail wird urheberrechtlich verfolgt.
_______________________________________________
Sup-devel mailing list
[email-masked]
http://rubyforge.org/mailman/listinfo/sup-devel

---

From: Michael Stapelberg <[email-masked]>
Date: Mon, 01 Mar 2010 14:45:54 +0100

Hi Rich,

Excerpts from Rich Lane's message of Sa Feb 27 19:05:58 +0100 2010:
> The problem is sign_start will be nil if the text isn't on a line by
> itself, causing a crash a few lines later. This happened to me when I
How about using the following solution?

gpg_start = "-----BEGIN PGP SIGNED MESSAGE-----"
gpg_end = "-----END PGP SIGNED MESSAGE-----"
gpg = lines.select { |l| true if l =~ /#{gpg_start}/ .. l =~ /#{gpg_end}/ }
msg.body = gpg.join("\n")

Is there a way to avoid the ugly "true if"? When just leaving it out, ruby
complained saying "ArgumentError: bad value for range".

Best regards,
Michael

---

From: Christian Dietrich <[email-masked]>
Date: Mon, 01 Mar 2010 15:36:10 +0100

> complained saying "ArgumentError: bad value for range".

Hi, there,

This works for me:

517         ## Check for inline-PGP
518         if body =~ /^-----BEGIN PGP SIGNED MESSAGE-----/
519             gpg_start = "^-----BEGIN PGP SIGNED MESSAGE-----"
520             gpg_signature = "^-----BEGIN PGP SIGNATURE-----"
521             gpg_end = "^-----END PGP SIGNED MESSAGE-----"
522             gpg = lines.select { |l| true if l =~ /#{gpg_start}/ .. l =~ /#{gpg_end}/ }
523             body = lines.select { |l| true if l =~ /#{gpg_start}/ .. l =~ /#{gpg_signature}/ }
524             msg = RMail::Message.new
525             msg.body = gpg.join("\n")
526 
527             payload = RMail::Message.new
528             payload.body = body[1..-2].join("\n")
529 
530             File.open("/tmp/msg", "w+") {|f| f.write(msg.body)}
531             File.open("/tmp/payload", "w+") {|f| f.write(payload.body)}
532             return [CryptoManager.verify(nil, msg, false), message_to_chunks(payload)].flatten.compact
533         end

greetz didi

---

From: Michael Stapelberg <[email-masked]>
Date: Thu, 18 Feb 2010 12:40:45 +0100

Hi,

Best regards,
Michael

---

From: Michael Stapelberg <[email-masked]>
Date: Fri, 12 Mar 2010 12:02:00 +0100

Hi Rich,

> comparisons? I'd also like those strings to be constants but I won't
Good point, I changed that.

> The body assignment should be a ternary.
I avoided that because the line gets incredibly long then (91 characters
vs. 79 characters inside the if. Are you sure you want that ternary?
If so, please just change it yourself.

> to do this. Please package those selects into a commented Enumerable
Done.

> documented methods. message_to_chunks is already too complicated.
Done.

Best regards,
Michael

---

From: Rich Lane <[email-masked]>
Date: Sat, 27 Feb 2010 13:05:58 -0500

Excerpts from Michael Stapelberg's message of 2010-02-27 08:11:28 -0500:
> But I guess that no client would put any data before/after these lines,
> so adding ^ and $ to the regex in 530 seems right.

The problem is sign_start will be nil if the text isn't on a line by
itself, causing a crash a few lines later. This happened to me when I
was running with this patch on my edge branch. I'd remove the
duplication by making the condition of the if the sign_start assignment.
_______________________________________________
Sup-devel mailing list
[email-masked]
http://rubyforge.org/mailman/listinfo/sup-devel

---

From: Michael Stapelberg <[email-masked]>
Date: Mon, 01 Mar 2010 17:49:30 +0100

Hi didi,

Best regards,
Michael

---

From: Michael Stapelberg <[email-masked]>
Date: Sat, 27 Feb 2010 14:11:28 +0100

Hi Rich,

> regex on 530 does not. (check my edge branch for line numbers)
Right. RFC 2440 is not really clear about that. In 6.2 it only says:

"An Armor Header Line consists of the appropriate header line text
 surrounded by five (5) dashes ('-', 0x2D) on either side of the
 header line text."

Let me know if you need any help validating this patch.

Best regards,
Michael

---

From: Michael Stapelberg <[email-masked]>
Date: Tue, 09 Mar 2010 17:43:03 +0100

Hi everybody,

Hope it is good enough to get merged now ;-).

Best regards,
Michael