Conversation
ghost
marked this pull request as ready for review
|
@SuzukiShuntarou |
|
@harada-webdev |
|
@SuzukiShuntarou |
|
@harada-webdev |
sekito1107
force-pushed
the
feature/prohibit-style-tag-and-onload-event-in-markdown
branch
5 times, most recently
from
96418e0 to
d1b6e63
Compare
|
良ければこちらのレビューお願いできないでしょうかー😀 |
|
@sekito1107 |
|
@sugiwe |
sugiwe
left a comment
sugiwe
left a comment
There was a problem hiding this comment.
@sekito1107
お待たせいたしました！レビューいたしました。
動作は無事確認できまして、内容についても全体的に問題ないように思います🙆
文章が長くなってしまい恐縮なのですが命名関連で2箇所と、テストについて1箇所コメントをさせていただきましたので、ご確認お願いいたします🙏
| const escapeTags = ['style'] | ||
| const escapeAttributes = ['onload'] | ||
|
|
||
| md.core.ruler.push('sanitize', function (state) { |
There was a problem hiding this comment.
sanitizeというルール名は、シンプルさはとても良いのですがhtmlタグも含めた全てを除去することを連想してしまうかもしれないと思いました👀
特に、issueの方も拝見しましたが今回は「あえてhtmlタグは除外しない」という点が特徴だと思いますので、issue・PRのタイトルになっているstyle・onloadを除外することを明示的に名前にするのが良いかなと思ったのですがいかがでしょうか？（例えば、ちゃんと説明するならescape_style_and_onload、少し省略するならescape_style_onloadなど）
またこれに関連して、ファイル名のmarkdown-it-sanitizer.jsについても検討の余地があるかもしれないと思いました。
全く同名の markdown-it-sanitizer というnpmがあるので、今回のissueからの文脈を把握していれば良いのですが、将来的にコードだけを見た時に「なぜnpmのmarkdown-it-sanitizerを使わずに自前のコードになっているのだろう」と疑問を持たれるかもしれません。
（何を隠そう、僕が「なんでnpmのmarkdown-it-sanitizerを剥がして自前にしたんだろう？」と疑問に思ってしまいました。今回でいうとissueをしっかり読まずにPRとコードの方を先に読んでしまった僕が悪いのですが😂）
なので、ルール名の方に合わせるなどして、例えばですがmarkdown-it-escape-style-and-onload.jsなどはいかがでしょうか？
（少し長いような気もしていますが現時点で他にmarkdown-it-task-lists-initializer.jsといったファイルもあるので、個人的には長めでも具体的な名前のほうが良いかなとは思いますが、長さの塩梅についてはお任せいたします🙏）
There was a problem hiding this comment.
両方の命名、結構僕もしっくりきてないところありました！
ファイル名の背景なのですが、同名のnpmがあることはこのIssueが元々そのnpmを利用する予定だったので存じていました。
しかし最終公開が9年前ということもあり問題ないかと思ったのですが、今確認してみると週当たりのダウンロード数は未だに多いようなので、おっしゃって頂いているように変更するのは妥当な選択かと思いました。
ご提案頂いたファイルの命名についてはIssueの内容を良く表していると思う一方で、現在はstyleタグとonload属性だけをエスケープしていますが、後々回避したいタグ、属性が増えることを予想して簡単にエスケープ対象を追加出来るように設計したので、ある程度汎用性を持たせた名前にしておかないと回避するタグ、属性が増えた際にまたファイル名を変更しなくてはいけないという問題があると思っています。
ルール名をescapeにしてファイル名をmarkdown-it-escapeくらいにしてまとめるのが落としどころとして良いのではないかと思うのですがいかがでしょうか？
こちらは今回の修正に含めず、合意を得られる形を見つけてから修正したいと思いますー。
There was a problem hiding this comment.
@sekito1107
なるほどです、今後のカスタマイズを見越しての命名だったのですね。確かにmarkdown-it-escape-style-and-onloadにしてしまうとカスタマイズに伴ってまた名前を変更しなくてはいけないし、この命名ルールだと回避する要素がどんどん名前に連なっていくことになり、ちょっと厳しいですね💦
同名npmのmarkdown-it-sanitizerを避けてmarkdown-it-escapeにするアイデア、とても良いと思いました！
| const parser = new DOMParser() | ||
| const doc = parser.parseFromString(`<body>${html}</body>`, 'text/html') | ||
|
|
||
| doc.querySelectorAll('*').forEach((el) => { |
There was a problem hiding this comment.
elはelementの略称かと思います、認知度も高く一般的な略称かとは思うのですが、bootcampアプリ内では今では数の減ったvue.js関連ファイル以外ではほぼelではなくelementを使っているようでしたので、コードが少し長くはなるもののelementに揃えるのが良いかなと思いましたがいかがでしょうか？
（nitsで個人的な感想な気もしているので、elでも全然大丈夫だとは思います！）
There was a problem hiding this comment.
他のjavascriptの実装まで確認して頂いたのですね！ありがとうございます！
プロジェクトとしては、たしかに揃ってるほうが可読性としては優れていると思いますので、修正させて頂きました！
test/system/markdown_test.rb
Outdated
| color = page.evaluate_script(<<~JS) | ||
| getComputedStyle(document.querySelector('p')).color | ||
| JS | ||
| assert_not_equal 'rgb(255, 0, 0)', color |
There was a problem hiding this comment.
こちらの部分ですが、実際に表示されているCSSのカラーをJSで取得して、「赤が表示されていない」ことを確認しているテストとお見受けしています。
色を実際に確認しているのは非常に丁寧で良いなと思ったのですが、assert_text '<style>p { color: red; }</style>'によって{ color: red; }がエスケープされ文字列のまま表示されていることは保証できているので、その上でJSを使って色を取得し確認する、ということまでしなくても良いかもしれないと思いましたがいかがでしょうか？
最近、システムテストをどこまで書くかというQAもあり、明確な定義は難しいという回答もありましたが、今回でいうとassert_textの方でエスケープされた表示を確認できれば良いかな？と感じました。
僕が掴めていない意図などもあるかもしれませんのでその場合は何なりとおっしゃってください、ご検討よろしくお願いいたします🙏
There was a problem hiding this comment.
その通りですね、過剰な検証だったと思いますー。
こちらエスケープされてることだけを確認するように、修正させて頂きました👍
sekito1107
left a comment
sekito1107
left a comment
There was a problem hiding this comment.
@sugiwe
丁寧なレビューありがとうございます、大変参考になりました🙇♂️
修正と併せて、1点命名についてご相談させて頂いたので確認して頂ければと思います🙏
| const escapeTags = ['style'] | ||
| const escapeAttributes = ['onload'] | ||
|
|
||
| md.core.ruler.push('sanitize', function (state) { |
There was a problem hiding this comment.
両方の命名、結構僕もしっくりきてないところありました！
ファイル名の背景なのですが、同名のnpmがあることはこのIssueが元々そのnpmを利用する予定だったので存じていました。
しかし最終公開が9年前ということもあり問題ないかと思ったのですが、今確認してみると週当たりのダウンロード数は未だに多いようなので、おっしゃって頂いているように変更するのは妥当な選択かと思いました。
ご提案頂いたファイルの命名についてはIssueの内容を良く表していると思う一方で、現在はstyleタグとonload属性だけをエスケープしていますが、後々回避したいタグ、属性が増えることを予想して簡単にエスケープ対象を追加出来るように設計したので、ある程度汎用性を持たせた名前にしておかないと回避するタグ、属性が増えた際にまたファイル名を変更しなくてはいけないという問題があると思っています。
ルール名をescapeにしてファイル名をmarkdown-it-escapeくらいにしてまとめるのが落としどころとして良いのではないかと思うのですがいかがでしょうか？
こちらは今回の修正に含めず、合意を得られる形を見つけてから修正したいと思いますー。
| const parser = new DOMParser() | ||
| const doc = parser.parseFromString(`<body>${html}</body>`, 'text/html') | ||
|
|
||
| doc.querySelectorAll('*').forEach((el) => { |
There was a problem hiding this comment.
他のjavascriptの実装まで確認して頂いたのですね！ありがとうございます！
プロジェクトとしては、たしかに揃ってるほうが可読性としては優れていると思いますので、修正させて頂きました！
test/system/markdown_test.rb
Outdated
| color = page.evaluate_script(<<~JS) | ||
| getComputedStyle(document.querySelector('p')).color | ||
| JS | ||
| assert_not_equal 'rgb(255, 0, 0)', color |
There was a problem hiding this comment.
その通りですね、過剰な検証だったと思いますー。
こちらエスケープされてることだけを確認するように、修正させて頂きました👍
|
@sekito1107 |
|
@sugiwe |
sugiwe
left a comment
sugiwe
left a comment
There was a problem hiding this comment.
@sekito1107
修正ありがとうございます、確認いたしました！
問題ないと思いますのでApproveとさせていただきます〜！
|
@sugiwe |
sekito1107
force-pushed
the
feature/prohibit-style-tag-and-onload-event-in-markdown
branch
from
dc1cd90 to
c99701c
Compare
|
こちらレビューお願いできないでしょうかー。 |
|
@sekito1107 さん |
|
@kutimiti1234 宜しくお願いしますー！ |
sekito1107
force-pushed
the
feature/prohibit-style-tag-and-onload-event-in-markdown
branch
from
dcfd371 to
c58da0e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
app/controllers/welcome_controller.rb(1 hunks)app/javascript/initializeComment.js(2 hunks)app/javascript/markdown-initializer.js(2 hunks)app/javascript/new-comment.js(1 hunks)app/javascript/packs/application.js(1 hunks)app/javascript/stylesheets/initializers/_reset.sass(0 hunks)app/javascript/textarea-initializer.js(2 hunks)
💤 Files with no reviewable changes (1)
- app/javascript/stylesheets/initializers/_reset.sass
✅ Files skipped from review due to trivial changes (1)
- app/javascript/new-comment.js
🚧 Files skipped from review as they are similar to previous changes (2)
- app/javascript/markdown-initializer.js
- app/javascript/textarea-initializer.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.js
⚙️ CodeRabbit configuration file
**/*.js: - どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない。
- fetchメソッドの代わりにrequest.jsを使う。
Files:
app/javascript/packs/application.jsapp/javascript/initializeComment.js
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/controllers/welcome_controller.rb
🧠 Learnings (1)
📓 Common learnings
Learnt from: tyrrell-IH
PR: fjordllc/bootcamp#8807
File: app/views/welcome/job_support.html.slim:391-392
Timestamp: 2025-06-21T22:30:20.116Z
Learning: このプロジェクトにはRailsの組み込みmarkdownヘルパが利用できないため、markdownのHTML変換には自前でヘルパを実装する必要がある。
🧬 Code graph analysis (1)
app/javascript/initializeComment.js (1)
app/javascript/new-comment.js (3)
markdownInitializer(38-38)commentEditorPreview(41-43)editorTextarea(44-46)
🪛 ESLint
app/javascript/packs/application.js
[error] 87-87: 'VueMounter' is defined but never used.
(no-unused-vars)
[error] 88-88: 'Questions' is defined but never used.
(no-unused-vars)
[error] 89-89: 'UserProducts' is defined but never used.
(no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
| @faqs = FAQCategory.find_by(name: FAQ_CATEGORY_NAME)&.faqs || [] | ||
| end |
There was a problem hiding this comment.
未定義定数 FAQ_CATEGORY_NAME を参照しています。元のヘルパを使って順序と戻り値型の整合も維持しましょう。
FAQ_CATEGORY_NAMEは本クラスに未定義で NameError になります。- さらに
faqs_forを介さないことでorder(:position)が失われ、FAQ.noneではなく[]が返るため型（Relation→Array）が不一致になります。
修正案（定数・順序・型の整合を復元）:
- @faqs = FAQCategory.find_by(name: FAQ_CATEGORY_NAME)&.faqs || []
+ @faqs = faqs_for(FAQ_CATEGORY_NAME_FOR_TRAINING)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @faqs = FAQCategory.find_by(name: FAQ_CATEGORY_NAME)&.faqs || [] | |
| end | |
| @faqs = faqs_for(FAQ_CATEGORY_NAME_FOR_TRAINING) | |
| end |
🤖 Prompt for AI Agents
In app/controllers/welcome_controller.rb around lines 40-41, the code directly
references an undefined constant FAQ_CATEGORY_NAME and bypasses the existing
faqs_for helper (losing order and returning an Array instead of an
ActiveRecord::Relation). Replace the direct find_by fallback with a call to the
existing faqs_for helper so the category lookup, order(:position) and relation
type are preserved; if FAQ_CATEGORY_NAME must be used, reference or define it
from the module where it lives (or pass the appropriate category name/symbol
into faqs_for) so no NameError occurs and the returned value remains an
ActiveRecord::Relation, not [].
| const commentDisplay = comment.querySelector('.js-comment-display') | ||
| const commentDisplayContent = | ||
| commentDisplay?.querySelector('.js-comment-html') | ||
| const commentDisplayContent = commentDisplay?.querySelector('.js-comment-html') |
There was a problem hiding this comment.
commentRaw/commentDisplayContent が存在しない場合に保存時に例外が発生します。null安全な参照に修正を。
- 行83: commentRaw が null の時に
textContent参照でクラッシュします。 - 行25, 32-34: 取得時は optional chaining を統一し、後段の参照もnull安全に。
最小修正案:
-const commentRaw = commentDisplay.querySelector(
+const commentRaw = commentDisplay?.querySelector(
'.js-comment-raw'
)- commentRaw.textContent = editorTextarea.value
+ commentRaw?.textContent = editorTextarea.value加えて（選択肢、推奨）保存後のHTML更新もnull安全にしてください（該当行は変更外ですが重要）:
// 82行あたり
if (commentDisplayContent) {
commentDisplayContent.innerHTML = markdownInitializer.render(savedComment)
}（任意だが有益）nullを配列に含めていると toggleVisibility が落ちます。nullを除外する実装にすると堅牢です:
// 関数 toggleVisibility
function toggleVisibility(elements, className) {
elements.filter(Boolean).forEach((el) => el.classList.toggle(className))
}（任意の改善）fetchの代わりに @rails/request.js を使うガイドラインがあります。updateComment/deleteComment の置き換えを検討ください。
Also applies to: 32-34, 83-83
🤖 Prompt for AI Agents
In app/javascript/initializeComment.js around lines 25, 32-34 and 83, references
to commentRaw/commentDisplayContent can be null and currently cause exceptions;
make the DOM queries and subsequent accesses null-safe by using optional
chaining when selecting elements (e.g., commentDisplay?.querySelector(...)) and
guard all downstream property accesses (like textContent) with null checks or
optional chaining before using them; also update the post-save HTML update to
only set innerHTML when commentDisplayContent exists, and consider filtering out
null values passed to toggleVisibility so it ignores falsey entries (e.g.,
elements.filter(Boolean).forEach(...)).
| import VueMounter from '../VueMounter.js' | ||
| import Questions from '../components/questions.vue' | ||
| import UserProducts from '../components/user-products.vue' |
There was a problem hiding this comment.
未使用のVue関連インポートでESLintエラー。用途がなければ削除、使うなら実際にマウントしてください。
ESLint: no-unused-vars が3件。このままではCIを落とします。現時点で参照されていないため削除が安全です。将来使用する場合は VueMounter でマウントする等、使用箇所を追加してください。
適用案（未使用のインポートを削除）:
-import VueMounter from '../VueMounter.js'
-import Questions from '../components/questions.vue'
-import UserProducts from '../components/user-products.vue'補足: 本PRの目的（Markdownサニタイズ）と無関係なVueコードの追加は、ガイドライン「どうしても避けられない時以外に新規にVue.js, Reactのコードを追加しない」にも抵触します。別PR分割を推奨します。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import VueMounter from '../VueMounter.js' | |
| import Questions from '../components/questions.vue' | |
| import UserProducts from '../components/user-products.vue' |
🧰 Tools
🪛 ESLint
[error] 87-87: 'VueMounter' is defined but never used.
(no-unused-vars)
[error] 88-88: 'Questions' is defined but never used.
(no-unused-vars)
[error] 89-89: 'UserProducts' is defined but never used.
(no-unused-vars)
🤖 Prompt for AI Agents
In app/javascript/packs/application.js around lines 87 to 89, there are three
unused Vue-related imports (VueMounter, Questions, UserProducts) causing ESLint
no-unused-vars failures; remove these unused import lines to satisfy linting (or
if you intend to use them in this PR, instead call VueMounter to mount the
components where appropriate). Ensure the final change either deletes the three
import statements or replaces them with actual mounting code using VueMounter so
the imports are referenced; keep this PR focused on Markdown sanitization and
move any new Vue additions to a separate PR if not needed here.
9 participants
Issue
概要
Markdownエディタ内で、許可されているタグ、属性以外を使えないようにしました。
変更確認方法
feature/prohibit-style-tag-and-onload-event-in-markdownをローカルに取り込むi.
git fetch origin feature/prohibit-style-tag-and-onload-event-in-markdownii.
git checkout feature/prohibit-style-tag-and-onload-event-in-markdownforeman start -f Procfile.devで開発環境を立ち上げる.komagata、パスワードtesttestでログインするMovie
変更前
development.Docs._.FBC.-.Google.Chrome.2025-04-22.18-28-42.mp4
変更後
development.Docs._.FBC.-.Google.Chrome.2025-06-01.20-44-41.mp4
Summary by CodeRabbit
新機能
バグ修正 / 改善
スタイル
テスト