[JENKINS-65558] Added Configuration as Code support for email templates#1480
[JENKINS-65558] Added Configuration as Code support for email templates#1480Woeter69 wants to merge 5 commits intojenkinsci:mainfrom
Conversation
|
@jenkinsci/core-security-review Can you look this over and tell me if you see any security issues with this? I think it should be ok since admins would be the ones using JCasC to provision Jenkins, but I just want to make sure there is no issue. |
There was a problem hiding this comment.
Some questionable decisions. I would not be surprised at all to find out that this was vibecoded.
https://github.com/Woeter69/email-ext-plugin/blob/7e820bc4a2157202a7d5f46b01b92592ccacfb7b/src/main/java/hudson/plugins/emailext/ExtendedEmailPublisherDescriptor.java#L942 means that this would allow Overall/Manage users to configure by submitting crafted JSON form submissions. This would be a big security issue.
I would not accept it in its current form, but the general idea is reasonable.
TBH my suggestion would be to move away from file based templates. Also a nice opportunity to move away from the current set of (weird) templates (who's Larry!?).
| // Canonical path check: ensure resolved file is inside the templates directory | ||
| String canonicalDir = templatesDir.getCanonicalPath(); | ||
| String canonicalFile = templateFile.getCanonicalPath(); |
There was a problem hiding this comment.
Doesn't work on Windows before Java 24.
Also, why all this effort here when it's already done in EmailTemplate (or vice versa)?
| // Canonical path check: ensure resolved file is inside the templates directory | ||
| String canonicalDir = templatesDir.getCanonicalPath(); | ||
| String canonicalFile = templateFile.getCanonicalPath(); | ||
| if (!canonicalFile.startsWith(canonicalDir + File.separator)) { |
There was a problem hiding this comment.
Can be done more nicely with java.nio.Path#startsWith.
| public List<EmailTemplate> getEmailTemplates() { | ||
| return emailTemplates; | ||
| } | ||
|
|
||
| /** | ||
| * Sets and provisions email templates from Configuration as Code. | ||
| * | ||
| * <p> | ||
| * Each template is validated for security (safe filename, allowed extension, | ||
| * no path traversal) and then written to | ||
| * {@code $JENKINS_HOME/email-templates/}. | ||
| * | ||
| * @param emailTemplates the list of templates to provision | ||
| */ | ||
| @DataBoundSetter | ||
| public void setEmailTemplates(List<EmailTemplate> emailTemplates) { |
There was a problem hiding this comment.
This will never delete previously defined email templates, and therefore potentially accumulate a list of obsolete (possibly deemed unsafe/inadvisable) templates.
| String trimmedName = name.trim(); | ||
|
|
||
| if (trimmedName.isEmpty()) { | ||
| throw new IllegalArgumentException("Template name must not be empty"); | ||
| } | ||
|
|
||
| // Reject path traversal and directory separators | ||
| if (trimmedName.contains("/") || trimmedName.contains("\\") || trimmedName.contains("..")) { | ||
| throw new IllegalArgumentException( | ||
| "Template name must not contain path separators or relative path components: " + trimmedName); | ||
| } | ||
|
|
||
| // Reject null bytes | ||
| if (trimmedName.contains("\0")) { | ||
| throw new IllegalArgumentException("Template name must not contain null bytes"); | ||
| } |
There was a problem hiding this comment.
All of this is unnecessary with the regex (possibly better errors?).
src/main/java/hudson/plugins/emailext/ExtendedEmailPublisherDescriptor.java
Show resolved
Hide resolved
|
Yes, I'll start working on the things mentioned ASAP. |
|
Hi @Woeter69! After reviewing daniel-beck's feedback, here's a summary of what needs to be addressed:
|
|
i've done some changes, please review my changes and provide me with some feedback, Thanks! |
|
Hi @Woeter69! Great work addressing the feedback. Here's what I can see has been fixed:
|
|
@slide i've made some changes after the review, i'm looking forward to some feedback, thank you. |
|
I'll review soon |
Resolves #1356 / [JENKINS-65558]
Problem
Custom email templates used by the Email Extension Plugin (Groovy, Jelly, etc.) must be manually placed in the
$JENKINS_HOME/email-templates/directory on the Jenkins master. There was no way to manage these templates programmatically using Jenkins Configuration as Code (JCasC), which complicates automated provisioning and infrastructure-as-code deployments.Solution
This PR adds support for provisioning email templates directly via JCasC YAML configuration.
Templates defined within the
email-extunclassified configuration are automatically validated and written to the$JENKINS_HOME/email-templates/directory during Jenkins startup or when CasC configuration is reloaded.Security Implementation
Strict validation is applied to prevent any malicious file writing:
/,\,..), null bytes, or spaces..groovy,.jelly, or.template.email-templates/directory.Usage (JCasC)