fix incorrect encoding of filenames with spaces in #2090

ara4n · 2017-04-01T02:43:26Z

ara4n · 2017-04-01T02:46:20Z

this PR sponsored by http://stackoverflow.com/questions/93551/how-to-encode-the-filename-parameter-of-content-disposition-header-in-http/216777#216777

btw, it looks like this never particularly deliberately special-cased non-utf8; when Mjark added utf8 support in #259 he just left the original code path alone, assuming it worked.

ara4n · 2017-04-01T08:41:36Z

bah, guess i need to fix the tests. will wait for PR review first

ara4n · 2017-04-01T19:44:21Z

matrix-org/sytest#350 should cheer up sytest.

erikjohnston · 2017-04-03T08:46:51Z

Given the compatibility problems I wonder if it would be a good idea to only use the utf8 foo when we need it?

Also, I wonder if we should start giving out the url with the filename at the end: e.g. https://jki.re/_matrix/media/v1/download/matrix.org/zFHVTzeUHJnsbomaeHEehfSI/matthew.jpg

ara4n · 2017-04-11T11:23:09Z

i don't think any modern browsers have compatibility problems with the utf-8 encoding? or is there something specific i'm missing?

is your proposal to only use the utf-8 encoding if the filename includes spaces, then? this feels magical and fragile; we don't want it randomly breaking in weird ways on files with spaces - they surely shouldn't be special-cased at all.

Agreed that it might be nicer to switch to putting the filename on the end of the URL as the most reliable solution, but i think this one actually works?

ara4n · 2017-04-19T10:56:36Z

this is getting really annoying, my downloads folder is filling up with files with beautiful names like 2017-04-12.1%20Vector%20Stuff%20Whatever.pptx :(

richvdh · 2017-09-05T17:16:29Z

@matrixbot retest this please

synapse/rest/media/v1/_base.py

ara4n · 2018-08-15T22:03:38Z

empirically my Downloads folder is no longer filling up with 2017-04-12.1%20Vector%20Stuff%20Whatever.pptx, but also this never got merged. any idea what changed?

richvdh · 2019-02-27T13:30:30Z

empirically my Downloads folder is no longer filling up with 2017-04-12.1%20Vector%20Stuff%20Whatever.pptx, but also this never got merged. any idea what changed?

I suspect you've updated to a more recent version of Chrome, which (incorrectly, afaict) decodes the %-encoding in the filename. This is still a bug on Firefox, at least as of 62.0

richvdh · 2019-02-27T13:40:37Z

for the record: RFC6266 defines both filename and filename*.

filename is defined to be a value, which is defined by RFC2616 to be a token or a quoted-string, where a token is (essentially) a single US-ASCII word, and a quoted-string is a US-ASCII string surrounded by double-quotes, using backslash as an escape charater. Note that %-encoding is not permitted.

filename* is defined to be an ext-value, which is defined in RFC5987 to be charset "'" [ language ] "'" value-chars, where value-chars is essentially a %-encoded string in the given charset.

So the fix in this PR is correct inasmuch as it means we always use the filename* variant, where %-encoding is permitted.

richvdh · 2019-02-27T13:43:44Z

however, I am minded to think that we should instead fix the escaping used in the filename parameter, for maximum compatibility.

codecov · 2019-02-27T23:10:51Z

Codecov Report

Merging #2090 into develop will decrease coverage by 16.5%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##           develop    #2090       +/-   ##
============================================
- Coverage    75.08%   58.58%   -16.51%     
============================================
  Files          340      324       -16     
  Lines        34895    33769     -1126     
  Branches      5717     5576      -141     
============================================
- Hits         26201    19782     -6419     
- Misses        7080    12609     +5529     
+ Partials      1614     1378      -236

…me-escaping

richvdh · 2019-02-27T23:15:46Z

I've substantially rewritten this, so a sanity-check from someone else on the team would be appreciated. I suggest just looking at the diff, which should speak for itself, rather than trying to pick your way through the history.

Tests at matrix-org/sytest#350.

ara4n · 2019-02-27T23:32:51Z

synapse/rest/media/v1/_base.py

+        # [2]: https://tools.ietf.org/html/rfc2616#section-3.6
+        # [3]: https://tools.ietf.org/html/rfc5987#section-3.2.1
+
+        # We avoid the quoted-string version of filename, because (a) synapse didn't


i'd suggest saying `filename` here, to make it clear you're talking about `filename` rather than `filename*`

point, done

ara4n · 2019-02-27T23:33:23Z

synapse/rest/media/v1/_base.py

+        # correctly interpret those as of 0.99.2 and (b) they are a bit of a pain and we
+        # may as well just do the filename* version.
+        if _can_encode_filename_as_token(upload_name):
+            disposition = 'inline; filename=%s' % (upload_name, )


why not do filename* everywhere?

the main thinking is that some things won't have good support for filename, and we should cater to simple clients where possible.

you mean good support for filename*?

okay, fair enough.

sorry, yes that's what I mean.

…server_specific_tests * 'develop' of github.com:matrix-org/sytest: (44 commits) test endpoint for updating backup versions (#559) Add test for transferring bans on a room upgrade (#563) fix tests for matrix-org/synapse#2090 (#350) Fix typo Fixup Incorporate review Remove prev_state from federation API Make the userdir synced not rely on being able to search for yourself (#567) Add basic soft fail test Check that event_id is given over state fetching over federation (#566) Fix registration rate limiting settings Fix comments Make sytest support auth rate limiting Fixup to not send 100 messages Test history visibility works for backfill Check that Server ACLs are preserved on room upgrade Better diagnostics from synapse startup (#561) Don't set PYTHONPATH when running synapse (#560) Regression test for redactions in room v3 (#558) Add test for PDU limit on transactions API (#555) ...

always treat repo filenames as utf-8

d833855

fixing element-hq/element-web#3155

better commenting.

70c8861

btw, it looks like this never particularly deliberately special-cased non-utf8; when Mjark added utf8 support in #259 he just left the original code path alone, assuming it worked.

ara4n added a commit to matrix-org/sytest that referenced this pull request Apr 1, 2017

fix tests for matrix-org/synapse#2090

b5555cb

ara4n assigned erikjohnston Apr 11, 2017

erikjohnston force-pushed the develop branch from 2af75da to 47574c9 Compare July 4, 2017 09:20

richvdh reviewed Sep 5, 2017

View reviewed changes

synapse/rest/media/v1/_base.py Outdated Show resolved Hide resolved

erikjohnston assigned ara4n and unassigned erikjohnston May 3, 2018

neilisfragile assigned richvdh Feb 14, 2019

richvdh added 3 commits February 27, 2019 15:43

Merge branch 'develop' into matthew/fix-filename-escaping

10131f7

Update to use the ascii filename where possible

918920a

changelog

c758d36

Merge remote-tracking branch 'origin/develop' into matthew/fix-filena…

d1967f8

…me-escaping

richvdh changed the title ~~always treat repo filenames as utf-8~~ fix incorrect encoding of filenames with spaces in Feb 27, 2019

richvdh requested a review from a team February 27, 2019 23:15

ara4n commented Feb 27, 2019

View reviewed changes

clarify comment

c403033

erikjohnston approved these changes Mar 5, 2019

View reviewed changes

richvdh pushed a commit to matrix-org/sytest that referenced this pull request Mar 11, 2019

fix tests for matrix-org/synapse#2090 (#350)

83ef6a4

richvdh merged commit 2326e00 into develop Mar 11, 2019

richvdh deleted the matthew/fix-filename-escaping branch March 11, 2019 09:53

richvdh mentioned this pull request Mar 11, 2019

file uploads with spaces in the filename get turned into %20's on download element-hq/element-web#3155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix incorrect encoding of filenames with spaces in #2090

fix incorrect encoding of filenames with spaces in #2090

ara4n commented Apr 1, 2017

ara4n commented Apr 1, 2017

ara4n commented Apr 1, 2017

ara4n commented Apr 1, 2017

erikjohnston commented Apr 3, 2017

ara4n commented Apr 11, 2017

ara4n commented Apr 19, 2017

richvdh commented Sep 5, 2017

ara4n commented Aug 15, 2018

richvdh commented Feb 27, 2019

richvdh commented Feb 27, 2019

richvdh commented Feb 27, 2019

codecov bot commented Feb 27, 2019

richvdh commented Feb 27, 2019

ara4n Feb 27, 2019

richvdh Feb 28, 2019

ara4n Feb 27, 2019

richvdh Feb 27, 2019

ara4n Feb 27, 2019

richvdh Feb 28, 2019

fix incorrect encoding of filenames with spaces in #2090

fix incorrect encoding of filenames with spaces in #2090

Conversation

ara4n commented Apr 1, 2017

ara4n commented Apr 1, 2017

ara4n commented Apr 1, 2017

ara4n commented Apr 1, 2017

erikjohnston commented Apr 3, 2017

ara4n commented Apr 11, 2017

ara4n commented Apr 19, 2017

richvdh commented Sep 5, 2017

ara4n commented Aug 15, 2018

richvdh commented Feb 27, 2019

richvdh commented Feb 27, 2019

richvdh commented Feb 27, 2019

codecov bot commented Feb 27, 2019

Codecov Report

richvdh commented Feb 27, 2019

ara4n Feb 27, 2019

Choose a reason for hiding this comment

richvdh Feb 28, 2019

Choose a reason for hiding this comment

ara4n Feb 27, 2019

Choose a reason for hiding this comment

richvdh Feb 27, 2019

Choose a reason for hiding this comment

ara4n Feb 27, 2019

Choose a reason for hiding this comment

richvdh Feb 28, 2019

Choose a reason for hiding this comment