Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mk:apiExternalの送信先を連合中のサーバーに限定 #12047

Closed
wants to merge 12 commits into from

Conversation

FineArchs
Copy link
Contributor

@FineArchs FineArchs commented Oct 16, 2023

What

Mk:apiExternalの送信先がfederation/instances {blocked: false}のレスポンスに含まれないものfederation/show-instanceがnullを返す/isBlockedがtrueの場合にエラーを出すようにします。

Why

#12040

Additional info (optional)

apiExternal自体の動作確認は出来ていません

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

@github-actions github-actions bot added the packages/frontend Client side specific issue/PR label Oct 16, 2023
@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (f80ae7f) 78.81% compared to head (aba8106) 78.91%.

Files Patch % Lines
packages/frontend/src/scripts/api.ts 0.00% 13 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12047      +/-   ##
===========================================
+ Coverage    78.81%   78.91%   +0.10%     
===========================================
  Files          958      958              
  Lines       104272   104281       +9     
  Branches      8335     8365      +30     
===========================================
+ Hits         82177    82296     +119     
+ Misses       22095    21985     -110     

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

if (!/^https?:\/\//.test(hostUrl)) throw new Error('invalid host name');
if (endpoint.includes('://')) throw new Error('invalid endpoint');
const knownUrls = (await api('federation/instances', { blocked: false })).map(v => v.host);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • ページネーションしない限りfederation/instancesで取得できるのはごく一部
  • かといって全て取得するのもパフォーマンス的に現実的ではない

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

URLを渡せば連合中であるかを返してくれるエンドポイントとかってありましたっけ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

自己解決しました

@github-actions
Copy link
Contributor

このPRによるapi.jsonの差分

差分はこちら

Get diff files from Workflow Page

@syuilo
Copy link
Member

syuilo commented Oct 30, 2023

うーーーむパフォーマンス上の影響が気になるわね

@FineArchs
Copy link
Contributor Author

@syuilo

うーーーむパフォーマンス上の影響が気になるわね

詳しくお願いします

@syuilo
Copy link
Member

syuilo commented Dec 8, 2023

CORSのpreflight requestみたいに、APIリクエストするのに自分と相手の両方のサーバーにリクエストが発生するのはスマートではない気がするという感じですね

@FineArchs
Copy link
Contributor Author

とはいえ、トークンというセンシティブな情報を扱う(可能性がある)以上、preflightに相当する手順は必須になるような気がしますが…

@syuilo
Copy link
Member

syuilo commented Dec 8, 2023

そうなるとやっぱりapiExternalは提供を保留した方が良いかも(このPRを取り込んだとしてもセキュリティ的に万全になるわけではないし)

@anatawa12
Copy link
Member

信頼できないプラグインは入れた時点で他のことで何やるかわからんし、信頼できてるなら防御しなくてもいいわけで、正直私はいらないと思う。

ブラウザで見るページと違い、手動で入れなければならないので、普通のブラウザとは話が違うと思う

@syuilo
Copy link
Member

syuilo commented Dec 8, 2023

apiExternalはPlayでも使えなかったっけ

@u1-liquid
Copy link
Sponsor Contributor

u1-liquid commented Dec 8, 2023

ソフトウェア開発の経験の無い人に今入れようとしているスクリプトが信用できるかできないかを判断するのはすごく難しいものだし、結構長いスクリプトの中に悪意のあるコードが紛れ込むのは全然あり得る話なのでユーザーが良いと言ったから大丈夫とするのはあまり良くないと思います

@Sayamame-beans
Copy link
Collaborator

Playもソースは見られるので、その点ではプラグインでもPlayでも変わらない気がします…?
(それはそれとして、無条件に使えるとセキュリティ的に気になるというのも分かります)

@u1-liquid
Copy link
Sponsor Contributor

u1-liquid commented Dec 8, 2023

ソースコード見てからPlayを実行する人が存在するとは思えないですがapiExternalのためにPlayもソースコードを確認してから実行しなければならないものとするのですか?

@FineArchs
Copy link
Contributor Author

セキュリティ万全とはいかないまでも、このPRにより、トークンを詐取するのに必要なハードルが「'api/...'のエンドポイントを受け付けるサーバーが必要」から「最低限のactivityPubの機能を実装したサーバーが必要」まで上がるので、かなり効果があるのではと考えています。

@anatawa12
Copy link
Member

anatawa12 commented Dec 8, 2023

playってトークン扱えるんでしたっけ

使えないならブラウザもあんまり防御してないしあんまり細かくやらんでもいいんじゃねと思ったり

@FineArchs
Copy link
Contributor Author

Playもソースは見られるので、その点ではプラグインでもPlayでも変わらない気がします…?
(それはそれとして、無条件に使えるとセキュリティ的に気になるというのも分かります)

PlayはapiExternalさえなければほぼ安全ですが、プラグインは既にopen_urlがあるので今更…という感じではあります

@Sayamame-beans
Copy link
Collaborator

ソースコード見てからPlayを実行する人が存在するとは思えないですがapiExternalのためにPlayもソースコードを確認してから実行しなければならないものとするのですか?

そのように主張したつもりはありませんでした。
少なくとも警告表示等はあった方が良いと思っています。(そのようなissueがapiExternal絡みとしてではなくどこかにあったような気はします)

@syuilo
Copy link
Member

syuilo commented Dec 8, 2023

apiExternalが存在することによるメリットよりそういった懸念の方が上回りそうだから提供は一時停止する方向にします🙏

@syuilo syuilo closed this Dec 8, 2023
@FineArchs
Copy link
Contributor Author

@syuilo
今の所、apiExternalの危険性は警戒すれば避けられるもので、かつ最悪の場合でもトークンを削除することで対処できます。
機能を丸ごと削除するほどの懸念はないと思います。

また、下手に危険性を指摘すると機能ごと削除されてしまうような環境では、リスクを指摘する人々が萎縮してしまいます。
すでにapiExternalを使用しているプラグインをいくつか確認していますし、機能の削除にはもう少し慎重になるべきです。

@syuilo
Copy link
Member

syuilo commented Dec 8, 2023

慎重に判断しました

@syuilo
Copy link
Member

syuilo commented Dec 8, 2023

FYI: misskey.ioではapiExternalは削除される方針っぽい

@FineArchs
Copy link
Contributor Author

慎重に判断しました

なら仕方ないですが、プラグインの方だけでも残す方向にはできませんか?
Plugin:open_urlとの危険性の差はほぼ無いはずです。

@syuilo
Copy link
Member

syuilo commented Dec 8, 2023

そっちは残しても良いと思う

@syuilo
Copy link
Member

syuilo commented Dec 8, 2023

そっちは残しても良いと思う

いや、自分の情報が外部サーバーに勝手に送信される可能性があるのは結構危険だからプラグインについても削除した方が良さそうに思えてきた

@syuilo
Copy link
Member

syuilo commented Dec 8, 2023

Plugin:open_url についても議論の余地がありそう

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/frontend Client side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants