TextEditor: Checksum not invalidated on file change #23782

Closed
dragotin opened this Issue Apr 4, 2016 · 15 comments

Projects

None yet

5 participants

@dragotin
Member
dragotin commented Apr 4, 2016

Steps to reproduce

  1. Create a text file with the sync client
  2. Wait until it is synced.
  3. Check that it has a checksum in the database, which was sent from the client.
  4. Edit the file from the built in texteditor, let it save.
  5. See that the checksum of the file has not changed.

Expected behaviour

When a file is changed, the checksum on the server needs to be recalculated. If that can not be done, it needs at least to be invalidated.

Having a wrong checksum for a file is a critical problem as the client will complain about a corrupted file.

Server configuration

Operating system:
Linux
Web server:
apache
Database:
mysql
PHP version:
5.5.14
ownCloud version: (see ownCloud admin page)
9.0.x, master branch, 15833d9
Updated from an older ownCloud or fresh install:

Where did you install ownCloud from:
git
Are you using external storage, if yes which one: local/smb/sftp/...
no external storage
Are you using encryption: yes/no
no encryption
Are you using an external user-backend, if yes which one: LDAP/ActiveDirectory/Webdav/...
no user-backend.

@MorrisJobke
Member

sev3 because there is not yet any client out there that uses the checksums.

@MorrisJobke MorrisJobke added this to the 9.1-current milestone Apr 4, 2016
@rullzer
Contributor
rullzer commented Apr 4, 2016

mmmm I don't get this. Doesn't the texteditor use dav?

@rullzer
Contributor
rullzer commented Apr 4, 2016

Ah no it seems it uses internal calls. So when we modify a file we must just reset the checksum to the empty string. @icewind1991 can you help with that?

I think it only has to be handled in the view.php::file_put_contents function right?

@rullzer rullzer added sev2-high and removed sev3-medium labels Apr 4, 2016
@rullzer
Contributor
rullzer commented Apr 4, 2016

Raised to sev2 as this has to be fixed for 9.0.2 from my POV. @cmonteroluque

@dragotin
Member
dragotin commented Apr 4, 2016

@MorrisJobke ...exept the few desktop clients (version 2.1.1) that some enthusiasts might use ;-). The client uses the checksum to validate the download from the server by comparing the server stored checksum to one that the client calculates after download.

With this bug, the client will refuse the download of the file once it was saved by the text editor because the checksums do not match. I wonder which other apps use "internal calls" to store data and have the same problem.

Well, morris, the fix is easy once the users call because of this: Just update the filecache and set all checksums to empty. 😢

@MorrisJobke
Member

exept the few desktop clients (version 2.1.1) that some enthusiasts might use ;-)

Oh. Really? I thought it will be present in the next major release 🙈 Sorry

@MorrisJobke
Member

Raised to sev2 as this has to be fixed for 9.0.2 from my POV. @cmonteroluque

Yep.

@cmonteroluque
Contributor

I agree

@PVince81 PVince81 added sev1-critical and removed sev2-high labels Apr 19, 2016
@rullzer
Contributor
rullzer commented Apr 19, 2016

Ok let me dive into this.

@rullzer rullzer self-assigned this Apr 19, 2016
@rullzer
Contributor
rullzer commented Apr 19, 2016

Ok there are actually a few scenarios here.

There is the scenario where somebody calls file_put_content. I have that fixed locally I think. We then just clear the checksum.

But what to do if somebody calls fopen. I think it is safest just to also clear the checksum if the file is opened with write. We may look into the future into a wrapper or something that only clears is actually something is writen/deleted. But for now clearing seems best. CC: @PVince81 @dragotin

@rullzer rullzer added a commit that referenced this issue Apr 19, 2016
@rullzer rullzer When calling file_put_contents clear the checksum
Part 1 of #23782

If we overwrite a file we should clear the stored checksum as it is no
longer valid.

* Unit test added
8224fd8
@rullzer
Contributor
rullzer commented Apr 19, 2016

PR 1 of 2 in #24098

@PVince81
Collaborator

@rullzer yes, I guess you meant only for fopen in one of the write modes, not read.
You might also be able to do it at close time, see https://github.com/owncloud/core/blob/v9.0.1/lib/private/files/view.php#L1092 (this code is run after the file was closed).

It's probably best to clear the checksum in one of the existing database calls instead of adding new ones though (which makes the "close time" approach less interesting)

@rullzer
Contributor
rullzer commented Apr 19, 2016

Yes I agree. But the current calls don't allow me to easily inject it there.. without doing it all the time. For example on move and copy we don't want to clear the checksum etc.

@rullzer rullzer added a commit that referenced this issue Apr 19, 2016
@rullzer rullzer When the scanner detects a file is changed clear checksum
Fixes #23782 and #23783

If the file scanner detects a changed file we clear the checksum while
we update the cache.

* Unit test added
9e3c146
@rullzer rullzer added a commit that referenced this issue Apr 19, 2016
@rullzer rullzer When the scanner detects a file is changed clear checksum
Fixes #23782 and #23783

If the file scanner detects a changed file we clear the checksum while
we update the cache.

* Unit test added
0dbcbc4
@rullzer
Contributor
rullzer commented Apr 19, 2016

PR that fixes both cases in #24098 now

@rullzer rullzer added a commit that referenced this issue Apr 20, 2016
@rullzer rullzer When the scanner detects a file is changed clear checksum
Fixes #23782 and #23783

If the file scanner detects a changed file we clear the checksum while
we update the cache.

* Unit test added
2296552
@rullzer
Contributor
rullzer commented Apr 20, 2016

Stable 9 backport in #24129

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment