aws-samples / aws-samples/sample-autonomous-cloud-coding-agents

fix(security): Object.assign on channel_metadata is a Blocking SAST finding on main — blocks every pre-push

オープン
#879 コメント 1 件 リアクション 0 件 担当者 1 名 @scottschreckengaust が担当を希望しています GitHub で見る
approved
主要言語
TypeScript
スター
146
フォーク
46
平均マージ
3日 10時間
マージ済み PR(30日)
24

説明

`mise run security:sast` fails on `main` with one **Blocking** semgrep finding, so the `pre-push` git hook rejects every push from every branch until it is fixed:

```
cdk/src/handlers/linear-webhook-processor.ts
❯❱ javascript.lang.security.insecure-object-assign.insecure-object-assign
❰❰ Blocking ❱❱
Depending on the context, user control data in `Object.assign` can cause web response to
include data that it should not have or can lead to a mass assignment vulnerability.
Details: https://sg.run/2R0D
926┆ Object.assign(channelMetadata, vaultMetadata(resolved));
```

Introduced by #831 (`12c9b63f`).

## Why CI is green while local pushes are blocked

The whole-repo `security:sast` runs in `security.yml` (scheduled / `main`) and in the `pre-push` hook. `security-pr.yml` runs only the **ranged** variants (`security:secrets:range`, `security:sast:masking:range`, `security:deps`, `security:gh-actions`). So a finding that lands on `main` never reds a PR but does block every contributor's `git push` — the asymmetry that makes this worth fixing rather than waiting for it to surface in CI.

## Exploitability

Not exploitable today. `vaultMetadata` returns a freshly-built literal whose two keys are hard-coded (`linear_provider_name`, `linear_vault_user_id`), so there is no attacker-controlled key to smuggle a `__proto__` through. The finding is about the *capability*, which is real: `Object.assign` copies via `[[Set]]`, which invokes the `__proto__` setter, so the pattern is one refactor away from being a prototype-pollution sink.

## Proposed fix

Use object spread, matching the four sibling `channel_metadata` builders (lines 2223, 2448, 2617, 2747):

```ts
channelMetadata = { ...channelMetadata, ...vaultMetadata(resolved) };
```

Spread uses `CreateDataProperty` (define, not set), so `__proto__` from an untrusted source would become an ordinary own property instead of mutating the prototype — the capability is removed, not relocated.

Deliberately **not** "explicit keyed writes". `vaultMetadata`'s contract (line 638–650) is that every builder *spreads* it; restating the field list at this one site is the exact bug the helper exists to prevent — add a third vault field later and this path silently drops it, handing the agent no provider on a vault-managed workspace.

Bonus: the source-level guard in `cdk/test/handlers/linear-webhook-processor.test.ts` keys off the literal-spread form, which is why (per the comment at lines 917–921) the one path it was written for was the one path it never covered. Converting to spread brings this site under that guard.

## Scope

- `cdk/src/handlers/linear-webhook-processor.ts` — one statement; `const channelMetadata` becomes `let` because it is now reassigned.
- Regression test asserting the vault fields still reach `channel_metadata` on the vault-onboarded path, plus the source-level guard now covering this builder.
- No behaviour change: the two keys written are identical.

## Also noticed (not in scope)

Line 925 (`channelMetadata.linear_workspace_id = workspaceId;`) is a redundant re-write — line 861 already sets that key to the same value in the declaring literal. Harmless; flagging rather than folding it in to keep the security fix reviewable on its own.

コントリビューションガイド

コントリビューションガイドを開く

評価

この issue はまだ評価されていません。

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。