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

フロント入力項目のサニタイズ #5081

Merged
merged 9 commits into from
Jul 9, 2021

Conversation

kurozumi
Copy link
Contributor

@kurozumi kurozumi commented Jul 1, 2021

概要(Overview・Refs Issue)

以下のIssueの対応です。
#5063

方針(Policy)

実装に関する補足(Appendix)

テスト(Test)

新規会員登録ページで攻撃の可能性のあるテキストを入力してサブミットしたらそのテキストが削除されるのを確認しました。

相談(Discussion)

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

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

レビュワー確認項目

  • 動作確認
  • コードレビュー
  • E2E/Unit テスト確認(テストの追加・変更が必要かどうか)
  • 互換性が保持されているか
  • セキュリティ上の問題がないか

@okazy okazy added the enhancement 機能追加 label Jul 5, 2021
@okazy okazy added this to the 4.1 milestone Jul 5, 2021
@kurozumi
Copy link
Contributor Author

kurozumi commented Jul 6, 2021

@okazy
プルリクはテストが通らないと見てもらえないんでしたっけ?

テストが落ちているのは #5072testReadCsvFileWithTrailingBlankLines のテストのみということをご報告しておきます。

@chihiro-adachi
Copy link
Contributor

@kurozumi
反応遅れてすみません、PRありがとうございます。
こちら確認すすめていきます。

@chihiro-adachi
Copy link
Contributor

@kurozumi
会社名や住所のビル名に&が入るケースがあり、以下のようにエスケープされるようです。

H&M -> H&M

&を除外することは可能でしょうか?

@chihiro-adachi
Copy link
Contributor

@kurozumi
TextTypeだけでなく、TextAreaTypeも適用可能でしょうか?
注文時のお問い合わせ欄(dtb_order.message)も対象にしたく。

@kurozumi
Copy link
Contributor Author

kurozumi commented Jul 7, 2021

@chihiro-adachi

& を除外することができなかったので & が入力されたら全角に変換するよう対応しました。

TextAreaTypeTextType を継承しているので TextType と同様に適用されます。

@chihiro-adachi
Copy link
Contributor

@kurozumi

& を除外することができなかったので & が入力されたら全角に変換するよう対応しました。

ありがとうございます。

TextAreaType は TextType を継承しているので TextType と同様に適用されます。

ありがとうございます、こちらも承知しました。

@chihiro-adachi
Copy link
Contributor

@kurozumi
10万件程データを流してみたのですが、意図せず変換されてしまうものは以下でした。

&︙会社名やビル名等、で&が入るケース
<>(><)みたいな顔文字。注文時の問い合わせ欄で利用されるケース

お手数なのですが、<>も、&と同様に全角変換いただくことはできますでしょうか。

実運用始まれば変換したくない文字種がもう少しでてくるかもしれないですが、いったんは&><を除外できれば大丈夫かなと思っています。

@chihiro-adachi
Copy link
Contributor

@kurozumi
修正ありがとうございます。
composer.json等、関係のない修正が入っているようなのでそちらだけご対応おねがいできますでしょうか。

@kurozumi
Copy link
Contributor Author

kurozumi commented Jul 9, 2021

@chihiro-adachi

修正しました。

composer.lockやsymfony.lockは大丈夫ですかね?

composer.lockのplugin-api-versionが 2.0.0 から 1.1.0に書き換わっているのでこちらは修正したほうが良いですか?
また、content-hashも書き換わっていますがこちらはこのままで大丈夫でしょうか?

@chihiro-adachi
Copy link
Contributor

@kurozumi
ありがとうございます、助かります。
composer.lock等はマージの際にこちらで調整しますので、そのままで大丈夫です。

@kurozumi
Copy link
Contributor Author

kurozumi commented Jul 9, 2021

@chihiro-adachi

<>を全角変換対象にしたことで不十分になっていたテストを修正しました。

@chihiro-adachi
Copy link
Contributor

@kurozumi
ありがとうございます、確認します。

@chihiro-adachi
Copy link
Contributor

ユニットテストが落ちていますが、既知の問題のためマージします。

@chihiro-adachi chihiro-adachi merged commit d554e44 into EC-CUBE:4.1-core Jul 9, 2021
@chihiro-adachi
Copy link
Contributor

@kurozumi
ありがとうございます、マージしました。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 機能追加
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants