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

Redirect to index after session expiration on /generate #4496

Merged
merged 2 commits into from
Jun 4, 2019

Conversation

redshiftzero
Copy link
Contributor

@redshiftzero redshiftzero commented May 31, 2019

Status

Ready for review

Description of Changes

Fixes #4490.

Previous logic when sessions expire on /generate:

  1. On /generate page, codename is added to the session.
  2. When expiration occurs, session is cleared.
  3. A flashed message indicating that the session has expired
    would be added to the current session (this is done via passing
    _flashes on Flask's session object
    ).
  4. Execution enters the view function associated with /create.
    But /create expects codename to be in the session (which was
    cleared in step 2), thus a KeyError will occur.

If you're wondering "but why does step 4 even happen" - it's because we don't immediately redirect to the index when a session expires. When the session expires when the @login_required decorator is on the view function corresponding to the request path, execution doesn't end up entering the view function itself, as the logic in @login_required will redirect the user to the login page if logged_in is not in the session object (and logged_in won't be in the session object, because it gets cleared in step 2).

Logic with this diff when sessions expire on /generate:

  1. On /generate page, codename is added to the session
  2. When expiration occurs, session is cleared and user is immediately redirected to the index.

Testing

  1. Follow STR in Expired Source Interface session causes server error on new session #4490, confirm bug is fixed
  2. Revert fix, make sure regression test fails

Deployment

No special instructions (deployed via securedrop-app-code deb)

Checklist

If you made changes to the server application code:

  • Linting (make lint) and tests (make -C securedrop test) pass in the development container

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

Previous logic when sessions expire on /generate:
1. On /generate page, codename is added to the session.
2. When expiration occurs, session is cleared.
3. A flashed message indicating that the session has expired
would be added to the current session (this is done via passing
`_flashes` on the session object [0]).
4. Execution enters the view function associated with `/create`.
But `/create` expects codename to be in the session (which was
cleared in step 2), thus a KeyError will occur.

Logic now when sessions expire on /generate:
1. On /generate page, codename is added to the session
2. When expiration occurs, session is cleared and user is redirected to the index.

[0] https://github.com/pallets/flask/blob/cd4023d9d2ab630ce4f95856f065072ef8badb2b/flask/helpers.py#L449
@codecov-io
Copy link

Codecov Report

Merging #4496 into develop will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4496      +/-   ##
===========================================
+ Coverage    83.21%   83.22%   +<.01%     
===========================================
  Files           45       45              
  Lines         3069     3070       +1     
  Branches       332      332              
===========================================
+ Hits          2554     2555       +1     
  Misses         430      430              
  Partials        85       85
Impacted Files Coverage Δ
securedrop/securedrop/source_app/__init__.py 92.63% <0%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d164ece...7fafd7c. Read the comment docs.

@redshiftzero redshiftzero marked this pull request as ready for review June 3, 2019 23:03
@redshiftzero redshiftzero requested a review from emkll June 3, 2019 23:03
@redshiftzero redshiftzero changed the title [wip] redirect to index after session expiration on /generate redirect to index after session expiration on /generate Jun 3, 2019
@redshiftzero
Copy link
Contributor Author

After consideration, I'm going to leave this be, this is a simple fix - I was considering moving the setting of expires on the session from setup_g() to two places in the view functions - login and create - but I don't think it's worth the marginal increase in complexity. Ready for review.

@eloquence eloquence changed the title redirect to index after session expiration on /generate Redirect to index after session expiration on /generate Jun 4, 2019
@eloquence
Copy link
Member

eloquence commented Jun 4, 2019

Can confirm in Docker-based development environment:

==================================================================================================== FAILURES =====================================================================================================
______________________________________________________________________________________ test_source_session_expiration_create ______________________________________________________________________________________

config = <sdconfig.SDConfig object at 0x7f684ffd7450>, source_app = <Flask 'source_app'>

    def test_source_session_expiration_create(config, source_app):
        with source_app.test_client() as app:
    
            seconds_session_expire = 1
            config.SESSION_EXPIRATION_MINUTES = seconds_session_expire / 60.
    
            # Make codename, and then wait for session to expire.
            resp = app.get(url_for('main.generate'))
            assert resp.status_code == 200
    
            time.sleep(seconds_session_expire + 0.1)
    
            # Now when we click create, the session will have expired.
>           resp = app.post(url_for('main.create'), follow_redirects=True)

tests/test_source.py:690: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/usr/local/lib/python2.7/dist-packages/werkzeug/test.py:840: in post
    return self.open(*args, **kw)
/usr/local/lib/python2.7/dist-packages/flask/testing.py:200: in open
    follow_redirects=follow_redirects
/usr/local/lib/python2.7/dist-packages/werkzeug/test.py:803: in open
    response = self.run_wsgi_app(environ, buffered=buffered)
/usr/local/lib/python2.7/dist-packages/werkzeug/test.py:716: in run_wsgi_app
    rv = run_wsgi_app(self.application, environ, buffered=buffered)
/usr/local/lib/python2.7/dist-packages/werkzeug/test.py:923: in run_wsgi_app
    app_rv = app(environ, start_response)
/usr/local/lib/python2.7/dist-packages/flask/app.py:2309: in __call__
    return self.wsgi_app(environ, start_response)
/usr/local/lib/python2.7/dist-packages/flask/app.py:2295: in wsgi_app
    response = self.handle_exception(e)
/usr/local/lib/python2.7/dist-packages/flask/app.py:1741: in handle_exception
    reraise(exc_type, exc_value, tb)
/usr/local/lib/python2.7/dist-packages/flask/app.py:2292: in wsgi_app
    response = self.full_dispatch_request()
/usr/local/lib/python2.7/dist-packages/flask/app.py:1815: in full_dispatch_request
    rv = self.handle_user_exception(e)
/usr/local/lib/python2.7/dist-packages/flask/app.py:1718: in handle_user_exception
    reraise(exc_type, exc_value, tb)
/usr/local/lib/python2.7/dist-packages/flask/app.py:1813: in full_dispatch_request
    rv = self.dispatch_request()
/usr/local/lib/python2.7/dist-packages/flask/app.py:1799: in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
source_app/main.py:56: in create
    session['codename'])
