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.

Sign up for GitHub

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

Jump to bottom

Don't encode paths that will be passed to the davclient #7765

Merged
merged 2 commits into from
Jan 10, 2018
Merged

Don't encode paths that will be passed to the davclient #7765

merged 2 commits into from
Jan 10, 2018

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Jan 10, 2018 •
edited
Loading

Fixes #5574

Those paths will be encoded by the davclient again. So in certain situations this leads to double encoded urls. Which can fail.

To test:

  1. Create a user x@y.z
  2. Try to upload a large enough file (pick something like 20MB to be safe).

Before: 💥
Now: 🕺

@rullzer
Return proper principal on the uploads collection listing
e5cc8eb 
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
Don't encode paths passed to the OC.Files.Client
d4f163b 
This is handled already in the client. So double encoding breaks things
in some situations.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer added this to the Nextcloud 13 milestone Jan 10, 2018
@MorrisJobke MorrisJobke mentioned this pull request Jan 10, 2018
Merged
18 tasks
@codecov
Copy link

codecov bot commented Jan 10, 2018 •
edited
Loading

Codecov Report

Merging #7765 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #7765      +/-   ##
============================================
- Coverage     51.22%   51.22%   -0.01%     
  Complexity    24967    24967              
============================================
  Files          1607     1607              
  Lines         94989    94990       +1     
  Branches       1376     1376              
============================================
  Hits          48662    48662              
- Misses        46327    46328       +1
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/Upload/UploadHome.php 0% <0%> (ø) 12 <0> (ø) ⬇️

MorrisJobke
MorrisJobke approved these changes Jan 10, 2018
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@nickvergessen
Copy link
Member

This replaces #4528 ?!

@MorrisJobke
Copy link
Member

This replaces #4528 ?!

Seems so - I also had something like this in mind, but the solution here looks better AFAIK.

@enoch85
Copy link
Member

enoch85 commented Jan 10, 2018 •
edited
Loading

Yup it works. Thanks! 👍

@MorrisJobke MorrisJobke merged commit 00a823f into master Jan 10, 2018
@MorrisJobke MorrisJobke deleted the fix_5574 branch January 10, 2018 20:00
@nickvergessen nickvergessen mentioned this pull request Feb 16, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@enoch85 enoch85 enoch85 approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@danxuliu danxuliu Awaiting requested review from danxuliu

Assignees
No one assigned
Labels
3. to review Waiting for reviews bug feature: files
Projects
None yet
Milestone
Nextcloud 13
Development

Successfully merging this pull request may close these issues.
4 participants
@rullzer @nickvergessen @MorrisJobke @enoch85