monsieurbiz / SyliusSearchPlugin

A search plugin using Elasticsearch for Sylius
MIT License
45 stars 38 forks source link

[error] - monsieurbiz_search_search with POST empty throw 500 #201

Open cbastienbaron opened 10 months ago

cbastienbaron commented 10 months ago

Hi ,

Facing an issue when a user post an empty search or try to discover some 500 on app

To reproduce

curl -I -XPOST http://127.0.0.1/fr_FR/search
HTTP/1.1 500 Internal Server Error
Server: nginx/1.21.6
... 

https://github.com/monsieurbiz/SyliusSearchPlugin/blob/master/src/Controller/SearchController.php#L102

    public function postAction(Request $request): RedirectResponse
    {
        $query = (array) $request->request->all()['monsieurbiz_searchplugin_search'] ?? [];
        $query = $query['query'] ?? '';

        return $this->redirect(
            $this->generateUrl(
                'monsieurbiz_search_search',
                ['query' => urlencode($query)]
            )
        );
    }

if query param no exist, an empty string is assigned $query but route monsieurbiz_search_search required 1 char minimum https://github.com/monsieurbiz/SyliusSearchPlugin/blob/master/src/Resources/config/routing/shop.yaml#L7

monsieurbiz_search_search:
  path: /search/{query}
  methods: [GET]
  defaults:
    _controller: MonsieurBiz\SyliusSearchPlugin\Controller\SearchController::searchAction
  requirements:
    query: .+
  condition: "not(context.getPathInfo() matches '`^%sylius.security.new_api_route%`') and context.checkElasticsearch()"

a possible solution will be to authorize 0 to n char

monsieurbiz_search_search:
  path: /search/{query}
  methods: [GET]
  defaults:
    _controller: MonsieurBiz\SyliusSearchPlugin\Controller\SearchController::searchAction
  requirements:
    query: .*
  condition: "not(context.getPathInfo() matches '`^%sylius.security.new_api_route%`') and context.checkElasticsearch()"
delyriand commented 1 month ago

Hi @cbastienbaron!

In fact, because of the monsieurbiz_search_post route, you can POST an empty value and this causes a 500 error. Instead of the PR #202, I suggest generating a 404 in the postAction method if $query is empty.

For information, is it possible to reproduce this error with our test app : curl -I -XPOST https://localhost:8000/en_US/search

cbastienbaron commented 1 month ago

Hey @delyriand :vulcan_salute:
yup the goal is to avoid a 500 (empty search or 404 is fine to me :+1: )

delyriand commented 1 month ago

@cbastienbaron,

I pushed a new commit on your PR to throw a 404