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

After a redirect the body must still be send to the server #389

Closed

Conversation

autoantwort
Copy link

@autoantwort autoantwort commented Apr 5, 2024

Fixes nextcloud/server#43609 (comment)

I don't know if this is the right fix. But it fixes the problem.

@tobixen
Copy link
Member

tobixen commented Oct 20, 2024

I'm so sorry for not looking into this before.

I think it would be better to pass allow_redirects=True in the call to the requests library?

@autoantwort
Copy link
Author

I think it would be better to pass allow_redirects=True in the call to the requests library?

No allow_redirects=True does not resend the body.

@tobixen
Copy link
Member

tobixen commented Oct 22, 2024

Right, I see.

In the HTTP-world, it's normal procedure to accept POST data and then do a redirect to some status page, to avoid browsers doubly posting the data on a reload. I don't know if any caldav servers is following this practice, but in any case I think it would be a breach of the HTTP-standard to do this. I consider this to be a tweak that may be needed for some servers, but could be harmful for other servers. I do have a plan for a way to pass "server-side compatiblity hints" to the client/davconnect object.

I think I will proritize to make a new release with the stuff that's already added to master since last release, then fix the "server-side compatibility hints", and make this repost into an optional feature.

@autoantwort
Copy link
Author

Maybe the body should only be send again when a redirect from http to https was done.

@tobixen
Copy link
Member

tobixen commented Oct 22, 2024

Reading up a bit on the standards, a 303 redirect explicitly forbids the client from reposting the data, while a 307 or 308 tells that the data should be reposted. A 302 is historically often used to indicate "your post has been accepted, please continue at this URL" and hence it should NOT cause a repost to be made. For 301, it's simply not defined if the data should be reposted or not. But if you're trying to do a HTTP-request towards a server that wants HTTPS, this sounds like a configuration problem to me, did you specify HTTPS in the configuration?

Another normal problem is if a two-layer design on the server side where one application (like nginx, hitch, apache, haproxy, etc) terminates the https traffic and proxies the http-request to the application server, the application server believes the communication with the client goes via http and sends http-links back to the client.

My plan now is to go quickly through the outstanding issues and pull requests, update the CHANGELOG, test the master branch towards as many server implementations as possible, release 1.4.0, then work on "server connection hints" and try to release a 1.5.0 including this code on an opt-in basis. In the very best case, it's a matter of weeks. Unfortunately, as work on the python caldav library is a hobby project competing with many other hobby projects, it tend to end up deep in my TODO-stack whenever more important matters props up, so in worst case it will be a year or so before 1.5.0 is released. Feel free to nag on me :-)

@autoantwort
Copy link
Author

According to my own comment here is seems that I fixed that by changing the configuration from http to https. So yeah, maybe this should not be fixed here and instead in the proxy configuration. And using https and http in the configuration is probably the best fix.

@tobixen
Copy link
Member

tobixen commented Oct 22, 2024

I think perhaps the right fix is to detect the redirect and update the URL in the davclient object while doing the authentication negotiation.

@tobixen
Copy link
Member

tobixen commented Oct 26, 2024

I will close this one, as we've found it was a configuration problem.

@tobixen tobixen closed this Oct 26, 2024
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.

[Bug]: Sabre\Xml\ParseException: The input element to parse is empty. Do not attempt to parse
2 participants