archtechx / laravel-seo

SEO package for Laravel
https://archte.ch/blog/introducing-laravel-seo
MIT License
291 stars 30 forks source link

Appears to be vulnerable to XSS #30

Closed neoighodaro closed 1 year ago

neoighodaro commented 1 year ago

It may appear (may be wrong) that the package is vulnerable to XSS as it does not properly sanitize the input. Example:

$description = 'Quick hotkey to save time instead of pressing "Share" -> "With timecode" -> "Copy" from UI';

$seo->title('Some name')
    ->description($description)
    ->twitter()
    ->twitterCreator('sample');

Completely breaks the description and seems to push the meta directly into the body of the page. Potentially dangerous. Let me know if you need help reproducing it.

Screenshot 2023-05-31 at 08 03 15 Screenshot 2023-05-31 at 08 04 03
neoighodaro commented 1 year ago

For anyone who wants a quick fix (I don't think it's comprehensive but since this is the entry point to blade, we fix the custom blade seo directive):

  1. Add "archtechx/laravel-seo" to composer.json as dont-discover so the service provider is not loaded
  2. Create your own service provider extending the one that comes with this package
  3. Register that provider manually in app.php
  4. Override the boot method as seen below, the only change is the addition of the e helper to seo()->get()
    public function boot(): void
    {
        // ...

        BladeHelper::directive('seo', function (...$args) {
            // ...

            return e(seo()->get($args[0]));
        });
    }
stancl commented 1 year ago

Thank you for reporting this. I've fixed the issue here https://github.com/archtechx/laravel-seo/commit/f6d85e3dfef57f54931a3f3b427f8b1d93deef4e and will make a release shortly.

I've approached the fix in a similar way (see the SEOManager diff in the linked commit). Main difference is that we don't wrap EVERY output in e(), only get() and set() calls. Also the @seo directive calls render(), not get().

Reason for not sanitizing every output of render() is that URLs may come from it (currently only when using flipp or previewify calls) and those shouldn't be escaped. All other values get wrapped in e() and that fixes any potential XSS issues, yeah.

Thanks!