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

Fixed decoded URI handling inside DavResource constructor #253

Closed
wants to merge 1 commit into from

Conversation

Holdo
Copy link

@Holdo Holdo commented Jun 1, 2016

This fixes URI encoding related problems like white spaces in the DavResource class. I had a problem with listing files on a shared drive URI which included white spaces.

I did not run the tests (they depend on http://test.cyberduck.ch which was not available).

There is a new exception UnsupportedEncodingException, which I handled in SyncCollectionReport class but it is being propagated via IOException in the DavResource class.

@Holdo
Copy link
Author

Holdo commented Jun 1, 2016

The build failed because some URLs in the test like http://sardine.googlecode.com/svn/trunk/README.html are not available (404).

@dkocher
Copy link
Collaborator

dkocher commented Jun 1, 2016

Hrefs in the XML response are already required to be URI encoded per the RFC specification. With this patch, the references end up double encoded for servers that comply to the specification.

@Holdo
Copy link
Author

Holdo commented Jun 1, 2016

So... our Microsoft Sharepoint server does not comply to the specification?

@dkocher
Copy link
Collaborator

dkocher commented Jun 1, 2016

@Holdo Right.

@Holdo
Copy link
Author

Holdo commented Jun 1, 2016

Cool. Can we put my alternative (re-try) in the URISyntaxException catch block?

@Holdo
Copy link
Author

Holdo commented Jun 3, 2016

I updated the commit to give the parsing a second chance.

@dkocher
Copy link
Collaborator

dkocher commented Dec 14, 2018

This should be ammended with a unit test. I suppose this is not correct as we should use URI encoding (not application/x-www-form-urlencoded) and only encode single path components.

@dkocher dkocher closed this Dec 14, 2018
kkofler added a commit to kkofler/sardine that referenced this pull request Jul 19, 2022
In SardineImpl.enablePreemptiveAuthentication(String, int, int,
Charset), do not pass -1 to the HttpHost constructor. The later cache
lookup is always done with an explicit port, so port -1 never matches
and the entry does nothing. Instead, we need to use the default port
explicitly, then it will work, even when connecting with a URL that does
not include the port.

Since the other overloads delegate to this one, this fixes all overloads
to do the right thing, in particular, the
enablePreemptiveAuthentication(String) (hostname-only) and
enablePreemptiveAuthentication(URL) (when using a URL without an
explicit port) ones.

Fixes lookfirst#253.
@kkofler
Copy link
Contributor

kkofler commented Jul 19, 2022

Sorry, wrong issue number in my pull request, so it ended up referencing this pull request.

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.

None yet

3 participants