Encourage the use of status codes for unsafe requests (#40)

This PR checks that `render` and `redirect_to` are provided an explicit
status code in response to [unsafe request
methods](https://developer.mozilla.org/en-US/docs/Glossary/Safe/HTTP)
(e.g. POST/PUT/PATCH/DELETE).

#### `redirect_to`

When redirecting a POST/PUT/PATCH/DELETE to a GET location, we should
use 303, rather than a 302. A 302 means that the request method should
not be altered, but:

> Many web browsers implemented this code in a manner that violated this
standard, changing the request type of the new request to
[GET](https://en.wikipedia.org/wiki/HTTP_GET_request), regardless of the
type employed in the original request (e.g.
[POST](https://en.wikipedia.org/wiki/POST_(HTTP))).[[1]](https://en.wikipedia.org/wiki/HTTP_302#cite_note-1)
For this reason, HTTP/1.1 (RFC
[2616](https://datatracker.ietf.org/doc/html/rfc2616)) added the new
status codes [303](https://en.wikipedia.org/wiki/HTTP_303) and
[307](https://en.wikipedia.org/wiki/HTTP_307) to disambiguate between
the two behaviours, with 303 mandating the change of request type to
GET, and 307 preserving the request type as originally sent.
> -- https://en.wikipedia.org/wiki/HTTP_302

Sinatra's [`redirect`
method](https://github.com/sinatra/sinatra/blob/5640495babcb4cfd69ba650b293660b7446402da/lib/sinatra/base.rb#L307-L321)
automatically uses 303 for non-GET requests. [This
PR](rails/rails#45393) aims to do this for Rails
applications.

When autocorrect is enabled, `status: :see_other` will be added to
redirects used in the `create`, `update`, and `delete` actions.

#### `render`

Calling `render` in the `create`, `update`, and `destroy` actions is
almost always associated with error handling, so a 4xx status should be
used. In the vast majority of cases, we should be using 422
Unprocessable Entity.

```ruby
if something.save
  redirect_to something, status: :see_other
else
  render :new, status: :unprocessable_entity
end
```

There are exceptions, though. For example, sometimes you want to show a
confirmation page that says "We've received your request". In which
case, 201 or 202 might be a more appropriate choice.


When autocorrect is enabled, `status: :unprocessable_entity` will be
added to renders used in the `create`, `update`, and `delete` actions.
This is a pretty unreasonable thing to do, but could help people update
these usages en-masse. For this reason, Autocorrect is disabled by
default for this rule.

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    betterlint (1.9.0)
+    betterlint (1.10.0)
       rubocop (~> 1.62.0)
       rubocop-performance (~> 1.21.0)
       rubocop-rails (~> 2.24.0)

README.md

@@ -210,3 +210,20 @@ This cop is capable of autocorrecting offenses, but it's not entirely safe. If y
 Betterment/HardcodedID:
   AutoCorrect: true
 ```
+
+### Betterment/RedirectStatus
+
+The `redirect_to` method defaults to a `302 Found`, but when redirecting a POST/PUT/PATCH/DELETE
+to a GET location, the correct response code is `303 See Other`.
+
+This cop requires you to explictly provide an HTTP status code when redirecting from the create,
+update, and destory actions. When autocorrecting, this will automatically add `status: :see_other`.
+
+### Betterment/RenderStatus
+
+The `render` method defaults to a `200 OK`. Calling `render` in the create, update, and destroy
+actions often indicates error handling (e.g. `422 Unprocessable Entity`).
+
+This cop requires you to explicitly provide an HTTP status code when rendering a response in the
+create, update, and destroy actions. When autocorrecting, this will automatically add
+`status: :unprocessable_entity` or `status: :ok` depending on what you're rendering.

betterlint.gemspec

@@ -5,7 +5,7 @@ $LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib)
 
 Gem::Specification.new do |s|
   s.name = "betterlint"
-  s.version = "1.9.0"
+  s.version = "1.10.0"
   s.authors = ["Development"]
   s.email = ["development@betterment.com"]
   s.summary = "Betterment rubocop configuration"

config/default.yml

@@ -76,6 +76,18 @@ Betterment/UnsafeJob:
 Betterment/UnscopedFind:
   StyleGuide: '#bettermentunscopedfind'
 
+Betterment/RedirectStatus:
+  SafeAutoCorrect: false
+  Description: Detect missing status codes when redirecting POST, PUT, PATCH, or DELETE responses
+  Include:
+    - app/controllers/**/*.rb
+
+Betterment/RenderStatus:
+  SafeAutoCorrect: false
+  Description: Detect missing status codes when rendering POST, PUT, PATCH, or DELETE responses
+  Include:
+    - app/controllers/**/*.rb
+
 FactoryBot/ConsistentParenthesesStyle:
   Enabled: false
 

lib/rubocop/cop/betterment.rb

@@ -20,3 +20,5 @@
 require 'rubocop/cop/betterment/hardcoded_id'
 require 'rubocop/cop/betterment/vague_serialize'
 require 'rubocop/cop/betterment/fetch_boolean'
+require 'rubocop/cop/betterment/render_status'
+require 'rubocop/cop/betterment/redirect_status'

lib/rubocop/cop/betterment/redirect_status.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require_relative 'utils/response_status'
+
+module RuboCop
+  module Cop
+    module Betterment
+      class RedirectStatus < Base
+        extend AutoCorrector
+
+        include Utils::ResponseStatus
+
+        MSG = <<~MSG.gsub(/\s+/, " ")
+          Did you forget to specify an HTTP status code? The default is `status: :found`, which
+          is usually inappropriate in this situation. Use `status: :see_other` when redirecting a
+          POST, PUT, PATCH, or DELETE request to a GET resource.
+        MSG
+
+        def on_def(node)
+          each_offense(node, :redirect_to) do |responder|
+            add_offense(responder) do |corrector|
+              corrector.insert_after(responder, ", status: :see_other")
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/betterment/render_status.rb

@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+require_relative 'utils/response_status'
+
+module RuboCop
+  module Cop
+    module Betterment
+      class RenderStatus < Base
+        extend AutoCorrector
+
+        include Utils::ResponseStatus
+
+        MSG = <<~MSG.gsub(/\s+/, " ")
+          Did you forget to specify an HTTP status code? The default is `status: :ok`, which might
+          be inappropriate in this situation. Rendering after a POST, PUT, PATCH or DELETE request
+          typically represents an error (e.g. `status: :unprocessable_entity`).
+        MSG
+
+        def on_def(node)
+          each_offense(node, :render) do |responder|
+            add_offense(responder) do |corrector|
+              corrector.insert_after(responder, ", status: #{infer_status(responder).inspect}")
+            end
+          end
+        end
+
+        private
+
+        def infer_status(responder)
+          case extract_template(responder).to_s
+            when 'new', 'edit'
+              :unprocessable_entity
+            else
+              :ok
+          end
+        end
+
+        # @!method extract_template(node)
+        def_node_matcher :extract_template, <<~PATTERN
+          (send nil? :render {(sym $_) (str $_)} ...)
+        PATTERN
+      end
+    end
+  end
+end

lib/rubocop/cop/betterment/utils/response_status.rb

@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Betterment
+      module Utils
+        module ResponseStatus
+          extend RuboCop::NodePattern::Macros
+
+          UNSAFE_ACTIONS = %i(create update destroy).freeze
+
+          private
+
+          def each_offense(node, responder_name, &block)
+            if UNSAFE_ACTIONS.include?(node.method_name)
+              on_missing_status(node, responder_name, &block)
+            end
+          end
+
+          # @!method on_missing_status(node)
+          def_node_search :on_missing_status, <<~PATTERN
+            (send nil? %1 ... !(hash <(pair (sym :status) _) ...>))
+          PATTERN
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/betterment/redirect_status_spec.rb

@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe RuboCop::Cop::Betterment::RedirectStatus, :config do
+  it 'adds an offense when redirecting without a status' do
+    expect_offense(<<~RUBY)
+      def create
+        redirect_to '/'
+        ^^^^^^^^^^^^^^^ Did you forget to specify an HTTP status code? [...]
+      end
+
+      def update
+        redirect_to '/'
+        ^^^^^^^^^^^^^^^ Did you forget to specify an HTTP status code? [...]
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      def create
+        redirect_to '/', status: :see_other
+      end
+
+      def update
+        redirect_to '/', status: :see_other
+      end
+    RUBY
+  end
+
+  it 'does not add offenses for valid usage' do
+    expect_no_offenses(<<~RUBY)
+      def index
+        redirect_to '/'
+      end
+
+      def show
+        redirect_to '/'
+      end
+
+      def new
+        redirect_to '/'
+      end
+
+      def edit
+        redirect_to '/'
+      end
+
+      def create
+        redirect_to '/', status: :found
+        redirect_to '/', status: :see_other
+      end
+    RUBY
+  end
+end

spec/rubocop/cop/betterment/render_status_spec.rb

@@ -0,0 +1,69 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe RuboCop::Cop::Betterment::RenderStatus, :config do
+  it 'adds an offense when rendering without a status' do
+    expect_offense(<<~RUBY)
+      def create
+        render :new
+        ^^^^^^^^^^^ Did you forget to specify an HTTP status code? [...]
+        render 'new'
+        ^^^^^^^^^^^^ Did you forget to specify an HTTP status code? [...]
+        render :other
+        ^^^^^^^^^^^^^ Did you forget to specify an HTTP status code? [...]
+        render 'other'
+        ^^^^^^^^^^^^^^ Did you forget to specify an HTTP status code? [...]
+        render plain: 'OK'
+        ^^^^^^^^^^^^^^^^^^ Did you forget to specify an HTTP status code? [...]
+      end
+
+      def update
+        render :edit
+        ^^^^^^^^^^^^ Did you forget to specify an HTTP status code? [...]
+        render 'edit'
+        ^^^^^^^^^^^^^ Did you forget to specify an HTTP status code? [...]
+      end
+    RUBY
+
+    expect_correction(<<~RUBY)
+      def create
+        render :new, status: :unprocessable_entity
+        render 'new', status: :unprocessable_entity
+        render :other, status: :ok
+        render 'other', status: :ok
+        render plain: 'OK', status: :ok
+      end
+
+      def update
+        render :edit, status: :unprocessable_entity
+        render 'edit', status: :unprocessable_entity
+      end
+    RUBY
+  end
+
+  it 'does not add offenses for valid usage' do
+    expect_no_offenses(<<~RUBY)
+      def index
+        render :new
+      end
+
+      def show
+        render :new
+      end
+
+      def new
+        render :new
+      end
+
+      def edit
+        render :new
+      end
+
+      def create
+        render plain: "OK", status: :ok
+        render :new, status: :unprocessable_entity
+      end
+    RUBY
+  end
+end