Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy into local file #22604

Merged
merged 2 commits into from
Feb 25, 2016
Merged

Copy into local file #22604

merged 2 commits into from
Feb 25, 2016

Conversation

LukasReschke
Copy link
Member

Using the Guzzle stream directly here will only return 1739 characters for fread instead of all data. This leads to the problem that the stream is read incorrectly and thus the data cannot be properly decrypted => 💣
(see #22590 (comment) for more context)

This approach copies the data into a local temporary file, as done before in all stable releases as well as other storage connectors.

While this approach will load the whole file into memory, this is already was has happened before in any stable release as well. See d608c37 for the breaking change. I believe for 9.0 we should keep it like that.

To test this enable Google Drive as external storage and upload some files with encryption enabled. Reading the file should fail now.

Fixes #22590


@PVince81 please test.
@icewind1991 Other ideas?

@LukasReschke
Copy link
Member Author

Alternatively we need to get somehow a proper stream there that will be read properly 🙊

@LukasReschke
Copy link
Member Author

@icewind1991 Do you know why the streaming stuff behaves wrongly as described at #22590 (comment)? You added that code at #18653

@PVince81
Copy link
Contributor

@LukasReschke are you sure this fills memory ? I don't think so, it would just stream copy the contents to the target. If in doubt you can also use the Util class's steamCopy to copy the contents to a temp file first.

Not sure if related, but the stream wrapper code lies about being seekable, see #22476. We could try fixing the stream wrapper first.

If that doesn't work, then we'll merge @LukasReschke's fix which is still better than the original code from v8.2.2.

@LukasReschke
Copy link
Member Author

@LukasReschke are you sure this fills memory ? I don't think so, it would just stream copy the contents to the target.

stream_get_contents here returns a string. So that would load it into memory from what I can tell.

If in doubt you can also use the Util class's steamCopy to copy the contents to a temp file first.

Somehow I don't manage to get this working here 🙈

@LukasReschke
Copy link
Member Author

While wanting to test this against the DAV backend, it turns out: This one is at the moment completely broken, at least against other ownClouds.

#22605

@PVince81
Copy link
Contributor

Somehow I don't manage to get this working here

If you say streamCopy didn't work, maybe it's because it also uses fread and bumps into the same as the original issue ?

@LukasReschke
Copy link
Member Author

If you say streamCopy didn't work, maybe it's because it also uses fread and bumps into the same as the original issue ?

👍

@PVince81
Copy link
Contributor

CC @schiesbn

@PVince81
Copy link
Contributor

Use sink option to pipe response into file, see http://docs.guzzlephp.org/en/latest/request-options.html

@LukasReschke
Copy link
Member Author

Use sink option to pipe response into file, see http://docs.guzzlephp.org/en/latest/request-options.html

Good point. We can't use sink directly here since that does not exist in our version of Guzzle but save_to.

Using the Guzzle stream directly here will only return 1739 characters for `fread` instead of all data. This leads to the problem that the stream is read incorrectly and thus the data cannot be properly decrypted => 💣

This approach copies the data into a local temporary file, as done before in all stable releases as well as other storage connectors.

While this approach will load the whole file into memory, this is already was has happened before in any stable release as well. See d608c37 for the breaking change.

To test this enable Google Drive as external storage and upload some files with encryption enabled. Reading the file should fail now.

Fixes #22590
@LukasReschke
Copy link
Member Author

Adjusted.

@LukasReschke LukasReschke force-pushed the fix-google-drive-encryption branch from 44f4935 to 3b5ddb4 Compare February 25, 2016 08:41
@PVince81
Copy link
Contributor

Doesn't seem to work:

{"reqId":"h+QP6SOPjl5LxrDqhpHv","remoteAddr":"192.168.1.2","app":"core","message":"Generating preview for \"\/gdrive\/20110610_005.jpg\" with \"OC\\Preview\\JPEG\"","level":0,"time":"2016-02-25T09:02:30+00:00","method":"GET","url":"\/owncloud\/index.php\/core\/preview.png?file=%2Fgdrive%2F20110610_005.jpg&c=56cec324010a2&x=32&y=32&forceIcon=0"}
{"reqId":"h+QP6SOPjl5LxrDqhpHv","remoteAddr":"192.168.1.2","app":"PHP","message":"Call to a member function getStatusCode() on a non-object at \/srv\/www\/htdocs\/owncloud\/apps\/files_external\/lib\/google.php#448","level":3,"time":"2016-02-25T09:02:30+00:00","method":"GET","url":"\/owncloud\/index.php\/core\/preview.png?file=%2Fgdrive%2F20110610_005.jpg&c=56cec324010a2&x=32&y=32&forceIcon=0"}

@LukasReschke
Copy link
Member Author

{"reqId":"h+QP6SOPjl5LxrDqhpHv","remoteAddr":"192.168.1.2","app":"core","message":"Generating preview for "/gdrive/20110610_005.jpg" with "OC\Preview\JPEG"","level":0,"time":"2016-02-25T09:02:30+00:00","method":"GET","url":"/owncloud/index.php/core/preview.png?file=%2Fgdrive%2F20110610_005.jpg&c=56cec324010a2&x=32&y=32&forceIcon=0"}
{"reqId":"h+QP6SOPjl5LxrDqhpHv","remoteAddr":"192.168.1.2","app":"PHP","message":"Call to a member function getStatusCode() on a non-object at /srv/www/htdocs/owncloud/apps/files_external/lib/google.php#448","level":3,"time":"2016-02-25T09:02:30+00:00","method":"GET","url":"/owncloud/index.php/core/preview.png?file=%2Fgdrive%2F20110610_005.jpg&c=56cec324010a2&x=32&y=32&forceIcon=0"}

That's actually unrelated and caused by the fact that I defined the proper use statement. The exception code path was NEVER called before. Let me add some more debugging code to this.

@PVince81
Copy link
Contributor

Works now 👍

@PVince81
Copy link
Contributor

Second review please @icewind1991 @Xenopathic @davitol

@icewind1991
Copy link
Contributor

Code looks good 👍

@davitol
Copy link
Contributor

davitol commented Feb 25, 2016

WFM 👍

@davitol davitol added the tested label Feb 25, 2016
DeepDiver1975 added a commit that referenced this pull request Feb 25, 2016
@DeepDiver1975 DeepDiver1975 merged commit 202bf17 into master Feb 25, 2016
@DeepDiver1975 DeepDiver1975 deleted the fix-google-drive-encryption branch February 25, 2016 13:35
@lock
Copy link

lock bot commented Aug 7, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing signature with encryption on GDrive
6 participants