When malicious data is passed in the url, invalid json+ld data is generated. This could possibly lead to security issues?

This can be reproduced by requesting a page using the following wget command:

wget http://localhost/index.php/filter/WOORD%5c%22}]DitMagNietKunnen,alert(1)" --content-on-error

Which leads to invalid json+ld data in ListItem 3:

<script type="application/ld+json">
{
          "@context": "https://schema.org",
          "@type": "BreadcrumbList",
          "itemListElement": [{
            "@type": "ListItem",
            "position": "1",
            "name": "Home",
            "item": "http://localhost/index.php/"
          },{
              "@type": "ListItem",
              "position": "2",
              "name": "Filter",
              "item": "http://localhost/index.php/"
            },{
              "@type": "ListItem",
              "position": "3",
              "name": "WOORD\\"}]DitMagNietKunnen,alert(1)",
              "item": "http://localhost/index.php/"
            }]}
</script>

I don't know if there is a specific reason why the EasyBreadcrumbStructuredDataJsonLd class generates the json+ld data by concatenating strings, but I think that should be replaced by json_encode. I'll create a MR for that shortly.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

Ruuds created an issue. See original summary.

ruuds’s picture

Status: Active » Needs review

The test failures are not related. The tests are broken anyway.

spuky’s picture

Status: Needs review » Reviewed & tested by the community

looks good to me json encode is the for sure the better way to go...

norman.lol’s picture

Status: Reviewed & tested by the community » Needs work

Isn't $position always 1 now?

norman.lol’s picture

A single json_encode($value) without any flag also is not enough. You need at least json_encode($value, JSON_UNESCAPED_SLASHES) to prevent slashes in the URLs being escaped.

norman.lol’s picture

Assigned: Unassigned » norman.lol
norman.lol’s picture

Status: Needs work » Needs review
ruuds’s picture

Great suggestion, looks good to me.

norman.lol’s picture

Assigned: norman.lol » Unassigned
spuky’s picture

  • spuky committed 17a06632 on 2.x authored by Ruuds
    Issue #3446802 by leymannx, Ruuds, spuky, emrekerimar: Invalid json+ld...

spuky credited emrekerimar.

spuky’s picture

Status: Needs review » Fixed
norman.lol’s picture

Yeah, I think we've just fixed the related issue as well. json_encode together with the constants we use makes it quite safe against XSS.

  • spuky committed 17a06632 on feature/megre_dynamic-breadcrumbs authored by Ruuds
    Issue #3446802 by leymannx, Ruuds, spuky, emrekerimar: Invalid json+ld...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.