Thinking about the public suffixes thing, I guess we just have to fetch at an interval in the broker? I don't really see another way. (The only other thing I can think about is doing it outside the broker and adding SIGHUP config reloading support. While reloading is nice to have, having a separate thing to fetch the list complicates setup.)
But it looks like the list at publicsuffix.org doesn't have Cache-Control headers. So we probably want to add functionality to override cache age in our fetching code. Plus we should keep using stale lists if fetching fails.
Could probably do something crude to handle failed fetches. Like still set a Redis TTL, but x3 the cache age, and then allow 3 fetches to fail before crashing. No complicated retry timer logic for MVP.
Today I spent a bit writing a Mailgun agent, but when it came time to write tests I realized that the only tests for the Postmark agent are the E2E tests, which are enabled due to the fact that Postmark supports sending test emails using a test API token. Unfortunately, Mailgun doesn't have a similar API. It looks like most Mailgun clients either mock the API, or use the actual, production API (which requires credentials, and incurs costs). If I contributed the agent, I'd definitely want there to be some sort of automated tests to make sure it doesn't break. I'm very much a novice when it comes to Rust, but if someone could take a look at how we could write Rust tests for the Postmark agent that don't actually hit the API, and simply compare the sent request to a fixture in the test, I could implement the same thing for a Mailgun provider.
(Also, if this would be more appropriate in a GitHub issue please let me know!)
Thinking we could add a hidden
postmark_email_api (doesn't need docs) that defaults to the currently hardcoded "https://api.postmarkapp.com/email". Then tests can start a dummy server and point the broker at it. (In the broker postmark code, we can then also get rid of the hacky is_test_request path.)
@dstaley Btw, thanks for looking into this, and the IE stuff! Really cool. Let me know if you want to try your hand at the above, or would rather have me look into it. (I may hopefully have some time this weekend.)
Just so I understand correctly, you're thinking of spawing a mock server, pointing the broker binary at that server via an environment variable, and then testing the requests to the mock server? If so, would those tests be Rust unit tests, or would this be part of the E2E test?
@dstaley Yes, that's correct. The test harness does the mocking in-process, for example: https://github.com/portier/portier-broker/blob/793c5987e744c6a864f38aff4dc8abcd55eccef2/tests/e2e/src/mailbox.js#L50-L61
I think right there is an okay place to add support for that, inside an
I think right there is an okay place to add support for that, inside an
if (TEST_MAILER === "mailgun") { ... }
I found E2E testing easier in this case. Especially for the Agent stuff, I guessed unit testing would be more difficult. :)
Feels like most of the code in the E2E test harness right now is fairly specific to what we're testing?
@stephank I spent some time today working on figuring out the best way to make assertions against network requests in the E2E test. In order to help me understand how all the pieces worked, and to give me some additional confidence things were working correctly, I wrote TypeScript-compatible JSDoc comments in the tests. The TypeScript compiler was able to pull types for every library except the node portier client. I went to look at how hard it would be to add types, and was surprised to see it was already in TypeScript! Would you mind if I also convert the E2E tests to TypeScript as well? (I can also send a PR to enable declaration emission for the node-portier library as well)
Thinking the semantics are too vague. Its real purpose is add checks to browser cookies after a domain has already been resolved (and thus verified existing). For example, this issue reiterates this: publicsuffix/list#570
So really, the question here is: do we support the use case of using the public suffix list on arbitrary inputs, or do we only support the use case of determining the public suffix for a domain which is in the DNS?
I guess the PR is still a better implementation of the ICANN TLD list. And we should still add the fetch & refresh code just for that. (Plus it adds allow/block lists)
But I kinda wanted to do a DNS check any way, so don't think the public suffix list adds much.
The last time the problem was that the DNS lookup could not be made async?
PRs from outside of the org, that is
Every now and then I see outlook.com fail our DMARC rules. I assume this is legit bad actors trying to send mail from portier.io, but I'm not sure if someone has an account there and can maybe do a quick check to see if https://demo.portier.io works?
Bleh, it looks like cargo cache is getting corrupted on macOS CI. I fixed it once for #227, but now it's broken again on master. Considering we should just disable cache. :/
It's going to be added to the macOS images in a few weeks
colemickens:matrix.org @colemickens:matrix.org waves at Dylan
Bleh, we also need to figure out why Safari tests are timing out 50% of the time :/