4
\$\begingroup\$

I've recently started work on a hobby project that's basically a sandbox file system written in Java, slightly inspired by PhysicsFS. The point of the project is to separate what files a user can read/write to by restricting read/write to predefined locations on the disk. A "location" can be either a directory (folder) or an archive (*.zip file).

An example use-case of this would be when creating an application that can load user plugins, for example a 3D-modelling program that can load scripts, and only allowing read/writes to the "Plugins" folder. By using the sandbox file system for loaded plugins you ensure that those untrusted plugins cannot mess with any user's files outside of the "Plugins" folder.

I would like potential reviewers to mostly focus on readability and code style since I plan to make this a public project, and also some focus on good/bad practices when it comes to file systems and I/O (either something bad I'm doing or something good I should be doing). If there are some edge cases that would crash the program or allow a user to navigate outside the sandbox that would be nice to know too!

Note that there are a few files I have not included in the review, mostly because they are either very simple wrappers around Strings or java File objects, or they are an Enum (but they can be included if needed).

Here's an implementation of the file system using Java NIO I/O:

public class NIOFileSystem implements FileSystem<NIOFSFile> {
    private static final Logger LOGGER = LoggerFactory.getLogger(NIOFileSystem.class);

    private final List<NIOFSRegistration> registrations;
    private FilePath writePath;

    public NIOFileSystem() {
        this.registrations = new ArrayList<>();
        this.writePath = null;
    }

    @Override
    public boolean addToSearchPath(final FilePath path, final FileLocation location) {
        requireNonNull(path, "path must not be null");
        requireNonNull(location, "location must not be null");

        final FilePath minimized = path.minimize();
        if (!verifyFilePathAndLog(minimized)) return false;

        try {
            LOGGER.debug("Attempting to add '{}' ({}) to the search path.", path, location);
            final NIOFSRegistration reg = new NIOFSRegistration(minimized, location);
            if (registrations.contains(reg)) return false;
            registrations.add(reg);
            LOGGER.info("'{}' ({}) added to search path", path, location);
            return true;
        } catch (final URISyntaxException e) {
            LOGGER.error("Could not add '{}' ({}) to the search path: {}", path.toString(),
                    location, e.toString());
            return false;
        }
    }

    @Override
    public boolean isOnSearchPath(final FilePath path) {
        requireNonNull(path, "path must not be null");

        final FilePath minimized = path.minimize();
        if (!verifyFilePathAndLog(minimized)) return false;
        return registrations.stream().anyMatch(reg -> reg.getFilePath().equals(minimized));
    }

    @Override
    public boolean addAllArchivesToSearchPath(final FilePath path, final FileLocation location) {
        requireNonNull(path, "path must not be null");
        requireNonNull(location, "location must not be null");

        final FilePath minimized = path.minimize();
        if (!verifyFilePathAndLog(minimized)) return false;

        try {
            LOGGER.debug("Adding all archives in path '{}' ({})", path, location);
            final Path filePath = FSUtils.constructNIOPath(path, location);
            final Iterator<Path> it = Files.walk(filePath, 1).iterator();

            // If iterator is somehow empty something went wrong.
            if (!it.hasNext()) {
                LOGGER.warn("Iterator over path {} is empty", filePath);
                return false;
            }

            // The first element in the iterator is the input path
            // and we don't want to add that.
            it.next();
            boolean success = true;
            while (it.hasNext()) {
                final Path p = it.next();
                final FilePath fp = FilePath.from(p.toString());
                final FileType fileType = FSUtils.getFileType(p);
                if (fileType != FileType.ARCHIVE) {
                    LOGGER.trace("Not adding child '{}' because it is a {}", fp, fileType);
                    continue;
                }

                // If we couldn't add the archive to the search path we have
                // to return false according to the spec.
                if (!addToSearchPath(fp, location)) success = false;
            }

            return success;
        } catch (final URISyntaxException e) {
            LOGGER.error("Could not create java.nio.file.Path path to '{}': {}", path, e);
            return false;
        } catch (final IOException e) {
            LOGGER.error("Could not walk file '{}': {}", path, e);
            return false;
        }
    }

    @Override
    public boolean setWriteDirectory(final FilePath path) {
        requireNonNull(path, "path must not be null");
        this.writePath = path;
        return true;
    }

    @Override
    public Optional<FilePath> getWriteDirectory() {
        return Optional.ofNullable(writePath);
    }

    @Override
    public Optional<NIOFSFile> open(final FilePath path, final FileAccessType accessType) {
        requireNonNull(path, "path must not be null");
        requireNonNull(accessType, "accessType must not be null");

        final FilePath minimized = path.minimize();
        if (!verifyFilePathAndLog(minimized)) return Optional.empty();

        try {
            LOGGER.debug("Attempting to open file '{}' ({})", path, accessType);
            switch (accessType) {
            case READ:
                LOGGER.trace("Number of registered input directories: {}", registrations.size());
                for (final NIOFSRegistration reg : registrations) {
                    LOGGER.trace("Looking for file '{}' in '{}'", path, reg.getFilePath());
                    if (reg.getType() == FileType.ARCHIVE) {
                        final Path zipFile = reg.getPath();
                        if (!Files.exists(zipFile)) continue;

                        try (java.nio.file.FileSystem fs = FileSystems.newFileSystem(zipFile,
                                null)) {
                            final Path filePath = fs.getPath(path.toString());
                            if (!Files.exists(filePath)) continue;
                            LOGGER.trace("Found file '{}' in '{}'", path, reg.getFilePath());
                            return Optional.of(new PreReadNIOFSFile(Files.readAllBytes(filePath)));
                        } catch (final IOException e) {
                            LOGGER.error("Could not fetch file '{}' from archive '{}': {}", path,
                                    zipFile, e);
                            return Optional.empty();
                        }
                    }

                    final Path nioPath = reg.getPath(minimized);
                    if (!Files.exists(nioPath)) continue;
                    LOGGER.trace("Found file '{}' in '{}'", path, nioPath);
                    return Optional.of(new NIOFSFile(nioPath, false));
                }
                LOGGER.debug("Could not find file '{}'", path);
                return Optional.empty();
            case WRITE:
                final FilePath fqPath = writePath.append(minimized);
                final Path nioPath = FSUtils.constructNIOPath(fqPath, FileLocation.EXTERNAL);
                LOGGER.trace("Looking for file '{}' in '{}' ({})", path, writePath,
                        nioPath.toAbsolutePath());
                return Optional.of(new NIOFSFile(nioPath, false));
            default:
                throw new IllegalArgumentException("Cannot handle accessType " + accessType);
            }
        } catch (final URISyntaxException e) {
            LOGGER.error("Could not create directory '{}': {}", path, e);
            return Optional.empty();
        }
    }

    @Override
    public boolean createDirectory(final FilePath path) {
        requireNonNull(path, "path must not be null");

        final FilePath minimized = path.minimize();
        if (!verifyFilePathAndLog(minimized)) return false;
        if (isNull(writePath)) {
            LOGGER.debug("Trying to create directory {} but no write path is set.", minimized);
            return false;
        }

        try {
            final FilePath fqPath = writePath.append(minimized);
            final Path nioPath = FSUtils.constructNIOPath(fqPath, FileLocation.EXTERNAL);
            final Path createdPath = Files.createDirectories(nioPath);
            LOGGER.debug("Created directory at '{}'", createdPath.toString());
            return true;
        } catch (final IOException | URISyntaxException e) {
            LOGGER.error("Could not create directory '{}': {}", path, e);
            return false;
        }
    }

    @Override
    public Optional<NIOFSFile> createFile(final FilePath path) {
        requireNonNull(path, "path must not be null");

        final FilePath minimized = path.minimize();
        if (!verifyFilePathAndLog(minimized)) return Optional.empty();
        if (isNull(writePath)) {
            LOGGER.debug("Trying to create file {} but no write path is set.", minimized);
            return Optional.empty();
        }

        try {
            final FilePath fqPath = writePath.append(minimized);
            final Path nioPath = FSUtils.constructNIOPath(fqPath, FileLocation.EXTERNAL);
            LOGGER.debug("Trying to create file at '{}'", nioPath.toAbsolutePath());
            final Path createdFilePath = Files.createFile(nioPath);
            LOGGER.debug("Created file at '{}'", createdFilePath);
            return Optional.of(new NIOFSFile(createdFilePath, true));
        } catch (final FileAlreadyExistsException e) {
            LOGGER.warn("Could not create file '{}' because it already exists", path);
            return open(minimized, FileAccessType.WRITE);
        } catch (URISyntaxException | IOException e) {
            LOGGER.error("Could not create file '{}': {}", path, e);
            return Optional.empty();
        }
    }

    @Override
    public boolean delete(final FilePath path) {
        return deleteInternal(path, false);
    }

    @Override
    public boolean forceDelete(final FilePath path) {
        return deleteInternal(path, true);
    }

    private boolean deleteInternal(final FilePath path, final boolean force) {
        requireNonNull(path, "path must not be null");

        final FilePath minimized = path.minimize();
        if (!verifyFilePathAndLog(minimized)) return false;
        if (isNull(writePath)) {
            LOGGER.debug("Trying to remove {} but no write path is set.", minimized);
            return false;
        }

        final FilePath fqPath = writePath.append(minimized);
        try {
            final Path nioPath = FSUtils.constructNIOPath(fqPath, FileLocation.EXTERNAL);
            LOGGER.debug("Trying to delete '{}'", nioPath.toAbsolutePath());
            if (force) {
                return forceDelInternal(nioPath);
            } else {
                return delInternal(nioPath);
            }
        } catch (final DirectoryNotEmptyException e) {
            if (!force)
                LOGGER.warn(
                        "Could not delete {} because it is a non-empty directory. Use FileSystem#forceDelete instead.",
                        path);
            else
                LOGGER.error(
                        "Fatal error trying to delete {}, directory not empty error even though delete was forced!",
                        path);
            return false;
        } catch (final URISyntaxException | IOException | SecurityException e) {
            LOGGER.error("Could not delete '{}': {}", path, e);
            return false;
        }
    }

    private boolean forceDelInternal(final Path nioPath) throws IOException {
        requireNonNull(nioPath, "nioPath must not be null");
        // Delete all files (not folders). We have to do this first
        // because we can't delete non-empty folders.
        final boolean allFilesDeleted = Files.walk(nioPath).filter(Files::isRegularFile)
                .map(Path::toFile).map(File::delete).allMatch(wasDeleted -> wasDeleted);
        if (!allFilesDeleted) {
            LOGGER.warn("Could not empty all files from sub-directories of {}",
                    nioPath.toAbsolutePath());
            return false;
        }

        // Delete all folders too
        final boolean allDirsDeleted = Files.walk(nioPath).map(Path::toFile).map(File::delete)
                .allMatch(wasDeleted -> wasDeleted);
        if (!allDirsDeleted) {
            LOGGER.warn("Could not delete empty directories at {}", nioPath.toAbsolutePath());
            return false;
        }

        LOGGER.info("Deleted {}", nioPath.toAbsolutePath());
        return true;
    }

    private boolean delInternal(final Path nioPath) throws IOException {
        requireNonNull(nioPath, "nioPath must not be null");
        final boolean success = Files.deleteIfExists(nioPath);
        if (success)
            LOGGER.info("Deleted '{}'", nioPath.toAbsolutePath());
        else
            LOGGER.warn("Failed to delete file '{}'", nioPath.toAbsolutePath());
        return success;
    }

    private static boolean verifyFilePathAndLog(final FilePath path) {
        requireNonNull(path, "path must not be null");
        return FSUtils.isSafePath(path, LOGGER);
    }
}

Interface with documentation and specifications, if you would like to read that too:

/** Interface that represents an abstract file system that restricts reading / writing to only
 * specified directories. A file system can handle both directories (e.g. "documents/savegames/") or
 * archives (e.g. "mods/coolmod.zip").
 *
 * @author {real name redacted} */
public interface FileSystem<T extends FSFile> {
    /**
     * <p>
     * Adds the given {@link FilePath} to the search path so that files in that path can be found
     * with {@link FileSystem#open(FilePath, FileAccessType)}.
     * </p>
     *
     * <p>
     * The path can either point to a directory (e.g. <code>"savegames/"</code>) or an archive (e.g.
     * <code>"graphics.zip"</code>).
     * </p>
     *
     * @param path
     *            the path to add
     * @param location
     *            where the file resides
     * @return <code>true</code> if path was added to the search path, <code>false</code> otherwise.
     * @see FileLocation
     * @see FilePath */
    boolean addToSearchPath(FilePath path, FileLocation location);

    /** Returns true if and only if the {@link FilePath} is on the search path. False otherwise.
     *
     * @param path
     *            the path
     * @return true if and only if the {@link FilePath} is on the search path. False otherwise */
    boolean isOnSearchPath(FilePath path);

    /** Adds all immediate child archives of <code>path</code> to the search path, but not the path
     * itself. Useful for adding things like a "mods" directory. E.g:
     *
     * <code>
     * <pre>
     * // mods/
     * //   coolmod.zip
     * //   soundmod.zip
     * //   4kgraphics.zip
     * //   logs/
     * //      2019-03-31.txt
     * //   someotherfolder/
     * //      somezip.zip
     * FilePath path = FilePath.from("mods/");
     *
     * // coolmod.zip, soundmod.zip, 4kgraphics.zip are now all added to the search path,
     * // but <b>NOT</b> either logs/, logs/2019-03-31.txt, or somezip.zip.
     * // mods/ itself is also <b>NOT</b> added to the search path.
     * fs.addAllArchivesToSearchPath(path);
     * </pre>
     * </code>
     *
     * @param path
     *            where to look for archives.
     * @param location
     *            where the archives reside.
     * @return true if and only if ALL archives were added to the file path, false otherwise.
     * @see FileSystem#addToSearchPath(FilePath, FileLocation)
     * @see FileLocation
     * @see FilePath */
    boolean addAllArchivesToSearchPath(FilePath path, FileLocation location);

    /** Sets the write directory. The write directory must not be an archive or a file. The write
     * directory is always {@link FileLocation#EXTERNAL}. There can only ever exist one write
     * directory for safety reasons.
     *
     * @param path
     *            the directory
     * @return true if the directory was set, false otherwise
     * @see FileLocation
     * @see FilePath */
    boolean setWriteDirectory(FilePath path);

    /** Returns an {@link Optional} containing the current write directory, or an empty Optional if
     * no write directory has been set.
     *
     * @return an {@link Optional} containing the current write directory. */
    Optional<FilePath> getWriteDirectory();

    /** Opens a file for reading or writing.
     *
     * @param path
     *            the path to the file
     * @param accessType
     *            how the file should be opened.
     * @return an optional containing the {@link FSFile}, or an empty optional if the file does not
     *         exist.
     * @see FilePath
     * @see FileAccessType */
    Optional<T> open(FilePath path, FileAccessType accessType);

    /** Creates a new directory in the write directory. If the path contains several directories all
     * intermediate directories will be created, e.g. "documents/logs/crashes/" will create
     * "documents/" and "documents/logs/" if they don't already exist.
     *
     * @param path
     *            where to create the directory.
     * @return true if and only if the <i>final</i> directory in the path was created (so "crashes/"
     *         in "documents/logs/crashes/"). */
    boolean createDirectory(FilePath path);

    /** Creates a new file in the write directory. If the directory the file is located in does not
     * exist this method returns an empty {@link Optional}, otherwise it returns an optional with
     * the created file.
     *
     * @param path
     *            where to create the file
     * @return an optional containing the created file, or an empty optional if file could not be
     *         created. */
    Optional<T> createFile(FilePath path);

    /** Removes a file or directory present in the write directory. Cannot remove a directory if it
     * contains files or non-empty child directories.
     *
     * @param path
     *            the path
     * @return true if the file/directory was removed, false otherwise
     * @see FileSystem#forceDelete(FilePath) */
    boolean delete(FilePath path);

    /** Removes a file or directory present in the write directory, including all files in that
     * directory and all child directories.
     *
     * @param path
     *            the path to the directory
     * @return true if the file/directory was removed, false otherwise
     * @see FileSystem#delete(FilePath) */
    boolean forceDelete(FilePath path);

    /** Convenience function that converts a string to a {@link FilePath} then calls
     * {@link FileSystem#delete(FilePath)}.
     *
     * @param path
     * @return true if the file was removed, false otherwise */
    default boolean delete(final String path) {
        return delete(FilePath.from(path));
    }

    /** Convenience function that converts a string to a {@link FilePath} then calls
     * {@link FileSystem#forceDelete(FilePath)}.
     *
     * @param path
     *            the path
     * @return true if the directory was removed, false otherwise */
    default boolean forceDelete(final String path) {
        return forceDelete(FilePath.from(path));
    }

    /** Convenience function that converts a string to a {@link FilePath} then calls
     * {@link FileSystem#addToSearchPath(FilePath, FileLocation)}.
     *
     * @see FileSystem#addToSearchPath(FilePath, FileLocation)
     * @param path
     *            the string path
     * @return <code>true</code> if path was added to the search path, <code>false</code> otherwise.
     * @see FileLocation
     * @see FilePath */
    default boolean addToSearchPath(final String path, final FileLocation location) {
        return addToSearchPath(FilePath.from(path), location);
    }

    /** Convenience function that converts a string to a {@link FilePath} then calls
     * {@link FileSystem#addAllArchivesToSearchPath(FilePath, FileLocation)}.
     *
     * @param path
     *            the string path.
     * @param location
     *            where the archives reside.
     * @return true if and only if ALL archives were added to the file path, false otherwise.
     * @see FileSystem#addToSearchPath(FilePath, FileLocation)
     * @see FileLocation
     * @see FilePath */
    default boolean addAllArchivesToSearchPath(final String path, final FileLocation location) {
        return addAllArchivesToSearchPath(FilePath.from(path), location);
    }

    /** Convenience function that converts a string to a {@link FilePath} then calls
     * {@link FileSystem#setWriteDirectory(FilePath)}.
     *
     * @see FileSystem#setWriteDirectory(FilePath)
     * @param path
     *            the string path
     * @return <code>true</code> if path was set at the write directory, <code>false</code>
     *         otherwise.
     * @see FileLocation
     * @see FilePath */
    default boolean setWriteDirectory(final String path) {
        return setWriteDirectory(FilePath.from(path));
    }

    /** Convenience function that converts a string to a {@link FilePath} then calls
     * {@link FileSystem#open(FilePath, FileAccessType)}.
     *
     * @see FileSystem#open(FilePath, FileAccessType)
     * @param path
     *            the string path
     * @param accessType
     *            which directory we should open the file from
     * @return an optional containing the {@link FSFile}, or an empty optional if the file does not
     *         exist.
     * @see FilePath
     * @see FileAccessType */
    default Optional<T> open(final String path, final FileAccessType accessType) {
        return open(FilePath.from(path), accessType);
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Your code reads really well. I would prefix most methods with try* though. I would expect open to throw an exception when the file does not exist, and tryOpen to return optional.empty. \$\endgroup\$ – dfhwze Jun 14 at 17:38
  • 1
    \$\begingroup\$ What prevents this user plugin from calling FileSystems.getDefault() and reading and writing from anywhere? Seem to me you want to install a SecurityManager. \$\endgroup\$ – AJNeufeld Jun 15 at 21:38
  • \$\begingroup\$ @AJNeufeld The point is that you probably would not use Java for your plugins, but preferably a scripting language like Javascript, Lua, Python, etc. and then you would route all I/O operations through my code. It is not supposed to be a magical solution that prevents any sort of tampering with the file system - it simply an interface that guarantees that all I/O operations that flows through it can only access predefined locations. Making sure that everything actually flows through it is the end-developer's problem :P \$\endgroup\$ – Charanor Jun 15 at 23:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.