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

append.c: only call out to annotator on email #5236

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rjbs
Copy link
Collaborator

@rjbs rjbs commented Jan 22, 2025

The annotator exists so that installations can update email messages to have extra flags or annotations. Because "everything" is an email, it was also running on sieve scripts and other non-emails. This was probably harmless on many installations, but not intended.

This branch updates the annotator to run only on messages being appended to mailboxes of type e.

It also fixes a bug in sieve_db, wherein we only set the authstate for the onSuccessActivateScript side effect, not the rest of script creation.

@rjbs rjbs force-pushed the cyr-1139-only-annotate-email branch 2 times, most recently from 4962039 to 4c08c1a Compare January 23, 2025 15:02
@rjbs rjbs force-pushed the cyr-1139-only-annotate-email branch from 4c08c1a to dec29ed Compare January 30, 2025 15:16
@rjbs rjbs requested a review from ksmurchison January 30, 2025 15:22
Copy link
Contributor

@ksmurchison ksmurchison left a comment

Choose a reason for hiding this comment

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

Othe rthan my comment, LGTM

imap/append.c Outdated
@@ -1107,7 +1107,10 @@ EXPORTED int append_fromstage_full(struct appendstate *as, struct body **body,
goto out;
}

if (config_getstring(IMAPOPT_ANNOTATION_CALLOUT)) {
if (
config_getstring(IMAPOPT_ANNOTATION_CALLOUT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit. We (the core devs) typically don't put like breaks after the opening '(' and before the closing ')'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Happy to address. (Also, eager to see what autoformatting we could agree on…)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ksmurchison I believe my latest commit matches the style I've now found elsewhere in append.c. Look right to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes sir. All good!

rjbs added 5 commits February 6, 2025 19:55
The "/*" nonsense is so that the test annotator won't choke on the
comments needed to make the content valid Sieve.
When we do a SieveScript/set.create to make a script, plus a
onSuccessActivateScript, we would set the authstate *only* for the
onSuccessActivateScript.  This worked in two parts:

1. append happens, as nobody, which works because of an "anyone p" ACL
2. flag set happens on onSuccessActivateScript, which worked because we
   set the auth state

In writing tests for the Annotator, I found that it couldn't be
instructed to add flags, because it didn't have permission.  That's
because the annotator runs during step 1, append, when it was acting as
nobody.

We don't want the annotator to run, but we also want it (and other
things) to see the right auth state.  So, we set the auth state up
correctly always, now.
@rjbs rjbs force-pushed the cyr-1139-only-annotate-email branch from d08b8e0 to 25f21ce Compare February 6, 2025 08:58
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