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

authres-version difference from RFC on 4.0pre3 #222

Closed
Altomb opened this issue Nov 14, 2020 · 5 comments
Closed

authres-version difference from RFC on 4.0pre3 #222

Altomb opened this issue Nov 14, 2020 · 5 comments
Assignees
Labels
invalid Not a problem of the add-on

Comments

@Altomb
Copy link

Altomb commented Nov 14, 2020

Hi,

from what I understand from RFC authres-version is facultative (assumed ver 1 if absent) but current version (4.0pre3) fails if it is absent as there is no handling of that case.

@lieser
Copy link
Owner

lieser commented Nov 15, 2020

Yes, explicitly specifying the authres-version should is optional. And the parsing in the add-on should be able to handle it. E.g. the examples form the RFC without a version work without a problem.

You you please post (or send via e-mail) an affected header (no need for the complete e-mail)?

Note that the relaxed parsing is currently not working, see #221.

@lieser lieser self-assigned this Nov 15, 2020
@Altomb
Copy link
Author

Altomb commented Nov 15, 2020

Wish I could have been able to analyse it further, but that regexp used to get autores-version is really scary.

Here is an unaltered sample taken from the string when the match function failed:
spf=pass (sender IP is 192.30.252.204)\r\n smtp.mailfrom=github.com; hotmail.com; dkim=test (signature was verified)\r\n header.d=github.com;hotmail.com; dmarc=pass action=none\r\n header.from=github.com;compauth=pass reason=100\r\n

@lieser
Copy link
Owner

lieser commented Nov 15, 2020

Note that it would be more helpful If you post the complete ARH header (from the e-mail source) instead of the error (or debugger) output in this case.

I you would like to understand the RegExp, I would not recommend to look at the finished ones (this can indeed by quite scary), but how they are constructed. E.g. for the authserv-id and authres-version
${value_cp}(?:${CFWS_p}([0-9]+)${CFWS_op})?
The one for the authres-version is the second part, (?:${CFWS_p}([0-9]+)${CFWS_op}). The ? at the end of it makes it optional.

I however do not believe this has anything to do with the autores-version.

But I see multiple other problems with the part of the header you shared (in regards to RFC 7601, have not yet looked at RFC 8601; #219 ):

  • multiple hotmail.com; in the middle: there should only be one authserv-id per header, and at the very beginning
  • dkim=test: test is invalid, as it is not one of the specified allowed results (https://tools.ietf.org/html/rfc7601#section-2.7.1)
  • dmarc=pass action=none: action=none is not valid. Don't now if it was intended as a comment (parenthesis are missing) or a propspec (this should always consist of two parts<ptype>.<property>).

@Altomb
Copy link
Author

Altomb commented Nov 15, 2020

Yeah, I noticed the odd spamming about hotmail in the header, however since I was not able to understand what the regexp was trying to do and how it failed, I had to base myself on was should have happened afterwards and saw it should have tried to process this. So I thought the regexp was trying to find it and failing because it was not there.

So probably it is just that matching at this point goes a little further than that to make sure to get the right thing. Would this pass with relaxed checks? Here is the header as in the email:

Authentication-Results: spf=pass (sender IP is 192.30.252.204)
smtp.mailfrom=github.com; hotmail.com; dkim=test (signature was verified)
header.d=github.com;hotmail.com; dmarc=pass action=none
header.from=github.com;compauth=pass reason=100

The second hotmail.com is there also. I can confirm you that the line you linked is the place where it is failing.

@lieser
Copy link
Owner

lieser commented Nov 15, 2020

The header you posted shows another problem: the authserv-id is required to be there at the very beginning, and unlike the authres-version is not optional.

So probably it is just that matching at this point goes a little further than that to make sure to get the right thing.

It is trying to match the authserv-id and authres-version, which like I said, should be at the very beginning. Because the authserv-id is not there, this fails.

Would this pass with relaxed checks?

This would also not pass with relaxed checks. And I considered there to many violations to extend the relaxed parsing to allow this.
You could try to contact hotmail and ask them to fix the header they write.

Will close this issue, as this is not a problem in the add-on. Feel free to still ask any question you may still have.

@lieser lieser closed this as completed Nov 15, 2020
@lieser lieser added the invalid Not a problem of the add-on label Nov 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid Not a problem of the add-on
Projects
None yet
Development

No branches or pull requests

2 participants