fix: Fix 2 template properties deserialization WPB-28090

[spoonman01]

• Add 2 new templates for PR ready for review and reopened

---

PR Submission Checklist for internal contributors

• The PR Title

  • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
  • contains a reference JIRA issue number like SQPIT-764
  • answers the question: If merged, this PR will: ... ³

• The PR Description

  • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

---

What's new in this PR?

Issues

Add new template for PR re-opened
Add new template for PR ready for review
Fix for “pull_request_review_comment.created fails even though there's a template for it“
Fix for review.body field not always being present
Fix for "merged" field not always being present
"template not found" logged as INFO instead of ERROR
Health checks are logged, polluting the logs

Causes (Optional)

Template not to work for such events

Solutions

Make fields nullable, also add 2 more events, log correctly ignoring health

---

References

1. https://sparkbox.com/foundry/semantic_commit_messages
2. https://github.com/wireapp/.github#usage
3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

[semgrep-code-wireapp]

Semgrep identified an issue in your code:

The app service maps GHAPP_SERVER_PORT without a host-IP restriction, exposing the GitHub app on all host interfaces. Any network-reachable attacker can scan the host and send requests to the application port.

More details about this

The app service publishes ${GHAPP_SERVER_PORT:-8083} as a host port without a host IP, so Docker binds it to all host interfaces (0.0.0.0). If GHAPP_SERVER_PORT is unset, the application listens at http://<host>:8083; if it is set, the same value controls the externally reachable host port. This makes the GitHub app endpoint accessible from any network that can reach the Docker host, rather than only from the local machine or an explicitly trusted interface.

A plausible attack is:

1. An attacker scans the host, for example with nmap -p 8083 <host-ip>, and finds the port open because the Compose mapping exposes it on every interface.
2. The attacker sends requests such as curl http://<host-ip>:8083/ or probes application API routes served by the app container.
3. The app processes those requests as external traffic; any unauthenticated route, exposed debugging endpoint, or request-handling flaw can then be used remotely to access application data or invoke GitHub integration behavior.
4. Because GHAPP_SERVER_PORT also comes from the environment, changing that variable can unintentionally move the same service to another publicly reachable port without changing the Compose file.

To resolve this comment:

✨ Commit fix suggestion

Suggested change

	- "${GHAPP_SERVER_PORT:-8083}:${GHAPP_SERVER_PORT:-8083}"
	- "127.0.0.1:${GHAPP_SERVER_PORT:-8083}:${GHAPP_SERVER_PORT:-8083}"

View step-by-step instructions

1. Bind the published application port to localhost by changing the mapping to 127.0.0.1:${GHAPP_SERVER_PORT:-8083}:${GHAPP_SERVER_PORT:-8083}.
2. Alternatively, if the application does not need host access, remove the ports entry and let other Compose services access it through the internal network.
3. Remove Redis’s ports entry unless Redis must be accessed from the host. The app service can reach Redis using redis:6379 without publishing Redis externally.
4. Alternatively, if external access is required, replace 127.0.0.1 with the specific trusted host interface address and restrict access with firewall rules. Binding without a host address exposes the port on all host interfaces.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

• /fp <comment> for false positive
• /ar <comment> for acceptable risk
• /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by port-all-interfaces.

_{You can view more details about this finding in the Semgrep AppSec Platform.}

[claude]

Code Review — fix: Fix 2 template properties deserialization WPB-28090

Thanks for this — the direction is right (GitHub webhook payloads are much more nullable than they look, and the two missing templates are a real gap). A few things I'd like to raise before merge, plus some smaller notes.

Note: there is no CLAUDE.md in this repo, so I reviewed against the existing conventions in the codebase (kotlinx.serialization models, detekt config, template layout).

---

🔴 1. The same bug class is left unfixed: body is still non-nullable

PullRequest.body, Issue.body and Comment.body are all declared val body: String with no default:

• src/main/kotlin/com/wire/github/response/model/PullRequest.kt:11
• src/main/kotlin/com/wire/github/response/model/Issue.kt:11
• src/main/kotlin/com/wire/github/response/model/Comment.kt:8

GitHub sends "body": null whenever a PR/issue is opened with an empty description — which is common. KtxSerializer has coerceInputValues = true, but coercion only kicks in for properties that have a default value, so an explicit null here still throws SerializationException and the whole event is dropped. This is exactly the failure mode you're fixing for merged/Review, so it seems worth fixing in the same PR:

val body: String? = null

(Templates already handle this fine — mustache renders null as empty, and pull_request.opened.template doesn't reference body at all.)

🔴 2. Deserialization failures still return HTTP 500 to GitHub

Routing.kt:87:

val response = KtxSerializer.json.decodeFromString<GitHubResponse>(payload)

This is unguarded, so any payload shape we haven't modelled yet propagates out as a 500. GitHub treats that as a delivery failure and retries, so one unmodelled field turns into repeated failing deliveries. Making individual fields nullable is whack-a-mole against an unbounded payload surface; a try/catch here would make the endpoint robust to the next field we get wrong:

val response = try {
    KtxSerializer.json.decodeFromString<GitHubResponse>(payload)
} catch (exception: SerializationException) {
    application.log.error("Failed to deserialize $event delivery $delivery", exception)
    return@post call.response.status(HttpStatusCode.OK) // or BadRequest, if you want GitHub to surface it
}

Worth deciding deliberately between OK (swallow, no retries) and BadRequest (visible in GitHub's delivery UI, no retries) — either is better than 500.

🟠 3. Review.body = null now sends an empty message to the conversation

This one is a behaviour regression introduced by this PR. pull_request_review.submitted.template wraps its entire content in {{#review.body}}:

{{#review.body}}**[{{repository.fullName}}]** Pull request review ... {{/review.body}}

When a reviewer approves without leaving a comment, GitHub sends "body": null:

• Before this PR: deserialization threw → 500 → nothing sent (bad, but visible).
• After this PR: it deserializes, the section is skipped, and populateTemplate returns "\n" — a non-null, whitespace-only string. Routing.kt:95 only null-checks (messageTemplate?.let { ... }), so we now post a blank message into the Wire conversation on every comment-less approval.

Suggested fix in TemplateHandler.populateTemplate (or at the call site):

.toString()
.takeIf { it.isNotBlank() }

🟠 4. Review.user is nullable but the template dereferences it unguarded

pull_request_review.submitted.template:1 uses {{review.user.login}}. With user: User? = null that silently renders was submitted by ****. If user really can be absent, the template should guard it:

{{#review.user}}**{{login}}**{{/review.user}}

Alternatively — is user actually nullable in the review payload? If the real cause was body, I'd keep user non-null rather than loosening a field we then dereference. Loosening a type we depend on just moves the failure from "loud 500" to "malformed message".

---

🟡 Smaller points

Scope creep vs. the PR description. The description only mentions nullability + 2 templates, but the diff also changes docker-compose.yml, helm/.../NOTES.txt and two log levels. They look individually fine (I verified nothing in the codebase references WIRE_SDK_USER_ID / WIRE_SDK_EMAIL / WIRE_SDK_PASSWORD / WIRE_SDK_ENVIRONMENT / WIRE_ENV anymore, so those removals are correct cleanup) — but a line in the description, or a separate commit, would make them easier to review.

docker-compose.yml port default is only half applied. You added :-8083 to the ports mapping but not to the env var on line 10:

- GHAPP_SERVER_PORT=${GHAPP_SERVER_PORT}   # <- still undefaulted
ports:
  - "${GHAPP_SERVER_PORT:-8083}:${GHAPP_SERVER_PORT:-8083}"

If GHAPP_SERVER_PORT is unset, Compose still injects GHAPP_SERVER_PORT= (key present, value empty). EnvironmentVariables.kt uses getOrDefault("GHAPP_SERVER_PORT", "8083"), which only defaults on a missing key — so it gets "" and "".toInt() throws NumberFormatException at startup. Two options:

- GHAPP_SERVER_PORT=${GHAPP_SERVER_PORT:-8083}

and/or harden the parsing: System.getenv("GHAPP_SERVER_PORT")?.toIntOrNull() ?: 8083.

Logging. Downgrading the trace { } block from info to debug is a nice improvement — that text can include request details, so it shouldn't be at default level. Two nits on the TemplateHandler changes:

• logger.info("Template found for this action: ${template.name}") fires on every single webhook delivery. debug seems more appropriate for a per-request success path.
• logger.info("MustacheNotFoundException: $exception") — downgrading from error is right (unsupported events are expected, not errors), but consider debug plus a human-readable, parameterised message so the string isn't built when the level is off:

  logger.debug("No template for event={} action={}", event, response.action)

Template duplication / wording. pull_request.opened, .reopened and .ready_for_review are now three byte-identical templates apart from the headline. A mustache partial ({{>pull_request_summary}}) would keep them in sync as fields evolve. Also:

• "New Pull Request Reopened!" reads a little oddly — a reopened PR isn't new. "Pull Request Reopened!" is cleaner.
• ready_for_review reuses 🟢, identical to opened, so the two are indistinguishable at a glance. Maybe 👀 or 📣 for ready-for-review.
• reopened attributes to {{pullRequest.user.login}} (the PR author), whereas pull_request.closed.template and issues.reopened.template use {{sender.login}} (whoever acted). For a reopen, sender is probably what you want — the person reopening isn't necessarily the author.

---

🧪 Test coverage

This is the biggest gap for me. There are no tests for either new template, and no pull_request.ready_for_review.json / pull_request.reopened.json fixtures.

More importantly: src/test/fixtures/events/*.json and src/test/fixtures/messages/*.txt exist but nothing in src/test/kotlin references them — I grepped for fixtures, getResource, readText and File( and found no usages, so the whole corpus is currently dead weight. A small data-driven test would have caught both bugs this PR fixes, plus #1 and #3 above:

@ParameterizedTest
@MethodSource("fixtures")
fun `given fixture event, when handled, then rendered message matches expected`(name: String) {
    val payload = readFixture("events/$name.json")
    val expected = readFixture("messages/$name.txt")
    val event = name.substringBeforeLast('.')

    val actual = TemplateHandler().handleEvent(
        event = event,
        response = KtxSerializer.json.decodeFromString(payload)
    )

    assertEquals(expected, actual)
}

Worth adding fixtures for the two new events (ideally captured from real GitHub deliveries, including one PR with "body": null and one approval with "review": {"body": null}), and asserting that a comment-less approval yields null rather than a blank string.

Also noticed an orphan: src/test/fixtures/events/pull_request.created.json has no matching messages/ file and no pull_request.created.template — GitHub doesn't emit a created action for pull_request, so that fixture looks stale and could be dropped (the existing ApplicationTest payload also uses "action": "created" for a pull_request event, which isn't a real GitHub action either).

🔒 Security & performance

Nothing concerning. The trace-log downgrade is a small positive for not leaking payload details into default-level logs. Signature validation is untouched and still runs before deserialization, which is the correct order. DefaultMustacheFactory caches compiled templates, so the new templates add no meaningful per-request cost.

---

Summary: the two new templates and the merged nullability are good changes. I'd suggest addressing #1 (body nullability), #2 (guarded deserialization) and #3 (blank-message guard) before merge — #3 in particular is a user-visible regression this PR introduces. #4 and the test-coverage items would be fine as follow-ups. Happy to take another look once updated. 🙂

[bbaarriiss]

From the description we don't see what actually is fixed. It could be nice to write better description shortly.

Other than that I added a comment about one test-case. If you want we can test together since that case should be tested with two different users.

[spoonman01]

Luca's comment on PR

[spoonman01]

Comment 2

[bbaarriiss]

TEST 1 - inline comment

[spoonman01]

New inline comment

[spoonman01]

New inline comment, pleeeeease

[spoonman01]

Inline comment, see if it triggers both events or not

[bbaarriiss]

mic check 🎤

[spoonman01]

Comment review

[spoonman01]

Comment on review

[bbaarriiss]

👉🏼 This is a multi line comment from Conversation page.
line 2 lalala
line 3 lelele

line 5 (line 4 was empty line)

[bbaarriiss]

We better remove {pullRequest.title} here. Because it is already shown in the PR line at the end like it is in other templates.

Also, the title lenght will always change and it will make the first line less readable.

[bbaarriiss]

It will be better to have this line as the last line.
Becasue the length of this one can change. So it is better to have it at the end. So we will see Repository and PR lines always in the same place. But the dynamic length text content will be at the end.

[bbaarriiss]

👉🏼 This is a multi line comment from Files changed page.
line 2 lalala
line 3 lelele

line 5 (line 4 was empty line)