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

fully resolve well-known URI #481

Merged
merged 2 commits into from
Jul 26, 2016
Merged

Conversation

posativ
Copy link
Contributor

@posativ posativ commented Jul 16, 2016

Currently, find_dav only resolves a single redirect. When using Baïkal behind a proxy with HTTPS, this becomes an issue:

  1. dav.example.com/.well-known/caldav (Apache rewrite to http)
  2. dav.example.com/dav.php (http)
  3. dav.example.com/dav.php (https)

The Apache configuration is provided by Baïkal, hence it is also possible to fix it server-side:

-RewriteRule /.well-known/carddav /dav.php [R,L]
+RewriteRule /.well-known/carddav https://dav.example.com/dav.php [R,L]
-RewriteRule /.well-known/caldav /dav.php [R,L]
+RewriteRule /.well-known/caldav https://dav.example.com/dav.php [R,L]

This commit follows the well-known URI up to 5 times and returns the (hopefully) correctly resolved url.

Currently, `find_dav` only resolves a single redirect. When using
Baïkal behind a proxy with HTTPS, this becomes an issue:

1. dav.example.com/.well-known/caldav (Apache rewrite to http)
2. dav.example.com/dav.php (http)
3. dav.example.com/dav.php (https)

The Apache configuration is provided by Baïkal, hence it is also
possible to fix it server-side:

  -RewriteRule /.well-known/carddav /dav.php [R,L]
  +RewriteRule /.well-known/carddav https://dav.example.com/dav.php [R,L]
  -RewriteRule /.well-known/caldav /dav.php [R,L]
  +RewriteRule /.well-known/caldav https://dav.example.com/dav.php [R,L]

This commit follows the well-known URI up to 5 times and returns the
(hopefully) correctly resolved url.
@posativ
Copy link
Contributor Author

posativ commented Jul 16, 2016

The redirection code is based on DavDroid's (dav4android) implementation: DavResource.java#L114-129.


Side note: this is probably a side-effect with requests. The request to the not fully resolved URL (http instead of https) causes Baikal to not return a proper principle url. This issue vanished when I changed the Content-Type: application/xml; charset=UTF-8 to lowercase Content-Type: application/xml; charset=utf-8 (charset). The redirection of Apache (mod_rewrite) uses a latin1 encoding. I didn't investigate further, but something is weird with either my setup or requests.

@WhyNotHugo
Copy link
Member

Rather than just 5, I'd:

  • Add each url to a list.
  • If the URL is not on the list, continue following redirects.
  • If the URL is on the list, bail (circular redirection).

That's something like:

def find_dav(self):
    uri = self._well_known_uri
    tried_uris = []
    while uri not in tried_uris:
        tried_uris.append(uri)
        # ...

@untitaker
Copy link
Member

This redirect logic would already be in requests itself (allow_redirects=True, and reading the response.url parameter for the final URL). The reason I did it this way was to spare an unnecessary request at the end, but this request is now necessary to check if another redirect occurs.

On 16 July 2016 22:44:21 CEST, Martin Zimmermann notifications@github.com wrote:

Currently, find_dav only resolves a single redirect. When using
Baïkal behind a proxy with HTTPS, this becomes an issue:

  1. dav.example.com/.well-known/caldav (Apache rewrite to http)
  2. dav.example.com/dav.php (http)
  3. dav.example.com/dav.php (https)

The Apache configuration is provided by Baïkal, hence it is also
possible to fix it server-side:

-RewriteRule /.well-known/carddav /dav.php [R,L]
Side note
+RewriteRule /.well-known/carddav https://dav.example.com/dav.php [R,L]
-RewriteRule /.well-known/caldav /dav.php [R,L]
+RewriteRule /.well-known/caldav https://dav.example.com/dav.php [R,L]

This commit follows the well-known URI up to 5 times and returns the
(hopefully) correctly resolved url.
You can view, comment on, or merge this pull request online at:

#481

-- Commit Summary --

  • add support for multiple (up to 5) url redirections

-- File Changes --

M vdirsyncer/storage/dav.py (30)

-- Patch Links --

https://github.com/pimutils/vdirsyncer/pull/481.patch
https://github.com/pimutils/vdirsyncer/pull/481.diff


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#481

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@posativ
Copy link
Contributor Author

posativ commented Jul 18, 2016

So you propose to remove the manual redirection handling and use requests' allow_redirects=True?

@WhyNotHugo
Copy link
Member

WhyNotHugo commented Jul 18, 2016

I was unaware of requests' feature. I'd strongly vote to use that instead, yes.

@untitaker
Copy link
Member

Yup

On 18 July 2016 16:13:04 CEST, Hugo Osvaldo Barrera notifications@github.com wrote:

I was unaware of requests feature. I'd strongly vote to use that
instead, yes.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#481 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@untitaker
Copy link
Member

Hey @posativ any update on this?

Currently, `find_dav` only resolves a single redirect. When using
Baïkal behind a proxy with HTTPS, this becomes an issue:

1. dav.example.com/.well-known/caldav (Apache rewrite to http)
2. dav.example.com/dav.php (http)
3. dav.example.com/dav.php (https)

The Apache configuration is provided by Baïkal, hence it is also
possible to fix it server-side:

  -RewriteRule /.well-known/carddav /dav.php [R,L]
  +RewriteRule /.well-known/carddav https://dav.example.com/dav.php [R,L]
  -RewriteRule /.well-known/caldav /dav.php [R,L]
  +RewriteRule /.well-known/caldav https://dav.example.com/dav.php [R,L]
@posativ posativ changed the title add support for multiple (up to 5) url redirections fully resolve well-known URI Jul 25, 2016
@posativ
Copy link
Contributor Author

posativ commented Jul 25, 2016

Yup, I've rebased the new fix.

@untitaker
Copy link
Member

Crashes because of PyCQA/flake8-import-order#79. The other build issue with Davical is my fault.

@untitaker untitaker merged commit 48d72aa into pimutils:master Jul 26, 2016
@untitaker
Copy link
Member

Thanks!

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.

3 participants