Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SkipStatusCodePagesAttribute should run before AuthorizeAttribute #10317

Closed
huan086 opened this issue May 17, 2019 · 6 comments · Fixed by #38509
Closed

SkipStatusCodePagesAttribute should run before AuthorizeAttribute #10317

huan086 opened this issue May 17, 2019 · 6 comments · Fixed by #38509
Assignees
Labels
affected-medium This issue impacts approximately half of our customers bug This issue describes a behavior which is not expected - a bug. feature-mvc-execution-pipeline Features related to how MVC executes a controller or razor page old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels severity-minor This label is used by an internal tool
Milestone

Comments

@huan086
Copy link
Contributor

huan086 commented May 17, 2019

Is your feature request related to a problem? Please describe.

[SkipStatusCodePages] is meant to be used in actions that are API calls, so that the StatusCodePagesMiddleware does not interfere with the response status code and body.

API actions are almost always decorated with [Authorize]. When user is not authorized, AuthorizeFilter short circuits and returns 401. Due to the short circuit, IResourceFilter, which SkipStatusCodePagesAttribute inherits, does not run, thus StatusCodePagesMiddleware runs and modifies the status code and body. The API caller does not receive 401 with empty body.

Describe the solution you'd like

Ideally, the StatusCodePagesMiddleware does not run when [SkipStatusCodePages], thus the API caller receives 401 with empty body.

This can be achieved by having SkipStatusCodePagesAttribute inherit from IAlwaysRunResultFilter instead.

Describe alternatives you've considered

Modifying the middleware pipeline with custom middleware. But this dissociates the action that needs SkipStatusCodePages from the code that does the work

Additional context

@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 17, 2019
@mkArtakMSFT mkArtakMSFT added 1 - Ready bug This issue describes a behavior which is not expected - a bug. labels May 20, 2019
@mkArtakMSFT mkArtakMSFT added this to the 3.0.0-preview8 milestone May 20, 2019
@mkArtakMSFT
Copy link
Member

Thanks for contacting us, @huan086. Would you like to send a PR for this? We'd happily consider it!

@mkArtakMSFT mkArtakMSFT added the help wanted Up for grabs. We would accept a PR to help resolve this issue label May 20, 2019
@huan086
Copy link
Contributor Author

huan086 commented May 30, 2019

I'm thinking we need to implement IOrderedFilter as well, so that when multiple IAlwaysRunResultFilter are present and some of them short-circuits, it'll be possible to make sure SkipStatusCodePagesAttribute runs first. @mkArtakMSFT what do you think?

huan086 added a commit to huan086/AspNetCore that referenced this issue Jun 2, 2019
huan086 added a commit to huan086/AspNetCore that referenced this issue Jun 2, 2019
@mkArtakMSFT mkArtakMSFT modified the milestones: 3.0.0-preview8, Backlog Jun 25, 2019
@pranavkm pranavkm removed the help wanted Up for grabs. We would accept a PR to help resolve this issue label Sep 2, 2019
@kevinchalet
Copy link
Contributor

@mkArtakMSFT is there a chance this bug could be fixed in 5.0? It sadly makes [SkipStatusCodePages] hard to use in mixed API/views applications using token authentication.

@mkArtakMSFT
Copy link
Member

mkArtakMSFT commented Jun 1, 2020

@kevinchalet, as much as I wish we could, I don't think we will get to this. Not during 5.0 timeline.

@trannamtrung1st
Copy link

Any update on this? Currently I'm using a custom middleware. It seems a little bit weird but it's working. I can pass my custom options into the middleware constructor.

public class ApiStatusCodeMiddleware
    {
        private readonly RequestDelegate _next;

        public ApiStatusCodeMiddleware(RequestDelegate next)
        {
            _next = next;
        }

        public async Task InvokeAsync(HttpContext context)
        {
            await _next(context);
            var isApiRequest = context.Request.Path.StartsWithSegments("/api");
            if (isApiRequest && context.Response.StatusCode >= 400 && context.Response.StatusCode < 500)
            {
                using var body = context.Response.Body;
                body.Flush();
            }
        }

    }

@captainsafia captainsafia added affected-medium This issue impacts approximately half of our customers severity-minor This label is used by an internal tool labels Feb 19, 2021
@javiercn javiercn added the feature-mvc-execution-pipeline Features related to how MVC executes a controller or razor page label Apr 18, 2021
@ComtelJeremy
Copy link

Hello, will this perhaps be fixed in 6?

We just ran into this and burned a day as our develop environment worked fine, but then we hit issues with UseStatusCodePagesWithReExecute called in our staging environment. Nothing about this behavior is in the docs that we could find either, but if it is there and we missed it, we apologize.

@pranavkm pranavkm added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-medium This issue impacts approximately half of our customers bug This issue describes a behavior which is not expected - a bug. feature-mvc-execution-pipeline Features related to how MVC executes a controller or razor page old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels severity-minor This label is used by an internal tool
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants