Skip to content

Commit e633085

Browse files
dwnusbaumdwnusbaum
authored
Merge pull request #148 from dwnusbaum/fix-cache-deletion
Fix library cache deletion after SECURITY-2586
2 parents 24fa0ab + 403a8d1 commit e633085

File tree

6 files changed

+200
-15
lines changed

6 files changed

+200
-15
lines changed

‎src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryAdder.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,14 +190,15 @@ static List<URL> retrieve(@NonNull LibraryRecord record, @NonNull LibraryRetriev
190190

191191
if(versionCacheDir.exists()) {
192192
listener.getLogger().println("Library " + name + "@" + version + " is cached. Copying from home.");
193-
lastReadFile.touch(System.currentTimeMillis());
194193
} else {
195194
listener.getLogger().println("Caching library " + name + "@" + version);
196195
versionCacheDir.mkdirs();
197196
retrieveLockFile.touch(System.currentTimeMillis());
198197
retriever.retrieve(name, version, changelog, versionCacheDir, run, listener);
199198
retrieveLockFile.delete();
200199
}
200+
lastReadFile.touch(System.currentTimeMillis());
201+
versionCacheDir.withSuffix("-name.txt").write(name, "UTF-8");
201202
versionCacheDir.copyRecursiveTo(libDir);
202203
} else {
203204
retriever.retrieve(name, version, changelog, libDir, run, listener);

‎src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryCachingCleanup.java

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package org.jenkinsci.plugins.workflow.libs;
22

3+
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
34
import hudson.Extension;
45
import hudson.FilePath;
56
import hudson.model.AsyncPeriodicWork;
@@ -8,29 +9,51 @@
89
import java.io.IOException;
910
import java.util.concurrent.TimeUnit;
1011

12+
import jenkins.util.SystemProperties;
13+
1114
@Extension public class LibraryCachingCleanup extends AsyncPeriodicWork {
12-
private final Long recurrencePeriod;
13-
private final Long unreadCacheClearTime;
15+
@SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "non-final for script console access")
16+
public static int EXPIRE_AFTER_READ_DAYS =
17+
SystemProperties.getInteger(LibraryCachingCleanup.class.getName() + ".EXPIRE_AFTER_READ_DAYS", 7);
1418

1519
public LibraryCachingCleanup() {
1620
super("LibraryCachingCleanup");
17-
recurrencePeriod = Long.getLong("jenkins.workflow-libs.cacheCleanupPeriodDays", TimeUnit.DAYS.toMillis(7));
18-
unreadCacheClearTime = Long.getLong("jenkins.workflow-libs.unreadCacheClearDays", TimeUnit.DAYS.toMillis(7));
1921
}
2022

2123
@Override public long getRecurrencePeriod() {
22-
return recurrencePeriod;
24+
return TimeUnit.HOURS.toMillis(12);
2325
}
2426

2527
@Override protected void execute(TaskListener listener) throws IOException, InterruptedException {
2628
FilePath globalCacheDir = LibraryCachingConfiguration.getGlobalLibrariesCacheDir();
27-
for (FilePath library: globalCacheDir.list()) {
28-
for (FilePath version: library.list()) {
29-
final FilePath lastReadFile = new FilePath(version, LibraryCachingConfiguration.LAST_READ_FILE);
30-
if (lastReadFile.exists() && (lastReadFile.lastModified() + unreadCacheClearTime) < System.currentTimeMillis()) {
31-
version.deleteRecursive();
29+
for (FilePath library : globalCacheDir.list()) {
30+
if (!removeIfExpiredCacheDirectory(library)) {
31+
// Prior to the SECURITY-2586 fix, library caches had a two-level directory structure.
32+
// These caches will never be used again, so we delete any that we find.
33+
for (FilePath version: library.list()) {
34+
if (version.child(LibraryCachingConfiguration.LAST_READ_FILE).exists()) {
35+
library.deleteRecursive();
36+
break;
37+
}
3238
}
3339
}
3440
}
3541
}
42+
43+
/**
44+
* Delete the specified cache directory if it is outdated.
45+
* @return true if specified directory is a cache directory, regardless of whether it was outdated. Used to detect
46+
* whether the cache was created before or after the fix for SECURITY-2586.
47+
*/
48+
private boolean removeIfExpiredCacheDirectory(FilePath library) throws IOException, InterruptedException {
49+
final FilePath lastReadFile = new FilePath(library, LibraryCachingConfiguration.LAST_READ_FILE);
50+
if (lastReadFile.exists()) {
51+
if (System.currentTimeMillis() - lastReadFile.lastModified() > TimeUnit.DAYS.toMillis(EXPIRE_AFTER_READ_DAYS)) {
52+
library.deleteRecursive();
53+
library.withSuffix("-name.txt").delete();
54+
}
55+
return true;
56+
}
57+
return false;
58+
}
3659
}

‎src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryCachingConfiguration.java

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,12 @@
1111

1212
import java.io.File;
1313
import java.io.IOException;
14+
import java.io.InputStream;
15+
import java.nio.charset.StandardCharsets;
1416
import java.util.Arrays;
1517
import java.util.Collections;
1618
import java.util.List;
19+
import org.apache.commons.io.IOUtils;
1720
import org.apache.commons.lang.StringUtils;
1821

1922
public final class LibraryCachingConfiguration extends AbstractDescribableImpl<LibraryCachingConfiguration> {
@@ -83,10 +86,21 @@ public static FilePath getGlobalLibrariesCacheDir() {
8386
public FormValidation doClearCache(@QueryParameter String name) throws InterruptedException {
8487
Jenkins.get().checkPermission(Jenkins.ADMINISTER);
8588

86-
FilePath cacheDir = new FilePath(LibraryCachingConfiguration.getGlobalLibrariesCacheDir(), name);
8789
try {
88-
if (cacheDir.exists()) {
89-
cacheDir.deleteRecursive();
90+
if (LibraryCachingConfiguration.getGlobalLibrariesCacheDir().exists()) {
91+
for (FilePath libraryNamePath : LibraryCachingConfiguration.getGlobalLibrariesCacheDir().list("*-name.txt")) {
92+
// Libraries configured in distinct locations may have the same name. Since only admins are allowed here, this is not a huge issue, but it is probably unexpected.
93+
String cacheName;
94+
try (InputStream stream = libraryNamePath.read()) {
95+
cacheName = IOUtils.toString(stream, StandardCharsets.UTF_8);
96+
}
97+
if (libraryNamePath.readToString().equals(name)) {
98+
FilePath libraryCachePath = LibraryCachingConfiguration.getGlobalLibrariesCacheDir()
99+
.child(libraryNamePath.getName().replace("-name.txt", ""));
100+
libraryCachePath.deleteRecursive();
101+
libraryNamePath.delete();
102+
}
103+
}
90104
}
91105
} catch (IOException ex) {
92106
return FormValidation.error(ex, "The cache dir was not deleted successfully");

‎src/main/resources/org/jenkinsci/plugins/workflow/libs/LibraryCachingConfiguration/config.jelly

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,5 +31,7 @@ THE SOFTWARE.
3131
<f:entry title="${%Versions to exclude}" field="excludedVersionsStr">
3232
<f:textbox />
3333
</f:entry>
34-
<f:validateButton title="${%Clear cache}" progress="${%Clearing...}" method="clearCache" with="name" />
34+
<j:if test="${h.hasPermission(app.ADMINISTER)}">
35+
<f:validateButton title="${%Clear cache}" progress="${%Clearing...}" method="clearCache" with="name" />
36+
</j:if>
3537
</j:jelly>

‎src/test/java/org/jenkinsci/plugins/workflow/libs/LibraryCachingCleanupTest.java

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
/*
2+
* The MIT License
3+
*
4+
* Copyright 2022 CloudBees, Inc.
5+
*
6+
* Permission is hereby granted, free of charge, to any person obtaining a copy
7+
* of this software and associated documentation files (the "Software"), to deal
8+
* in the Software without restriction, including without limitation the rights
9+
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
10+
* copies of the Software, and to permit persons to whom the Software is
11+
* furnished to do so, subject to the following conditions:
12+
*
13+
* The above copyright notice and this permission notice shall be included in
14+
* all copies or substantial portions of the Software.
15+
*
16+
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
17+
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
18+
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
19+
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
20+
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
21+
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
22+
* THE SOFTWARE.
23+
*/
24+
package org.jenkinsci.plugins.workflow.libs;
25+
26+
import hudson.ExtensionList;
27+
import hudson.FilePath;
28+
import hudson.util.StreamTaskListener;
29+
import java.io.File;
30+
import java.time.ZonedDateTime;
31+
import jenkins.plugins.git.GitSCMSource;
32+
import jenkins.plugins.git.GitSampleRepoRule;
33+
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
34+
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
35+
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
36+
import org.junit.Rule;
37+
import org.junit.Test;
38+
import org.jvnet.hudson.test.JenkinsRule;
39+
40+
import static org.hamcrest.MatcherAssert.assertThat;
41+
import static org.hamcrest.Matchers.not;
42+
import static org.hamcrest.io.FileMatchers.anExistingDirectory;
43+
import static org.hamcrest.io.FileMatchers.anExistingFile;
44+
45+
public class LibraryCachingCleanupTest {
46+
47+
@Rule
48+
public JenkinsRule r = new JenkinsRule();
49+
@Rule
50+
public GitSampleRepoRule sampleRepo = new GitSampleRepoRule();
51+
52+
@Test
53+
public void smokes() throws Throwable {
54+
sampleRepo.init();
55+
sampleRepo.write("vars/foo.groovy", "def call() { echo 'foo' }");
56+
sampleRepo.git("add", "vars");
57+
sampleRepo.git("commit", "--message=init");
58+
LibraryConfiguration config = new LibraryConfiguration("library",
59+
new SCMSourceRetriever(new GitSCMSource(null, sampleRepo.toString(), "", "*", "", true)));
60+
config.setDefaultVersion("master");
61+
config.setImplicit(true);
62+
config.setCachingConfiguration(new LibraryCachingConfiguration(30, null));
63+
GlobalLibraries.get().getLibraries().add(config);
64+
// Run build and check that cache gets created.
65+
WorkflowJob p = r.createProject(WorkflowJob.class);
66+
p.setDefinition(new CpsFlowDefinition("foo()", true));
67+
WorkflowRun b = r.buildAndAssertSuccess(p);
68+
LibrariesAction action = b.getAction(LibrariesAction.class);
69+
LibraryRecord record = action.getLibraries().get(0);
70+
FilePath cache = LibraryCachingConfiguration.getGlobalLibrariesCacheDir().child(record.getDirectoryName());
71+
assertThat(new File(cache.getRemote()), anExistingDirectory());
72+
// Run LibraryCachingCleanup and show that cache is not deleted.
73+
ExtensionList.lookupSingleton(LibraryCachingCleanup.class).execute(StreamTaskListener.fromStderr());
74+
assertThat(new File(cache.getRemote()), anExistingDirectory());
75+
assertThat(new File(cache.withSuffix("-name.txt").getRemote()), anExistingFile());
76+
// Run LibraryCachingCleanup after modifying LAST_READ_FILE to be an old date and and show that cache is deleted.
77+
long oldMillis = ZonedDateTime.now().minusDays(LibraryCachingCleanup.EXPIRE_AFTER_READ_DAYS + 1).toInstant().toEpochMilli();
78+
cache.child(LibraryCachingConfiguration.LAST_READ_FILE).touch(oldMillis);
79+
ExtensionList.lookupSingleton(LibraryCachingCleanup.class).execute(StreamTaskListener.fromStderr());
80+
assertThat(new File(cache.getRemote()), not(anExistingDirectory()));
81+
assertThat(new File(cache.withSuffix("-name.txt").getRemote()), not(anExistingDirectory()));
82+
}
83+
84+
@Test
85+
public void preSecurity2586() throws Throwable {
86+
FilePath cache = LibraryCachingConfiguration.getGlobalLibrariesCacheDir().child("name").child("version");
87+
cache.mkdirs();
88+
cache.child(LibraryCachingConfiguration.LAST_READ_FILE).touch(System.currentTimeMillis());
89+
ExtensionList.lookupSingleton(LibraryCachingCleanup.class).execute(StreamTaskListener.fromStderr());
90+
assertThat(new File(cache.getRemote()), not(anExistingDirectory()));
91+
assertThat(new File(cache.getParent().getRemote()), not(anExistingDirectory()));
92+
}
93+
94+
}

‎src/test/java/org/jenkinsci/plugins/workflow/libs/LibraryCachingConfigurationTest.java

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,16 +24,34 @@
2424

2525
package org.jenkinsci.plugins.workflow.libs;
2626

27+
import hudson.ExtensionList;
28+
import hudson.FilePath;
29+
import java.io.File;
30+
import jenkins.plugins.git.GitSCMSource;
31+
import jenkins.plugins.git.GitSampleRepoRule;
32+
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
33+
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
34+
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
2735
import org.junit.Before;
36+
import org.junit.Rule;
2837
import org.junit.Test;
2938
import org.jvnet.hudson.test.Issue;
39+
import org.jvnet.hudson.test.JenkinsRule;
40+
import org.jvnet.hudson.test.WithoutJenkins;
3041

3142
import static org.hamcrest.MatcherAssert.*;
3243
import static org.hamcrest.Matchers.*;
44+
import static org.hamcrest.io.FileMatchers.anExistingDirectory;
45+
import static org.hamcrest.io.FileMatchers.anExistingFile;
3346
import static org.junit.Assert.assertFalse;
3447
import static org.junit.Assert.assertTrue;
3548

3649
public class LibraryCachingConfigurationTest {
50+
@Rule
51+
public JenkinsRule r = new JenkinsRule();
52+
@Rule
53+
public GitSampleRepoRule sampleRepo = new GitSampleRepoRule();
54+
3755
private LibraryCachingConfiguration nullVersionConfig;
3856
private LibraryCachingConfiguration oneVersionConfig;
3957
private LibraryCachingConfiguration multiVersionConfig;
@@ -72,29 +90,34 @@ public void createCachingConfiguration() {
7290

7391
@Issue("JENKINS-66045") // NPE getting excluded versions
7492
@Test
93+
@WithoutJenkins
7594
public void npeGetExcludedVersions() {
7695
assertFalse(nullVersionConfig.isExcluded(NEVER_EXCLUDED_VERSION));
7796
}
7897

7998
@Test
99+
@WithoutJenkins
80100
public void getRefreshTimeMinutes() {
81101
assertThat(nullVersionConfig.getRefreshTimeMinutes(), is(REFRESH_TIME_MINUTES));
82102
assertThat(oneVersionConfig.getRefreshTimeMinutes(), is(NO_REFRESH_TIME_MINUTES));
83103
}
84104

85105
@Test
106+
@WithoutJenkins
86107
public void getRefreshTimeMilliseconds() {
87108
assertThat(nullVersionConfig.getRefreshTimeMilliseconds(), is(60 * 1000L * REFRESH_TIME_MINUTES));
88109
assertThat(oneVersionConfig.getRefreshTimeMilliseconds(), is(60 * 1000L * NO_REFRESH_TIME_MINUTES));
89110
}
90111

91112
@Test
113+
@WithoutJenkins
92114
public void isRefreshEnabled() {
93115
assertTrue(nullVersionConfig.isRefreshEnabled());
94116
assertFalse(oneVersionConfig.isRefreshEnabled());
95117
}
96118

97119
@Test
120+
@WithoutJenkins
98121
public void getExcludedVersionsStr() {
99122
assertThat(nullVersionConfig.getExcludedVersionsStr(), is(NULL_EXCLUDED_VERSION));
100123
assertThat(oneVersionConfig.getExcludedVersionsStr(), is(ONE_EXCLUDED_VERSION));
@@ -103,6 +126,7 @@ public void getExcludedVersionsStr() {
103126
}
104127

105128
@Test
129+
@WithoutJenkins
106130
public void isExcluded() {
107131
assertFalse(nullVersionConfig.isExcluded(NULL_EXCLUDED_VERSION));
108132
assertFalse(nullVersionConfig.isExcluded(""));
@@ -131,4 +155,31 @@ public void isExcluded() {
131155
assertFalse(substringVersionConfig.isExcluded(null));
132156
}
133157

158+
@Test
159+
public void clearCache() throws Exception {
160+
sampleRepo.init();
161+
sampleRepo.write("vars/foo.groovy", "def call() { echo 'foo' }");
162+
sampleRepo.git("add", "vars");
163+
sampleRepo.git("commit", "--message=init");
164+
LibraryConfiguration config = new LibraryConfiguration("library",
165+
new SCMSourceRetriever(new GitSCMSource(null, sampleRepo.toString(), "", "*", "", true)));
166+
config.setDefaultVersion("master");
167+
config.setImplicit(true);
168+
config.setCachingConfiguration(new LibraryCachingConfiguration(30, null));
169+
GlobalLibraries.get().getLibraries().add(config);
170+
// Run build and check that cache gets created.
171+
WorkflowJob p = r.createProject(WorkflowJob.class);
172+
p.setDefinition(new CpsFlowDefinition("foo()", true));
173+
WorkflowRun b = r.buildAndAssertSuccess(p);
174+
LibrariesAction action = b.getAction(LibrariesAction.class);
175+
LibraryRecord record = action.getLibraries().get(0);
176+
FilePath cache = LibraryCachingConfiguration.getGlobalLibrariesCacheDir().child(record.getDirectoryName());
177+
assertThat(new File(cache.getRemote()), anExistingDirectory());
178+
assertThat(new File(cache.withSuffix("-name.txt").getRemote()), anExistingFile());
179+
// Clear the cache. TODO: Would be more realistic to set up security and use WebClient.
180+
ExtensionList.lookupSingleton(LibraryCachingConfiguration.DescriptorImpl.class).doClearCache("library");
181+
assertThat(new File(cache.getRemote()), not(anExistingDirectory()));
182+
assertThat(new File(cache.withSuffix("-name.txt").getRemote()), not(anExistingFile()));
183+
}
184+
134185
}

0 commit comments

Comments
 (0)