Encrypt attachments in encrypted rooms, #533

NegativeMjark · 2016-11-02T16:30:17Z

TODO:

Decrypt image attachments when displaying them.
Do something when we get an error trying to decrypt.
Decrypt m.video
Decrypt m.audio
Decrypt m.file
Allow people to download encrypted attachments.
~~Thumbnail image attachments before sending.~~
~~Thumbnail video attachments before sending.~~

…n displaying them

matrixbot · 2016-11-02T16:30:23Z

Can one of the admins verify this patch?

dbkr · 2016-11-03T11:53:36Z

@matrixbot test this please

dbkr

The main thing here is probably the fact that it doesn't work with the gif hover behaviour, but I'm not about to ask you to rewrite our react code to support this. Maybe one of us can fix this on the PR branch.

dbkr · 2016-11-03T11:57:40Z

src/components/views/messages/MImageBody.js

+        }
+    },
+
+    decryptFile: function(file) {


I don't love the fact that a function called decryptFile downloads and decrypts a file.

...and also then displays it.

dbkr · 2016-11-03T13:11:04Z

src/components/views/messages/MImageBody.js

+
+    decryptFile: function(file) {
+        var url = MatrixClientPeg.get().mxcUrlToHttp(file.url);
+        var self = this;


We tend to use arrow functions rather than the self = this hack nowadays

dbkr · 2016-11-03T13:12:23Z

src/components/views/messages/MImageBody.js

+            // The attachment is decrypted in componentDidMount.
+            return (
+                <span className="mx_MImageBody" ref="body">
+                <img className="mx_MImageBody_thumbnail" src="img/encrypted-placeholder.svg" ref="image"


Where does the placeholder come from?

dbkr · 2016-11-03T13:23:15Z

src/components/views/messages/MImageBody.js

+            self.refs.image.onload = function() {
+                window.URL.revokeObjectURL(blobUrl);
+            };
+        });


Yeah, this post-render DOM mangling isn't great, but we do appear to do it for animated gifs anyway. Relatedly, this will mean the current play-on-hover behaviour of animated gifs won't work with encrypted attachments, so this is a thing we'll need to fix too.

…matrix-react-sdk into markjh/encrypted-attachments

… so that it can be used normally by the exising templates

…ween different components

NegativeMjark · 2016-11-04T14:02:26Z

Thumb-nailing in the browser sounds hard and I think this PR is big enough already.
@dbkr PTAL.

…tachments are actually downloaded

…rypt it itself.

NegativeMjark · 2016-11-07T13:41:32Z

@dbkr Right I think this is done PTAL

dbkr

This is looking nice. Have made some language comments. Maybe a warning somewhere about it downloading full content of everything currently, until thumbnails are sorted out?

dbkr · 2016-11-07T14:40:32Z

src/utils/DecryptFile.js

+limitations under the License.
+*/
+
+'use struct';


It's spelt 'strict'

We don't need these anyway: babel adds them.

dbkr · 2016-11-07T14:41:31Z

src/utils/DecryptFile.js

+'use struct';
+
+// Pull in the encryption lib so that we can decrypt attachments.
+var encrypt = require("browser-encrypt-attachment");


If you're making a new file, import encrypt from 'browser-encrypt-attachment'; please.

dbkr · 2016-11-07T14:43:46Z

src/utils/DecryptFile.js

+ */
+
+function readBlobAsDataUri(file) {
+    var deferred = q.defer();


const please

dbkr · 2016-11-07T14:43:58Z

src/utils/DecryptFile.js

+
+
+export function decryptFile(file) {
+    var url = MatrixClientPeg.get().mxcUrlToHttp(file.url);


more const

dbkr · 2016-11-07T14:45:38Z

src/components/views/messages/MImageBody.js

    },

    componentDidMount: function() {
        this.dispatcherRef = dis.register(this.onAction);
        this.fixupHeight();
+        var content = this.props.mxEvent.getContent();


dbkr · 2016-11-07T14:47:00Z

src/components/views/messages/MImageBody.js

@@ -77,14 +86,41 @@ module.exports = React.createClass({
        imgElement.src = this._getThumbUrl();
    },

+    _getContentUrl: function() {
+        var content = this.props.mxEvent.getContent();


const again

dbkr · 2016-11-07T14:53:28Z

src/components/views/messages/MImageBody.js

 var MatrixClientPeg = require('../../../MatrixClientPeg');
 var ImageUtils = require('../../../ImageUtils');
 var Modal = require('../../../Modal');
 var sdk = require('../../../index');
 var dis = require("../../../dispatcher");
+var DecryptFile = require('../../../utils/DecryptFile');


Generally we would follow convention within the file for require vs import, or migrate the whole file. Probably decide which though rather than adding both. ;)

dbkr · 2016-11-07T14:54:01Z

src/components/views/messages/MFileBody.js

+            );
+        }
+
+        var contentUrl = this._getContentUrl();


more consts please

dbkr · 2016-11-07T14:56:00Z

src/components/views/messages/MFileBody.js

+        var content = this.props.mxEvent.getContent();
+        var self = this;
+        if (content.file !== undefined && this.state.decryptedUrl === null) {
+            DecryptFile.decryptFile(content.file).then(function(url) {


s/function(x)/(x) => / then you can ditch the var self = this and just use this.

dbkr · 2016-11-07T14:57:38Z

src/components/views/messages/MFileBody.js

@@ -47,22 +54,89 @@ module.exports = React.createClass({
        return linkText;
    },

+    _getContentUrl: function() {
+        var content = this.props.mxEvent.getContent();


NegativeMjark · 2016-11-08T16:32:45Z

I think I've responded to the review comments @dbkr PTAL.

Unfortunately I've discovered that the data URLs don't work reliably on chrome if the URL is bigger than 2MB. https://bugs.chromium.org/p/chromium/issues/detail?id=69227.

I have a plan for getting around it, but for now I guess this "sort of works"? It certainly should be enough for people to test against with small attachments.

dbkr

Thank you!

#533 originally landed in the wrong branch, and was reverted by #546. #548 attempted to land it on the develop branch, but omitted a small amount of the patch. This lands the final part, which got missed out.

Encrypt attachments in encrypted rooms, decrypt image attachments whe…

e0cea74

…n displaying them

NegativeMjark assigned dbkr Nov 2, 2016

dbkr reviewed Nov 3, 2016

View reviewed changes

dbkr and others added 11 commits November 3, 2016 15:53

Actually add isomorphic-fetch

4630172

Handle decryption errors

e949d91

Merge branch 'markjh/encrypted-attachments' of github.com:matrix-org/…

d61714a

…matrix-react-sdk into markjh/encrypted-attachments

The variable is called roomId here

842b5ae

Include the mimetype with the file info. Store the objectURL in state…

12fc70c

… so that it can be used normally by the exising templates

Move decryptFile into a utility function so that it can be shared bet…

b6653dd

…ween different components

Decypt m.video events

48cfd4f

Remove spurious TODO

71c002d

Fix unencrypted video thumbnail

9112343

Decrypt m.audio attachments

94bfe31

Decrypt m.file attachments

6ea0085

NegativeMjark added 4 commits November 4, 2016 15:39

Use data:// URI rather than blob: URI to avoid XSS

8f778f5

Make everything use MFileBody for downloads, ensure that encrypted at…

1529396

…tachments are actually downloaded

Pass the decryptedUrl to the MFileBody so that it doesn't need to dec…

4214c67

…rypt it itself.

Fix gif hoverover

dc679a8

dbkr suggested changes Nov 7, 2016

View reviewed changes

NegativeMjark added 3 commits November 8, 2016 11:42

Review comments

911f9e4

Make imports more consistent, fix m.video

93ddb55

Make the promises be q promises

c838164

dbkr approved these changes Nov 9, 2016

View reviewed changes

dbkr merged commit 3e9414b into master Nov 9, 2016

dbkr mentioned this pull request Nov 11, 2016

Revert "Encrypt attachments in encrypted rooms," #546

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encrypt attachments in encrypted rooms, #533

Encrypt attachments in encrypted rooms, #533

NegativeMjark commented Nov 2, 2016 •

edited

Loading

matrixbot commented Nov 2, 2016

dbkr commented Nov 3, 2016

dbkr left a comment

dbkr Nov 3, 2016

dbkr Nov 3, 2016

dbkr Nov 3, 2016

dbkr Nov 3, 2016

dbkr Nov 3, 2016

NegativeMjark commented Nov 4, 2016

NegativeMjark commented Nov 7, 2016

dbkr left a comment

dbkr Nov 7, 2016

dbkr Nov 7, 2016

dbkr Nov 7, 2016

dbkr Nov 7, 2016

dbkr Nov 7, 2016

dbkr Nov 7, 2016

dbkr Nov 7, 2016

dbkr Nov 7, 2016

dbkr Nov 7, 2016

dbkr Nov 7, 2016

NegativeMjark commented Nov 8, 2016

dbkr left a comment



		export function decryptFile(file) {
		var url = MatrixClientPeg.get().mxcUrlToHttp(file.url);

Encrypt attachments in encrypted rooms, #533

Encrypt attachments in encrypted rooms, #533

Conversation

NegativeMjark commented Nov 2, 2016 • edited Loading

matrixbot commented Nov 2, 2016

dbkr commented Nov 3, 2016

dbkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NegativeMjark commented Nov 4, 2016

NegativeMjark commented Nov 7, 2016

dbkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NegativeMjark commented Nov 8, 2016

dbkr left a comment

Choose a reason for hiding this comment

NegativeMjark commented Nov 2, 2016 •

edited

Loading