Skip to content

enhance: Content-TypeがHTMLじゃない場合は接続を打ち切るように - #75

Open
kakkokari-gtyih wants to merge 2 commits into
misskey-dev:masterfrom
kakkokari-gtyih:enh-reject-unsupported-content-type
Open

kakkokari-gtyih wants to merge 2 commits into
misskey-dev:masterfrom
kakkokari-gtyih:enh-reject-unsupported-content-type

Conversation

@kakkokari-gtyih

Copy link
Copy Markdown
Member

なんでもかんでもダウンロードが完了するまで行ってから判定していたのを、ヘッダーが降ってきた時点でContent-TypeがHTMLじゃないものがついていたら破棄する

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: d360db82-6083-45c5-9ac7-d642b10e119d

📥 Commits

Reviewing files that changed from the base of the PR and between ae1369f and cf17373.

📒 Files selected for processing (3)
  • src/index.ts
  • src/utils/got.ts
  • test/index.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Redirect resolution now uses getResponse for HEAD requests. getResponse checks response IP addresses and successful-response content types when headers arrive, and aborts requests when checks fail. Tests cover private-IP redirects, streamed responses, and content-type filtering.

Changes

Redirect resolution and response validation

Layer / File(s) Summary
Response address and content-type checks
src/utils/got.ts, test/index.test.ts
getResponse validates socket IP addresses and filters content types on successful responses when headers arrive. Tests check streamed non-HTML responses, missing content types, and rejection before a response body finishes.
Redirect resolution integration
src/index.ts, test/index.test.ts
HEAD redirect resolution uses getResponse with configured request options and sets actualUrl from the response URL. Tests check private-IP redirect rejection and following a redirect whose initial response has a filtered content type.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to cf173

The change rejects disallowed responses earlier while preserving redirect resolution and status-error behavior. No actionable merge-blocking issue was identified; merge after normal checks pass.

Security Architecture Review

Security architecture risk: 🔵 Low · up to cf173

The change strengthens response validation without establishing a new security weakness. Remaining uncertainty concerns whether rejected responses always stop redirect continuation and release streams, and how existing proxy and plugin policies behave after HEAD-resolution failures.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — If an integrating application accepts attacker-controlled URLs, outbound requests operate within the host or configured agent's network reach. Address checks run after dispatch when response headers arrive, so they do not establish prevention of the initial connection or request to a private destination. This limitation predates the PR; actual tenant, service and environment exposure is not established by the scoped evidence.

Trust Boundaries and Controls

  • observed — Within a shared request, address or successful-response type rejection sets the same abort signal supplied to the HTTP client. The redirect hook checks that signal before continuation, and final-response validation remains as a backstop. These controls strengthen the previously separate HEAD path without adding a new authority source.

Resilience and Maintainability Implications

  • observed — Request abort state is local, but agent configuration is process-global and persists when later calls omit an agent. This shared authority state and its private-IP bypass existed before the PR. A failed HEAD also remains nonterminal for the overall operation because extraction can issue a fresh request; the redirect regression asserts only that the redirected HEAD handler is not reached.

Hardening Proposals

  • proposed — Establish the pinned HTTP client's rejection invariant across multi-hop redirects and interruption: a rejected response must not dispatch the next hop, its stream must close, and any HEAD-to-GET fallback must remain a separately validated operation. This would resolve the remaining enforcement uncertainty rather than address an observed regression.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: aborting the connection when the Content-Type is not HTML.
Description check ✅ Passed The description accurately explains that the response is rejected when the headers show a non-HTML Content-Type, before download completion.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit checks the headers bright,
Then stops a private hop mid-flight.
A page may stream, but types come first,
A missing label ends the burst.
The redirect finds its title’s nest,
And leaves the blocked request at rest.

Comment @coderabbitai help to get the list of available commands.

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