LoginSignup
1
4

More than 5 years have passed since last update.

@takehanKosuke(こーすけ)inCyberAgent 21 Engineers

第1回クソコードレビュー大会解説！

Last updated at Posted at 2019-04-18

はじめに

この記事は僕参加しているグループ内でプログラミングの知識の底上げのために行った大会の解説のための記事です。
良ければみなさんもやって見てください！
https://github.com/takehanKosuke/bad_cord_test_of_rails

そしてやった後はぜひ感想やなどのフォードバックを何かしらで教えていただけると嬉しいです！

指摘して欲しかったジャンル

１. とりあえず直さないといけない
２. きれいなコーディング(レベル１)※フォーマット周り
３. きれいなコーディング(レベル２)※gemのメソッドとか
＝＝＝＝ここより下は独学でプログラミングしてたら多分かなり難しいと思っている＝＝＝＝
４. きれいなコーディング(レベル３)※処理回数を減らす的なやつ
５. セキュリティー対策※XSS

問題項目一覧（全20問）

  • 余計な文字列が出現している
  • 正常に式展開がされていない
  • 記事の削除ができない
  • userのarticlesのアソシエーションにdependent: :destroyをつけていない
  • バリデーションを掛けていない
  • save/update/destroyでエラーハンドリングをしていない
  • userのenumであるroleカラムにdefault値をつけていない
  • 他の人になりすましてデータを作成更新削除ができてしまう
  • seeds.rbのcreateに「!」をつけてない
  • インデントが揃っていない
  • シングルクォーテーションとダブルクォーテーションが混在
  • new/editでの共通部分を部分テンプレート化していない
  • article_pathの引数が「article.id」になっている
  • enumの検索メソッドを使用していない
  • authenticate_user!を使用していない
  • n+1問題が発生している
  • ビジネスロジックをcontrollerに書いている
  • デメテルの法則を使用していない
  • Articleにindexを貼っていない
  • XSS（クロスサイトスクリプティング）攻撃からの脆弱性がある

１、とりあえず直さないといけない

  • 余計な文字列が出現している
  • 正常に式展開がされていない
  • 記事の削除ができない
  • userのarticlesのアソシエーションにdependent: :destroyをつけていない
  • バリデーションを掛けていない
  • save/update/destroyでエラーハンドリングをしていない
  • statusにデフォルト値をつけていない
  • 他の人になりすましてデータを作成更新削除ができてしまう
  • seeds.rbのcreateに「!」をつけてない

余計な文字列が出現している

views/articles/index.html.erb
<!--修正前-->
</tr>
<%= @articles.each do |article| %>
　　<tr>

<!--修正後-->
</tr>
<% @articles.each do |article| %>
　　<tr>

スクリーンショット 2019-03-31 13.43.52.png
これはもう解説不要な気がしますが、ここに”＝”はいらないです。
気をつけましょうw

正常に式展開がされていない

view/articles/index.html.erb
<!--修正前-->
<td><%= '#{article.pv}pv' %></td>

<!--修正後-->
<td><%= "#{article.pv}pv" %></td>

スクリーンショット 2019-04-02 1.28.27.png

rubyでは式展開を行うときにには シングルクォーテーション ではなく ダブルクォーテーション を使う必要があります。

じゃあそんなこと言うならシングルクォーテーション使わなければいいんじゃね？って話になりそうですが、、、

下の記事にもあるようにシングルクォーテーションの方がわずかながら処理スピードが速くなるそうなので、処理速度をめちゃめちゃ意識した場合はその辺のことも気をつけていければいいかなと思います。
https://qiita.com/Kenji_TAJIMA/items/78555053a36c214be350

記事の削除ができない

view/articles/index.html.erb
<!--修正前-->
<td><%= link_to '削除', article_path(article.id), data: { confirm: "#{article.title}を削除しますか？" } %></td>

<!--修正後-->
<td><%= link_to '削除', article_path(article.id), method: :delete, data: { confirm: "#{article.title}を削除しますか？" } %></td>

railsのpath は何も指定しない場合getメソッドを呼んでしまうためdeleteメソッドを使う場合はしっかりとmethod: :deleteというようにメソッド名を指定しないといけません
今回の場合method名を指定しない場合はarticleのshowアクションに移動します

userのarticlesのアソシエーションにdependent: :destroyをつけていない

models/user.rb
#修正前
class User < ApplicationRecord
  # Include default devise modules. Others available are:
  # :confirmable, :lockable, :timeoutable, :trackable and :omniauthable
  devise :database_authenticatable, :registerable,
         :recoverable, :rememberable, :validatable

  has_many :articles

  validates :name, presence: true

  enum role: { normal: 1, admin: 2 }
end

#修正後
class User < ApplicationRecord
  # Include default devise modules. Others available are:
  # :confirmable, :lockable, :timeoutable, :trackable and :omniauthable
  devise :database_authenticatable, :registerable,
         :recoverable, :rememberable, :validatable

  has_many :articles, dependent: :destroy  # 追記

  validates :name, presence: true

  enum role: { normal: 1, admin: 2 }
end

もしこのdependent: :destroyをつけなかった場合アドミンユーザーでユーザーの削除を行うと
スクリーンショット 2019-03-31 13.19.40.png
こんな感じのエラーが返されると思います。
これはどういうことかというと、、、
記事の一覧を表示させるときにユーザーの名前を出しているのですが、その記事に紐づくユーザーが削除されてしまっているため参照するユーザー名がないよ〜
というようなエラーです。

これを解決するためには、ユーザーを削除したときにそれと一緒にユーザーの作った記事も一緒に削除するというのが一般的ですので、
今回はuserのarticlesのアソシエーションにdependent: :destroyをつけてuserが削除された時にそのユーザーに紐付く記事も一緒に削除しています

バリデーションを掛けていない

models/article.rb
#修正前
class Article < ApplicationRecord
  belongs_to :user
end

#修正後
class Article < ApplicationRecord
  belongs_to :user

  validates :title, presence: true
  validates :body, presence: true
end

記事を作成する際タイトルや内容が空だと不自然ですよね？
バリデーションを掛けないとタイトルや内容が空のまま記事が作成できてしまいます。
今回はデータがあることを強要するvalidatesを使ってバリデーションをかけています。

バリデーションについての詳細は↓の記事がかなりよかったので気になるひとは是非読んでみてください
https://qiita.com/shunhikita/items/772b81a1cc066e67930e

save/updateでエラーハンドリングしていない

controllers/articles_controller.rb
#修正前
  def create
    @article = Article.new(article_params)
    @article.save
    redirect_to root_path
  end

  def update
    @article = Article.find(params[:id])
    @article.update(article_params)
    redirect_to root_path
  end

#修正後
def create
   @article = Article.new(article_params)
   if @article.save
     redirect_to @article, flash: { success: 'articleが作成されました' }
   else
     render :new
   end
end

def update
   if @article.update(article_params)
     redirect_to @article, flash: { success: 'articleが更新されました' }
   else
     render :edit
   end
end

そもそもエラーハンドリングって？

エラーハンドリングとは、プログラムの処理中に処理が妨げられる事象が発生した際、その処理をエラーとして対処する処理のことである。 例外処理とも呼ばれる。
(出典：weblio)

今回の場合でエラーハンドリングをちゃんとしないと、、、
スクリーンショット 2019-03-17 20.57.06.png
スクリーンショット 2019-03-17 20.57.34.png
このように新規作成ボタンを押してもボタンが無効になるだけでデータが作成されないです

なので、save,updateなどの処理を挟む時は必ずif文を使ってエラーハンドリングをして
データが作成されない時のフォローをしてあげましょう。

userのenumであるroleカラムにdefault値をつけていない

db/migrate/20190220135142_devise_create_users.rb
#修正前
 # カスタムカラム
t.string :name, null: false
t.integer :role, null: false

#修正後
 # カスタムカラム
t.string :name, null: false
t.integer :role, null: false, default: 1

もしroleカラムにデフォルト値をつけなかった場合ユーザーを新規に作成するときに以下のようなエラーが返されると思います。
スクリーンショット 2019-03-31 13.16.41.png

