Skip to content

fix: escape commit message and other templated values when building card#121

Merged
ahanoff merged 4 commits into
mainfrom
fix/commit-message-json-injection
Jun 19, 2026
Merged

fix: escape commit message and other templated values when building card#121
ahanoff merged 4 commits into
mainfrom
fix/commit-message-json-injection

Conversation

@ahanoff

@ahanoff ahanoff commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Problem

opsless/ms-teams-github-actions@2.0.0 crashes with the following error whenever the workflow run's commit message contains a JSON-significant character (e.g. a double quote, backslash, or control character):

Expected ',' or '}' after property value in JSON at position ...

This is not a webhook-uri or token issue. It is a bug in how the action builds the Adaptive Card payload.

Root cause

In src/main.ts:

  1. The card template is a JS object that interpolates runtime values via ${$root.*} expressions (e.g. line 58 puts ${$root.commit.message} inside a JSON string field).
  2. Line 160 did JSON.stringify(temlpateData) before passing it to new Template(...).
  3. The adaptivecards-templating engine substitutes ${...} textually inside that JSON string — it does not JSON-escape the inserted values.
  4. Line 199 then did JSON.parse(content) on the result.

So a commit message like build: fix "quoted" task produces:

"value":"[build: fix "quoted" task](https://github.com/org/repo/commit/...)"

which is invalid JSON, and JSON.parse throws. head_commit.message is only trimmed to the first line (line 144), never escaped, so this triggers on every commit title containing ", \, raw tabs, etc. The same latent issue exists for workflow.name.

The duplicate error lines come from core.error(error) followed by core.setFailed(error.message) (lines 245–246).

Fix

Pass the template as a JS object to new Template(...) and use the expanded object directly as the attachment content. Node's JSON.stringify then handles all escaping correctly when serializing webhookBody for the HTTP request.

-  const rawdata = JSON.stringify(temlpateData)
-  const template = new Template(rawdata)
+  const template = new Template(temlpateData)
   const content = template.expand({ $root: { ... } })
   ...
   const webhookBody = {
     type: 'message',
     attachments: [{
       contentType: 'application/vnd.microsoft.card.adaptive',
-      content: JSON.parse(content)
+      content
     }]
   }

This eliminates the stringify → expand → parse round-trip entirely, fixing the issue for all templated values (commit message, workflow name, repository name, etc.), not just commit.message.

Verification

Reproduced the original failure and validated the fix against these commit messages:

  • build: fix "quoted" task
  • fix: path\with\backslashes
  • feat: emoji 🎉 and 'single' quotes
  • chore: tabbed\tmessage
  • fix: trailing backslash \
  • multi-line \n messages (first line kept, per existing behaviour)
  • normal ASCII messages

All cases now round-trip cleanly through JSON.stringify/JSON.parse. TypeScript build and Prettier check pass. The 5 pre-existing ESLint errors in src/main.ts (enum shadow, any, English-text rule) are unrelated to this change and present on main as well.

dist/main/index.js rebuilt via npm run package (the action.yml entrypoint points there).

ahanoff added 2 commits June 19, 2026 10:08
The Adaptive Card template was JSON.stringify-ed before being passed to the adaptivecards-templating engine, then the expanded result was JSON.parse-d back. Because the engine substitutes ${...} expressions textually inside the JSON string, any value containing a JSON-significant character (double quote, backslash, control chars, ...) corrupted the payload.

This affected at least head_commit.message (src/main.ts:144 only trims to the first line) and workflow.name, producing errors like:

  Expected ',' or '}' after property value in JSON at position ...

Fix: pass the template as a JS object to new Template(...) and use the expanded object directly as the attachment content. Node's JSON.stringify then handles all escaping when serializing webhookBody for the HTTP request, eliminating the round-trip entirely.

Verified locally with commit messages containing double quotes, backslashes, emoji, tabs and newlines; all round-trip through JSON.stringify/parse cleanly now.
Patch release for the JSON injection fix.
No code change. Forces a fresh workflow run to validate the updated MS_TEAMS_WEBHOOK_URI secret.
@ahanoff ahanoff force-pushed the fix/commit-message-json-injection branch from cf7391f to 44d4944 Compare June 19, 2026 03:07
ahanoff added a commit that referenced this pull request Jun 19, 2026
…122)

## Summary

Bumps the two GitHub-provided actions in `.github/workflows/test.yml` to
the **latest majors**:

- `actions/checkout@v4` → `actions/checkout@v7` (latest: v7.0.0, Jun
2026)
- `actions/setup-node@v4` → `actions/setup-node@v6` (latest: v6.4.0, Apr
2026)

## Why

`@v4` of both actions runs on Node.js 20, which was deprecated on GitHub
Actions runners and is now force-run on Node.js 24, producing this
warning on every workflow run:

> Node.js 20 is deprecated. The following actions target Node.js 20 but
are being forced to run on Node.js 24: actions/checkout@v4,
actions/setup-node@v4.

Both new majors target Node.js 24 natively (since v5), eliminating the
warning.

## Why latest (v7 / v6) and not the minimum v5 bump

v5 of each action is the minimum required to fix the Node 20
deprecation. Going straight to the latest major is preferred here
because **neither breaking change affects this workflow**:

| Action | Breaking change in new major | Affects us? |
|---|---|---|
| `actions/checkout@v7` | Persists creds to a separate file; blocks
fork-PR checkout for `pull_request_target` and `workflow_run` triggers |
No — this workflow only triggers on `push` and `pull_request` |
| `actions/setup-node@v6` | Limits automatic caching to npm only | No —
this workflow does not use the `cache` input |

Also picks up ESM migration and various dependency/security updates in
`actions/checkout@v7`.

Ref:
https://github.blog/changelog/2025-09-19-deprecation-of-node-20-on-github-actions-runners/

## Scope

Workflow-only change. No source code or built artifact touched. Separate
from #121.
## Summary

Adds the first unit tests in the repo. Extracts the Adaptive Card
construction logic from `src/main.ts` into a new `src/card.ts` so it can
be exercised without going through the GitHub Actions runtime, then adds
`src/__tests__/card.test.ts` with 28 cases pinning the JSON-safety
contract from #121.

**Stacked on #121.** After #121 merges, this PR will auto-retarget to
`main`.

## Why

The repo currently has **zero unit tests** — the only test surface is
the end-to-end smoke workflow in `test.yml`, which posts to the real MS
Teams webhook. That made verifying #121's fix expensive: 4 test commits
× 4 Node versions = 16 messages to the Teams channel per test session,
and enough volume to trip Power Automate rate limits.

With these unit tests, the JSON-safety regression can be verified
locally in **~1.5 seconds** with **zero webhook calls**. Future
contributors who touch `card.ts` get immediate feedback if they break
the contract.

## Changes

### `src/card.ts` (new)

- Moves the inline `temlpateData` template object out of `main.ts` (also
fixes the pre-existing typo → `templateData`).
- Exports `CardData` (typed shape of the data passed into the template)
and `WebhookBody` (typed shape of the Teams payload envelope).
- Exports `buildWebhookBody(data: CardData): WebhookBody` — a pure
function that runs `new Template(templateData).expand({ $root: data })`
and wraps the result in the `type: 'message'` envelope. This is the
exact code path #121 fixed.

### `src/main.ts`

- Imports `buildWebhookBody` and `CardData` from `./card`.
- Replaces the inline template + 50-line `template.expand({...})` block
with a typed `cardData` object passed to `buildWebhookBody(cardData)`.
Behaviour is unchanged.

### `src/__tests__/card.test.ts` (new)

28 test cases:

