Remove insecure mode

[coltonlw]

This removes insecure mode

Fixes: #410

[nagem]

LGTM.

[kofalt]

Is this entire diff related to the purpose of the PR?

I feel like catching the base class is a good idea here rather than letting it go up the stack.
Suggest leaving the catch but remove the inner if statement.

[coltonlw]

@kofalt I added a ref to issue #451, which could be addressed as a separate change but since this except branch was clearly added to facilitate debug mode I removed it. I chose to let it go up the stack because without debug mode, the behavior would always be "log the error and send a message saying 'Internal Server Error'" to the client, which we can do just by letting the exception bubble up. Also it seems to me having 500's return JSON is not a good idea because other times we may not catch an error and thus the body may or may not be JSON encoded, which could cause issues for the requester. Status code 500 defined as "A generic error message, given when an unexpected condition was encountered and no more specific message is suitable"

[coltonlw]

I guess whether or not 500's should be JSON is debatable, from description of 5xx status codes: "Except when responding to a HEAD request, the server SHOULD include an entity containing an explanation of the error situation" I think in the case of 500 specifically JSON should not be included since this is the code used for an uncaught exception, compared to other more specific 5xx codes purposefully generated by the server.

[kofalt]

As long as there is there is defined behaviour for Exception bubbling up I'm fine with it 👍

[coltonlw]

webapp2 doesn't define a default handler for any error codes, so it will call WSGIApplication._internal_error which logs to the root logger with level error, however it only logs the exception and not a traceback. I'm going to remove #451 from the list of issues this PR fixes, and update it to include an action item for adding a 500 handler to log the traceback.

[kofalt]

This looks great 👍 lots of branches removed.

@ryansanford Can you confirm we don't need insecure & debug for anything anymore? I don't use it...

[gsfr]

It was my understanding that we were going to remove insecure mode and debug links, but not debug mode, which prints stack traces, IIRC.

I strongly suggest that we keep this PR limited to the first two items and discuss the last one separately.

Just adding a couple of comments on what's mentioned above:

• Given that we are open source, printing a stack trace in prod, doesn't reveal any secrets. Whether it's good form or not is debatable. It certainly makes debugging easier.
• I vote that the API never ever returns anything but JSON.

[coltonlw]

@gsfr I agree leaving 500 errors JSON for the sake of consistency makes sense. Right now we may have some 500's which are not json-encoded, #451 will address that in addition to logging the traceback. Thus, I reverted my changes to that try-except block

I also restored debug mode, I know other frameworks have had security issues where debug mode has a remote code execution vulnerability and thus having a way to enable it in production carries some risk. That's definitely not what this PR was about so I've reverted that change as well.

[gsfr]

Per offline discussion, revert change in api/api.py and then we should be good to go.

[coltonlw]

@gsfr @nagem Can I get a once over and final sign-off from you two before I merge this?

[gsfr]

Thanks, @coltonlw. LGTM, but untested.

[nagem]

I can test tomorrow but just looking it over it LGTM2.