これはどういうことかというと、、、「roleカラムが指定されてないんだけど〜」っていうエラーですね
今回ユーザーのroleカラムはユーザーの新規登録時に操作させません
（というか、新規登録時にユーザー自身アドミンユーザーかノーマルユーザーかを決められたらアドミンユーザーの意味がw）
ですが、ユーザーのroleカラムは全てのユーザーが持っていなければならないのです。
そのようなときは最初からroleにデフォルト値を設定してフォローするのがベターだと思われます。

またenumは基本的にデフォルト値をつけるのが慣習となっているので（多分w）
そう言った意味でもデフォルト値をつけるべきでしょう

他の人になりすましてデータを作成更新削除ができてしまう

controller/articles_controller.rb
#修正前
#一部省略

  def new
    @article = Articles.new
  end

  def create
    @article = Article.new(article_params)
    @article.save
    redirect_to root_path
  end

  def edit
    @article = Article.find(params[:id])
  end

  def update
    @article = Article.find(params[:id])
    @article.update(article_params)
    redirect_to root_path
  end

  def destroy
    @article = Article.find(params[:id])
    @article.destroy
    redirect_to root_path
  end

#一部省略

def article_params
  params.require(:article).permit(
    :title,
    :body,
    :user_id
  )
end


#修正後
before_action :article_author, only: %i[edit update destroy]

#一部省略

def new
  @article = current_user.articles.new
end

def create
  @article = current_user.articles.new(article_params)
  if @article.save
    redirect_to @article, flash: { success: 'articleが作成されました' }
  else
    render :new
  end
end

#一部省略
private
def article_author
  @article = current_user.articles.find(params[:id])
end

def article_params
  params.require(:article).permit(
    :title,
    :body,
  )
end

どういうことか実際にやってみましょう！
スクリーンショット 2019-03-31 13.32.52.png
新規作成の画面を開き、開発者ツールを開き上記の写真のuserIDを指定しているところを見つけましょう。
そこのIDを今ログインしているユーザー以外のIDを指定して新規作成をおします。
すると以下のようにログインしているユーザー以外で記事を作成できてしまいます
スクリーンショット 2019-03-31 13.33.12.png
この現象を避けるため、今回はuserとarticleとの紐付けをhidden_fieldで現在のユーザーのidを送るのではなく、

@article = current_user.articles.new

というような記述の仕方でユーザーと記事の紐付けを行いました。

seeds.rbのcreateに「!」をつけてない

