🔒 [security fix: restrict storage directory permissions]

[segin]

🎯 What: Fixed a security vulnerability where the application's storage directory was created with overly permissive 0755 permissions on POSIX systems.

⚠️ Risk: With 0755 permissions, other users on the same system could read the contents of the configuration directory, potentially exposing sensitive user data or session information.

🛡️ Solution: Restricted the mkdir mode to 0700 in System::createStoragePath(). This ensures that the directory is readable, writable, and searchable only by the user running the application. Added a new regression test tests/test_storage_permissions.cpp and integrated it into the build system to verify this behavior.

---

PR created automatically by Jules for task 8809830024587260637 started by @segin

[google-labs-jules]

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.

---

For security, I will only act on instructions from the user who triggered this task.

[gemini-code-assist]

Code Review

This pull request enhances security by restricting storage directory permissions to 0700 and adds a verification test. The review feedback recommends improving the test's reliability by utilizing unique temporary directories and RAII for automatic cleanup to avoid path conflicts and ensure proper resource management.

[gemini-code-assist]

This test can be made more robust and cleaner.

1. Hardcoded temporary path: Using /tmp/psymp3_test_perms can cause conflicts if tests are run concurrently or by different users. It's better to create a unique temporary directory using mkdtemp.
2. Manual cleanup: The cleanup logic using system("rm -rf ...") is repeated. Using a simple RAII helper ensures that the temporary directory is cleaned up automatically on all exit paths, making the code cleaner and safer.

Here is a suggested refactoring of the main function. Note that you'll need to add #include <string> and #include <unistd.h> to your includes.

int main() {
    char test_root_template[] = "/tmp/psymp3_test_perms_XXXXXX";
    char* test_root = mkdtemp(test_root_template);
    if (!test_root) {
        perror("mkdtemp failed");
        return 1;
    }

    // Use a simple RAII struct to ensure cleanup on all exit paths.
    struct DirCleaner {
        char* path;
        ~DirCleaner() {
            if (path) {
                std::string command = "rm -rf ";
                command += path;
                system(command.c_str());
            }
        }
    } cleaner{test_root};

    std::string storage_path_str = std::string(test_root) + "/psymp3";
    const char* storage_path = storage_path_str.c_str();

    setenv("XDG_CONFIG_HOME", test_root, 1);

    std::cout << "Target storage path: " << System::getStoragePath().to8Bit(true) << std::endl;

    if (!System::createStoragePath()) {
        std::cerr << "Failed to create storage path" << std::endl;
        return 1;
    }

    struct stat st;
    if (stat(storage_path, &st) != 0) {
        perror("stat");
        return 1;
    }

    mode_t actual_mode = st.st_mode & 0777;
    std::cout << "Actual permissions: 0" << std::oct << actual_mode << std::dec << std::endl;

    if (actual_mode != 0700) {
        std::cerr << "VULNERABILITY DETECTED: Insecure permissions 0" << std::oct << actual_mode << std::dec << " (expected 0700)" << std::endl;
        return 1;
    }

    std::cout << "SUCCESS: Permissions are restricted to 0700" << std::endl;

    return 0;
}

[Copilot]

Pull request overview

This PR tightens POSIX storage directory creation permissions to mitigate local information disclosure risks by ensuring the app’s config/storage directory is not world-readable, and adds a regression test to verify the intended mode.

Changes:

• Change System::createStoragePath() POSIX mkdir mode from 0755 to 0700.
• Add a new permissions regression test test_storage_permissions.
• Wire the new test into tests/Makefile.am build/test targets.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
src/system.cpp	Restricts newly-created storage directory permissions to 0700 on POSIX.
tests/test_storage_permissions.cpp	New regression test validating storage directory permissions.
tests/Makefile.am	Adds the new test binary to the test build/run lists.

---

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

[Copilot]

On POSIX, returning true for errno == EEXIST leaves existing installations vulnerable if the directory already exists with overly-permissive mode (e.g., previously created as 0755). Consider: when mkdir fails with EEXIST, stat() the path, ensure it’s a directory (not a file), and chmod it to 0700 (or at least clear group/other bits) so upgrades actually remediate the issue.

[Copilot]

TESTS is already defined as $(check_PROGRAMS) earlier in this file, so adding TESTS += test_storage_permissions will include this test twice (once via $(check_PROGRAMS) and once explicitly). This can cause the test to run twice under make check; please drop the extra TESTS += ... line and rely on check_PROGRAMS only.

Suggested change

TESTS += test_storage_permissions

[Copilot]

This test uses a fixed global path under /tmp and deletes it via system("rm -rf ..."). That’s brittle (depends on a shell + rm), and it can race/fail if tests run in parallel (two processes can remove each other’s directories). Prefer creating a unique temp directory per run (e.g., mkdtemp or std::filesystem temp dir + random suffix) and remove it using filesystem APIs instead of invoking a shell.

[Copilot]

The assertion actual_mode != 0700 is potentially flaky because the process umask can mask out bits from the requested mode, yielding a more restrictive value than 0700. If the security requirement is “no group/other access”, assert that actual_mode & 0077 == 0 (and optionally that the owner bits are sufficient), or set umask(0) for the duration of the directory creation (restoring the previous umask afterward).

Suggested change

	if (actual_mode != 0700) {
	std::cerr << "VULNERABILITY DETECTED: Insecure permissions 0" << std::oct << actual_mode << std::dec << " (expected 0700)" << std::endl;
	// Clean up
	system("rm -rf /tmp/psymp3_test_perms");
	return 1;
	}
	std::cout << "SUCCESS: Permissions are restricted to 0700" << std::endl;
	if ((actual_mode & 0077) != 0) {
	std::cerr << "VULNERABILITY DETECTED: Insecure permissions 0" << std::oct << actual_mode << std::dec
	<< " (group/other permissions must be unset)" << std::endl;
	// Clean up
	system("rm -rf /tmp/psymp3_test_perms");
	return 1;
	}
	std::cout << "SUCCESS: Permissions restrict access to owner only" << std::endl;

[Copilot]

storage_path is hardcoded to /tmp/psymp3_test_perms/psymp3, which duplicates logic from System::getStoragePath() and can get out of sync if the storage path logic changes (or if XDG_CONFIG_HOME handling changes). Consider deriving the expected path from System::getStoragePath() and then stat() that computed path instead of a fixed string.