aws / chalice

Python Serverless Microframework for AWS
Apache License 2.0
10.66k stars 1.01k forks source link

Request: Route Checking and Response #630

Open wollerman opened 6 years ago

wollerman commented 6 years ago

It's extremely easy setup routes. However, this means it's also easy to make a mistake. The current error message on a bad route is ambiguous and doesn't seem to relate to the missing route. For example, given the below route without the leading /:

@app.route('status', cors=True)
def index():
    return {'status': 'available'}

A request returns:

> http localhost:8000/status
HTTP/1.1 403 Forbidden
Content-Length: 43
Content-Type: application/json
Date: Tue, 05 Dec 2017 18:06:30 GMT
Server: BaseHTTP/0.3 Python/2.7.12
x-amzn-ErrorType: UnauthorizedException
x-amzn-RequestId: 52e78aea-aaf5-455e-8f6f-fece9ceffc80

{
    "message": "Missing Authentication Token"
}

It would make more sense to have information about a bad route. Or perhaps providing a console warning about a request to a non-existent route. Currently the console only shows a 403:

> chalice local
Serving on localhost:8000
127.0.0.1 - - [05/Dec/2017 10:06:30] "GET /status HTTP/1.1" 403 -  # response when above request is made
jamesls commented 6 years ago

Thanks for the feedback, marking as an enhancement.

leason commented 6 years ago

Is anyone working on this? If not I'm happy to open a PR and try to share a patch for it. No idea how complex the fix is yet, but I get bit by this a fair amount since my team is divided into frontend and backend developers and sometimes they fat finger a route. This error causes confusion because it looks like a valid route was requested.

wollerman commented 6 years ago

@leason: I would wager no one is currently working on it.

Looking into the local.py it seems like match_route() is throwing a nice message about no matching route. However, that boils up into a _generate_lambda_event() call which stomps on the lower ValueError. So it seems like it would be really easy to name the ValueError and include that message in the ForbiddenError.

leason commented 6 years ago

Interesting - but that's just for local development server, right? This problem actually affects the deployed lambdas in the same way. For example: https://api.tekata.io/v1/warmup VS https://api.tekata.io/v1/wakeup

warmup is not a valid route, but wakeup is.

wollerman commented 6 years ago

That's true the change I suggested is only for local.

When deployed, I think that might be determined by API gateway. A quick google shows that the error message has been like that since 2015. It seems like a lot of people have this issue. I haven't looked at how the API gateway team is addressing it.

stealthycoin commented 6 years ago

In Chalice 1.1.1 with a real deployed project I get with debug mode on I get a 502 back with a stacktrace of:

Traceback (most recent call last):
  File "/var/task/chalice/app.py", line 587, in __call__
    raise ChaliceError("No view function for: %s" % resource_path)
chalice.app.ChaliceError: No view function for: /status

Which seems pretty clear to me.

Locally I get the same as you. Which I think is an issue of parity. At some point the real one was fixed and the local version got left behind.

jamesls commented 6 years ago

Looking at this now, it seems like things have changed. Here's what I tried:

$ chalice new-project test-bad-route
$ cd test-bad-route/
$ cat app.py
from chalice import Chalice

app = Chalice(app_name='test-bad-route')

@app.route('/')
def index():
    return {'hello': 'world'}

@app.route('status', cors=True)
def bad_route():
    return {'never': 'seen'}

# Now deploy the app and try to request /status
$ chalice deploy
Creating deployment package.
Creating IAM role: test-bad-route-dev
Creating lambda function: test-bad-route-dev
Creating Rest API
Resources deployed:
  - Lambda ARN: arn:aws:lambda:us-west-2:123:function:test-bad-route-dev
  - Rest API URL: https://rest-api.execute-api.us-west-2.amazonaws.com/api/

$ http https://rest-api.execute-api.us-west-2.amazonaws.com/api/status
HTTP/1.1 502 Bad Gateway
Connection: keep-alive
Content-Length: 36
Content-Type: application/json
Date: Mon, 14 May 2018 18:53:14 GMT
Via: 1.1 6acd4ebf1a0179dd8e00eb58764e453a.cloudfront.net (CloudFront)
X-Amz-Cf-Id: HkuXqpV2Bj1AVQw9kATMa-oQnQRXG4jwmk17DIndyrEAEjoTSlx4gQ==
X-Cache: Error from cloudfront

{
    "message": "Internal server error"
}

In local mode I get:

$ http localhost:8000/status
HTTP/1.1 403 Forbidden
Content-Length: 43
Content-Type: application/json
Date: Mon, 14 May 2018 18:54:26 GMT
Server: BaseHTTP/0.6 Python/3.6.5
x-amzn-ErrorType: UnauthorizedException
x-amzn-RequestId: 8a0363a2-c8da-4b2c-853c-bcf4775a66a8

{
    "message": "Missing Authentication Token"
}

Interestingly, the swagger document has the bad route in it:

$ chalice package out
$ cat out/sam.json
...

    "RestAPI": {
      "Type": "AWS::Serverless::Api",
      "Properties": {
        "StageName": "api",
        "DefinitionBody": {
          "swagger": "2.0",
          "info": {
            "version": "1.0",
            "title": "test-bad-route"
          },
          "schemes": [
            "https"
          ],
          "paths": {
            "/": {
              "get": {
                "consumes": [
                  "application/json"
                ],
                "produces": [
                  "application/json"
                ],
                "responses": {
                  "200": {
                    "description": "200 response",
                    "schema": {
                      "$ref": "#/definitions/Empty"
                    }
                  }
                },
                "x-amazon-apigateway-integration": {
...
                }
              }
            },
            "status": {  # <---- bad route
        }

Should we just validate that routes must start with /? We can optionally try to make this "just work", but I think validation makes more sense here. Thoughts?

wollerman commented 6 years ago

@jamesls I think validation is the most elegant solution. With a nice message it will provide clarity to what exactly is wrong. And then you don't have to worry about extra handling that may change in the future.

chmreid commented 5 years ago

Bump - is there any work happening on this issue?