Repository: packit/packit
Issue: [BUG] Packit breaks the spec file when PR commits reference macros

While attempting to integrate Packit, I've been playing around to see what works and what doesn't, and what compatibility changes need to be made. At some point I've started seeing that it's failing with the following error:
```console
2022-09-15 18:12:44.648 utils.py          ERROR  Preparation of the repository for creation of an SRPM failed: /tmp/tmpqy71ei4a: line 269: Macro % has illegal name (%define)
```

Since this is the only information that appears in the log, it took me a while to get to debugging it. I've started a `packit/packit` container in podman, tried out `packit srpm` and didn't see any more details of what's happening. And by the time the command exited, there was no temporary file on disk anymore, to check out what's in it.
So I've gone ahead, grepped the source for where this exception is originating and started adding `ipdb.set_trace()` in places.
This allowed me to confirm that it is the call here https://github.com/packit/packit/blob/ad9ef49f7f7f09ad269d66dd6b46ec47b9517fc6/packit/upstream.py#L976 that produces the exception, I've gone down the stack and found that this specific invocation of `rpm.spec()` errors out at https://github.com/packit/specfile/blob/91743a5705e3049ff98619a103da79f70d30d1e0/specfile/rpm.py#L319. But the surrounding code "eats" the stderr/out so it wasn't as obvious.
While holding a breakpoint, I've been able to get a hold of the copy of that modified spec file and copied it out of the container. I checked the diff and learned that the only difference is the newly generated changelog section. And the error line number checked out — it indeed contained the `%define` text in it.

From there, it was obvious that it was coming from the commits in the PR. There's a fragment of the changelog generated by Packit:
```rpm-spec
- wip! echo 2 spec (Sviatoslav Sydorenko)
- wip! echo (Sviatoslav Sydorenko)
- wip! drop "%define" prefix (Sviatoslav Sydorenko)
- wip! use "%define" in user macros (Sviatoslav Sydorenko)
- Revert "wip! use "%define" in the spec" (Sviatoslav Sydorenko)
- wip! use "%define" in the spec (Sviatoslav Sydorenko)
- wip! Define `upstream_version` macro via user conf (Sviatoslav Sydorenko)
```

So I think that to mitigate this silly (but nontrivial to track down) error, Packit should escape the changelog entries it sources with something like `changelog.replace('%', '%%')` by default.

Changed files:
tests/integration/test_changelog_helper.py, tests/unit/test_utils.py, packit/utils/changelog_helper.py

diff --git a/packit/utils/changelog_helper.py b/packit/utils/changelog_helper.py
index f45d613e..dbb54606 100644
--- a/packit/utils/changelog_helper.py
+++ b/packit/utils/changelog_helper.py
@@ -1,6 +1,7 @@
 # Copyright Contributors to the Packit project.
 # SPDX-License-Identifier: MIT
 
+import re
 import shutil
 import logging
 from typing import Optional
@@ -67,6 +68,15 @@ class ChangelogHelper:
 
         return "\\n".join(map(lambda line: line.rstrip(), messages))
 
+    @staticmethod
+    def sanitize_entry(entry: str) -> str:
+        # escape macro references and macro/shell/expression expansions
+        # that could break spec file parsing
+        entry = re.sub(r"(?<!%)%(?=(\\w+|[{[(]))", "%%", entry)
+        # prepend asterisk at the start of a line with a space in order
+        # not to break identification of entry boundaries
+        return re.sub(r"^[*]", " *", entry, flags=re.MULTILINE)
+
     def update_dist_git(self, full_version: str, upstream_tag: str) -> None:
         """
         Update the spec-file in dist-git:
@@ -92,6 +102,7 @@ class ChangelogHelper:
                 after=self.up.get_last_tag(upstream_tag), before=upstream_tag
             )
         )
+        comment = self.sanitize_entry(comment)
         try:
             self.dg.set_specfile_content(
                 self.up.specfile,
diff --git a/tests/integration/test_changelog_helper.py b/tests/integration/test_changelog_helper.py
index 51d1ef8e..2295e8dc 100644
--- a/tests/integration/test_changelog_helper.py
+++ b/tests/integration/test_changelog_helper.py
@@ -114,3 +114,22 @@ def test_do_not_update_distgit_with_autochangelog(
 
     with downstream._specfile.sections() as sections:
         assert "%autochangelog" in sections.changelog
+
+
+def test_update_distgit_unsafe_commit_messages(upstream, distgit_instance):
+    _, downstream = distgit_instance
+    package_config = upstream.package_config
+    flexmock(upstream).should_receive("get_commit_messages").and_return(
+        "* 100% of tests now pass\\n"
+        "* got rid of all shell (%(...)) and expression (%[...]) expansions\\n"
+        "* removed all %global macros\\n"
+        "* cleaned up %install section\\n"
+    )
+
+    ChangelogHelper(upstream, downstream, package_config).update_dist_git(
+        upstream_tag="0.1.0", full_version="0.1.0"
+    )
+
+    # FIXME: this doesn't currently work because of a RPM bug
+    # make sure only one changelog entry has been added
+    # assert len(downstream.specfile.rpm_spec.sourceHeader[rpm.RPMTAG_CHANGELOGTEXT]) == 2
diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py
index e2044128..e1e40724 100644
--- a/tests/unit/test_utils.py
+++ b/tests/unit/test_utils.py
@@ -27,6 +27,7 @@ from packit.utils.source_script import create_source_script
 from packit.utils.upstream_version import requests, get_upstream_version
 from packit.utils.lookaside import configparser, pyrpkg, get_lookaside_sources
 from packit.utils.koji_helper import KojiHelper
+from packit.utils.changelog_helper import ChangelogHelper
 
 
 @pytest.mark.parametrize(
@@ -844,3 +845,21 @@ def test_koji_helper_get_candidate_tag(branch, tag):
 )
 def test_koji_helper_get_stable_tags(tag, stable_tags):
     assert KojiHelper.get_stable_tags(tag) == stable_tags
+
+
+@pytest.mark.parametrize(
+    "original, sanitized",
+    [
+        ("- 100% of tests now pass", "- 100% of tests now pass"),
+        ("- removed all %global macros", "- removed all %%global macros"),
+        ("- cleaned up %install section", "- cleaned up %%install section"),
+        (
+            "- got rid of all shell (%(...)) and expression (%[...]) expansions",
+            "- got rid of all shell (%%(...)) and expression (%%[...]) expansions",
+        ),
+        ("- first item\\n* second item", "- first item\\n * second item"),
+        ("* first item\\n* second item", " * first item\\n * second item"),
+    ],
+)
+def test_changelog_helper_sanitize_entry(original, sanitized):
+    assert ChangelogHelper.sanitize_entry(original) == sanitized