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

Enforce check of existing session in `vibe.http.server.HTTPServerResponse.terminateSession` #472

Closed
ilya-stromberg opened this Issue Jan 20, 2014 · 2 comments

Comments

Projects
None yet
2 participants
@ilya-stromberg
Contributor

ilya-stromberg commented Jan 20, 2014

Currently you have only assert, not enforce:

assert(m_session, "Try to terminate a session, but none is started.");

In release mode I have only this error:

vibe-d-0.7.18/source/vibe/http/session.d(96): null this

Note that hacker can visit "logout" page as often as he want. Probably, it's not a security bug, but it's still bad because:

  1. the error message isn't so informative as it can be
  2. the session behavior can be changed in future
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 20, 2014

Member

The assertion indicates that this is a programming error. You should check req.session before calling terminateSession to avoid the error in the first place. Having said that, maybe making terminateSession return gracefully in case of !m_session would be acceptable - I can't think of a situation in which this would hide a significant bug.

Member

s-ludwig commented Jan 20, 2014

The assertion indicates that this is a programming error. You should check req.session before calling terminateSession to avoid the error in the first place. Having said that, maybe making terminateSession return gracefully in case of !m_session would be acceptable - I can't think of a situation in which this would hide a significant bug.

@ilya-stromberg

This comment has been minimized.

Show comment
Hide comment
@ilya-stromberg

ilya-stromberg Jan 20, 2014

Contributor

The assertion indicates that this is a programming error. You should check req.session before calling terminateSession to avoid the error in the first place.

Yes, I already add manual check. But example (http://vibed.org/docs, see Sessions section) doesn't talk that it's necessary.
So, it's bug (may be documentation bug). I suggest to replace assert to enforce, because example illustrates the most often case usage and it's allows to simplify user code. But if you really want to keep assert, please fix documentation.

Having said that, maybe making terminateSession return gracefully in case of !m_session would be acceptable - I can't think of a situation in which this would hide a significant bug.

Yes, it's also can be good alternative. Just do nothing if we haven't got started session.

Contributor

ilya-stromberg commented Jan 20, 2014

The assertion indicates that this is a programming error. You should check req.session before calling terminateSession to avoid the error in the first place.

Yes, I already add manual check. But example (http://vibed.org/docs, see Sessions section) doesn't talk that it's necessary.
So, it's bug (may be documentation bug). I suggest to replace assert to enforce, because example illustrates the most often case usage and it's allows to simplify user code. But if you really want to keep assert, please fix documentation.

Having said that, maybe making terminateSession return gracefully in case of !m_session would be acceptable - I can't think of a situation in which this would hide a significant bug.

Yes, it's also can be good alternative. Just do nothing if we haven't got started session.

@s-ludwig s-ludwig closed this in a8b7730 Jan 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment