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

Wizard: 301 'Moved Permanently' redirection is not being followed #5954

Closed
SamuAlfageme opened this issue Aug 11, 2017 · 15 comments · Fixed by #6009
Closed

Wizard: 301 'Moved Permanently' redirection is not being followed #5954

SamuAlfageme opened this issue Aug 11, 2017 · 15 comments · Fixed by #6009
Assignees
Labels
Milestone

Comments

@SamuAlfageme
Copy link
Contributor

Actual behavior

The client appends /status.php and /owncloud/status.php to the shortened entrypoint URL, which results on a 404 since that resource was moved permanently to the URL on the Location header. Error is displayed.

Expected behavior

The client should be able to follow a redirection chain.

Steps to reproduce

  1. Create a short URL to an ownCloud instance using e.g. https://goo.gl
  2. Try to connect using the short URL
@SamuAlfageme
Copy link
Contributor Author

SamuAlfageme commented Aug 11, 2017

Connected to #5553 (that scenario works though) - this one is about only the "entrypoint" (server URL) being behind the redirection - does fixing this make sense?

cc/ @ckamm

@guruz guruz changed the title 301 'Moved Permanently' redirection is not being followed Wizard: 301 'Moved Permanently' redirection is not being followed Aug 14, 2017
@guruz
Copy link
Contributor

guruz commented Aug 14, 2017

FYI @ogoffart for discussion
I guess it would make sense.

@SamuAlfageme
Copy link
Contributor Author

Will be fixed by: #6009

@SamuAlfageme SamuAlfageme added this to the 2.4.0 milestone Sep 11, 2017
@ckamm
Copy link
Contributor

ckamm commented Sep 11, 2017

@SamuAlfageme Thanks for the pointer, I'll check whether my patch does indeed address this.

@ckamm
Copy link
Contributor

ckamm commented Sep 11, 2017

No, this is actually an independent issue. The issue here is that when the user enter http://foo the client queries http://foo/status.php. Url shorteners don't forward subpaths, so this url will actually be a 404!

Suggestion: In the wizard only, start off by querying the url the user entered and handle permanent redirects. Only then change the path to look for the owncloud instance.

@SamuAlfageme
Copy link
Contributor Author

@ckamm you're absolutely right. I forgot the original reason of the issue.

start off by querying the url the user entered and handle permanent redirects

IMO, we should go for this, yup 👍

@ckamm
Copy link
Contributor

ckamm commented Sep 11, 2017

@SamuAlfageme Now there's a PR for it :)

ckamm added a commit that referenced this issue Sep 12, 2017
Grab any permanent redirects from the base url the user entered
before attempting to connect to a modified url (with status.php
added).
ckamm added a commit that referenced this issue Sep 15, 2017
Grab any permanent redirects from the base url the user entered
before attempting to connect to a modified url (with status.php
added).
ckamm added a commit that referenced this issue Sep 15, 2017
Grab any permanent redirects from the base url the user entered
before attempting to connect to a modified url (with status.php
added).
@ckamm ckamm added the ReadyToTest QA, please validate the fix/enhancement label Sep 15, 2017
@SamuAlfageme
Copy link
Contributor Author

SamuAlfageme commented Oct 18, 2017

@ckamm I just saw a SAML scenario where this broke OAuth2 workflow. Probably the edgiest case of them all but might ask for a revert of this 😕

Successful scenario (what we used to do)

  1. Client checks https://<server>/status.php (200) and then https://<server>/remote.php/webdav/ (which includes the WWW-Authenticate: Bearer realm="ownCloud")

Unsuccessful scenario

  1. Client checks https://<server>/ -> server replies 302; location: https://<server>/<IDP>
  2. Redirection is followed and the client launches the WebView before the WWW-Authenticate: Bearer realm="ownCloud" is received (which does not always happens)

@ckamm ckamm removed the ReadyToTest QA, please validate the fix/enhancement label Oct 19, 2017
@ckamm
Copy link
Contributor

ckamm commented Oct 19, 2017

Hrm. We could query and use the base-url redirect only if the base-url/status.php request fails.

@ckamm
Copy link
Contributor

ckamm commented Oct 19, 2017

@SamuAlfageme Please test the new PR!

ckamm added a commit that referenced this issue Oct 24, 2017
Unfortunately checking the base-url for redirects in all cases lead
to incorrect behavior in some SAML/OAuth2 edge cases.

This new iteration checks the base url for redirects only if the
standard CheckServerJob can't reach the server. That way the 2.3
behavior is only changed in cases that would have lead to errors.

See #5954
ckamm added a commit that referenced this issue Oct 24, 2017
Unfortunately checking the base-url for redirects in all cases lead
to incorrect behavior in some SAML/OAuth2 edge cases.

This new iteration checks the base url for redirects only if the
standard CheckServerJob can't reach the server. That way the 2.3
behavior is only changed in cases that would have lead to errors.

See #5954
@SamuAlfageme
Copy link
Contributor Author

@ckamm apparently the WWW-Authenticate headers are only replied by the server when doing a PROPFIND to the DAV endpoint, we're currently doing a GET and that's why we were following the SAML redirection.

I think we'd need to switch to PROPFIND cc/ @ogoffart

@ckamm
Copy link
Contributor

ckamm commented Oct 31, 2017

@SamuAlfageme Did you test the version that has my adjustments? The behavior shouldn't change at all from 2.3.3 with the current approach.

@SamuAlfageme
Copy link
Contributor Author

@ckamm yeah, I think the problem was not in the end related with this issue but with specific (and stricter) firewall configs. My mistake.

@ckamm
Copy link
Contributor

ckamm commented Oct 31, 2017

@SamuAlfageme Okay. We probably want to make that work for 2.4 even if it's not a regression: could you make a new ticket about it and describe how to reproduce? There's no guarantee we can get this to work within the backwards-compatibility constraints though.

@SamuAlfageme
Copy link
Contributor Author

Ticket for reference: #6135

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants