Skip to content

fix(backend): skip inbox activities without an actor instead of throwing TypeError#17558

Open
sasagar wants to merge 1 commit into
misskey-dev:developfrom
ikaskey:fix/inbox-getapid-undefined-actor
Open

fix(backend): skip inbox activities without an actor instead of throwing TypeError#17558
sasagar wants to merge 1 commit into
misskey-dev:developfrom
ikaskey:fix/inbox-getapid-undefined-actor

Conversation

@sasagar

@sasagar sasagar commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

What

Inbox activities delivered without an actor property crash the inbox queue job with an opaque error instead of being skipped:

TypeError: Cannot read properties of undefined (reading 'id')
    at getApId (packages/backend/src/core/activitypub/type.ts)
    at InboxProcessorService.process (packages/backend/src/queue/processors/InboxProcessorService.ts)

This PR:

  1. Guards getApId() against null / undefined, so it throws the intended Error('cannot determine id') instead of a raw TypeError (also fixes the deteminedetermine typo).
  2. Rejects actor-less activities early in InboxProcessorService.process() with Bull.UnrecoverableError, so they are skipped cleanly like other invalid inputs (and are not retried 128×).

Fixes #17557. Related: #12720.

Why

InboxProcessorService.process() calls getApId(activity.actor) in several places before any validation (the isDelete short-circuit getApId(activity.actor) === getApId(activity.object), and getAuthUserFromApId(getApId(activity.actor))). When activity.actor is missing, getApId(undefined) evaluates undefined.id and throws.

On a small federated instance this produces on the order of ~100 failed jobs/day from malformed / actor-less deliveries, flooding error tracking and masking real problems.

Additional info

  • No behaviour change for valid activities: every activity that reaches this code already requires an actor (it is used immediately for HTTP-Signature / authorship checks), so an activity without one was always going to fail — this just fails it cleanly and early instead of with a confusing TypeError.
  • I was not able to spin up a full local federation instance to reproduce end-to-end; the diagnosis is from production stack traces, and the logic was verified by inspection. InboxProcessorService has no unit test suite (cf. fix: change bare activity.actor to getApId(activity.actor) in InboxPr… #17340).

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

…ing TypeError

- guard getApId() against null/undefined (and fix the 'detemine' typo)
- skip actor-less inbox activities early with Bull.UnrecoverableError

Fixes misskey-dev#17557
@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jun 9, 2026
@github-actions github-actions Bot added the packages/backend Server side specific issue/PR label Jun 9, 2026
@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.94%. Comparing base (a75f3ad) to head (c6ea087).
⚠️ Report is 5 commits behind head on develop.

Files with missing lines Patch % Lines
packages/backend/src/core/activitypub/type.ts 0.00% 1 Missing and 1 partial ⚠️
...kend/src/queue/processors/InboxProcessorService.ts 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #17558      +/-   ##
===========================================
- Coverage    24.87%   16.94%   -7.94%     
===========================================
  Files         1161     1161              
  Lines        39629    39631       +2     
  Branches     11041    11042       +1     
===========================================
- Hits          9859     6714    -3145     
- Misses       23849    26284    +2435     
- Partials      5921     6633     +712     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

このPRによるapi.jsonの差分
差分はありません。
Get diff files from Workflow Page

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Backend memory usage comparison

Before GC

Metric base (MB) head (MB) Diff (MB) Diff (%)
VmRSS 305.53 MB 299.75 MB -5.78 MB -1.89%
VmHWM 305.53 MB 299.75 MB -5.78 MB -1.89%
VmSize 23169.81 MB 23164.80 MB -5.00 MB -0.02%
VmData 1370.50 MB 1364.92 MB -5.57 MB -0.40%

After GC

Metric base (MB) head (MB) Diff (MB) Diff (%)
VmRSS 305.60 MB 299.84 MB -5.76 MB -1.88%
VmHWM 305.60 MB 299.84 MB -5.76 MB -1.88%
VmSize 23170.31 MB 23164.96 MB -5.34 MB -0.02%
VmData 1371.00 MB 1365.09 MB -5.91 MB -0.43%

After Request

Metric base (MB) head (MB) Diff (MB) Diff (%)
VmRSS 305.88 MB 300.20 MB -5.67 MB -1.85%
VmHWM 305.88 MB 300.20 MB -5.67 MB -1.85%
VmSize 23170.31 MB 23164.96 MB -5.34 MB -0.02%
VmData 1371.00 MB 1365.09 MB -5.91 MB -0.43%

See workflow logs for details

sasagar added a commit to ikaskey/misskey that referenced this pull request Jun 9, 2026
…ing TypeError

Backport of misskey-dev#17558 to the ikaskey fork.
- guard getApId() against null/undefined
- skip actor-less inbox activities early with Bull.UnrecoverableError

Release v2026.5.4-ikaskey-c
@syuilo

syuilo commented Jun 9, 2026

Copy link
Copy Markdown
Member

IActivity.actor 自体はnonNullに定義されていますが、これは型定義を修正すべきでしょうか?

@syuilo

syuilo commented Jun 9, 2026

Copy link
Copy Markdown
Member

もしくは不正な形式のジョブデータをそもそもジョブキューに積むべきではないかもしれません

@sasagar

sasagar commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@syuilo

ありがとうございます。2点ともおっしゃる通りで、根本は inbox エンドポイントの request.body as IActivity(ActivityPubServerService)という未検証キャストだと思います。信頼できない入力を検証せず IActivity と見なしてキュー投入しているため、actor 欠落のような構造的に不正なボディがそのまま流れていました。

その観点では、enqueue 時点で検証し不正なジョブはキューに積まないのが一番良さそうです:

  • inbox ハンドラで queueService.inbox() の前にボディの最低限の構造(actor の存在など)を検証し、満たさなければ 400 等で返してキュー投入しない
  • これにより無駄なリトライも発生せず、HTTP 層で送信元へ適切に応答できます
  • 型の乖離(前コメント)も、この境界で緩い型から検証して narrow する形にすれば解消できます

ただ inbox は意図的に軽量で、重い検証(署名・actor 解決)は processor 側に寄せていると理解しています。なので enqueue 時の検証は構造的・低コストなチェックに留めるのが良いかと思います

getApId の null ガードは、getOneApId が空配列で getApId(undefined) を踏むケースもあるため、独立した汎用防御として残す想定です。

この方針で PR を更新してよければ対応しようかと思います。processor 側の早期ガードは defense-in-depth として残すか、enqueue 検証に一本化するかは判断をお願いできたらと...。

@syuilo

syuilo commented Jun 10, 2026

Copy link
Copy Markdown
Member

それで良さそうです。
processor側は型がnon nullになっている以上nullチェックは要らないと思います。

@sasagar

sasagar commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

@syuilo
かしこまりです。そうしたはそっちで再対応するので少しお待ちを……

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

packages/backend Server side specific issue/PR size:S This PR changes 10-29 lines, ignoring generated files.

Projects

Development

Successfully merging this pull request may close these issues.

Inbox: getApId() throws an uninformative TypeError when activity.actor is undefined

2 participants