SonarCloudで検出されたHTMLのSecurity Hotspotを対策する

[berryzplus]

PR の目的

#1504 で新たに検出された Security Hotspot を対策して、30日以内に作成されるPRの静的解析結果に悪影響を与えないようにします。

カテゴリ

• リファクタリング

PR の背景

#1505 でHTMLの Bugs を修正した際に、新たに4件の Security Hotspot が検出されました。
https://sonarcloud.io/project/security_hotspots?id=sakura-editor_sakura&sinceLeakPeriod=true

単純にこれを放置しておくと、新規にマズいコードを書かなくても
30日間は「新規コードにSecurity Hotspotが4件」と出てしまうと考えられます。
これはちょっとイヤなので、なんとか対応できないか検討してみました。

Security Hotsopotは「絶対マズい」という内容ではなく、
「大丈夫かどうかレビューして、OKと言えるなら無問題にしていい」
という性質のものらしいです。
内容をレビューして「問題ありませんでした。」なら、そのまま終わらせても大丈夫です。
なので指摘された内容をちゃんと調べてみました。

Security Hotspot 4件の内容はすべて同じ「rel=noopenerを付けなくて大丈夫か確認してください」でした。
PR #1505 のときはウインドウ制御に関するスクリプト系の指摘なので放置します、で終わらせましたが、対応したほうが良さそうです。

最近はタブブラウザが多いですが、rel=noopenerを付けないとオープンされた側からオープン元タブの中身を自由に書き換えることができてしまうんだそうです。

対象のHTMLはヘルプの一部ですので、ヘルプとして利用する分には問題ありません。
しかし、これはWebサイトの一部として公開するファイルでもあるので「対応しなくて良い」とまでは言えないです。

PR のメリット

• SonarCloudのSecurity Hotspot警告が消えます。

PR のデメリット (トレードオフとかあれば)

• HTMLヘルプのソース文字数が増えます。

仕様・動作説明

アプリの仕様・機能には影響しません。

変更仕様は「target="_blank" を持つ a タグを全置換して rel="noopener" を付加します」です。
　参考サイト： https://forest.watch.impress.co.jp/docs/serial/yajiuma/1291549.html

解説には rel=noopener を書かれていますが、属性値は引用符付きで統一したので、引用符付きで入れます。
（引用符を外す話は、HTML5への移行ができるようになったときに考える話だと思っています。）

PR の影響範囲

• HTMLヘルプから外部サイトリンクを開いたあとの挙動に影響します。
• Web公開しているヘルプから外部サイトリンクを開いたあとの挙動に影響します。

テスト内容

一般的な security suggestion を適用する内容なので、とくに行いません。

関連 issue, PR

#1505
#1504

参考資料

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[berryzplus]

👆SonarCloud対応は、正直言ってこういうモノを出したかったんです。

静的解析の結果、問題ありませんでした。

こういうのがあれば、レビュアーは内容の確認に専念できるような気がしたからです。

[AppVeyorBot]

✅ Build sakura 1.0.3341 completed (commit 1a7d32e1c7 by @berryzplus)

[sanomari]

気になることが３点あります。

1. 発生していた2件のBugsはどこへ行きました？
   後追いで手動レビューしたのかな？と思いますが、
   そのレビューは、わたしがやっても良いものです？
2. 発生しているのは4件ですが修正対象45件です。
   45件はSonarCloudでみたHTMLのSecurity Hotspotの総数と一致します。
   合ってますか？
3. 修正の意図を確認したいです。
   警告は駆逐する方針にします？
   今回だけの対応にします？

[berryzplus]

気になることが３点あります。

あい。

1. 発生していた2件のBugsはどこへ行きました？
   後追いで手動レビューしたのかな？と思いますが、
   そのレビューは、わたしがやっても良いものです？

発生していたBugsは won't be fix とマークしてcloseしました。
frameを使用すべきでないのBugsはframesetを使用すべきでないのBugと実質的に同件で、framesetを対応するときに消えることが明らかです。残しておくと対処すべき課題が3件あるように見えてややこしいので消しました。
レビューは誰がやってもよいと思います。

レビュー権限の話はちゃんと調べていませんが、
GitHubと連携しているのでメンバーであれば変えられると思います。
変更できなかったら言ってください。

2. 発生しているのは4件ですが修正対象45件です。
   45件はSonarCloudでみたHTMLのSecurity Hotspotの総数と一致します。
   合ってますか？

対処したい指摘4件に対し、修正45件で合っています。
指摘理由が同じなので、他41件を対応しない理由はないと思います。

対応すべきなのはWebサイトに公開するファイルのみなので、
HTMLとしては対応を行わずリリース手順にこの修正を含めるやり方でもよいとは思っています。

3. 修正の意図を確認したいです。
   警告は駆逐する方針にします？
   今回だけの対応にします？

今回だけの対応のつもりです。

最終的に C++ ソース部分の Bugs と Security Hotspot は駆逐したいです。
このPRの対象領域はHTMLなので、駆逐する意欲はそんなでもないです。

[berryzplus]

レビューありがとうございます。
とりあえずマージしちゃいます。