Email option to embed images as base64 instead of link #32061

sommerf-lf · 2024-09-17T16:22:32Z

Documentation: https://gitea.com/gitea/docs/pulls/69

Example

Content:

Result in Email:

Result with source code:
(first image is external image, 2nd is now embedded)

sommerf-lf · 2024-09-18T09:18:24Z

Please have a look at this and let me know if I did some structural things wrong, as I have zero experience with Go but came across the mentioned issues myself and tried to add the optional setting to fix them.

lunny · 2024-09-19T00:05:36Z

Could you have some tests?

sommerf-lf · 2024-09-19T07:28:14Z

Could you have some tests?

I'll have a look. But I'm not quite sure as to what the focus of these test should be, as outgoing emails aren't tested anywhere. I could simply test my own additions whithout any actual email integration (the parsing of the html and replacement of the img tags and whether other image tags stay the same). Sounds fine?

lunny · 2024-09-19T21:53:55Z

Could you have some tests?

I'll have a look. But I'm not quite sure as to what the focus of these test should be, as outgoing emails aren't tested anywhere. I could simply test my own additions whithout any actual email integration (the parsing of the html and replacement of the img tags and whether other image tags stay the same). Sounds fine?

Yes. Thank you.

sommerf-lf · 2024-09-20T17:23:46Z

I added test to this. however I can't integrate them into services/mailer/mail_test.go because I need the testing environment which would form an import cycle. Now I exported the MailCommentContext and my newly added functions. Let me know if there is a way around this, but I see this as nothing big as these functions are not context specific. I'll squash the commits after you reviewed it

sommerf-lf · 2024-09-21T21:58:30Z

Still fails because I didn't account for using minio. Will rework...

… image attachments + tests

services/mailer/mail.go

tests/integration/email_embed_b64_images_test.go

yp05327 · 2024-09-26T01:18:02Z

https://github.blog/changelog/2023-05-09-more-secure-private-attachments/
The discussion in the issue is outdated. See the link above.
Personally, I don't suggest to do this.

sommerf-lf · 2024-09-30T21:01:11Z

https://github.blog/changelog/2023-05-09-more-secure-private-attachments/ The discussion in the issue is outdated. See the link above. Personally, I don't suggest to do this.

I see your/their point. Even though it is not the exact same situation, the motivation for doing this is what our issue (that we are trying to resolve) is. For Github I see this as a more sensitive issue, as it is one instance that is publically hosted. With this change we are not allowing any urls to access attachments on gitea repos without authentification (no chance that this change will open any possibility of brute-forcing attachments), but instead we are "trusting" the email chain. (and for images only)
As I implemented this as an opt-in option, I suggest that I add more info (one sentence about the security implications) to the documentation and the app.example.ini to warn about this possible concern.
Does this resolve your concerns?

yp05327 · 2024-09-30T23:31:38Z

I see. You added an option. It is acceptable.
But the default value is false in code and true in app.sample.ini

sommerf-lf · 2024-10-09T05:33:20Z

Should I add an info to the documentation of the implication of email trust with this feature?

wxiaoguang · 2024-10-13T08:52:39Z

Before reviewing some nits, actually there are some more important problems in my mind.

Is it safe to make AttachmentSrcToBase64DataURI read attachments directly? It means that it could read any attachment without permission check? If I remember correctly, there are some permission checks for attachment access.
If an attacker use <img src="/attachments/{a-big-release-file}>, would it bloat Gitea's memory and make it OOM?

yp05327 · 2024-10-15T00:40:08Z

Is it safe to make AttachmentSrcToBase64DataURI read attachments directly? It means that it could read any attachment without permission check? If I remember correctly, there are some permission checks for attachment access.

I think this PR is based on your comment here: #15081 (comment)
But as I mentioned above #32061 (comment), GitHub has changed it.
So I think this concern is necessary.

And as #32061 (comment) explained, by default, this feature will be turn off, and only for these internal instances with special needs. So from my side, by default, this feature will not work, so it will not break the security of existing instances. We can just mention the risk in docs and app.example.ini, if user want to use it, so maybe it is ok.
But, at last, I think we need TOC's decision.

sommerf-lf · 2024-10-17T00:02:51Z

Before reviewing some nits, actually there are some more important problems in my mind.

Is it safe to make AttachmentSrcToBase64DataURI read attachments directly? It means that it could read any attachment without permission check? If I remember correctly, there are some permission checks for attachment access.

If an attacker use <img src="/attachments/{a-big-release-file}>, would it bloat Gitea's memory and make it OOM?

I addressed both your points:

checked if COMMENT CREATOR has rights to view the attachment (still only works for images!), as the sender is unknown in the context of the mail (bulk mails). this is not ideal but a good check, as this prevent data loss from brute-forcing other attachments as a comment creator
added max size to app.ini (default 5242880 = 5mb)

sommerf-lf · 2024-10-17T00:04:36Z

Is it safe to make AttachmentSrcToBase64DataURI read attachments directly? It means that it could read any attachment without permission check? If I remember correctly, there are some permission checks for attachment access.

I think this PR is based on your comment here: #15081 (comment) But as I mentioned above #32061 (comment), GitHub has changed it. So I think this concern is necessary.

And as #32061 (comment) explained, by default, this feature will be turn off, and only for these internal instances with special needs. So from my side, by default, this feature will not work, so it will not break the security of existing instances. We can just mention the risk in docs and app.example.ini, if user want to use it, so maybe it is ok. But, at last, I think we need TOC's decision.

i will address the docu and app.example.ini later this week with a description of the limitation and followed by a warning of it's implication.

Sounds good to me so far.

wxiaoguang · 2024-11-09T14:09:11Z

custom/conf/app.example.ini

+;BASE64_EMBED_IMAGES = false
+;;
+;; The maximum size of an image attachment to be embedded in the email.
+;BASE64_EMBED_IMAGES_MAX_SIZE = 5242880


I guess we do not need to introduce a new option here. There are already too many config options.

~~A default value like 5M should be good enough for all users~~ (update: it should limit the whole size, see below). If there would be some special requirements, it won't be late to introduce an option then.

And one more thing, limiting a single file seems not enough. There could be 100 files * 5M , I guess nobody would really like to receive an email with 500M size.

According to AI:

Gmail: The maximum size for sending and receiving emails is 25 MB. Outlook: The maximum size for sending and receiving emails is 20 MB, but 10 MB for Exchange accounts. In Office 365, the maximum size is up to 150 MB. GMX: The maximum size for sending and receiving emails is 50 MB. ProtonMail: The maximum size for sending and receiving emails is 25 MB. Yahoo: The maximum size for sending and receiving emails is 25 MB. AOL: The maximum size for sending and receiving emails is 25 MB. Hotmail: The maximum size for sending and receiving emails is 10 MB.

So limiting the whole size to 10MB or 25MB seems reasonable.

Maybe you can just resize the bigger images to fixed width/height before base64 encode so they will take up much less space?

wxiaoguang · 2024-11-09T14:10:27Z

services/mailer/mail.go

+	}
+	defer fr.Close()
+
+	content, err := io.ReadAll(fr)


It should avoid using ReadAll when the size is not under control.

There are some limited readers for this case.

wxiaoguang · 2024-11-09T14:14:19Z

tests/testdata/data/attachments/1/b/1b267670-1793-4cd0-abc1-449269b7cff9

Do we really need the 1.4KB size to test?

A valid JPEG can be as small as 107 bytes

The smallest PNG file is 67 bytes

Since the image file is only used by this single test, I think a sample image file (and database fixture) could be generated on the fly?

wxiaoguang · 2024-11-09T14:19:17Z

tests/integration/email_embed_b64_images_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+func TestEmailEmbedBase64Images(t *testing.T) {


If I understand correctly, this test could be a "unit test" (in services/mailer/mail_xxx_test.go), but not a "integration test"? Then no need to expose mailCommentContext?

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 17, 2024

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 17, 2024

github-actions bot added modifies/go Pull requests that update Go code docs-update-needed The document needs to be updated synchronously labels Sep 17, 2024

pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 17, 2024

sommerf-lf force-pushed the feature_B64_EMBED_IMAGES branch 2 times, most recently from 6bfebf7 to 17439b9 Compare September 18, 2024 09:16

sommerf-lf force-pushed the feature_B64_EMBED_IMAGES branch 2 times, most recently from cc35853 to 37428d3 Compare September 18, 2024 15:02

sommerf-lf marked this pull request as ready for review September 18, 2024 15:17

sommerf-lf force-pushed the feature_B64_EMBED_IMAGES branch from 37428d3 to d76e949 Compare September 18, 2024 20:14

sommerf-lf force-pushed the feature_B64_EMBED_IMAGES branch from d76e949 to 49c1522 Compare September 20, 2024 17:18

pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 20, 2024

sommerf-lf force-pushed the feature_B64_EMBED_IMAGES branch from 49c1522 to d072c3e Compare September 20, 2024 17:22

sommerf-lf force-pushed the feature_B64_EMBED_IMAGES branch 2 times, most recently from 763b655 to 6814a96 Compare September 20, 2024 21:51

sommerf-lf changed the title ~~Email option to embed images as base64 instead of link~~ [WIP] Email option to embed images as base64 instead of link Sep 21, 2024

Option BASE64_EMBED_IMAGES (default false) in mail settings to inline…

1665cba

… image attachments + tests

sommerf-lf force-pushed the feature_B64_EMBED_IMAGES branch from cd3fea1 to 1665cba Compare September 22, 2024 12:26

sommerf-lf changed the title ~~[WIP] Email option to embed images as base64 instead of link~~ Email option to embed images as base64 instead of link Sep 22, 2024

lunny approved these changes Sep 22, 2024

View reviewed changes

GiteaBot removed the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 22, 2024

GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Sep 22, 2024

lunny added this to the 1.23.0 milestone Sep 22, 2024

yp05327 reviewed Sep 26, 2024

View reviewed changes

services/mailer/mail.go Show resolved Hide resolved

yp05327 reviewed Sep 26, 2024

View reviewed changes

tests/integration/email_embed_b64_images_test.go Outdated Show resolved Hide resolved

sommerf-lf added 2 commits September 30, 2024 22:20

break after dataURI replacement

b361ea7

default false in app.example.ini

13190eb

max size and check whether doer is allowed

79ce159

wxiaoguang reviewed Nov 9, 2024

View reviewed changes

lunny modified the milestones: 1.23.0, 1.24.0 Nov 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Email option to embed images as base64 instead of link #32061

Email option to embed images as base64 instead of link #32061

Email option to embed images as base64 instead of link #32061

Are you sure you want to change the base?

Email option to embed images as base64 instead of link #32061

Conversation

Example

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment