Repository: thephpleague/commonmark (php)

Issue:
Invalid multibyte support
### Version(s) affected

2.3.7

### Description

If `mb_internal_encoding()` is anything but UTF-8 and a multibyte character is used before an inline link, output will be garbage such as `ABC ÀÀ [label](url)` -> `ABC ÀÀ  [l[bel](url)`.

Bug is caused by a `mb_strlen` call not passing encoding in `InlineParserEngine`.

A quick regex search found 11 similar calls. Some may be safe because they don't expect multibyte input (URI cannot be multibyte in markdown). In this case they should be converted to non multibyte calls to avoid the confusion.

### How to reproduce

```
mb_internal_encoding('iso-8859-1');
(new CommonMarkConverter)->convert('AAA ÀÀ [label][(https://url)');
```

Pull request:
Fix mbstring calls without explicit encoding
Fixes parsing issues when `mb_internal_encoding()` is set to something other than `UTF-8`

Fixes #951

Patch:
diff --git a/.github/workflows/backwards-compatibility.yml b/.github/workflows/backwards-compatibility.yml
index 1a0079b0..0fab79f5 100644
--- a/.github/workflows/backwards-compatibility.yml
+++ b/.github/workflows/backwards-compatibility.yml
@@ -18,6 +18,13 @@ jobs:
               with:
                   fetch-depth: 0
 
+            # User in the container seems to be different than the cloned repo's owner.
+            # Git doesn't like that as the repo will then be unusable by the owner.
+            # We don't care about this here since this is only  used for running one test.
+            # See https://github.com/actions/runner/issues/2033
+            - name: Workaround directory permissions
+              run: mkdir -p /home/runner/work/_temp/_github_home && printf "[safe]\n\tdirectory = /github/workspace" > /home/runner/work/_temp/_github_home/.gitconfig
+
             - name: "BC Check"
               uses: docker://nyholm/roave-bc-check-ga
               with:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9a5fb421..dae1163d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,10 @@ Updates should follow the [Keep a CHANGELOG](https://keepachangelog.com/) princi
 
 ## [Unreleased][unreleased]
 
+### Fixed
+
+- Fixed parsing issues when `mb_internal_encoding()` is set to something other than `UTF-8` (#951)
+
 ## [2.3.7] - 2022-11-03
 
 ### Fixed
diff --git a/composer.json b/composer.json
index cae273f1..d432b804 100644
--- a/composer.json
+++ b/composer.json
@@ -94,7 +94,8 @@
     "autoload-dev": {
         "psr-4": {
             "League\\CommonMark\\Tests\\Unit\\": "tests/unit",
-            "League\\CommonMark\\Tests\\Functional\\": "tests/functional"
+            "League\\CommonMark\\Tests\\Functional\\": "tests/functional",
+            "League\\CommonMark\\Tests\\PHPStan\\": "tests/phpstan"
         }
     },
     "scripts": {
diff --git a/phpstan.neon.dist b/phpstan.neon.dist
index 84132847..fd6d9075 100644
--- a/phpstan.neon.dist
+++ b/phpstan.neon.dist
@@ -7,3 +7,5 @@ parameters:
      message: '#Parameter .+ of class .+Reference constructor expects string, string\|null given#'
    - path: src/Util/RegexHelper.php
      message: '#Method .+RegexHelper::unescape\(\) should return string but returns string\|null#'
+rules:
+  - 'League\CommonMark\Tests\PHPStan\MbstringFunctionCallRule'
diff --git a/src/Extension/Autolink/UrlAutolinkParser.php b/src/Extension/Autolink/UrlAutolinkParser.php
index e9071703..7fad7f55 100644
--- a/src/Extension/Autolink/UrlAutolinkParser.php
+++ b/src/Extension/Autolink/UrlAutolinkParser.php
@@ -105,7 +105,7 @@ final class UrlAutolinkParser implements InlineParserInterface
             $url = \substr($url, 0, -$diff);
         }
 
-        $cursor->advanceBy(\mb_strlen($url));
+        $cursor->advanceBy(\mb_strlen($url, 'UTF-8'));
 
         // Auto-prefix 'http://' onto 'www' URLs
         if (\substr($url, 0, 4) === 'www.') {
diff --git a/src/Extension/Footnote/Renderer/FootnoteBackrefRenderer.php b/src/Extension/Footnote/Renderer/FootnoteBackrefRenderer.php
index 80b3fda1..3b7bc3c2 100644
--- a/src/Extension/Footnote/Renderer/FootnoteBackrefRenderer.php
+++ b/src/Extension/Footnote/Renderer/FootnoteBackrefRenderer.php
@@ -44,7 +44,7 @@ final class FootnoteBackrefRenderer implements NodeRendererInterface, XmlNodeRen
 
         $attrs->append('class', $this->config->get('footnote/backref_class'));
         $attrs->set('rev', 'footnote');
-        $attrs->set('href', \mb_strtolower($node->getReference()->getDestination()));
+        $attrs->set('href', \mb_strtolower($node->getReference()->getDestination(), 'UTF-8'));
         $attrs->set('role', 'doc-backlink');
 
         $symbol = $this->config->get('footnote/backref_symbol');
diff --git a/src/Extension/Footnote/Renderer/FootnoteRefRenderer.php b/src/Extension/Footnote/Renderer/FootnoteRefRenderer.php
index 7989ac3a..c0c07d7c 100644
--- a/src/Extension/Footnote/Renderer/FootnoteRefRenderer.php
+++ b/src/Extension/Footnote/Renderer/FootnoteRefRenderer.php
@@ -40,7 +40,7 @@ final class FootnoteRefRenderer implements NodeRendererInterface, XmlNodeRendere
 
         $attrs = $node->data->getData('attributes');
         $attrs->append('class', $this->config->get('footnote/ref_class'));
-        $attrs->set('href', \mb_strtolower($node->getReference()->getDestination()));
+        $attrs->set('href', \mb_strtolower($node->getReference()->getDestination(), 'UTF-8'));
         $attrs->set('role', 'doc-noteref');
 
         $idPrefix = $this->config->get('footnote/ref_id_prefix');
@@ -48,7 +48,7 @@ final class FootnoteRefRenderer implements NodeRendererInterface, XmlNodeRendere
         return new HtmlElement(
             'sup',
             [
-                'id' => $idPrefix . \mb_strtolower($node->getReference()->getLabel()),
+                'id' => $idPrefix . \mb_strtolower($node->getReference()->getLabel(), 'UTF-8'),
             ],
             new HtmlElement(
                 'a',
diff --git a/src/Extension/Footnote/Renderer/FootnoteRenderer.php b/src/Extension/Footnote/Renderer/FootnoteRenderer.php
index feddc144..cdd027e6 100644
--- a/src/Extension/Footnote/Renderer/FootnoteRenderer.php
+++ b/src/Extension/Footnote/Renderer/FootnoteRenderer.php
@@ -41,7 +41,7 @@ final class FootnoteRenderer implements NodeRendererInterface, XmlNodeRendererIn
         $attrs = $node->data->getData('attributes');
 
         $attrs->append('class', $this->config->get('footnote/footnote_class'));
-        $attrs->set('id', $this->config->get('footnote/footnote_id_prefix') . \mb_strtolower($node->getReference()->getLabel()));
+        $attrs->set('id', $this->config->get('footnote/footnote_id_prefix') . \mb_strtolower($node->getReference()->getLabel(), 'UTF-8'));
         $attrs->set('role', 'doc-endnote');
 
         return new HtmlElement(
diff --git a/src/Normalizer/SlugNormalizer.php b/src/Normalizer/SlugNormalizer.php
index e56e1c7a..d41ea24c 100644
--- a/src/Normalizer/SlugNormalizer.php
+++ b/src/Normalizer/SlugNormalizer.php
@@ -41,14 +41,14 @@ final class SlugNormalizer implements TextNormalizerInterface, ConfigurationAwar
         // Trim whitespace
         $slug = \trim($slug);
         // Convert to lowercase
-        $slug = \mb_strtolower($slug);
+        $slug = \mb_strtolower($slug, 'UTF-8');
         // Try replacing whitespace with a dash
         $slug = \preg_replace('/\s+/u', '-', $slug) ?? $slug;
         // Try removing characters other than letters, numbers, and marks.
         $slug = \preg_replace('/[^\p{L}\p{Nd}\p{Nl}\p{M}-]+/u', '', $slug) ?? $slug;
         // Trim to requested length if given
         if ($length = $context['length'] ?? $this->defaultMaxLength) {
-            $slug = \mb_substr($slug, 0, $length);
+            $slug = \mb_substr($slug, 0, $length, 'UTF-8');
         }
 
         return $slug;
diff --git a/src/Parser/InlineParserContext.php b/src/Parser/InlineParserContext.php
index c88eff50..796f2f38 100644
--- a/src/Parser/InlineParserContext.php
+++ b/src/Parser/InlineParserContext.php
@@ -83,7 +83,7 @@ final class InlineParserContext
      */
     public function getFullMatchLength(): int
     {
-        return \mb_strlen($this->matches[0]);
+        return \mb_strlen($this->matches[0], 'UTF-8');
     }
 
     /**
diff --git a/src/Parser/InlineParserEngine.php b/src/Parser/InlineParserEngine.php
index a80255ce..42a1bcdc 100644
--- a/src/Parser/InlineParserEngine.php
+++ b/src/Parser/InlineParserEngine.php
@@ -50,7 +50,7 @@ final class InlineParserEngine implements InlineParserEngineInterface
             \assert($parser instanceof InlineParserInterface);
             $regex = $parser->getMatchDefinition()->getRegex();
 
-            $this->parsers[] = [$parser, $regex, \strlen($regex) !== \mb_strlen($regex)];
+            $this->parsers[] = [$parser, $regex, \strlen($regex) !== \mb_strlen($regex, 'UTF-8')];
         }
     }
 
@@ -134,7 +134,7 @@ final class InlineParserEngine implements InlineParserEngineInterface
     private function matchParsers(string $contents): array
     {
         $contents    = \trim($contents);
-        $isMultibyte = \mb_strlen($contents) !== \strlen($contents);
+        $isMultibyte = \mb_strlen($contents, 'UTF-8') !== \strlen($contents);
 
         $ret = [];
 
diff --git a/src/Reference/ReferenceParser.php b/src/Reference/ReferenceParser.php
index d278a39f..c01dd213 100644
--- a/src/Reference/ReferenceParser.php
+++ b/src/Reference/ReferenceParser.php
@@ -181,7 +181,7 @@ final class ReferenceParser
         $cursor->advance();
 
         // spec: A link label can have at most 999 characters inside the square brackets
-        if (\mb_strlen($this->label, 'utf-8') > 999) {
+        if (\mb_strlen($this->label, 'UTF-8') > 999) {
             return false;
         }
 
diff --git a/src/Util/LinkParserHelper.php b/src/Util/LinkParserHelper.php
index e0c7f8d7..e329669b 100644
--- a/src/Util/LinkParserHelper.php
+++ b/src/Util/LinkParserHelper.php
@@ -58,7 +58,7 @@ final class LinkParserHelper
             return 0;
         }
 
-        $length = \mb_strlen($match, 'utf-8');
+        $length = \mb_strlen($match, 'UTF-8');
 
         if ($length > 1001) {
             return 0;
diff --git a/src/Util/RegexHelper.php b/src/Util/RegexHelper.php
index 1bfdf06d..9e56b48c 100644
--- a/src/Util/RegexHelper.php
+++ b/src/Util/RegexHelper.php
@@ -104,13 +104,13 @@ final class RegexHelper
     public static function matchAt(string $regex, string $string, int $offset = 0): ?int
     {
         $matches = [];
-        $string  = \mb_substr($string, $offset, null, 'utf-8');
+        $string  = \mb_substr($string, $offset, null, 'UTF-8');
         if (! \preg_match($regex, $string, $matches, \PREG_OFFSET_CAPTURE)) {
             return null;
         }
 
         // PREG_OFFSET_CAPTURE always returns the byte offset, not the char offset, which is annoying
-        $charPos = \mb_strlen(\mb_strcut($string, 0, $matches[0][1], 'utf-8'), 'utf-8');
+        $charPos = \mb_strlen(\mb_strcut($string, 0, $matches[0][1], 'UTF-8'), 'UTF-8');
 
         return $offset + $charPos;
     }


Test patch:
diff --git a/tests/phpstan/MbstringFunctionCallRule.php b/tests/phpstan/MbstringFunctionCallRule.php
new file mode 100644
index 00000000..174c41eb
--- /dev/null
+++ b/tests/phpstan/MbstringFunctionCallRule.php
@@ -0,0 +1,108 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the league/commonmark package.
+ *
+ * (c) Colin O'Dell <colinodell@gmail.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace League\CommonMark\Tests\PHPStan;
+
+use PHPStan\Analyser\Scope;
+use PHPStan\Rules\Rule;
+use PhpParser\Node;
+
+/**
+ * Custom phpstan rule that:
+ *
+ * 1. Disallows the use of certain mbstring functions that could be problematic
+ * 2. Requires an explicit encoding be provided to all `mb_*()` functions that support it
+ */
+final class MbstringFunctionCallRule implements Rule
+{
+    private array $disallowedFunctionsThatAlterGlobalSettings = [
+        'mb_internal_encoding',
+        'mb_regex_encoding',
+        'mb_detect_order',
+        'mb_language',
+    ];
+
+    private array $encodingParamPositionCache = [];
+
+    public function getNodeType(): string
+    {
+        return Node\Expr\FuncCall::class;
+    }
+
+    public function processNode(Node $node, Scope $scope): array
+    {
+        if (! $node instanceof Node\Expr\FuncCall) {
+            return [];
+        }
+
+        if (! $node->name instanceof Node\Name) {
+            return [];
+        }
+
+        $functionName = $node->name->toString();
+        if (! str_starts_with($functionName, 'mb_')) {
+            return [];
+        }
+
+        if (\in_array($functionName, $this->disallowedFunctionsThatAlterGlobalSettings, true)) {
+            return [\sprintf('Use of %s() is not allowed in this library because it alters global settings', $functionName)];
+        }
+
+        $encodingParamPosition = $this->getEncodingParamPosition($functionName);
+        if ($encodingParamPosition === null) {
+            return [];
+        }
+
+        $arg = $node->args[$encodingParamPosition] ?? null;
+        if ($arg === null) {
+            return [\sprintf('%s() is missing the $encoding param (should be "UTF-8")', $functionName)];
+        }
+
+        if (! $arg instanceof Node\Arg) {
+            return [];
+        }
+
+        $encodingArg = $arg->value;
+        if (! ($encodingArg instanceof Node\Scalar\String_)) {
+            return [\sprintf('%s() must define the $encoding as "UTF-8"', $functionName)];
+        }
+
+        if ($encodingArg->value !== 'UTF-8') {
+            return [\sprintf('%s() must define the $encoding as "UTF-8", not "%s"', $functionName, $encodingArg->value)];
+        }
+
+        return [];
+    }
+
+    private function getEncodingParamPosition(string $function): ?int
+    {
+        if (isset($this->encodingParamPositionCache[$function])) {
+            return $this->encodingParamPositionCache[$function];
+        }
+
+        $reflection = new \ReflectionFunction($function);
+        $params     = $reflection->getParameters();
+
+        $encodingParamPosition = null;
+        foreach ($params as $i => $param) {
+            if ($param->getName() === 'encoding') {
+                $encodingParamPosition = $i;
+                break;
+            }
+        }
+
+        $this->encodingParamPositionCache[$function] = $encodingParamPosition;
+
+        return $encodingParamPosition;
+    }
+}
diff --git a/tests/unit/Parser/InlineParserEngineTest.php b/tests/unit/Parser/InlineParserEngineTest.php
index 5090b13c..cf216ba8 100644
--- a/tests/unit/Parser/InlineParserEngineTest.php
+++ b/tests/unit/Parser/InlineParserEngineTest.php
@@ -14,6 +14,9 @@ declare(strict_types=1);
 namespace League\CommonMark\Tests\Unit\Parser;
 
 use League\CommonMark\Environment\Environment;
+use League\CommonMark\Extension\CommonMark\Node\Inline\Link;
+use League\CommonMark\Extension\CommonMark\Parser\Inline\CloseBracketParser;
+use League\CommonMark\Extension\CommonMark\Parser\Inline\OpenBracketParser;
 use League\CommonMark\Node\Block\Paragraph;
 use League\CommonMark\Node\Inline\Text;
 use League\CommonMark\Parser\Inline\InlineParserMatch;
@@ -76,4 +79,33 @@ final class InlineParserEngineTest extends TestCase
         $this->assertTrue($child instanceof Text);
         $this->assertSame('The quick brown fox jumps over the lazy dog', $child->getLiteral());
     }
+
+    /**
+     * @see https://github.com/thephpleague/commonmark/issues/951
+     *
+     * @runInSeparateProcess to avoid polluting the global environment
+     */
+    public function testMultibyteDetectionRegressionFromIssue951(): void
+    {
+        \mb_internal_encoding('iso-8859-1'); // @phpstan-ignore-line
+
+        $environment = new Environment();
+        $environment->addInlineParser(new CloseBracketParser(), 30);
+        $environment->addInlineParser(new OpenBracketParser(), 20);
+
+        $engine    = new InlineParserEngine($environment, new ReferenceMap());
+        $paragraph = new Paragraph();
+        $engine->parse('AAA ÀÀ [label](https://url)', $paragraph);
+
+        $this->assertCount(2, $paragraph->children());
+
+        $text = $paragraph->firstChild();
+        $this->assertTrue($text instanceof Text);
+        $this->assertSame('AAA ÀÀ ', $text->getLiteral());
+
+        $link = $paragraph->lastChild();
+        $this->assertTrue($link instanceof Link);
+        $this->assertSame('label', $link->firstChild()->getLiteral());
+        $this->assertSame('https://url', $link->getUrl());
+    }
 }