Skip to content

Prevent winstone cleanup from removing files outside webroot - #541

Open
arpan-pramanik wants to merge 1 commit into
jenkinsci:masterfrom
arpan-pramanik:fix-26936
Open

Prevent winstone cleanup from removing files outside webroot#541
arpan-pramanik wants to merge 1 commit into
jenkinsci:masterfrom
arpan-pramanik:fix-26936

Conversation

@arpan-pramanik

Copy link
Copy Markdown

This fixes an issue where winstone's webroot cleanup accidentally follows symlinks and wipes files outside the configured webroot. specifically, during a jenkins upgrade restart, the cleanup was recursing into /proc/self/task/<pid>/cwd which is a symlink to the working directory.

to fix this, i updated deleteRecursive to check if the directory is a symbolic link before listing its children. if it is a symlink, it safely deletes the link itself instead of following it and causing out-of-bounds deletions.

resolves jenkinsci/jenkins#26936

Testing done

  • ran the full winstone maven test suite (mvn clean test) to ensure no existing tests or cleanup logic broke.
  • manually ran an end-to-end test mimicking the exact bug:
    1. started winstone with an explicit --webroot and a dummy war.
    2. placed a symlink inside the webroot pointing to an external safe directory containing dummy data.
    3. made the webroot stale to trigger the deleteRecursive cleanup.
    4. verified that winstone safely deleted the symlink without traversing it, leaving the external safe directory completely untouched.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests that demonstrate the feature works or the issue is fixed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes an unsafe webroot cleanup behavior in HostConfiguration where recursive deletion could follow symlinks and delete files outside the configured webroot (notably observed via /proc/self/task/<pid>/cwd during Jenkins restarts).

Changes:

  • Updates deleteRecursive to avoid descending into symlink directories (deleting the link itself instead).
  • Adds defensive exception handling around the symlink check to avoid aborting cleanup due to path-related failures.
Comments suppressed due to low confidence (1)

src/main/java/winstone/HostConfiguration.java:375

  • The broad catch (Exception) around the symlink check/listing hides unexpected failures and also repeats dir.toPath() (which can throw InvalidPathException) in multiple places. Consider resolving the Path once, narrowing the exceptions, and only logging/returning on truly unrecoverable path issues; this keeps the control flow clearer and avoids swallowing unrelated runtime exceptions.
    private void deleteRecursive(File dir) {
        try {
            if (!Files.isSymbolicLink(dir.toPath())) {
                File[] children = dir.listFiles();
                if (children != null) {
                    for (File child : children) {
                        deleteRecursive(child);
                    }
                }
            }
        } catch (Exception ex) {
            // Ignore path exceptions here; they will be handled by deleteIfExists below
        }
        try {
            Files.deleteIfExists(dir.toPath());
        } catch (Exception ex) {
            Logger.logDirectMessage(Level.WARNING, null, "Failed to delete dirs " + dir.getAbsolutePath(), ex);
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +359 to +366
try {
if (!Files.isSymbolicLink(dir.toPath())) {
File[] children = dir.listFiles();
if (children != null) {
for (File child : children) {
deleteRecursive(child);
}
}
Comment on lines +360 to +365
if (!Files.isSymbolicLink(dir.toPath())) {
File[] children = dir.listFiles();
if (children != null) {
for (File child : children) {
deleteRecursive(child);
}
@MarkEWaite MarkEWaite changed the title ? fixed jenkinsci/jenkins#26936 winstone cleanup going outside webroot Prevent winstone cleanup from removing files outside webroot Jul 3, 2026
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.

Winstone WAR cleanup recurses through /proc/self/task/.../cwd outside configured webroot

2 participants