Intent.AspNetCore.Identity.AccountController.RefreshToken is a potential security vulnerability

[shainegordon]

What happened?

The signature for RefreshToken is

[HttpPost]
[AllowAnonymous]
public async Task<ActionResult<TokenResultDto>> RefreshToken2(string authenticationToken, string refreshToken)

You'll see that the Swagger doc generated for this looked like this

"/api/Account/RefreshToken": {
      "post": {
        "tags": [
          "Account"
        ],
        "parameters": [
          {
            "name": "authenticationToken",
            "in": "query",
            "schema": {
              "type": "string"
            }
          },
          {
            "name": "refreshToken",
            "in": "query",
            "schema": {
              "type": "string"
            }
          }
        ],

As you can see, the parameters are expected to be in the query string, which is not ideal

IMO, the signature should be

[HttpPost]
[AllowAnonymous]
public async Task<ActionResult<TokenResultDto>> RefreshToken(RefreshTokenDto refreshTokenDto)

Which will result in the following swagger

"/api/Account/RefreshToken": {
      "post": {
        "tags": [
          "Account"
        ],
        "requestBody": {
          "content": {
            "application/json": {
              "schema": {
                "$ref": "#/components/schemas/RefreshTokenDto"
              }
            },
            "text/json": {
              "schema": {
                "$ref": "#/components/schemas/RefreshTokenDto"
              }
            },
            "application/*+json": {
              "schema": {
                "$ref": "#/components/schemas/RefreshTokenDto"
              }
            }
          }
        },

As you can see, this now has a request body instead

What version of Intent Architect are you using?

4.0 and 4.1 beta

Additional information

No response

[dandrejvv]

Good day @shainegordon , we can alter the pattern to be more DTO based so that should not be a problem, thank you for pointing that out. Will get back to you on this Issue.

[shainegordon]

Thanks @dandrejvv

Most of my comments/bugs are the result of using the Swagger OpenApi specification to generate API clients.

The following DTO's are also incorrect IMO

    public class TokenResultDto
    {
        public string? AuthenticationToken { get; set; }
        public string? RefreshToken { get; set; }
    }

    public class LoginDto
    {
        public string? Email { get; set; }
        public string? Password { get; set; }
    }

These fields should NOT be nullable, because this means when you generate for instance a Typescript client, you'll get something like the following

login: async(body: LoginDto | undefined, abortSignal?: AbortSignal | undefined) : Promise<TokenResultDto> => {
    ...
}

export interface LoginDto {
    email?: string | undefined;
    password?: string | undefined;
}

export interface TokenResultDto {
    authenticationToken?: string | undefined;
    refreshToken?: string | undefined;
}

A consumer should definitely not be able to make a login request where they send an email, and not a password. Same for the response, there should never be a case where the authenticationToken comes back as null/undefined, while the refreshToken has a value.

For now I am getting around this by updating the code, and using //IntentIgnore and //IntentMatch

[dandrejvv]

Hi @shainegordon, we can certainly do this as part of the original change. We have prioritized this ticket and will look into it soon.

[dandrejvv]

Good day @shainegordon. We have recently published a pre-release of the Intent.AspNetCore.Identity.AccountController aligning it more closely with the Microsoft one that comes with .NET 8, thus resulting in a major version bump of that module. Please try 3.0.1-pre.0 and let me know if this works for you.