seeds.rb
#修正前
puts "ユーザー作成中"
User.create(
  [
    {
      name: 'kosuke',
      email: 'test1@test.com',
      password: '111111',
      role: 2,

#修正後
puts "ユーザー作成中"
User.create!( #「!」をつけた
  [
    {
      name: 'kosuke',
      email: 'test1@test.com',
      password: '111111',
      role: 2,

seedファイルのcreateには「!」をつけないとエラーを吐いてくれません
つまり、seedが失敗してもそれに気づくことが難しくなってしまうのでseedファイルには必ず「!」をつけましょう

本当にエラー吐かないの？！って思う人はrake db:seedを「！」をつけたバージョンとつけなかったバージョンの２回叩いてみるとわかると思います

２、きれいなコーディング（レベル１）

  • インデントが揃っていない
  • シングルクォーテーションとダブルクォーテーションが混在
  • new/editでの共通部分を部分テンプレート化していない
  • article_pathの引数が「article.id」になっている

インデントが揃っていない

views/articles/index.html.erb
<!--修正前-->
<table>
  <tr>
    <th>タイトル</th>
      <th>内容</th>
      <th>pv数</th>
    <th>ライター</th>
    <th></th>
    <th></th>
  </tr>
  <% @articles.each do |article| %>
    <tr>
        <td><%= link_to '#{article.title}', article_path(article.id) %></td>
        <td><%= article.body.html_safe %></td>
        <td><%= "#{article.pv}pv" %></td>
        <td><%= "#{article.user.name}" %></td>
        <td><%= link_to "編集", edit_article_path(article.id) %></td>
      <% if article.user == current_user %>
        <td><%= link_to '削除', article_path(article.id), method: :delete, data: { confirm: "#{article.title}を削除しますか？" } %></td>
      <% end %>
      </tr>
    <% end %>
</table>

<!--修正後-->
<table>
  <tr>
    <th>タイトル</th>
    <th>内容</th>
    <th>pv数</th>
    <th>ライター</th>
    <th></th>
    <th></th>
  </tr>
  <% @articles.each do |article| %>
    <tr>
      <td><%= link_to '#{article.title}', article_path(article.id) %></td>
      <td><%= article.body.html_safe %></td>
      <td><%= "#{article.pv}pv" %></td>
      <td><%= "#{article.user.name}" %></td>
      <td><%= link_to "編集", edit_article_path(article.id) %></td>
      <% if article.user == current_user %>
        <td><%= link_to '削除', article_path(article.id), method: :delete, data: { confirm: "#{article.title}を削除しますか？" } %></td>
      <% end %>
    </tr>
  <% end %>
</table>

初めてインデントとかを気にし始めた時はかなり大変さを感じるかもしれないですが、それに慣れてしまうと、むしろインデントが揃ってないと違和感しか感じないですw

ちなみに各エディタにはインデントとかを揃えてくれるプラグインが存在するはずなので是非調べて見てください。
僕自身は「atom」を使っているのですが、「atom」では「atom-beautify」と言うプラグインがあり、それを使うと
control + option + B
で自動で整形してくれます。

シングルクォーテーションとダブルクォーテーションが混在

views/articles/index.html.erb
<!--21行目-->
<!--修正前-->
<td><%= link_to "編集", edit_article_path(article.id) %></td>
<% if article.user == current_user %>
  <td><%= link_to '削除', article_path(article.id), method: :delete, data: { confirm: "#{article.title}を削除しますか？" } %></td>

<!--修正後-->
<td><%= link_to '編集', edit_article_path(article.id) %></td>
<% if article.user == current_user %>
  <td><%= link_to '削除', article_path(article.id), method: :delete, data: { confirm: "#{article.title}を削除しますか？" } %></td>

プログラミングをする際はできるだけ書き方を統一しましょう。
今回はシングルクォーテーションに揃えていますが、プログラム全体でダブルクォーテーションに揃えているのであれば、ダブルクォーテーションで揃えるのが適切だと思います。

new/editの共通部分が部分テンプレート化していない

views/articles/new.html.erb
<!--修正前-->
<h1>新規作成</h1>
<% if @article.errors.any? %>
  <ul>
    <% @article.errors.full_messages.each do |message| %>
      <li><%= message %></li>
    <% end %>
  </ul>
<% end %>

<%= form_with model: @article, local: true do |f| %>
  <%= f.hidden_field :user_id, value: current_user.id %>
  <div>
    <%= f.label :title %>
    <%= f.text_field :title %>
  </div>
  <div>
    <%= f.label :body %>
    <%= f.text_area :body %>
  </div>
  <%= f.submit '作成' %>
<% end %>

<!--修正後-->
<%= render 'form', article: @article %>
views/articles/edit.html.erb
<!--修正前-->
<h1>編集</h1>
<% if @article.errors.any? %>
  <ul>
    <% @article.errors.full_messages.each do |message| %>
      <li><%= message %></li>
    <% end %>
  </ul>
<% end %>

<%= form_with model: @article, local: true do |f| %>
  <%= f.hidden_field :user_id, value: current_user.id %>
  <div>
    <%= f.label :title %>
    <%= f.text_field :title %>
  </div>
  <div>
    <%= f.label :body %>
    <%= f.text_area :body %>
  </div>
  <%= f.submit "更新" %>
<% end %>

<!--修正後-->
<%= render 'form', article: @article %>

※新規作成

views/articles/_form.html.erb
<h1><%= current_page?(new_article_path)? "新規作成" : "編集" %></h1>
<%= render 'layouts/errors', object: @article %>

<%= form_with model: article, local: true do |f| %>
  <%= f.hidden_field :user_id, value: current_user.id %>
  <div>
    <%= f.label :title %>
    <%= f.text_field :title %>
  </div>
  <div>
    <%= f.label :body %>
    <%= f.text_area :body %>
  </div>
  <%= f.submit "#{ current_page?(new_article_path)? "新規作成" : "更新" }" %>
<% end %>

※新規作成

views/layouts/_errors.html.erb
<% if object.errors.any? %>
  <ul>
    <% object.errors.full_messages.each do |message| %>
      <li><%= message %></li>
    <% end %>
  </ul>
<% end %>

新規登録と編集画面のフォームは全く一緒なので部分テンプレートとして切り出し、
エラーメッセージは他のところでも使う可能性があるので、さらに別のファイルに切り出して実装を行いました

今回は、「新規作成/編集」のところもcurrent_page?メソッド（現在のページのパスを返すrailsのメソッド）を使って変化させていますが、多少やりすぎ感は否めないかなとも思ってますw

article_pathの引数が「article.id」になっている

views/articles/index.html.erb
<!--修正前-->
<td><%= link_to article.title, article_path(article.id) %></td>


<!--修正後-->
<td><%= link_to article.title, article_path(article)%></td>

idを引数として渡す場合はオブジェクトそのものを渡してあげることもできる。
かなり細かいところではあるが、意外と重宝することがある

３、きれいなコーディング（レベル２）

  • enumの検索メソッドを使用していない
  • authenticate_user!を使用していない

enumの検索メソッドを使用していない

views/users/show.html.erb
#修正前
<% if current_user.role == "admin" %>

#修正後
<% if current_user.admin? %>

enumを使うことでcurrent_user.admin?というように記述することができよりシンプルな記述をすることができます。

参考文献
https://qiita.com/shizuma/items/d133b18f8093df1e9b70

authenticate_user!を使用していない

application_controller.rb
#修正前
def user_sign_in?
  if user_loged_in?
    redirect_to :user_session_path
  end
end

#修正後
before_action :authenticate_user!

gemのdeviseを使うことで様々なメソッドを使うことができるようになります

メソッド 用途
before_action :authenticate_user! コントローラーに設定して、ログイン済ユーザーのみにアクセスを許可する
user_signed_in? ユーザーがサインイン済かどうかを判定する
current_user サインインしているユーザーを取得する
user_session ユーザーのセッション情報にアクセスする

詳細は↓の記事から、、、
https://qiita.com/tobita0000/items/866de191635e6d74e392#user_session

４、きれいなコーディング（レベル３）

  • n+1問題が発生している
  • ビジネスロジックをcontrollerに書いている
  • デメテルの法則を使用していない
  • Articleにindexを貼っていない

n+1問題が発生している

controllers/articles_controller.rb
#修正前
  def index
    @articles = Article.all
  end

#修正後
  def index
    @articles = Article.all.includes(:user)
  end

そもそもn+1問題って？

めちゃめちゃ簡単に言うと無駄なSQL（データベースからデータを取ってくる命令）が発行されてしまっている現象です。
どれくらい違うかと言うと、、、

修正前
スクリーンショット 2019-03-30 13.10.03.png

修正後
スクリーンショット 2019-03-30 13.11.14.png

たった10個の初期データを表示するだけにも関わらず
ここまで発行量に差があります。

じゃあ、なんでこなんでこんなに違いがあるのかと言うと、、、
調べましょう！！！説明がめんどくさかったとかそう言うのじゃないんだからね！

まあでもとりあえず参考文献だけは置いておきます
https://qiita.com/TsubasaTakagi/items/8c3f4317ad917924b860

デメテルの法則を使用していない

models/article.rb
#追加
delegate :email,
         :name,
         to: :user,
         prefix: true
views/articles/_article_list.html.erb
<!--修正前-->
<tr>
  <td><%= link_to article.title, article_path(article) %></td>
  <td><%= article.body.html_safe %></td>
  <td><%= "#{article.pv}pv" %></td>
  <td><%= article.user.name %></td>

<!--修正後-->
<tr>
  <td><%= link_to article.title, article_path(article) %></td>
  <td><%= article.body.html_safe %></td>
  <td><%= "#{article.pv}pv" %></td>
  <td><%= article.user_name %></td>

デメテルの法則とは、

あるオブジェクトAは別のオブジェクトBのサービスを要求してもよい（メソッドを呼び出してもよい）が、オブジェクトAがオブジェクトBを「経由して」さらに別のオブジェクトCのサービスを要求してはならない。(wikipediaより引用)

まあよくわかんないと思うのでめちゃめちゃ簡単にいうと、、、
「.」を2つ以上繋げんなよ！って感じですな（ただこの解釈で固まりすぎると行きすぎると死ぬことあるのでその辺は注意、、、）

じゃあ、今回みたいに article.user.name みたいに書かなきゃいけない時はどうすればいいの？ってことが起きると思います。

models/article.rb
  def user_name
    user.name
  end

その時は上のようにarticleのモデルにこんな感じのメソッドを書いてあげれば
article.user_name みたいな感じで「.」を一つで呼ぶことができます。（※正確にいうとArticleモデルのメソッドとしてuser名を引っ張ってこれる）
なので今回の実装ではrailsのActiveSupportのコンポーネントのdelegateを使って実装を行いました。

↓参考文献
https://railsguides.jp/active_support_core_extensions.html#%E3%83%A1%E3%82%BD%E3%83%83%E3%83%89%E3%81%AE%E5%A7%94%E8%AD%B2

何を言ってるのかよくわからん！っていう人は、
「.」を2つ以上繋げるのはあんまり良くないんだな〜みたいな認識でオッケーかと思います。

Articleにindexを貼っていない

db/migrate/20190220133546_init.rb
#修正前
create_table :articles do |t|
  t.string :title, null: false
  t.text :body, null: false
  t.integer :pv, null: false, default: 0

  t.integer :user_id, null: false
  t.timestamps
end

#修正後
create_table :articles do |t|
  t.string :title, null: false
  t.text :body, null: false
  t.integer :pv, null: false, default: 0

  t.integer :user_id, null: false
  t.timestamps
end
add_index :articles, [:user_id] #追記

indexを張るってどういうこと？
プログラミングでindexというといろんな意味がありますが、、、
今回のindexの意味は検索を早くする仕組みのことです

なんでindexを張ると検索が早くなるのかは下の記事を見ていただくとして、、、
indexは基本的には外部キーに貼っておくといいとされているらしいので、よくわからん！！！って人もとりあえず外部キーには貼っておきましょう

参考文献
〜簡単に分かりたい人向け〜
https://wa3.i-3-i.info/word11906.html
〜割とちゃんと分かりたい人向け〜
https://www.atmarkit.co.jp/ait/articles/1703/01/news199.html

ビジネスロジックをモデルにかいていない

controller/articles_controller.rb
#修正前
def show
  @article = Article.find(params[:id])
  @article.pv += 1
  @article.save
end

#修正後
def show
  @article = Article.find(params[:id])
  Article.increment_pv(@article)
end
models/article.rb
def self.increment_pv(article)  
  article.pv += 1   
  article.save  
end

そもそもビジネスロジックってなんやねん！

ビジネスロジック（英: business logic）は、データベースとユーザーインタフェース間の情報のやりとりを制御する手順といったようなものを指す（技術的でない）用語である。(wikipediaより引用)

ってことだそうです、、、いや、どういうことやねんwww

まあちょっとよくわからないので
逆にcontrollerの役割について確認したいと思います。
スクリーンショット 2019-03-31 12.24.19.png
上の図はMVCフレームワークの大まかな流れなのですが
ズバリコントローラーの役割とは、、、

送られてきたリクエストを適切なアクションに振り分けること！

です。
そして、データベースとのやりとりはmodelが行うことになっています。

そこで改めて該当のコードを見てみると、、、

def show
  @article = Article.find(params[:id])
  @article.pv += 1
  @article.save
end

というようになっています。

ここで行なっている処理は
1. 該当のarticleを持ってくる
2. 持ってきたarticleのPV数を1を足す

まず、１の「該当のarticleを持ってくる」というところですが、
先ほどの話からするとこの作業は本来modelが担当するべきところですが、@article = Article.find(params[:id])
となっているので、まるでコントローラがDBにアクセスているように見えますが、このrailsのfindというメソッドは実はmodels/user.rbで継承されているActiveRecordのメソッドを呼び出しているのです！
なのでちゃんとMVC通りmodelでデータベースとやりとりを行なっていたんですね！（この辺は話すと長くなるのでこれくらいで割愛w）

そして、2の「持ってきたarticleのPV数を1を足す」という処理ですが、この作業も本来はmodelが担当するべきところですが修正前のコードではcontrollerでpv数を１足しています(ちなみにsaveもActiveRecordのメソッドです)
コントローラは処理を振り分けることなので、pv数に１を足すという処理をここで行なってしまうとMVCの考え方に反してしまいます。
なので今回はこれらの処理はメソッドとして切り出してmodelファイルに記述して、それを呼びだすという実装にしています。

ってまあ、ぶっちゃけよくわからん！ってなる人も多いと思います汗
なのでそんな人は、とりあえず、、、

よくわかんないけど、
普段使ってる「find」とか「save」とかはmodelファイルで定義しているメソッドをcontrollerで呼び出して使ってるだけなんだなぁ〜

ってことだけ頭の片隅にでも置いておいてもらえるといいかなと思います

ビジネスロジックをさっくり知りたい
https://wa3.i-3-i.info/word13666.html

もっと知りたい
〜MVCモデル〜
https://qiita.com/tshinsay/items/5b1724baf32b8b5113c2

〜ActiveRecord〜
https://railsguides.jp/active_record_basics.html

５、セキュリティー系

XSS（クロスサイトスクリプティング）攻撃からの脆弱性がある

views/articles/_article_list.html.erb
<!--修正前-->
<% articles.each do |article| %>
  <tr>
    <td><%= link_to article.title, article_path(article) %></td>
    <td><%= article.body.html_safe %></td><!--⬅︎修正するところ-->
    <td><%= "#{article.pv}pv" %></td>
    <td><%= article.user_name %></td>

<!--修正後-->
<% articles.each do |article| %>
  <tr>
    <td><%= link_to article.title, article_path(article) %></td>
    <td><%= simple_format(article.body) %></td> <!--⬅︎修正した-->
    <td><%= "#{article.pv}pv" %></td>
    <td><%= article.user_name %></td>

クロスサイトスクリプティングってなんぞ？
こればっかりは実際にやって見たほうが早いのでやって見ましょう！

<script>location.href="https://qiita.com/"</script>

これをbodyに書いて記事を作成してみましょう
すると、、、
おそらく記事の一覧画面に言った時にqiitaのトップページに勝手にリダイレクトされると思います。
今回はこれがqiitaのトップページであるため、問題はありませんが例えばこれの飛ばされる先が悪質なサイトである可能性は十分にあるわけです。。。
ではこれらはどのようにすれば防ぐことができるのでしょうか？

キーワードとしては サニタイズ です

サニタイズとは簡単にいうと特別な文字を他の文字に置き換えちゃおうぜって感じのことです。
railsでは基本的にrubyタグ（<%= %>こんな感じのやつ）を使うことでサニタイズをしてくれるのですが .html_safe を使うとサニタイズしてくれなくなってしまいます。
（safeって書いてあるのに全然安全じゃないってのがツッコミどころ満載な感じですが、あまりにツッコミどころ多いので逆に覚えやすいw）

でも、だからと言って<td><%= article.body %></td> と書いてしまうと、改行がうまくされませんそのため今回はsimple_formatというrailsのヘルパーを用いて実装を行っています。

参考文献
〜サニタイズについて〜
https://wa3.i-3-i.info/word16265.html
〜simple_formatについて〜
http://railsdoc.com/references/simple_format

お疲れ様です！！！

みなさん、問題の回答から解説の読了までお疲れ様でした！！！
今回の問題楽しんでいただけましたでしょうか？また何か新しい発見や学びはありましたでしょうか？

もし少しでも発見や学びがあればとても嬉しいなと思います！

というか今回の問題個人的にはなかなか難しいと思っていて、自分で作っていながらこれ俺が出されても１発で満点取れます！みたいなことは口が裂けても言えないなぁ〜って思っていますので、全然点数が取れなかった人もあまり気にせずにまたゆっくり一緒に勉強していければなと思います！

最後に、、、意見や質問、感想などあったら教えていただけると幸いです！

　　　

参考文献まとめ

1
4
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
1
4