allow token parameter from the POST body
#31561
Conversation
As per go-gitea#28390 this will be disallowed as a URL parameter for security reasons, but in the POST body it is secure and required for some flows.
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/XS
|
cc @jackHay22 @kdumontnu ; sorry for the @'s but would you be able to look at this? As I understand it, without this patch, OIDC introspection will break in 1.23 when |
|
I think this needs more discussions about the security. I couldn't find the big difference between GET and POST via API requests. |
|
Thanks @slingamn, this seems like a reasonable addition. Could you verify that the "token" field is not used in any other POSTs for the API as that would otherwise conflict, as well could you add some tests (that don't need to be super involved)? If not, let me know and I can make a complimentary PR. |
|
I don't think this is a good idea to accept that for all requests as this should be used for specific use cases in specific endpoints instead |
|
@lunny for the security rationale for the original change, see this comment, which clarifies that the issue is confined to GET and doesn't affect POST: #28390 (comment) |
pull-request-size
bot
added
size/M
See discussion on #31561 for some background. The introspect endpoint was using the OIDC token itself for authentication. This fixes it to use basic authentication with the client ID and secret instead: * Applications with a valid client ID and secret should be able to successfully introspect an invalid token, receiving a 200 response with JSON data that indicates the token is invalid * Requests with an invalid client ID and secret should not be able to introspect, even if the token itself is valid Unlike #31561 (which just future-proofed the current behavior against future changes to `DISABLE_QUERY_AUTH_TOKEN`), this is a potential compatibility break (some introspection requests without valid client IDs that would previously succeed will now fail). Affected deployments must begin sending a valid HTTP basic authentication header with their introspection requests, with the username set to a valid client ID and the password set to the corresponding client secret.
I encountered this while trying to patch Forgejo but thought that I should ask here as well about the validity of the proposed fix. It seems to me that the intent of #28390 was to deprecate passing
tokenas a URL parameter, which is a security issue. However, blocking attempts to read from(http.Request).Formmeans thattokencannot be read from the POST body either.Since OIDC introspection actually mandates reading the token from the POST body, the proposed fix here is to check
(http.Request).PostFormfirst and return successfully on finding the token there. (I'm only checking fortokenand not foraccess_token, since I'm not aware of any flows that sendaccess_tokenin the POST body.)Thanks for your time.