ensure no world writable permissions in Nextcloud sync folder

past versions may have wrongly set the write permissions for all remove this under the hypothesis that newly created files or folders gets more restrictive permissions and that past files or folders should be updated to get the same permissions Signed-off-by: Matthieu Gallien <[email protected]>
nextcloud · Nov 20, 2024 · 3135a1f · 3135a1f
1 parent 3014d6b
commit 3135a1f
Show file tree

Hide file tree

Showing 9 changed files with 27 additions and 11 deletions.
diff --git a/src/csync/csync.h b/src/csync/csync.h
@@ -217,6 +217,7 @@ struct OCSYNC_EXPORT csync_file_stat_s {
   bool is_hidden BITFIELD(1); // Not saved in the DB, only used during discovery for local files.
   bool isE2eEncrypted BITFIELD(1);
   bool is_metadata_missing BITFIELD(1); // Indicates the file has missing metadata, f.ex. the file is not a placeholder in case of vfs.
+  bool isPermissionsInvalid BITFIELD(1);
 
   QByteArray path;
   QByteArray rename_path;
@@ -244,6 +245,7 @@ struct OCSYNC_EXPORT csync_file_stat_s {
     , is_hidden(false)
     , isE2eEncrypted(false)
     , is_metadata_missing(false)
+    , isPermissionsInvalid(false)
   { }
 };
 

diff --git a/src/csync/vio/csync_vio_local_unix.cpp b/src/csync/vio/csync_vio_local_unix.cpp
@@ -170,5 +170,7 @@ static int _csync_vio_local_stat_mb(const mbchar_t *wuri, csync_file_stat_t *buf
   buf->inode = sb.st_ino;
   buf->modtime = sb.st_mtime;
   buf->size = sb.st_size;
+  buf->isPermissionsInvalid = (sb.st_mode & S_IWOTH) == S_IWOTH;
+
   return 0;
 }
diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp
@@ -1066,6 +1066,10 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
         if (_queryLocal != NormalQuery && _queryServer != NormalQuery)
             recurse = false;
 
+        if (localEntry.isPermissionsInvalid) {
+            recurse = true;
+        }
+
         if ((item->_direction == SyncFileItem::Down || item->_instruction == CSYNC_INSTRUCTION_CONFLICT || item->_instruction == CSYNC_INSTRUCTION_NEW || item->_instruction == CSYNC_INSTRUCTION_SYNC) &&
                 (item->_modtime <= 0 || item->_modtime >= 0xFFFFFFFF)) {
             item->_instruction = CSYNC_INSTRUCTION_ERROR;
@@ -1093,6 +1097,13 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
             }
         }
 
+        if (localEntry.isPermissionsInvalid && item->_instruction == CSyncEnums::CSYNC_INSTRUCTION_NONE) {
+            item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA;
+            item->_direction = SyncFileItem::Down;
+        }
+
+        item->isPermissionsInvalid = localEntry.isPermissionsInvalid;
+
         auto recurseQueryLocal = _queryLocal == ParentNotChanged ? ParentNotChanged : localEntry.isDirectory || item->_instruction == CSYNC_INSTRUCTION_RENAME ? NormalQuery : ParentDontExist;
         processFileFinalize(item, path, recurse, recurseQueryLocal, recurseQueryServer);
     };

diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp
@@ -348,6 +348,7 @@ void DiscoverySingleLocalDirectoryJob::run() {
         i.isSymLink = dirent->type == ItemTypeSoftLink;
         i.isVirtualFile = dirent->type == ItemTypeVirtualFile || dirent->type == ItemTypeVirtualFileDownload;
         i.isMetadataMissing = dirent->is_metadata_missing;
+        i.isPermissionsInvalid = dirent->isPermissionsInvalid;
         i.type = dirent->type;
         results.push_back(i);
     }

diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h
@@ -103,6 +103,7 @@ struct LocalInfo
     bool isVirtualFile = false;
     bool isSymLink = false;
     bool isMetadataMissing = false;
+    bool isPermissionsInvalid = false;
     [[nodiscard]] bool isValid() const { return !name.isNull(); }
 };
 

diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp
@@ -468,6 +468,7 @@ bool FileSystem::setFolderPermissions(const QString &path,
         case OCC::FileSystem::FolderPermissions::ReadOnly:
             break;
         case OCC::FileSystem::FolderPermissions::ReadWrite:
+            std::filesystem::permissions(stdStrPath, std::filesystem::perms::others_write, std::filesystem::perm_options::remove);
             std::filesystem::permissions(stdStrPath, std::filesystem::perms::owner_write, std::filesystem::perm_options::add);
             break;
         }

diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp
@@ -1461,15 +1461,9 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status)
                 try {
                     if (FileSystem::fileExists(propagator()->fullLocalPath(_item->_file))) {
                         FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_file), FileSystem::FolderPermissions::ReadOnly);
-                        qCDebug(lcDirectory) << "old permissions" << static_cast<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions());
-                        std::filesystem::permissions(propagator()->fullLocalPath(_item->_file).toStdWString(), std::filesystem::perms::owner_write | std::filesystem::perms::group_write | std::filesystem::perms::others_write, std::filesystem::perm_options::remove);
-                        qCDebug(lcDirectory) << "new permissions" << static_cast<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions());
                     }
                     if (!_item->_renameTarget.isEmpty() && FileSystem::fileExists(propagator()->fullLocalPath(_item->_renameTarget))) {
                         FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_renameTarget), FileSystem::FolderPermissions::ReadOnly);
-                        qCDebug(lcDirectory) << "old permissions" << static_cast<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions());
-                        std::filesystem::permissions(propagator()->fullLocalPath(_item->_renameTarget).toStdWString(), std::filesystem::perms::owner_write | std::filesystem::perms::group_write | std::filesystem::perms::others_write, std::filesystem::perm_options::remove);
-                        qCDebug(lcDirectory) << "new permissions" << static_cast<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions());
                     }
                 }
                 catch (const std::filesystem::filesystem_error &e)
@@ -1481,15 +1475,13 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status)
             } else {
                 try {
                     if (FileSystem::fileExists(propagator()->fullLocalPath(_item->_file))) {
+                        qCDebug(lcDirectory) << propagator()->fullLocalPath(_item->_file) << "old permissions" << static_cast<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions());
                         FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_file), FileSystem::FolderPermissions::ReadWrite);
-                        qCDebug(lcDirectory) << "old permissions" << static_cast<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions());
-                        std::filesystem::permissions(propagator()->fullLocalPath(_item->_file).toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::add);
-                        qCDebug(lcDirectory) << "new permissions" << static_cast<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions());
+                        qCDebug(lcDirectory) << propagator()->fullLocalPath(_item->_file) << "new permissions" << static_cast<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_file).toStdWString()).permissions());
                     }
                     if (!_item->_renameTarget.isEmpty() && FileSystem::fileExists(propagator()->fullLocalPath(_item->_renameTarget))) {
-                        FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_renameTarget), FileSystem::FolderPermissions::ReadWrite);
                         qCDebug(lcDirectory) << "old permissions" << static_cast<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions());
-                        std::filesystem::permissions(propagator()->fullLocalPath(_item->_renameTarget).toStdWString(), std::filesystem::perms::owner_write, std::filesystem::perm_options::add);
+                        FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_renameTarget), FileSystem::FolderPermissions::ReadWrite);
                         qCDebug(lcDirectory) << "new permissions" << static_cast<int>(std::filesystem::status(propagator()->fullLocalPath(_item->_renameTarget).toStdWString()).permissions());
                     }
                 }

diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp
@@ -363,6 +363,10 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item)
                 const bool isReadOnly = !item->_remotePerm.isNull() && !item->_remotePerm.hasPermission(RemotePermissions::CanWrite);
                 modificationHappened = FileSystem::setFileReadOnlyWeak(filePath, isReadOnly);
             }
+            if (item->isPermissionsInvalid) {
+                const auto isReadOnly = !item->_remotePerm.isNull() && !item->_remotePerm.hasPermission(RemotePermissions::CanWrite);
+                FileSystem::setFileReadOnly(filePath, isReadOnly);
+            }
 
             modificationHappened |= item->_size != prev._fileSize;
 

diff --git a/src/libsync/syncfileitem.h b/src/libsync/syncfileitem.h
@@ -340,6 +340,8 @@ class OWNCLOUDSYNC_EXPORT SyncFileItem
     bool _isAnyInvalidCharChild = false;
     bool _isAnyCaseClashChild = false;
 
+    bool isPermissionsInvalid = false;
+
     QString _discoveryResult;
 };