-
Notifications
You must be signed in to change notification settings - Fork 83
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
Add DKIM tests #174
Add DKIM tests #174
Conversation
95ff712
to
b5c5222
Compare
} | ||
|
||
async function revertForwarderChanges(email: string) { | ||
// Google sets their own Message-ID and put the original one in X-Google-Original-Message-ID when forwarding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your new logic for this; do we have a test though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I dont have a non-sensitive email that contains ARC changes which can be pushed to git. I used the one I have with existing tests and they are passing (after successful modifications)
|
||
const modified = await revertForwarderChanges(email.toString()); | ||
// If DKIM verification fails, revert common modifications made by ARC and try again. | ||
if (dkimResult.status.comment === "bad signature" && tryRevertARCChanges) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before we do this, we should add the zkp2p code where they try to process the email via doing the tabs -> spaces replacement as well as removing a [LABEL] tag from an email. i think we shuold try as hard as possible to get a dkim before moving to arc, and output that dkim failed and we are moving to arc before doing modifications and claiming dkim passes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tabs -> spaces replacement as well as removing a [LABEL] tag from an email
Do you know where this is implemented? I think those changes are also part of ARC? We should definitely be adding other ARC changes.
output that dkim failed and we are moving to arc
The revertCommonARCModifications
will print a log when modifications are done, or if no changes are done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tabs -> spaces replacement as well as removing a [LABEL] tag from an email
Do you know where this is implemented? I think those changes are also part of ARC? We should definitely be adding other ARC changes.
output that dkim failed and we are moving to arc
The
revertCommonARCModifications
will print a log when modifications are done, or if no changes are done.
Check out the zkp2p fixes -- basically like if you revert those changes, then you can get DKIM to pass, not just ARC. I think they noticed this on some Venmo emails and diffed them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are doing the same approach - right now only Message-Id is reverted, but can add removing labels.
https://github.com/zkp2p/zk-p2p/blob/develop/client/src/components/ProofGen/validation/hdfc.tsx#L85 seems to be very specific to hdfc pasted emails.
I think we can merge this and address 162, 165 and other sanitizations in another PR.
Might also be good to include the modifications in #162 here, maybe even #165 (the assigned guy isn't doing it). It's because ideally, to avoid people falling back to ARC when DKIM could work, I think best for possible DKIM verifications to be tried before ARC is tried. The zkp2p fixes are here: see: https://github.com/zkp2p/zk-p2p/blob/develop/client/src/components/ProofGen/validation/hdfc.tsx#L85 |
Description
Type of Change
Please delete options that are not relevant.
Checklist: