fix to reassign original_filename on store

[rajyan]

Trying to fix https://github.com/carrierwaveuploader/carrierwave/issues/2708

[rajyan]

Still thinking about what original_filename is expected to be ex. https://github.com/carrierwaveuploader/carrierwave/issues/1898 https://github.com/carrierwaveuploader/carrierwave/issues/1835

Also,

diff --git a/lib/carrierwave/uploader/cache.rb b/lib/carrierwave/uploader/cache.rb
index 28f2f77..91e59c5 100644
--- a/lib/carrierwave/uploader/cache.rb
+++ b/lib/carrierwave/uploader/cache.rb
@@ -132,8 +132,8 @@ module CarrierWave

         @identifier = nil
         @staged = true
+        self.original_filename = new_file.original_filename
         @filename = new_file.filename
-        self.original_filename = new_file.filename

this might be better?

[mshibuya]

Simply put, @original_filename is the filename used for storing cache files. I admit that it is named misleadingly, but it's not easy to change it because it was named like that in the first place...

That's why it's safeguarded not to contain unsafe characters. Cache files need to be stored in the filesystem safely. So you just can't use SanitizedFile#original_filename that will return unsanitized original filename.

[rajyan]

Thanks, I think I've might have started to understand some part of original_filename from your explanation. Before cache! in like RemoteFile#original_filename or SantizedFile#original_filename it stands for the original filename. After cache (in uploader) it's just a cache name.

[rajyan]

reference https://github.com/carrierwaveuploader/carrierwave/commit/a2e8a58eac3fb1d07c9487f086c73d185b884a82

[rajyan]

@mshibuya

After examining the history of original_filename, I'm now confident enough this change should work.

https://github.com/carrierwaveuploader/carrierwave/commit/bea5f998209a4ce110cb2caa08f004f1842420d3

original_filename in context of cache, is a identifier for the cache file. identifier is a identifier to retrieve from store.

After store! these can be same. I think assigning nil might be more a correct value for the current meaning of original_filename, but this change should work (and keeps the behavior before https://github.com/carrierwaveuploader/carrierwave/pull/2707), because whether the file is cached? is only handled by @cache_id in all places.

https://github.com/carrierwaveuploader/carrierwave/blob/b1dbd7c82f7c1740f18e2bf964dafb9462db1c23/lib/carrierwave/uploader/cache.rb#L78-L80

[ryan-mcneil]

@mshibuya Hi - I was curious if you intend on approving/merging this "fix", or not as it was not the intended behavior. I'm just wondering if I need to adapt my code to this new behavior (currently pinned to 3.0.3 despite CVE), or if this update will be available soon. Thank you!

[mshibuya]

@ryan-mcneil Thanks for the comment. Then could you answer this question? https://github.com/carrierwaveuploader/carrierwave/issues/2708#issuecomment-1823931120 It is unlikely to merge this PR, as setting what is not the original filename to @original_filename is just misleading. But if there's a very good reason to do so, the situation can change.

[ryan-mcneil]

Then could you answer this question?

@mshibuya I suppose I don't have a better reason than what has been presented by some of the other engineers on these threads. I'll dig in further while I try to adapt to the new version, and respond again if I come up with something more convincing.

I will side with the sentiment argued by others though. Whether or not the behavior was intended/per the specification, the update did in fact introduce breaking changes for some (many?) users facing a CVE, and likely deserved more than a patch version. Perhaps a 3.0.6 could satisfy these requests, and a 4.X could re-introduce these changes to better align with your intended use case?

Either way, thanks for any consideration, and thanks for doing what you're doing!

[mshibuya]

@ryan-mcneil You should have told me that your project is open source. I've opened a draft PR which fixes the specs, at least locally. Please take a look. https://github.com/department-of-veterans-affairs/vets-api/pull/15118

[mshibuya]

Closing as #2718 will alleviate this.