Skip to content

Add custom error page for uncaught exceptions#159

Merged
itziakos merged 12 commits into
masterfrom
write-error-page-for-uncaught-errors
Jul 29, 2016
Merged

Add custom error page for uncaught exceptions#159
itziakos merged 12 commits into
masterfrom
write-error-page-for-uncaught-errors

Conversation

@kitchoi

@kitchoi kitchoi commented Jul 25, 2016

Copy link
Copy Markdown
Contributor

Uncaught error such as #143 causes a plain "HTTP 500: Internal Server Error" page. This PR overloads tornado's write_error so that error message is displayed.

For #143, it displays something like this:
screenshot from 2016-07-26 11 29 31

@codecov-io

codecov-io commented Jul 25, 2016

Copy link
Copy Markdown

Current coverage is 88.61% (diff: 94.11%)

Merging #159 into master will increase coverage by 0.06%

@@             master       #159   diff @@
==========================================
  Files            36         36          
  Lines          1275       1291    +16   
  Methods           0          0          
  Messages          0          0          
  Branches        121        122     +1   
==========================================
+ Hits           1129       1144    +15   
  Misses          108        108          
- Partials         38         39     +1   

Powered by Codecov. Last update 9a921c7...684b9d6

@stefanoborini

Copy link
Copy Markdown
Contributor

I'm not sure it's a good idea to leak internal information that way. I would leave the details for the internal log, but stay generic for the presented page.

@kitchoi

kitchoi commented Jul 26, 2016

Copy link
Copy Markdown
Contributor Author

This error page captures all uncaught exception, any generic exception. If the content of the exception is not displayed, then it is no different from the original behaviour of a plain 500 server error page.

@stefanoborini

Copy link
Copy Markdown
Contributor

and that's the point. You don't want to leak internal information to externals. That way anyone can see exactly what went wrong and the path information. It is information leakage http://projects.webappsec.org/w/page/13246936/Information%20Leakage

@kitchoi

kitchoi commented Jul 26, 2016

Copy link
Copy Markdown
Contributor Author

@stefanoborini better?
screenshot from 2016-07-26 11 29 31

@kitchoi

kitchoi commented Jul 26, 2016

Copy link
Copy Markdown
Contributor Author

@stefanoborini I know this is not the best place for an HTTPError, but I would prefer improving on this after the PR list is shorter.

@kitchoi

kitchoi commented Jul 26, 2016

Copy link
Copy Markdown
Contributor Author

And this is how it looks to the admin

[I 160726 12:06:00 orm:131] Creating session to db: sqlite:///remoteappmanager.db
[E 160726 12:06:00 application:118] The database is not initialised properly.
    Traceback (most recent call last):
      File "/home/kit/.virtualenv/simremote/lib/python3.4/site-packages/traitlets/traitlets.py", line 501, in get
        value = obj._trait_values[self.name]
    KeyError: 'user'

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "/home/kit/.virtualenv/simremote/lib/python3.4/site-packages/traitlets/traitlets.py", line 501, in get
        value = obj._trait_values[self.name]
    KeyError: 'db'

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "/mnt/hgfs/VM_shared/SimPhoNy/s2imphony-remote/remoteappmanager/application.py", line 115, in _db_default
        return cls(**self.file_config.accounting_kwargs)
      File "/mnt/hgfs/VM_shared/SimPhoNy/s2imphony-remote/remoteappmanager/db/orm.py", line 176, in __init__
        self.check_database_readable()
      File "/mnt/hgfs/VM_shared/SimPhoNy/s2imphony-remote/remoteappmanager/db/orm.py", line 190, in check_database_readable
        'Sqlite database {} is not readable'.format(file_path))
    OSError: Sqlite database /mnt/hgfs/VM_shared/SimPhoNy/s2imphony-remote/jupyterhub/remoteappmanager.db is not readable
[E 160726 12:06:00 logging_mixin:26] Internal Server Error : HTTP 500: The database is not initialised properly.. Ref: efc99ca8-5320-11e6-b741-000c29ae0276


class TestBaseHandlerDatabaseError(TempMixin, utils.AsyncHTTPTestCase):
def setUp(self):
self._old_proxy_api_token = os.environ.get("PROXY_API_TOKEN", None)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code shows up again and again in all our tests. It's becoming a liability.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is always tricky to have to cleanup/recover the enviroment variables. Can you open an issue, this will need to be investigated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already there.

@itziakos itziakos merged commit ef8fef2 into master Jul 29, 2016
@itziakos itziakos deleted the write-error-page-for-uncaught-errors branch July 29, 2016 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants