Fix: Improve sanitization of folder and file names

[AhsanIsEpic]

This pull request fixes #208, where folders with a space at the end of their name cause the Google Drive import to fail.

I’m not very experienced with PHP, please review thoroughly

[AhsanIsEpic]

Could I get approval for workflows?

[AhsanIsEpic]

I’ve tested the code with a couple of scenarios and it works perfectly:

1. Verified that Google Drive migration completes successfully renaming invalid file names

However, I think I may have found a potential design issue:

The code is inconsistent on how file names are sanitised
All none letter and numbers in file names from Google Drive are replaced with hyphens (-) whereas folder name sanitation does not exist. This is not the same with the Google Photos API where only backslashes are replaced for album and photo names.

[AhsanIsEpic]

Updated the code be consistent and match the sanitation in nextcloud/server#27891

[marcelklehr]

Hello @AhsanIsEpic
Thank you for your work on this PR!
Mmmh, I'm not sure if the issue you referenced is authoritative. Perhaps we should use OCP\Files\IFilenameValidator instead here?

[AhsanIsEpic]

Hello @AhsanIsEpic Thank you for your work on this PR! Mmmh, I'm not sure if the issue you referenced is authoritative. Perhaps we should use OCP\Files\IFilenameValidator instead here?

I ended up creating a shared utility function to handle all the possible errors that can be thrown from IFilenameValidator. I could of just replaced the filename with the file ID however, I wanted to keep the filename intact as much as possible.

Introduction of Utility Function:

• lib/Service/Utils/FileUtils.php: Added a new utility class FileUtils with a static method sanitizeFilename to sanitize and validate filenames.

Integration of Utility Function:

• lib/Service/GoogleDriveAPIService.php:
  • Added FileUtils import.
  • Updated createDirsUnder method to use FileUtils::sanitizeFilename for directory names.
  • Updated getFileName method to use FileUtils::sanitizeFilename for file names.
• lib/Service/GooglePhotosAPIService.php:
  • Added FileUtils import.
  • Updated importPhotos method to use FileUtils::sanitizeFilename for album names.
  • Updated getPhoto method to use FileUtils::sanitizeFilename for photo names.

[marcelklehr]

Wow, this is a really elaborate. Thank you for the effort you put into this! I was actually thinking more of something like this: https://github.com/nextcloud/server/pull/51608/files#diff-911ea9939fad17c78ada50c38706874091e2478e37a2ef5a155481c0c4a81a98R144-R165 But it turns out the methods used there are not in OCP :(

[marcelklehr]

I'm inquiring now if we can get a proper sanitization method in OCP to avoid parsing error messages from the validator, which seems a bit brittle

[marcelklehr]

Here we go: nextcloud/server#52688

[marcelklehr]

Nextcloud 32 will have that method in OCP

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

[marcelklehr]

Are you still interested in finishing this PR?

[AhsanIsEpic]

Are you still interested in finishing this PR?

I'm still interested in finishing this PR, however I'm unsure if I should be waiting for nextcloud 32 to be released before completing this MR or would it be fine to use the development version?

[marcelklehr]

We can implement this against nextcloud 32 and release it when nextcloud 32 is released, excluding support for nextcloud < 32

[marcelklehr]

Optionally, we can also check if the method exists in the class and only use it if it exists. That would allow to support versions < 32

[AhsanIsEpic]

Optionally, we can also check if the method exists in the class and only use it if it exists. That would allow to support versions < 32

I've added a few lines to check if the OCP version is 32 or higher, and use the built-in validator in that case (still need to run some tests on it).

Would you prefer that I remove the sanitation logic I previously added and revert to the way they were originally for older versions, or should we keep it?

[marcelklehr]

Looks good!

[marcelklehr]

Thank you so much for your contribution @AhsanIsEpic

[marcelklehr]

Hi @AhsanIsEpic !
Thank you again for taking the time to contribute to Nextcloud! There is a community talk instance with a room for the integrations team that I'm part of. I'd like to invite you to join this room. You can send me a message to marcel.klehr@nextcloud.com if you are interested, I will add you. 💙

Cheers
Marcel

[AhsanIsEpic]

Sent an email across @marcelklehr