- **Payload envelope (3):** correct `type: 'message'` wrapper, `content`
is a plain object (not a JSON string — the original bug pattern),
Adaptive Card v1.4.
- **Commit message JSON safety (16 + 2):** each case verifies the full
webhook body survives `JSON.stringify` → `JSON.parse` round-trip, and
that the original commit message characters appear verbatim in the
parsed Commit fact. Covers:
  - `"double quotes"` — the original failing case from #121
  - `\` backslashes (single, trailing, mixed with `/`)
  - `\t`, `\r`, `\n` control characters
  - `'single quotes'`, `< > [ ]` brackets
  - Emoji 🚀, CJK 日本語中文 한국어
- `${1+1}` and `${$root.commit.message}` template-injection attempts
(verified NOT evaluated by the templating engine)
  - Empty string, plain ASCII baseline
- **`workflow.name` JSON safety (4):** same contract applied to the
workflow name field (also user-controlled via the workflow YAML).
- **`repository.name` JSON safety (3):** same contract applied to the
repository name.

### `.eslintignore`

- Adds `src/__tests__/`. The `i18n-text/no-en` rule from
`eslint-plugin-github` forbids English string literals, which is
incompatible with `describe()`/`it()` test descriptions. ts-jest still
type-checks the test files at runtime.

### `dist/main/index.js`

- Rebuilt via `npm run package` (the `action.yml` entrypoint points
here).

## Verification

```
$ npm test
Test Suites: 1 passed, 1 total
Tests:       28 passed, 28 total
Time:        1.509 s

$ npm run build         # tsc clean
$ npm run format-check  # prettier clean
$ npm run lint          # 0 new errors vs main (1 informational warning about the ignored test file)
```

## Follow-up (not in this PR)

The repo's `test.yml` workflow doesn't run `npm test` yet, so these
tests currently only run locally. A small follow-up PR could add a `npm
ci && npm test` step to the workflow so regressions are caught in CI
without needing the Teams webhook. Kept out of scope here to avoid
conflicting with #122 (which also touches `test.yml`).
@ahanoff ahanoff merged commit 8c099f4 into main Jun 19, 2026
4 checks passed
@ahanoff ahanoff deleted the fix/commit-message-json-injection branch June 19, 2026 05:34
ahanoff added a commit that referenced this pull request Jun 19, 2026
…onnectors (#123)

## Summary

Updates the README setup instructions to reflect that Microsoft has
retired the legacy Office 365 connectors / incoming webhooks in Teams,
and aligns the steps with the **current Microsoft Learn walkthrough**
(updated Oct 2025).

## Background

Microsoft has [retired the legacy Office 365 connectors / incoming
webhooks](https://devblogs.microsoft.com/microsoft365dev/retirement-of-office-365-connectors-within-microsoft-teams/).
The old-style URLs (`https://*.webhook.office.com/...` or
`https://outlook.office.com/webhook/...`) silently accept POSTs with a
2xx response but no longer deliver messages to the channel. From the
action's side everything looks healthy (workflow job succeeds, response
is `ok`), but nothing arrives in Teams.

## What changes

Replaces the previous one-liner step 1 with concrete instructions for
creating a **Workflows-based incoming webhook** via the Teams
**Workflows** app. The steps now mirror the canonical Microsoft Learn
article ([Create an Incoming
Webhook](https://learn.microsoft.com/en-us/microsoftteams/platform/webhooks-and-connectors/how-to/add-incoming-webhook),
last updated 2025-10-01):

- Open the channel → **... (More options)** → **Workflows**
- Search for and select the **"Send webhook alerts to a channel"**
template
- Configure workflow parameters (pick team + channel) → **Save**
- Copy the resulting
`https://prod-XX.<region>.logic.azure.com:443/workflows/.../triggers/manual/paths/invoke?...`
URL
- Store it as the `MS_TEAMS_WEBHOOK_URI` secret

Also adds:
- A link to the canonical Microsoft Learn walkthrough for users who want
more detail.
- A `> [!WARNING]` GitHub admonition (replacing the previous `> ⚠️`
emoji blockquote) describing the retirement of legacy connector URLs, so
users with an existing `MS_TEAMS_WEBHOOK_URI` secret understand why
their notifications silently stopped.

> [!NOTE]
> The template name changed: previous versions of the MS docs called it
*"Post to a channel when a webhook request is received"*; the current
name (as of Oct 2025) is *"Send webhook alerts to a channel"*. The
README now uses the current name.

## Scope

Docs-only change. No source or built artifact touched. Independent of
#121 (test/code) and #122 (CI bump, already merged).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant