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

fontawesomeにintegrity追加 #5969

Merged
merged 8 commits into from
May 25, 2023
Merged

fontawesomeにintegrity追加 #5969

merged 8 commits into from
May 25, 2023

Conversation

Atsu0601
Copy link
Contributor

@Atsu0601 Atsu0601 commented Mar 25, 2023

概要(Overview・Refs Issue)

下記の2ファイルにfontawesomeにintegrity追加しました。
/src/Eccube/Resource/template/admin/default_frame.twig
src/Eccube/Resource/template/admin/error.twig
Issue:#5946

方針(Policy)

実装に関する補足(Appendix)

テスト(Test)

相談(Discussion)

マイナーバージョン互換性保持のための制限事項チェックリスト

  • 既存機能の仕様変更はありません
  • フックポイントの呼び出しタイミングの変更はありません
  • フックポイントのパラメータの削除・データ型の変更はありません
  • twigファイルに渡しているパラメータの削除・データ型の変更はありません
  • Serviceクラスの公開関数の、引数の削除・データ型の変更はありません
  • 入出力ファイル(CSVなど)のフォーマット変更はありません

レビュワー確認項目

  • 動作確認
  • コードレビュー
  • E2E/Unit テスト確認(テストの追加・変更が必要かどうか)
  • 互換性が保持されているか
  • セキュリティ上の問題がないか
    • 権限を超えた操作が可能にならないか
    • 不要なファイルアップロードがないか
    • 外部へ公開されるファイルや機能の追加ではないか
    • テンプレートでのエスケープ漏れがないか

@chihiro-adachi chihiro-adachi added this to the 4.2.2 milestone Mar 27, 2023
@chihiro-adachi
Copy link
Contributor

@Atsu0601
ありがとうございます。以下エラーがでていますのでご確認お願いできますでしょうか。
image

@Atsu0601
Copy link
Contributor Author

@chihiro-adachi
ご確認ありがとうございます。
私のローカル環境では確認できなかったのですが、crossOrigin='anonymous' の記述を追加しようと思います。

先ほどコミットしたので、ご確認お願いできますでしょうか。
お手数ですが、宜しくお願い致します。

<link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.3.1/css/all.css">
<link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.3.1/css/v4-shims.css">
<link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.3.1/css/all.css" integrity="sha384-mzrmE5qonljUremFsqc01SB46JvROS7bZs3IO2EmfFsd15uHvIt+Y8vEf7N7fWAU" crossOrigin="anonymous">
<link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.3.1/css/v4-shims.css" integrity="sha384-mzrmE5qonljUremFsqc01SB46JvROS7bZs3IO2EmfFsd15uHvIt+Y8vEf7N7fWAU" crossOrigin="anonymous">
Copy link
Contributor

Choose a reason for hiding this comment

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

crossOrigin -> crossoriginでお願いします。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちらも変更致します。ありがとうございます。

<link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.3.1/css/all.css">
<link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.3.1/css/v4-shims.css">
<link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.3.1/css/all.css" integrity="sha384-mzrmE5qonljUremFsqc01SB46JvROS7bZs3IO2EmfFsd15uHvIt+Y8vEf7N7fWAU" crossOrigin="anonymous">
<link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.3.1/css/v4-shims.css" integrity="sha384-mzrmE5qonljUremFsqc01SB46JvROS7bZs3IO2EmfFsd15uHvIt+Y8vEf7N7fWAU" crossOrigin="anonymous">
Copy link
Contributor

Choose a reason for hiding this comment

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

v4-shims.cssのハッシュ値はall.cssと同じ値ですが問題ないでしょうか?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ハッシュ値は再度計算したものに差し替えいたします。

Copy link
Contributor

Choose a reason for hiding this comment

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

@Atsu0601
all.cssのハッシュ値は合っていると思われますので、v4-shims.cssの方を変更お願いできますでしょうか。

Copy link
Contributor

Choose a reason for hiding this comment

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

@chihiro-adachi
Copy link
Contributor

@Atsu0601
以下でお願いできますでしょうか。

<link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.3.1/css/all.css" integrity="sha384-mzrmE5qonljUremFsqc01SB46JvROS7bZs3IO2EmfFsd15uHvIt+Y8vEf7N7fWAU" crossorigin="anonymous">
<link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.3.1/css/v4-shims.css" integrity="sha384-lmquXrF9qn7mMo6iRQ662vN44vTTVUBpcdtDFWPxD9uFPqC/aMn6pcQrTTupiv1A" crossorigin="anonymous">

@codecov-commenter
Copy link

Codecov Report

Merging #5969 (3cc79d6) into 4.2 (fef2995) will increase coverage by 0.00%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff            @@
##                4.2    #5969   +/-   ##
=========================================
  Coverage     82.53%   82.54%           
  Complexity     6401     6401           
=========================================
  Files           475      475           
  Lines         25733    25734    +1     
=========================================
+ Hits          21240    21241    +1     
  Misses         4493     4493           
Flag Coverage Δ
E2E 69.44% <ø> (+<0.01%) ⬆️
Unit 79.70% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@shinya
Copy link
Contributor

shinya commented May 15, 2023

@Atsu0601
4.2(現メインブランチ)の最新を取り込み直していただくことで、おそらく落ちているテストが通る見込みです。
お手数ですがご対応をお願いします。

@shinya
Copy link
Contributor

shinya commented May 22, 2023

@Atsu0601
最新の取り込み対応ありがとうございます!

@kiy0taka @chihiro-adachi
GitHubActionsのテスト落ちていますが、ローカルの実行では問題なく成功しています。
(ソースコードの差分からも影響はないと判断できるかと思います)
こちらマージの対応に入りたく。最終レビュー頂けませんでしょうか

.gitignore Outdated
@@ -32,6 +32,7 @@ node_modules
!/html/user_data/.gitkeep
!/html/user_data/assets/css/customize.css
!/html/user_data/assets/js/customize.js
/html_bundle/*
Copy link
Contributor

Choose a reason for hiding this comment

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

この修正は不要なものかと思われますので、コミットごと削除お願いします。

Copy link
Contributor

Choose a reason for hiding this comment

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

@kiy0taka
ありがとうございます。見落としていました。

@Atsu0601
すみませんが上記の通り、こちらのコミットのご対応をお願い致します。
gitignoreの変更は今回のこの修正には関係の無いものとの認識です。
(もっと早くに気づけず失礼いたしました)

Copy link
Contributor

Choose a reason for hiding this comment

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

こちら私の方でrevertしておきます

@shinya shinya self-assigned this May 24, 2023
.gitignoreの変更は当branchの変更としては不要のため、Revert致します。
@shinya
Copy link
Contributor

shinya commented May 24, 2023

@kiy0taka
こちら該当のgitignoreの修正戻しておきました。
再レビューお願いいたします。

@shinya
Copy link
Contributor

shinya commented May 25, 2023

お二人共レビューありがとうございます。
マージします。

@shinya shinya merged commit 56310f7 into EC-CUBE:4.2 May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants