Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix body hash issue in relaxe mode #38 #37

Closed
wants to merge 2 commits into from

Conversation

mschneider82
Copy link

@mschneider82 mschneider82 commented Nov 19, 2020

I have added a test which failed before. With this fix i can sucessfully verify my testmail which was valid but the libray said body hash missmatch.

verify issue described in #38

@mschneider82 mschneider82 changed the title fix body hash issue in relaxe mode #36 fix body hash issue in relaxe mode #38 Nov 20, 2020
Copy link
Author

@mschneider82 mschneider82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since res has the same size(length) as the input b we cannot add \n when the last char is \r, so we need to remove the \r (only in relaxed mode).

@emersion
Copy link
Owner

So, it seems like you're doing essentially this:

b = bytes.TrimRight(b, "\r")

ie. removing trailing \r if any. Is this correct?

This should likely be part of the relaxedBodyCanonicalizer.Write loop, because it's specific to the relaxed mode.


This seems to be yet another quirk for relaxed mode. The RFC doesn't seem to mention it. Or maybe it's part of "Ignore all whitespace at the end of lines"? Or maybe "Ignore all empty lines at the end of the message body"?

@mschneider82
Copy link
Author

mschneider82 commented Nov 24, 2020

@emersion yes its basicly bytes.TrimRight, but i just remove exact one \r at the end. not more (TrimRight could trim more than one).

@mschneider82
Copy link
Author

mschneider82 commented Nov 24, 2020

are you thinking about?

b = fixCRLF(b)
b = bytes.TrimRight(b, "\r")

this seems like fixCRLF doesnt fix correct, thats why i included it into fixCRLF()

@emersion
Copy link
Owner

emersion commented Nov 24, 2020

Hmm, wait. Your bug only happens when a slice passed to Write ends with a \r. That likely happens because a CRLF sequence is split between two Write calls:

Write([]byte("line1\r"))
Write([]byte("\nline2"))

I've written a failing test:

func TestRelaxedCanonicalizer_CanonicalBody_splitCRLF(t *testing.T) {
	want := "line 1\r\nline 2\r\n"
	writes := [][]byte{
		[]byte("line 1\r"),
		[]byte("\nline 2"),
	}

	var b bytes.Buffer
	wc := new(relaxedCanonicalizer).CanonicalizeBody(&b)
	for _, b := range writes {
		if _, err := wc.Write(b); err != nil {
			t.Errorf("Expected no error while writing to relaxed body canonicalizer, got: %v", err)
		}
	}
	if err := wc.Close(); err != nil {
		t.Errorf("Expected no error while closing relaxed body canonicalizer, got: %v", err)
	} else if s := b.String(); s != want {
		t.Errorf("Expected canonical body to be %q, but got %q", want, s)
	}
}

@emersion
Copy link
Owner

Superseded by #39

@emersion emersion closed this Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants