Linked files with an absolute path can be opened again (#9129)

* Linked files with an absolute path can be opened again * Fix detectBadFileName(String) for absolute paths Co-authored-by: Christoph <siedlerkiller@gmail.com> * Update CHANGELOG.md * Make test OS specific * Fix architecture tests Co-authored-by: Christoph <siedlerkiller@gmail.com>
JabRef · Sep 3, 2022 · c92346c · c92346c
1 parent cf6b1ed
commit c92346c
Show file tree

Hide file tree

Showing 9 changed files with 131 additions and 28 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -91,7 +91,8 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 
 ### Fixed
 
-- We fixed an issue where linked fails containing parts of the main file directory could not be opened [#8991](https://github.com/JabRef/jabref/issues/8991)
+- We fixed an issue where linked fails containing parts of the main file directory could not be opened. [#8991](https://github.com/JabRef/jabref/issues/8991)
+- Linked files with an absolute path can be opened again. [#8991](https://github.com/JabRef/jabref/issues/8991)
 - We fixed an issue where the user could not rate an entry in the main table when an entry was not yet ranked. [#5842](https://github.com/JabRef/jabref/issues/5842)
 - We fixed an issue that caused JabRef to sometimes open multiple instances when "Remote Operation" is enabled. [#8653](https://github.com/JabRef/jabref/issues/8653)
 - We fixed an issue where linked files with the filetype "application/pdf" in an entry were not shown with the correct PDF-Icon in the main table [8930](https://github.com/JabRef/jabref/issues/8930)

diff --git a/src/main/java/org/jabref/logic/importer/util/FileFieldParser.java b/src/main/java/org/jabref/logic/importer/util/FileFieldParser.java
@@ -15,7 +15,36 @@
 public class FileFieldParser {
     private static final Logger LOGGER = LoggerFactory.getLogger(FileFieldParser.class);
 
+    private final String value;
+
+    private StringBuilder charactersOfCurrentElement;
+    private boolean windowsPath;
+
+    public FileFieldParser(String value) {
+        this.value = value;
+    }
+
+    /**
+     * Converts the string representation of LinkedFileData to a List of LinkedFile
+     *
+     * The syntax of one element is description:path:type
+     * Multiple elements are concatenated with ;
+     *
+     * The main challenges of the implementation are:
+     *
+     * <ul>
+     *     <li>that XML characters might be included (thus one cannot simply split on ";")</li>
+     *     <li>some characters might be escaped</li>
+     *     <li>Windows absolute paths might be included without escaping</li>
+     * </ul>
+     */
     public static List<LinkedFile> parse(String value) {
+        // We need state to have a more clean code. Thus, we instantiate the class and then return the result
+        FileFieldParser fileFieldParser = new FileFieldParser(value);
+        return fileFieldParser.parse();
+    }
+
+    public List<LinkedFile> parse() {
         List<LinkedFile> files = new ArrayList<>();
 
         if ((value == null) || value.trim().isEmpty()) {
@@ -32,51 +61,72 @@ public static List<LinkedFile> parse(String value) {
             }
         }
 
+        // data of each LinkedFile as split string
         List<String> linkedFileData = new ArrayList<>();
-        StringBuilder sb = new StringBuilder();
+
+        resetDataStructuresForNextElement();
         boolean inXmlChar = false;
         boolean escaped = false;
 
         for (int i = 0; i < value.length(); i++) {
             char c = value.charAt(i);
             if (!escaped && (c == '\\')) {
-                escaped = true;
-                continue;
+                if (windowsPath) {
+                    charactersOfCurrentElement.append(c);
+                    continue;
+                } else {
+                    escaped = true;
+                    continue;
+                }
             } else if (!escaped && (c == '&') && !inXmlChar) {
                 // Check if we are entering an XML special character construct such
                 // as "&#44;", because we need to know in order to ignore the semicolon.
-                sb.append(c);
+                charactersOfCurrentElement.append(c);
                 if ((value.length() > (i + 1)) && (value.charAt(i + 1) == '#')) {
                     inXmlChar = true;
                 }
             } else if (!escaped && inXmlChar && (c == ';')) {
                 // Check if we are exiting an XML special character construct:
-                sb.append(c);
+                charactersOfCurrentElement.append(c);
                 inXmlChar = false;
             } else if (!escaped && (c == ':')) {
-                // We are in the next LinkedFile data element
-                linkedFileData.add(sb.toString());
-                sb = new StringBuilder();
+                if ((linkedFileData.size() == 1) && // we already parsed the description
+                        (charactersOfCurrentElement.length() == 1)) { // we parsed one character
+                    // special case of Windows paths
+                    // Example: ":c:\test.pdf:PDF"
+                    // We are at the second : (position 3 in the example) and "just" add it to the current element
+                    charactersOfCurrentElement.append(c);
+                    windowsPath = true;
+                } else {
+                    // We are in the next LinkedFile data element
+                    linkedFileData.add(charactersOfCurrentElement.toString());
+                    resetDataStructuresForNextElement();
+                }
             } else if (!escaped && (c == ';') && !inXmlChar) {
-                linkedFileData.add(sb.toString());
+                linkedFileData.add(charactersOfCurrentElement.toString());
                 files.add(convert(linkedFileData));
 
                 // next iteration
-                sb = new StringBuilder();
+                resetDataStructuresForNextElement();
             } else {
-                sb.append(c);
+                charactersOfCurrentElement.append(c);
             }
             escaped = false;
         }
-        if (sb.length() > 0) {
-            linkedFileData.add(sb.toString());
+        if (charactersOfCurrentElement.length() > 0) {
+            linkedFileData.add(charactersOfCurrentElement.toString());
         }
         if (!linkedFileData.isEmpty()) {
             files.add(convert(linkedFileData));
         }
         return files;
     }
 
+    private void resetDataStructuresForNextElement() {
+        charactersOfCurrentElement = new StringBuilder();
+        windowsPath = false;
+    }
+
     /**
      * Converts the given textual representation of a LinkedFile object
      *

diff --git a/src/main/java/org/jabref/logic/util/io/FileNameCleaner.java b/src/main/java/org/jabref/logic/util/io/FileNameCleaner.java
@@ -6,6 +6,8 @@
  * This class is based on http://stackoverflow.com/a/5626340/873282
  * extended with LEFT CURLY BRACE and RIGHT CURLY BRACE
  * Replaces illegal characters in given file paths.
+ *
+ * Regarding the maximum length, see {@link FileUtil#getValidFileName(String)}
  */
 public class FileNameCleaner {
 

diff --git a/src/main/java/org/jabref/logic/util/io/FileUtil.java b/src/main/java/org/jabref/logic/util/io/FileUtil.java
@@ -91,6 +91,8 @@ public static String getBaseName(Path fileNameWithExtension) {
      * Returns a valid filename for most operating systems.
      * <p>
      * Currently, only the length is restricted to 255 chars, see MAXIMUM_FILE_NAME_LENGTH.
+     *
+     * See also {@link FileHelper#detectBadFileName(String)} and {@link FileNameCleaner#cleanFileName(String)}
      */
     public static String getValidFileName(String fileName) {
         String nameWithoutExtension = getBaseName(fileName);

diff --git a/src/main/java/org/jabref/model/entry/LinkedFile.java b/src/main/java/org/jabref/model/entry/LinkedFile.java
@@ -118,9 +118,6 @@ public boolean equals(Object o) {
 
     /**
      * Writes serialized object to ObjectOutputStream, automatically called
-     *
-     * @param out {@link ObjectOutputStream}
-     * @throws IOException
      */
     private void writeObject(ObjectOutputStream out) throws IOException {
         out.writeUTF(getFileType());
@@ -131,9 +128,6 @@ private void writeObject(ObjectOutputStream out) throws IOException {
 
     /**
      * Reads serialized object from ObjectInputStreamm, automatically called
-     *
-     * @param in {@link ObjectInputStream}
-     * @throws IOException
      */
     private void readObject(ObjectInputStream in) throws IOException {
         fileType = new SimpleStringProperty(in.readUTF());

diff --git a/src/main/java/org/jabref/model/util/FileHelper.java b/src/main/java/org/jabref/model/util/FileHelper.java
@@ -5,6 +5,7 @@
 import java.io.InputStream;
 import java.net.URL;
 import java.nio.file.Files;
+import java.nio.file.InvalidPathException;
 import java.nio.file.Path;
 import java.util.Arrays;
 import java.util.List;
@@ -42,7 +43,7 @@ public class FileHelper {
             20, 21, 22, 23, 24, 25, 26, 27, 28, 29,
             30, 31, 34,
             42,
-            58,
+            58, // ":"
             60, 62, 63,
             123, 124, 125
     };
@@ -133,10 +134,22 @@ public static Optional<Path> find(String fileName, List<Path> directories) {
     /**
      * Detect illegal characters in given filename.
      *
+     * See also {@link org.jabref.logic.util.io.FileNameCleaner#cleanFileName}
+     *
      * @param fileName the fileName to detect
-     * @return Boolean whether there is a illegal name.
+     * @return Boolean whether there is an illegal name.
      */
     public static boolean detectBadFileName(String fileName) {
+        // fileName could be a path, we want to check the fileName only (and don't care about the path)
+        // Reason: Handling of "c:\temp.pdf" is difficult, because ":" is an illegal character in the file name,
+        //         but a perfectly legal one in the path at this position
+        try {
+            fileName = Path.of(fileName).getFileName().toString();
+        } catch (InvalidPathException e) {
+            // in case the internal method cannot parse the path, it is surely illegal
+            return true;
+        }
+
         for (int i = 0; i < fileName.length(); i++) {
             char c = fileName.charAt(i);
             if (!isCharLegal(c)) {
@@ -161,12 +174,14 @@ public static boolean isCharLegal(char c) {
     public static Optional<Path> find(String fileName, Path directory) {
         Objects.requireNonNull(fileName);
         Objects.requireNonNull(directory);
-        // Explicitly check for an empty String, as File.exists returns true on that empty path, because it maps to the default jar location
-        // if we then call toAbsoluteDir, it would always return the jar-location folder. This is not what we want here
+
         if (detectBadFileName(fileName)) {
             LOGGER.error("Invalid characters in path for file {} ", fileName);
             return Optional.empty();
         }
+
+        // Explicitly check for an empty string, as File.exists returns true on that empty path, because it maps to the default jar location.
+        // If we then call toAbsoluteDir, it would always return the jar-location folder. This is not what we want here.
         if (fileName.isEmpty()) {
             return Optional.of(directory);
         }

diff --git a/src/test/java/org/jabref/architecture/MainArchitectureTests.java b/src/test/java/org/jabref/architecture/MainArchitectureTests.java
@@ -31,7 +31,7 @@ public static void doNotUseApacheCommonsLang3(JavaClasses classes) {
 
     @ArchTest
     public static void doNotUseSwing(JavaClasses classes) {
-        // This checks for all all Swing packages, but not the UndoManager
+        // This checks for all Swing packages, but not the UndoManager
         noClasses().that().areNotAnnotatedWith(AllowedToUseSwing.class)
                    .should().accessClassesThat()
                    .resideInAnyPackage("javax.swing",

diff --git a/src/test/java/org/jabref/logic/importer/util/FileFieldParserTest.java b/src/test/java/org/jabref/logic/importer/util/FileFieldParserTest.java
@@ -37,7 +37,7 @@ public void check(LinkedFile expected, List<String> input) {
         assertEquals(expected, FileFieldParser.convert(new ArrayList<>(input)));
     }
 
-    private static Stream<Arguments> stringsToParseTestData() throws Exception {
+    private static Stream<Arguments> stringsToParseTest() throws Exception {
         return Stream.of(
                 // null string
                 Arguments.of(
@@ -114,6 +114,18 @@ private static Stream<Arguments> stringsToParseTestData() throws Exception {
                         "desc:C\\:\\\\test.pdf:PDF"
                 ),
 
+                // handleNonEscapedFilePath
+                Arguments.of(
+                        Collections.singletonList(new LinkedFile("desc", Path.of("C:\\test.pdf"), "PDF")),
+                        "desc:C:\\test.pdf:PDF"
+                ),
+
+                // Source: https://github.com/JabRef/jabref/issues/8991#issuecomment-1214131042
+                Arguments.of(
+                        Collections.singletonList(new LinkedFile("Boyd2012.pdf", Path.of("C:\\Users\\Literature_database\\Boyd2012.pdf"), "PDF")),
+                        "Boyd2012.pdf:C\\:\\\\Users\\\\Literature_database\\\\Boyd2012.pdf:PDF"
+                ),
+
                 // subsetOfFieldsResultsInFileLink: description only
                 Arguments.of(
                         Collections.singletonList(new LinkedFile("", Path.of("file.pdf"), "")),
@@ -170,8 +182,8 @@ private static Stream<Arguments> stringsToParseTestData() throws Exception {
     }
 
     @ParameterizedTest
-    @MethodSource("stringsToParseTestData")
-    public void testParse(List<LinkedFile> expected, String input) {
+    @MethodSource
+    public void stringsToParseTest(List<LinkedFile> expected, String input) {
         assertEquals(expected, FileFieldParser.parse(input));
     }
 }
diff --git a/src/test/java/org/jabref/model/util/FileHelperTest.java b/src/test/java/org/jabref/model/util/FileHelperTest.java
@@ -4,13 +4,19 @@
 import java.nio.file.Path;
 import java.util.Optional;
 
+import org.jabref.architecture.AllowedToUseLogic;
+import org.jabref.logic.util.OS;
+
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
+@AllowedToUseLogic("Needs access to OS, because of OS-specific tests")
 class FileHelperTest {
 
     @Test
@@ -65,4 +71,25 @@ public void testDoesNotFindsFileStartingWithTheSameDirectoryHasASubdirectory(@Te
         Files.createFile(testFile);
         assertEquals(Optional.of(testFile.toAbsolutePath()), FileHelper.find("files/test.pdf", firstFilesPath));
     }
+
+    public void testCTemp() {
+        String fileName = "c:\\temp.pdf";
+        if (OS.WINDOWS) {
+            assertFalse(FileHelper.detectBadFileName(fileName));
+        } else {
+            assertTrue(FileHelper.detectBadFileName(fileName));
+        }
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"/mnt/tmp/test.pdf"})
+    public void legalPaths(String fileName) {
+        assertFalse(FileHelper.detectBadFileName(fileName));
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"te{}mp.pdf"})
+    public void illegalPaths(String fileName) {
+        assertTrue(FileHelper.detectBadFileName(fileName));
+    }
 }