/usr/local/lib/python2.7/dist-packages/werkzeug/local.py:377: in <lambda>
    __getitem__ = lambda x, i: x._get_current_object()[i]
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <SecureCookieSession {'expires': datetime.datetime(2019, 6, 4, 0, 57, 54, 2503...ou are not using Tor Browser, restart your browser.</p>\n  </div>\n</div>'))]}>, key = 'codename'

    def __getitem__(self, key):
        self.accessed = True
>       return super(SecureCookieSession, self).__getitem__(key)
E       KeyError: 'codename'

/usr/local/lib/python2.7/dist-packages/flask/sessions.py:83: KeyError
---------------------------------------------------------------------------------------------- Captured stdout setup ----------------------------------------------------------------------------------------------
seq  name             file                                                      
---  ---------------  ----------------------------------------------------------
0    main             /tmp/pytest-of-www-data/pytest-0/test_source_session_expir
====================================================================================== 1 failed, 37 passed in 111.80 seconds ======================================================================================

@zenmonkeykstop
Copy link
Contributor

Tested in qubes staging environment:

  • Followed STR to confirm bug fixed
  • Reverted fix, ran regression test with ./bin/dev-shell bin/run-test -k "test_source_session_expiration_create", confirmed test failed
  • Restored fix, ran regression test as above, confirmed test passed.

LGTM!

@zenmonkeykstop zenmonkeykstop merged commit 6a92407 into develop Jun 4, 2019
@redshiftzero redshiftzero deleted the fix-session-src branch June 25, 2019 23:08
@zenmonkeykstop zenmonkeykstop mentioned this pull request Jun 27, 2019
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expired Source Interface session causes server error on new session
4 participants