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

All HTTP responses return with status code 200 #411

Closed
KIKOmanasijev opened this issue Nov 6, 2024 · 3 comments
Closed

All HTTP responses return with status code 200 #411

KIKOmanasijev opened this issue Nov 6, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@KIKOmanasijev
Copy link
Contributor

Version

^5.0.0

What did you expect to happen?

I expect the following dummy endpoint to return status code 400, but whenever I test it via postman it returns status code 200. This code is on a fresh Bedrock+Sage instance, with no other plugins activated.

The bug only seems to be present if we are using ^5.0.0 Acorn, while on on Acorn ^4.3.0 the response status code from the following endpoint is 400.

add_action('rest_api_init', function () {
    register_rest_route('custom/v1', '/status', [
        'methods' => 'GET',
        'callback' => 'custom_status_endpoint',
    ]);
});

function custom_status_endpoint()
{
    $response = [
        'status' => 'ok',
    ];

    $rest_response = new WP_REST_Response($response);
    $rest_response->set_status(400);

    return $rest_response;
}

What actually happens?

The endpoint returns status code 200, although the response code is explicitly set to 400. (Throwing a generic exception, also returns status code 200).

Steps to reproduce

  1. Use Acorn ^5.0.0
  2. Create a WordPress REST API route.
  3. Make it explicitly throw an Exception or set the response status code to 4xx.
  4. Hit the endpoint via Postman.
  5. Status code is still 200, even though it should be 4xx.

System info

  • macOS 15.
  • Macbook M1 Pro
  • Herd Pro

Log output

No response

Please confirm this isn't a support request.

Yes

@KIKOmanasijev KIKOmanasijev added the bug Something isn't working label Nov 6, 2024
@KIKOmanasijev
Copy link
Contributor Author

The only temporary solution I found is to add the following code:

 if ( $response->isServerError() ){
            return;
        }

inside src/Roots/Acorn/Application/Concerns/Bootable.php after line 200.

Not sure if this solution is going to cause any side effects. Here is the full registerRequestHandler method, with my modified code:

protected function registerRequestHandler(
        \Illuminate\Http\Request $request,
        ?\Illuminate\Routing\Route $route
    ): void {
        $path = Str::finish($request->getBaseUrl(), $request->getPathInfo());

        $except = collect([
            admin_url(),
            wp_login_url(),
            wp_registration_url(),
        ])->map(fn ($url) => parse_url($url, PHP_URL_PATH))->unique()->filter();

        $api = parse_url(rest_url(), PHP_URL_PATH);

        if (
            Str::startsWith($path, $except->all()) ||
            Str::endsWith($path, '.php')
        ) {
            return;
        }

        if (
            $isApi = Str::startsWith($path, $api) &&
            redirect_canonical(null, false)
        ) {
            return;
        }

        add_filter('do_parse_request', function ($condition, $wp, $params) use ($route) {
            if (! $route) {
                return $condition;
            }

            return apply_filters('acorn/router/do_parse_request', $condition, $wp, $params);
        }, 100, 3);

        if ($route->getName() !== 'wordpress') {
            add_action('parse_request', fn () => $this->handleRequest($request));

            return;
        }

        $config = $this->config->get('router.wordpress', ['web' => 'web', 'api' => 'api']);

        $route->middleware($isApi ? $config['api'] : $config['web']);

        if ( $response->isServerError() ){
            return;
        }

        ob_start();

        remove_action('shutdown', 'wp_ob_end_flush_all', 1);
        add_action('shutdown', fn () => $this->handleRequest($request), 100);
    }

@trajche
Copy link
Contributor

trajche commented Nov 6, 2024

Confirmed from my side too. This breaks WooCommerce REST API too! 🚨

image

@Log1x
Copy link
Member

Log1x commented Nov 6, 2024

Thank you for reporting this. This would've been introduced in #291.

This bug is caused by Laravel's HTTP Response thinking the status is always 200 due to {any?} otherwise being a valid route.

This currently affects v5 alpha/beta as well as users of v4 who have opted into ACORN_ENABLE_EXPERIMENTAL_WORDPRESS_REQUEST_HANDLER.

We will get releases out for v4 and v5 beta as well as get some sort of announcement out to make sure those using the experimental flag or v5 in production get updated ASAP.

EDIT: https://discourse.roots.io/t/acorn-v4-v5-router-fix-for-response-codes/28890

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants