[SECURITY-1438] Add form secret encryption compatibility

- Bonus: reproduction test for the two case, in the HTML and in the file
- with dom4j exclusion to be able to run tests!

Author: Wadeck

pom.xml

@@ -101,6 +101,13 @@
       <artifactId>jenkins-client</artifactId>
       <version>0.3.7</version>
       <scope>test</scope>
+      <exclusions>
+        <!-- a forked version already included by the core -->
+        <exclusion>
+          <groupId>dom4j</groupId>
+          <artifactId>dom4j</artifactId>
+        </exclusion>
+      </exclusions>
     </dependency>
     <dependency>
       <groupId>org.jenkins-ci.plugins</groupId>

src/main/java/org/jenkinsci/plugins/gogs/GogsProjectProperty.java

@@ -42,9 +42,7 @@ public class GogsProjectProperty extends JobProperty<Job<?, ?>> {
 
     @Deprecated
     public GogsProjectProperty(String gogsSecret, boolean gogsUsePayload, String gogsBranchFilter) {
-        this.gogsSecret = Secret.fromString(gogsSecret);
-        this.gogsUsePayload = gogsUsePayload;
-        this.gogsBranchFilter = gogsBranchFilter;
+        this(Secret.fromString(gogsSecret), gogsUsePayload, gogsBranchFilter);
     }
 
     @DataBoundConstructor
@@ -53,8 +51,9 @@ public GogsProjectProperty(Secret gogsSecret, boolean gogsUsePayload, String gog
         this.gogsUsePayload = gogsUsePayload;
         this.gogsBranchFilter = gogsBranchFilter;
     }
-    public String getGogsSecret() {
-        return Secret.toString(this.gogsSecret);
+
+    public Secret getGogsSecret() {
+        return this.gogsSecret;
     }
 
     public boolean getGogsUsePayload() {

src/main/java/org/jenkinsci/plugins/gogs/GogsWebHook.java

@@ -27,6 +27,7 @@ associated documentation files (the "Software"), to deal in the Software without
 import hudson.model.Job;
 import hudson.model.UnprotectedRootAction;
 import hudson.security.ACL;
+import hudson.util.Secret;
 import net.sf.json.JSONObject;
 import org.acegisecurity.context.SecurityContext;
 import org.acegisecurity.context.SecurityContextHolder;
@@ -187,7 +188,7 @@ public void doIndex(StaplerRequest req, StaplerResponse rsp) throws IOException
                     /* secret is stored in the properties of Job */
                     final GogsProjectProperty property = (GogsProjectProperty) job.getProperty(GogsProjectProperty.class);
                     if (property != null) { /* only if Gogs secret is defined on the job */
-                        jSecret = property.getGogsSecret(); /* Secret provided by Jenkins */
+                        jSecret = Secret.toString(property.getGogsSecret()); /* Secret provided by Jenkins */
                         isRefMatched = property.filterBranch(ref);
                     }
                 } else {
@@ -207,7 +208,7 @@ public void doIndex(StaplerRequest req, StaplerResponse rsp) throws IOException
                         /* secret is stored in the properties of Job */
                         final GogsProjectProperty property = (GogsProjectProperty) job.getProperty(GogsProjectProperty.class);
                         if (property != null) { /* only if Gogs secret is defined on the job */
-                            jSecret = property.getGogsSecret(); /* Secret provided by Jenkins */
+                            jSecret = Secret.toString(property.getGogsSecret()); /* Secret provided by Jenkins */
                             isRefMatched = property.filterBranch(ref);
                         }
                     }

src/test/java/org/jenkinsci/plugins/gogs/GogsWebHookJenkinsTest.java

@@ -1,13 +1,23 @@
 package org.jenkinsci.plugins.gogs;
 
+import com.gargoylesoftware.htmlunit.html.HtmlPage;
+import hudson.model.FreeStyleProject;
 import hudson.util.Secret;
+import org.apache.commons.io.FileUtils;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.io.File;
+import java.nio.charset.StandardCharsets;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.not;
 import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThat;
 
 public class GogsWebHookJenkinsTest {
     final Logger log = LoggerFactory.getLogger(GogsWebHookJenkinsTest.class);
@@ -42,4 +52,42 @@ public void whenJobBranchNotMatchMustReturnError() {
 
         log.info("Test succeeded.");
     }
+
+    @Test
+    @Issue("SECURITY-1438")
+    public void ensureTheSecretIsEncryptedOnDisk() throws Exception {
+        Secret secret = Secret.fromString("s3cr3t");
+        FreeStyleProject p = prepareProjectWithGogsProperty(secret);
+
+        File configFile = p.getConfigFile().getFile();
+        String configFileContent = FileUtils.readFileToString(configFile, StandardCharsets.UTF_8);
+        assertThat(configFileContent, not(containsString(secret.getPlainText())));
+        assertThat(configFileContent, containsString(secret.getEncryptedValue()));
+    }
+
+    @Test
+    @Issue("SECURITY-1438")
+    public void ensureTheSecretIsEncryptedInHtml() throws Exception {
+        Secret secret = Secret.fromString("s3cr3t");
+        FreeStyleProject p = prepareProjectWithGogsProperty(secret);
+
+        JenkinsRule.WebClient wc = j.createWebClient();
+        // there are some errors in the page and thus the status is 500 but the content is there
+        wc.getOptions().setThrowExceptionOnFailingStatusCode(false);
+        HtmlPage htmlPage = wc.goTo(p.getUrl() + "configure");
+        String pageContent = htmlPage.getWebResponse().getContentAsString();
+        assertThat(pageContent, not(containsString(secret.getPlainText())));
+        assertThat(pageContent, containsString(secret.getEncryptedValue()));
+    }
+
+    private FreeStyleProject prepareProjectWithGogsProperty(Secret secret) throws Exception {
+        FreeStyleProject p = j.createFreeStyleProject();
+
+        GogsProjectProperty prop = new GogsProjectProperty(secret, true, "master");
+        p.addProperty(prop);
+
+        p.save();
+
+        return p;
+    }
 }