fix(UI): validate E2EE file names#10238
Conversation
…me is a single path segment before it can enter discovery. Signed-off-by: Rello <github@scherello.de>
Rello
requested review from
camilasan,
i2h3,
mgallien and
nilsding
as code owners
Rello
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68c082fe76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!isOriginalFilenameValid(f.originalFilename)) { | ||
| qCWarning(lcCseMetadata()) << "Could not add encrypted file with invalid original file name."; | ||
| _account->reportClientStatus(OCC::ClientStatusReportingStatus::E2EeError_GeneralError); | ||
| return; |
There was a problem hiding this comment.
When a POSIX client syncs an E2EE file whose basename contains a backslash, local discovery can still allow it unless Windows compatibility or server-forbidden characters are active, but this new guard only returns from addEncryptedFile(). PropagateUploadEncrypted::slotFolderEncryptedMetadataReceived() does not observe that failure and continues with the generated encrypted filename and upload, so the server can receive an encrypted blob that is missing from the folder metadata and cannot be mapped/decrypted in later syncs. Please make this rejection visible to the caller and abort the upload instead of silently dropping the metadata entry.
Useful? React with 👍 / 👎.
Signed-off-by: Rello <github@scherello.de>
mgallien
left a comment
mgallien
left a comment
There was a problem hiding this comment.
one request for change
the automated tests for encryption are failing and would need to pass
Removed file deletion logic for non-directory uploads. Signed-off-by: Rello <Rello@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09e3864fab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!isOriginalFilenameValid(originalFilename)) { | ||
| qCWarning(lcCseMetadata()) << "skipping encrypted file" << encryptedFilename << "metadata has an invalid file name"; | ||
| _account->reportClientStatus(OCC::ClientStatusReportingStatus::E2EeError_GeneralError); | ||
| return {}; |
There was a problem hiding this comment.
When an existing encrypted folder's metadata contains an invalid original name, this branch returns an empty EncryptedFile; setupExistingMetadata() then just skips appending it and still marks the metadata valid. In that scenario the corresponding server entry remains in the PROPFIND results without an e2eMangledName/encrypted flag, so discovery can treat the encrypted blob itself as a normal plaintext child under its mangled server name instead of failing the sync. Please make parsing fail the metadata setup, or otherwise filter that remote entry, rather than silently dropping the mapping.
Useful? React with 👍 / 👎.
Signed-off-by: Rello <Rello@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b05f17b43a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| metadata->addEncryptedFile(encryptedFile); | ||
| if (!metadata->addEncryptedFile(encryptedFile)) { | ||
| qCWarning(lcPropagateUploadEncrypted()) << "There was an error encrypting the file, aborting upload. Invalid metadata file name."; | ||
| emit error(); |
There was a problem hiding this comment.
When this new rejection is reached for an encrypted directory upload, such as a POSIX folder named foo\bar inside an E2EE parent, the only consumer in PropagateRemoteMkdir::slotMkdir() connects PropagateUploadEncrypted::error to a logging-only lambda (src/libsync/propagateremotemkdir.cpp:212), unlike file uploads which call done(). Emitting error() here therefore leaves the mkdir job in _activeJobList with no MKCOL or completion signal, so the sync can stall instead of failing the item; make the mkdir path finish/cleanup on this failure.
Useful? React with 👍 / 👎.
|
Artifact containing the AppImage: nextcloud-appimage-pr-10238.zip Digest: To test this change/fix you can download the above artifact file, unzip it, and run it. Please make sure to quit your existing Nextcloud app and backup your data. |
|
2 participants
Make FolderMetadata enforce that every decrypted E2EE filename is a single path segment before it can enter discovery.