Soften the cookie check if no cookies are sent #110
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When no cookies are sent it is not required to perform any check for the strict or lax cookie, it does not provide any significant security advantage.
It does however interfer with the Android client which requests thumbnails from the unofficial API at `/index.php/apps/files/api/v1/thumbnail/256/256/{filename}`. This endpoint expects the strict cookie to be existent to not leak the existence of files. The Android client authenticates against this endpoint using Basic Auth and without cookies in some cases at least. This will make these endpoints work again with such cases.
To test this issue the following cURL command once without the patch and once with:
> curl http://localhost/index.php/apps/files/api/v1/thumbnail/256/256/welcome.txt -u admin -v
Without the patch the request is redirected (which the client does not obey) and with the patch the preview is returned.
|
Tested, works and code looks good 👍 |
|
My wife has the issue, gonne apply it on production 👷 and report back |
|
I am still seeing 503s: |
|
Earlier today, @LukasReschke and I debugged this a bit. DavDroid just does not sent back the sameSite cookies as required by the server, ending up with the 503s. |
tobiasKaminsky
added a commit
to nextcloud/android
that referenced
this pull request
Jun 19, 2016
|
@LukasReschke I have created a PR for this on Android: nextcloud/android#37 |
|
Let's merge this for now and debug the DAVDroid problem independently. LGTM |
|
This fixed the Issue for me, too. |
This was referenced Aug 14, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When no cookies are sent it is not required to perform any check for the strict or lax cookie, it does not provide any significant security advantage.
It does however interfer with the Android client which requests thumbnails from the unofficial API at
/index.php/apps/files/api/v1/thumbnail/256/256/{filename}. This endpoint expects the strict cookie to be existent to not leak the existence of files. The Android client authenticates against this endpoint using Basic Auth and without cookies in some cases at least. This will make these endpoints work again with such cases.To test this issue the following cURL command once without the patch and once with:
Without the patch the request is redirected (which the client does not obey) and with the patch the preview is returned.
Fixes #101
@nextcloud/android Any chance that meanwhile we can push a hotfix to the Android app that sends the cookies
nc_sameSiteCookielaxandnc_sameSiteCookiestrictwith the value oftrueto all preview API requests? (or just to all). We can remove this patch later then but in the meanwhile users wouldn't have this bug 😉