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

Use axios and more safety around file uploads #466

Merged
merged 6 commits into from
Aug 17, 2020
Merged

Conversation

Half-Shot
Copy link
Contributor

Hopefully fixes #465

@Half-Shot Half-Shot requested a review from a team August 14, 2020 08:49
log.error("We have no client (or token) that can handle this file, sending as link");
sendAsLink = true;
} else if (maxUploadSize && file.size > maxUploadSize) {
log.warn(`File size too large (${file.size / 1024}KB > ${maxUploadSize / 1024} KB)`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KiB when dividing by 1024.

Suggested change
log.warn(`File size too large (${file.size / 1024}KB > ${maxUploadSize / 1024} KB)`);
log.warn(`File size too large (${file.size / 1024}KiB > ${maxUploadSize / 1024} KiB)`);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 i get those mixed up

@@ -676,12 +676,36 @@ export class BridgedRoom {
return;
}

let sendAsLink = false;
const authToken = this.SlackClient?.token;
Copy link
Contributor

@jaller94 jaller94 Aug 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to use client or why did you add that seemingly unused parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the param was used originally then removed. Removing it now.

Copy link
Contributor

@jaller94 jaller94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@@ -0,0 +1 @@
Don't automatically join new users to all public channels
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#463 is not this PR but #463 added changelog.d/431.bugfix... okaaaaaayyyy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think someone (me) might have left out a file from another PR :s

@Half-Shot Half-Shot merged commit f0b4a09 into develop Aug 17, 2020
@williamkray
Copy link

i've just deployed the docker container including this fix and now all image uploads from slack are corrupt in matrix.

when downloading and opening the image from matrix (e.g. https://synapse.jobmachine.org/_matrix/media/r0/download/jobmachine.org/pSdoKVhARBgOFICbhfvGrjLk and https://synapse.jobmachine.org/_matrix/media/r0/download/jobmachine.org/iDKjBHZSrMGwXPTAHDYTkRFR are a few examples from my testing) the file either just says "Error serving file", or shows that it is an image data file, but no image programs can open it properly (chromium, GIMP, feh, etc). GIMP says "Not a JPEG file: starts with 0xef 0xbf"

this testing includes examples where i both waited for the image processing dialogs to finish in slack before sending to the channel, and tests where i did not. i've used several images of varying sizes, but i haven't had a single one come through to matrix yet.

@jaller94 jaller94 deleted the hs/image-uploads branch August 17, 2020 21:18
@Half-Shot
Copy link
Contributor Author

i've just deployed the docker container including this fix and now all image uploads from slack are corrupt in matrix.

Sad :c. I'll take another look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When bridging files to Matrix from Slack, some requests can fail authentication and upload a slack login page
3 participants