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

[ CTA ] エスケープ処置&テスト調整 #1167

Merged
merged 5 commits into from
Feb 6, 2025

Conversation

kurudrive
Copy link
Member

@kurudrive kurudrive commented Feb 6, 2025

チケットへのリンク / 変更の理由(元のissueがあればリンクを貼り付ければOK)

@mtdkei 軽くコメントだけ確認しておいてくださいー。

@kurudrive kurudrive changed the title Fix/cta/iframe add test [ CTA ] エスケープ処置&テスト調整 Feb 6, 2025
@@ -478,9 +478,6 @@ public static function render_cta_content( $id ) {
// リセットしないと$postが改変されたままでコメント欄が表示されなくなるなどの弊害が発生する.
wp_reset_postdata();

// wp_kses_post でエスケープすると outerブロックが出力するstyle属性を無効化されるので使わないように.
// 出力時にエスケープしたいが、wp_kses_post だと style属性が無効化される / wp_kses でも allow_html で opacity を許可しても無視・削除される。
// 結局本文欄はどのみち HTML ブロックで <script>alert(0)</script> 入れられ標準でXSSは実行可能なので、ここでは処理していない。
return self::safe_kses_post( do_blocks( do_shortcode( $content ) ) );
Copy link
Member Author

Choose a reason for hiding this comment

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

@mtdkei #1165 の前はこのコメントの理由でエスケープしていなかったのですが、
エスケープするように変更したので、この "なぜエスケープしていないか" の理由は削除

Comment on lines +775 to +777
$tags['style'] = array(
'type' => true,
);
Copy link
Member Author

Choose a reason for hiding this comment

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

@mtdkei スタイルタグの許可を追加

Comment on lines +301 to +319
array(
'name' => 'class and style',
'input' => '<div class="class-name" style="margin-bottom:3rem">CTA</div>',
'expected' => '<div class="class-name" style="margin-bottom:3rem">CTA</div>',
),
array(
'name' => 'allow style tag',
'input' => '<div>CTA</div><style>.target-class { background-image: url(http://localhost:8888/image.jpg);}</style>',
'expected' => '<div>CTA</div><style>.target-class { background-image: url(http://localhost:8888/image.jpg);}</style>',
),
array(
'name' => 'allow style tag with media query',
'input' => '<div>CTA</div><style>@media screen and (max-width: 575.98px) {.target-class {
background-image: url(http://localhost:8888/image.jpg);
background-position: 50% 50%!important;}}</style>',
'expected' => '<div>CTA</div><style>@media screen and (max-width: 575.98px) {.target-class {
background-image: url(http://localhost:8888/image.jpg);
background-position: 50% 50%!important;}}</style>',
),
Copy link
Member Author

Choose a reason for hiding this comment

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

@mtdkei クラス名、スタイルの直書き、styleタグがエスケープされてしまわないかのテスト追加

);

foreach ( $test_cases as $case ) {
$actual = Vk_Call_To_Action::safe_kses_post( $case['input'] );
$this->assertEquals( $case['expected'], $actual );
$this->assertEquals( $case['expected'], $actual, $case['name'] );
Copy link
Member Author

Choose a reason for hiding this comment

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

@mtdkei テストがコケた時にコケた場所がわかりやすいようにテスト項目名追加

@kurudrive kurudrive merged commit 1610d9e into fix/cta/iframe Feb 6, 2025
@kurudrive kurudrive deleted the fix/cta/iframe-add-test branch February 6, 2025 07:52
@mtdkei
Copy link
Contributor

mtdkei commented Feb 6, 2025

@kurudrive
ありがとうございます。styleタグに対応したテストの追加、また、テスト項目名を追加いただきありがとうございます。おっしゃる通りそちらのほうがわかりやすいですね。

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.

2 participants