v3: authentication is run after validation

[cubic3d]

Seems like validation of path parameters on HTTP runs before authentication, which can lead to information leaking.

e.g. using

Field(3, "ip", String, "IPv4 address", func() {
	Format(FormatIP)
	Example("88.16.188.162")
})

in a method with e.g. BasicAuth security leads to:

HTTP/1.1 400 Bad Request
Content-Length: 189
Content-Type: application/json
Date: Fri, 24 May 2019 08:44:50 GMT

{
    "fault": false,
    "id": "2BW0dQwf",
    "message": "ip must be formatted as a ip but got value \"1.1.1\", \"1.1.1\" is an invalid ip value",
    "name": "invalid_format",
    "temporary": false,
    "timeout": false
}

instead of:

HTTP/1.1 401 Unauthorized
Content-Length: 105
Content-Type: application/json
Date: Fri, 24 May 2019 09:04:14 GMT
Goa-Error: unauthorized

{
    "fault": false,
    "id": "DMsOV7uv",
    "message": "NOPE",
    "name": "unauthorized",
    "temporary": false,
    "timeout": false
}

BTW: Goa's 400 response above is missing the Goa-Error header - is that a separate issue?

[nitinmohan87]

By design, the authentication runs at the endpoint level and not at the transport level. But the request validations happen at the transport level. I think this might be ok for most use cases. I will defer that to @raphael .

You can however provide your own HTTP middleware to do authentication before any of these validations occur.

[cubic3d]

I see the main problem on enums or regex validations containing exposed values:

http localhost:8000 test=test
HTTP/1.1 400 Bad Request
Content-Length: 197
Content-Type: application/json
Date: Wed, 29 May 2019 10:08:10 GMT

{
    "fault": false,
    "id": "u0aenl2T",
    "message": "value of body.test must be one of \"exposed\", \"enum\", \"values\" but got value \"test\"",
    "name": "invalid_enum_value",
    "temporary": false,
    "timeout": false
}

[raphael]

It should be possible to move the validation of payload (and responses in clients to be consistent) to the endpoint which would alleviate this issue. I'd happily merge a PR that did that.

[cubic3d]

@raphael Do you have any hints to get startet with that PR?

[eric-isakson]

I started looking into this one. I setup a project with a design that demonstrates the issue. It looks like this is only an issue on the http transport code where the authorization related error is merged with the validation errors, the grpc code returns the error right away on auth failure and doesn't get to the other validations. Of course I'm only looking at one simple use case and I know the templates are more complex than this. See my use cases here the http one:

https://github.com/eric-isakson/goa-playground/blob/master/gen/http/secured_service/server/encode_decode.go#L85

and here the grpc one:

https://github.com/eric-isakson/goa-playground/blob/master/gen/grpc/secured_service/server/encode_decode.go#L75

It looks like this code is generated by:

https://github.com/goadesign/goa/blob/v3/http/codegen/server.go#L137

I'm starting to get my head around how the generation works now that I've started investigating this...still a lot to learn. That method references the templates defined later in the same file. I'm still looking at those and started adding a test similar to query-string-validate from:

https://github.com/goadesign/goa/blob/v3/http/codegen/server_decode_test.go#L37

That will exercise this use case. It looks like this may be as easy as moving the authorization related blocks up in the templates and ensuring they return their errors rather than merging them with validation errors.

I'm not sure if there are any impacts or related changes required in the gokit plugin, it looks like it just wraps the functions generated by goa, so I think nothing has to change there.

[raphael]

Sorry @eric-isakson, I lost track of this issue - will take a look.

[eric-isakson]

@raphael no worries, I had not spent any real time on it other than using it to learn more of the generation details. The linked commit in my fork is just trying to capture the issue as a failing test, not really sure what the right fixes might be.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.