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

Session can remove itself and does not really invalidate the session for all endpoints #28881

Closed
SamuAlfageme opened this issue Sep 1, 2017 · 8 comments · Fixed by #29533
Closed
Assignees
Labels
p2-high Escalation, on top of current planning, release blocker regression settings:personal status/STALE Type:Bug
Milestone

Comments

@SamuAlfageme
Copy link

Steps to reproduce

  1. With a valid $SESSION (after login) - list all current sessions:
$ curl 'https://<server>/index.php/settings/personal/authtokens' \
    -H 'OCS-APIREQUEST: true' \
    -H 'Cookie: $SESSION'

[
   {
      "id":1,
      "name":"Mozilla\/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit\/537.36 (KHTML, like Gecko) Chrome\/60.0.3112.113 Safari\/537.36",
      "lastActivity":"1504262361",
      "type":"0",
      "canDelete":false
   }
]
  1. Remove itself (shouldn't be allowed - note the "canDelete":false)
$ curl 'https://<server>/index.php/settings/personal/authtokens/1' -X DELETE 
    -H 'OCS-APIREQUEST: true' \
    -H 'Cookie: $SESSION'
  1. Query any endpoint to check how the $SESSION is still valid
  2. Hit the first endpoint again:
$ curl -I 'https://<server>/index.php/settings/personal/authtokens' \
    -H 'OCS-APIREQUEST: true' \
    -H 'Cookie: $SESSION'

HTTP/1.1 503 Service Unavailable
[...]

Expected behavior

  1. If "canDelete":false, removing that session should be forbidden.
  2. The $SESSION should be invalidated for all endpoints and querying https://<server>/index.php/settings/personal/authtokens shouldn't cause a server error

Actual behavior

Described in repro steps.

Server configuration

@PVince81 PVince81 added the p2-high Escalation, on top of current planning, release blocker label Sep 1, 2017
@PVince81 PVince81 added this to the development milestone Sep 1, 2017
@PVince81
Copy link
Contributor

PVince81 commented Sep 1, 2017

Is this a regression ?

@SamuAlfageme
Copy link
Author

SamuAlfageme commented Sep 1, 2017

@PVince81 not sure. Will check with 9.1.X - Related though: #24911 -> #25276

@SamuAlfageme
Copy link
Author

SamuAlfageme commented Sep 1, 2017

The "does not really invalidate the session for all endpoints" part it's indeed a regression - working in 9.1.1

@IljaN
Copy link
Contributor

IljaN commented Sep 1, 2017

Should i change the status to 401 Unauthorized? 503 is clearly wrong here

@SamuAlfageme
Copy link
Author

SamuAlfageme commented Sep 4, 2017

@IljaN I'd expect the request in step 2. to be replied with an 403: Forbidden (after #25276) since the user cannot remove his own session in this manner. And yup, any requests after removing a session from a different one should be replied with 401: Unauthorized. (i.e. invalidate the session)

@SamuAlfageme
Copy link
Author

Fixed with @IljaN's 2ceecd2

On a side-note, I've also noticed <sessionID>s in https://<server>/index.php/settings/personal/authtokens/<sessionID> receive no existence check - i.e. when deleting a non-existent session id, server replies with a 200.

@IljaN
Copy link
Contributor

IljaN commented Sep 5, 2017

Yeah the there are a lot of edgecases handling missing, but this was also the case before the regression, I suggest that we improve this in a seperate task after realease (unless it is critical)

@lock
Copy link

lock bot commented Aug 1, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p2-high Escalation, on top of current planning, release blocker regression settings:personal status/STALE Type:Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants