[netbe]

Summary

• Add an authenticated Actions endpoint for sending custom Wire conversation messages
• Add a reusable GitHub composite action and workflow documentation
• Reuse the conversation secret for bearer-token authentication

[claude]

Review: feat: add custom GitHub Actions notifications

Nice, focused addition — the endpoint reads the body only after authenticating (better than the webhook route, which buffers the payload first), and reaching for MessageDigest.isEqual instead of == on the token comparison is exactly the right instinct. A few things below I'd want addressed before merge, roughly in priority order.

Note: I reviewed statically — ./gradlew was blocked in my environment, so the lint findings in §1 are hand-computed against config/detekt/detekt.yml / .editorconfig rather than observed. Please confirm locally.

---

1. CI will likely fail on lint (blocking)

.github/workflows/pull-request.yml runs ktlintCheck and detekt, and max_line_length = 100 is set in both .editorconfig and detekt.yml (MaxLineLength.maxLineLength: 100). Two new lines exceed it:

• Routing.kt:160 — signatureValidator.isBearerTokenValid(conversationId, conversationDomain, bearerToken) ≈ 102 chars
• Routing.kt:162 — application.log.warn("No secret stored for conversation $conversationId@$conversationDomain") ≈ 109 chars

Also, SwallowedException is active: true in the detekt config, and three new catch blocks never reference the caught exception:

• Routing.kt:161-164 (IOException) — the log message drops the exception, unlike the equivalent webhook handler at Routing.kt:86-91 which passes it to log.warn
• Routing.kt:169-171 (SerializationException)
• Routing.kt:176-178 (IllegalArgumentException)

Passing the exception into a log call in each fixes the rule and gives you the debuggability the webhook path already has.

2. Reusing the HMAC webhook secret as a plaintext bearer token (security)

This is the design point I'd push back on hardest. Today the conversation secret is an HMAC key: it's shared with GitHub and never travels over the network. This PR turns it into a bearer credential transmitted in plaintext in an Authorization header on every notify call.

Consequence: anywhere that header gets captured — a reverse-proxy access log, an APM trace, a TLS-terminating LB, a misconfigured endpoint: input using http:// — now leaks a credential that also lets the holder forge signed GitHub webhook deliveries for that conversation. One credential, two blast radii.

Cheap fix that keeps the single-secret UX: derive the bearer token rather than reusing the secret.

// token the Action sends = hex(HMAC-SHA256(secret, "github-actions-notify"))
fun isBearerTokenValid(conversationId: String, conversationDomain: String, token: String): Boolean {
    val secret = storage.get(conversationId.toStorageKey(domain = conversationDomain))
        ?: throw IOException("Missing secret for Conversation: $conversationId")
    return MessageDigest.isEqual(
        sha256(deriveActionsToken(secret)),
        sha256(token)
    )
}

EventsHandler.formatSetupInstructions can then print the derived token alongside the webhook secret. A leaked derived token only grants "post to this conversation", not signature forgery.

Two smaller notes regardless of the above:

• MessageDigest.isEqual is constant-time in content but short-circuits on length, so it leaks the secret's length. Hashing both sides to a fixed 32 bytes first (as sketched) removes that.
• it.startsWith("Bearer ") is case-sensitive; RFC 7235 defines the auth scheme as case-insensitive. call.request.parseAuthorizationHeader() handles this properly. (Worth considering Ktor's authenticate / bearer plugin more broadly — it would lift this out of the handler entirely.)

3. Correctness / consistency in the handler

• Missing metrics. The webhook path calls usageMetrics.onWebhookEventReceived and onNotificationSent; the new endpoint records nothing, so Actions notifications are invisible in Prometheus. Consider a distinct onActionsMessageSent() so the two sources stay separable.
• Validate the UUID first. An unauthenticated caller hitting /actions/foo/bar currently triggers a Redis GET before the UUID is ever parsed (Routing.kt:174). Moving the UUID.fromString parse above the auth check makes the reject path free and reads more naturally — conversationId is only ever a UUID.
• Empty error bodies. Every failure returns a bare status, while the webhook route returns "Invalid Signature for Conversation". The composite action uses --fail-with-body, so an operator debugging a 400 gets nothing back — they can't distinguish "blank text" from "bad JSON" from "bad UUID". Short distinct messages would pay for themselves.
• sendMessage is unguarded. If the SDK throws, the caller gets a 500 with a stack trace. Same as the webhook path so not a regression, but this endpoint is now reachable from CI jobs that will surface the failure loudly.
• @Suppress("LongMethod") on configureRouting was already carrying weight; this pushes it to ~160 lines. Extracting both handlers into private extension functions would let the suppression go away.
• No bound on text. call.receiveText() is unbounded and the text goes straight to Wire. A max-length check returning 400 beats letting the SDK fail at 500.

4. Composite action (.github/actions/notify-wire/action.yml)

Good: set -euo pipefail, secret passed via env: rather than interpolated into the script body (which correctly avoids shell injection from text:), JSON built by a real encoder instead of string concatenation, ${GHAPP_ENDPOINT%/} trailing-slash handling.

Suggestions:

• Secret lands in the process argv. -H "Authorization: Bearer ${CONVERSATION_SECRET}" is visible via /proc/*/cmdline to anything else on the runner. Negligible on ephemeral GitHub-hosted runners, real on shared self-hosted ones. curl --config with the header in a $RUNNER_TEMP file avoids it.
• No timeout or retry. A hung endpoint blocks the job until the job-level timeout. --max-time 30 --retry 3 --retry-connrefused is cheap insurance, and matters more given the README suggests calling this from if: failure() steps.
• python3 dependency. Present on GitHub-hosted runners, not guaranteed on slim self-hosted containers. jq -Rs '{text: .}' <<< "$MESSAGE_TEXT" is a smaller dependency, though not a free one either — whichever you pick, worth documenting the runner requirement.
• No scheme check on endpoint. Given §2, a typo'd http:// silently ships the secret in cleartext. A one-line guard rejecting non-https (with an escape hatch for local testing) would be worth it.
• README pins @v1 — make sure that tag exists before this doc ships, otherwise the copy-pasted snippet fails for the first user who tries it.

5. Test coverage

One happy-path test for a new authentication endpoint is thin — the paths that matter most here are the ones that must reject, and none are covered:

• no Authorization header → 403
• wrong scheme / malformed header → 403
• wrong token → 403 and sendMessage never called (verify(exactly = 0))
• missing secret in Redis (the IOException branch) → 403
• blank / whitespace-only text → 400
• malformed JSON body → 400
• non-UUID conversation id → 400

There's also no SignatureValidatorTest in the repo at all, so isBearerTokenValid — the actual credential comparison — has no unit test. A small one covering match / mismatch / missing-key would be valuable, especially if you adopt the derived-token change in §2.

Finally, the new test stubs isBearerTokenValid on a mock, so it exercises neither the Redis lookup nor the comparison — it proves the routing and serialization wiring only. Worth either saying so in the test name or adding one test that runs a real SignatureValidator against the mocked RedisCommands.

---

Nothing here is structural — the shape of the feature is right. §1 and §2 are the two I'd genuinely want resolved before merge; the rest are improvements.

[spoonman01]

Unsure if just by placing the action here it's somehow reachable by all the other repositories in wireapp org:

https://docs.github.com/en/actions/how-tos/reuse-automations/share-with-your-organization#sharing-actions-and-workflows-with-your-organization

[alexandreferris]

As this repository is public, it is open. The configurations/changes are only needed if the repository is set to private.

Also, after this PR is approved and merged, I will be creating a tag so the actions can be reachable by (or something like):

uses: wireapp/github-app/.github/actions/notify-wire@v1

[spoonman01]

This strucrure is not bad, but there is a lot of configuration to use ActionsTokenValidator (register in DI, imports and so on) and (besides) tests, it's just a function called in one place.

Not sure it justifies the infrastructure, maybe better to just have the function directly in the place its needed.

Or maybe instead of getting calls to Redis storage from many places, I would suggest a RedisRepository that contains the GET and SET operations for both the webhook and the actions (setWebhookSecret, setActionSecret, getWebhookSecret, getActionSecret). This way the code can just pass around conversationId and the parsing to conversationId.toActionsTokenStorageKey() can happen inside the RedisRepository and the rest of the code is unaware.

Again current setup works but I think it can be quite simpler

[alexandreferris]

hmmm makes sense to have the RedisRepository, but I would do it in another PR/ticket as this has grown quite a bit (given the custom notification + jvm sdk update and some other improvements).

We are still missing metrics for custom notifications (but not part of this PR/ticket) so maybe we can introduce both (metrics + redisRepository) in the future if you agree

[spoonman01]

This is too much for the README and too specific, just keep this part and the yaml example of the action IMHO

[alexandreferris]

Removed most of the explanation parts and kept the inputs table/yaml configuration and all commands at the bottom

[spoonman01]

Approving this but mostly because we are in a hurry to deploy the apps this sprint, I would really like to have the refactoring about RedisRepository

[alexandreferris]

Approving this but mostly because we are in a hurry to deploy the apps this sprint, I would really like to have the refactoring about RedisRepository

Yes!! will be worked on WPB-29141