Developer experience/security: Make dev/test blow up if current_user is used erroneously (#11237)

* Let dev/test blow up if current_user is used erroneously

* Fix logic

* Fix click-to-edit permissions

* Change test to check for new behavior

* linting

* Edit tests

* Change test

* Clean up implementation

* Update comment

* Fix instance vars

* Update app/controllers/concerns/caching_headers.rb

* Merge origin and add request_store gem

Gemfile

@@ -83,6 +83,7 @@ gem "recaptcha", "~> 5.6", require: "recaptcha/rails" # Helpers for the reCAPTCH
 gem "redcarpet", "~> 3.5" # A fast, safe and extensible Markdown to (X)HTML parser
 gem "redis", "~> 4.2.2" # Redis ruby client
 gem "redis-rails", "~> 5.0.2" # Redis for Ruby on Rails
+gem "request_store", "~> 1.5" # RequestStore gives you per-request global storage
 gem "reverse_markdown", "~> 2.0" # Map simple html back into markdown
 gem "rolify", "~> 5.3" # Very simple Roles library
 gem "rouge", "~> 3.24" # A pure-ruby code highlighter

Gemfile.lock

@@ -940,6 +940,7 @@ DEPENDENCIES
   redcarpet (~> 3.5)
   redis (~> 4.2.2)
   redis-rails (~> 5.0.2)
+  request_store (~> 1.5)
   reverse_markdown (~> 2.0)
   rolify (~> 5.3)
   rouge (~> 3.24)

app/assets/javascripts/initializers/initializeBaseUserData.js

@@ -31,6 +31,10 @@ function addRelevantButtonsToArticle(user) {
       let actions = [
         `<a class="crayons-btn crayons-btn--s crayons-btn--secondary" href="${articleContainer.dataset.path}/edit" rel="nofollow">Edit</a>`,
       ];
+      let clickToEditButton = document.getElementById("author-click-to-edit")
+      if (clickToEditButton) {
+        clickToEditButton.style.display = "inline-block"
+      }
       if (JSON.parse(articleContainer.dataset.published) === true) {
         actions.push(
           `<a class="crayons-btn crayons-btn--s crayons-btn--secondary ml-1" href="${articleContainer.dataset.path}/manage" rel="nofollow">Manage</a>`,

app/controllers/application_controller.rb

@@ -13,6 +13,7 @@ class ApplicationController < ActionController::Base
   include ImageUploads
   include VerifySetupCompleted
   include DevelopmentDependencyChecks if Rails.env.development?
+  include EdgeCacheSafetyCheck unless Rails.env.production?
   include Devise::Controllers::Rememberable
 
   rescue_from ActionView::MissingTemplate, with: :routing_error

app/controllers/concerns/caching_headers.rb

@@ -21,6 +21,8 @@ module CachingHeaders
 
     request.session_options[:skip] = true # no cookies
 
+    RequestStore.store[:edge_caching_in_place] = true # To be observed downstream.
+
     response.headers["Cache-Control"] = "public, no-cache" # Used only by Fastly.
     response.headers["X-Accel-Expires"] = max_age.to_s # Used only by Nginx.
     response.headers["Surrogate-Control"] = surrogate_control.presence || build_surrogate_control(

app/controllers/concerns/edge_cache_safety_check.rb

@@ -0,0 +1,16 @@
+module EdgeCacheSafetyCheck
+  extend ActiveSupport::Concern
+
+  CANNOT_USE_CURRENT_USER = "You may not use current_user in this cached code path.".freeze
+
+  def current_user
+    # In production, current_user will cause a cache leak if it's placed within an edge-cached code path.
+    # More information here:
+    # https://docs.forem.com/technical-overview/architecture/#we-cache-many-content-pages-on-the-edge
+    return super unless RequestStore.store[:edge_caching_in_place]
+
+    return if session_current_user_id.blank?
+
+    CANNOT_USE_CURRENT_USER
+  end
+end

app/views/articles/_sidebar.html.erb

@@ -1,7 +1,7 @@
 <div id="sidebar-wrapper-left" class="sidebar-wrapper sidebar-wrapper-left crayons-layout__sidebar-left">
   <div class="sidebar-bg" id="sidebar-bg-left"></div>
   <aside class="side-bar" aria-label="Primary sidebar">
-    <% cache(release_adjusted_cache_key("main-sidebar-nav--#{user_signed_in?}--#{current_user&.following_tags_count&.positive?}"), expires_in: 15.minutes) do %>
+    <% cache(release_adjusted_cache_key("main-sidebar-nav--#{user_signed_in?}"), expires_in: 15.minutes) do %>
       <%= render "articles/sidebar_nav" %>
       <% @sponsorships = Sponsorship.gold.live.includes(:organization).order(featured_number: :asc) %>
       <%# the pattern .present?/.each has the advantage of issuing only a single SQL query to load objects in memory %>

app/views/articles/_sidebar_nav.html.erb

@@ -46,7 +46,7 @@
 </nav>
 
 <nav class="mb-6" aria-label="Secondary sidebar nav">
-  <% if user_signed_in? && current_user.following_tags_count.positive? %>
+  <% if user_signed_in? %>
     <header class="p-2 pr-0 flex items-center justify-between">
       <h3 class="crayons-subtitle-3">My Tags</h3>
       <a href="/dashboard/following_tags" class="crayons-btn crayons-btn--icon crayons-btn--ghost-dimmed" aria-label="Customize tag priority" title="Customize tag priority">
@@ -54,42 +54,21 @@
       </a>
     </header>
     <div id="sidebar-nav-followed-tags" class="overflow-y-auto mb-2" style="max-height: 42vh;"></div>
-    <% elsif user_signed_in? %>
-     <header class="p-2 pr-0 flex items-center justify-between">
-       <h3 class="crayons-subtitle-3">Explore</h3>
-       <a href="<%= tags_path %>" class="fs-s crayons-link crayons-link--brand pr-2" title="Browse all tags">
-        All tags
-        </a>
-      </header>
-      <div id="sidebar-nav-explore-tags" class="overflow-y-auto" style="max-height: 42vh">
-        <% Tag.where(supported: true).order(hotness_score: :desc).limit(5).pluck(:id, :name).each do |tag_array| %>
-          <div class="sidebar-nav-element" id="default-sidebar-element-<%= tag_array.second %>">
-            <a class="crayons-link crayons-link--block py-2 px-2" href="<%= tag_path(tag_array.second) %>">
-              #<%= tag_array.second %>
-            </a>
-            <a class="follow-action-button sidebar-nav-link-follow crayons-btn crayons-btn--s"
-              href="#" id="sidebar-nav-link-follow-<%= tag_array.second %>"
-              data-info='{"id":<%= tag_array.first %>,"className":"Tag"}'>
-              Follow
-            </a>
+  <% else %>
+    <h3 class="crayons-subtitle-3 p-2">Popular Tags</h3>
+    <div id="sidebar-nav-default-tags" class="overflow-y-auto" style="max-height: 42vh">
+      <% Tag.where(supported: true).order(hotness_score: :desc).limit(30).pluck(:id, :name).each do |tag_array| %>
+        <div class="sidebar-nav-element" id="default-sidebar-element-<%= tag_array.second %>">
+          <a class="crayons-link crayons-link--block py-2 px-2" href="<%= tag_path(tag_array.second) %>">
+            #<%= tag_array.second %>
+          </a>
+          <a class="follow-action-button sidebar-nav-link-follow crayons-btn crayons-btn--s"
+            href="#" id="sidebar-nav-link-follow-<%= tag_array.second %>"
+            data-info='{"id":<%= tag_array.first %>,"className":"Tag"}'>
+            Follow
+          </a>
         </div>
       <% end %>
     </div>
-    <% else %>
-      <h3 class="crayons-subtitle-3 p-2">Popular Tags</h3>
-      <div id="sidebar-nav-default-tags" class="overflow-y-auto" style="max-height: 42vh">
-        <% Tag.where(supported: true).order(hotness_score: :desc).limit(30).pluck(:id, :name).each do |tag_array| %>
-          <div class="sidebar-nav-element" id="default-sidebar-element-<%= tag_array.second %>">
-            <a class="crayons-link crayons-link--block py-2 px-2" href="<%= tag_path(tag_array.second) %>">
-              #<%= tag_array.second %>
-            </a>
-            <a class="follow-action-button sidebar-nav-link-follow crayons-btn crayons-btn--s"
-              href="#" id="sidebar-nav-link-follow-<%= tag_array.second %>"
-              data-info='{"id":<%= tag_array.first %>,"className":"Tag"}'>
-              Follow
-            </a>
-          </div>
-      <% end %>
-    </div>
   <% end %>
 </nav>

app/views/articles/index.html.erb

@@ -25,8 +25,7 @@
 <% end %>
 
 <%= javascript_packs_with_chunks_tag "homePage", defer: true %>
-
-<% cache(release_adjusted_cache_key("main-stories-index-#{params}-#{user_signed_in?}-#{current_user&.following_tags_count&.positive?}"), expires_in: 90.seconds) do %>
+<% cache(release_adjusted_cache_key("main-stories-index-#{params}-#{user_signed_in?}"), expires_in: 90.seconds) do %>
   <div class="crayons-layout crayons-layout--3-cols crayons-layout--3-cols--drop-right-left" id="index-container"
       data-params="<%= params.merge(sort_by: "hotness_score", sort_direction: "desc").to_json(only: %i[tag username q sort_by sort_direction]) %>" data-which="<%= @list_of %>"
       data-tag=""
@@ -38,7 +37,7 @@
     <%# BEGIN Feed menu bar %>
     <main class="articles-list crayons-layout__content" id="articles-list" role="main">
       <h1 class="visually-hidden-header">Articles</h1>
-      <%= render(partial: "onboardings/task_card") if user_signed_in? && current_user.saw_onboarding? %>
+      <%= render(partial: "onboardings/task_card") if user_signed_in? %>
 
       <header class="flex items-center p-2 m:p-0 m:pb-2" id="on-page-nav-controls">
         <button type="button" class="crayons-btn crayons-btn--ghost crayons-btn--icon mr-2 inline-block m:hidden" id="on-page-nav-butt-left" aria-label="nav-button-left">

app/views/articles/show.html.erb

@@ -96,8 +96,8 @@
       <% if !@article.published %>
         <div class="crayons-notice crayons-notice--danger mb-4">
           <strong>Unpublished Post.</strong> This URL is public but secret, so share at your own discretion.
-          <% if user_signed_in? && policy(@article.object).update? %>
-            <a href="<%= @article.path %>/edit" class="fw-bold">Click to edit</a>.
+          <% if user_signed_in? %>
+            <a id="author-click-to-edit" href="<%= @article.path %>/edit" class="fw-bold" display="none">Click to edit</a>
           <% end %>
         </div>
       <% end %>

spec/system/articles/user_visits_an_article_spec.rb

@@ -135,9 +135,9 @@ RSpec.describe "Views an article", type: :system do
       let(:query_params) { "?preview=#{article.password}" }
       let(:article_user) { user }
 
-      it "shows the article edit link" do
+      it "shows the article edit link", js: true do
         visit article_path
-        expect(page).to have_link(link_text, href: href)
+        expect(page.body).to include('display: inline-block;">Click to edit</a>')
       end
     end
 
@@ -147,7 +147,7 @@ RSpec.describe "Views an article", type: :system do
 
       it "does not the article edit link" do
         visit article_path
-        expect(page).not_to have_link(link_text, href: href)
+        expect(page.body).not_to include('display: inline-block;">Click to edit</a>')
       end
     end
 
@@ -158,7 +158,7 @@ RSpec.describe "Views an article", type: :system do
       it "does not the article edit link" do
         sign_out user
         visit article_path
-        expect(page).not_to have_link(link_text, href: href)
+        expect(page.body).not_to include('display: inline-block;">Click to edit</a>')
       end
     end
 

spec/system/homepage/user_visits_homepage_spec.rb

@@ -113,17 +113,6 @@ RSpec.describe "User visits a homepage", type: :system do
       end
     end
 
-    context "when user does not follow tags" do
-      before do
-        visit "/"
-      end
-
-      it "shows 'Explore Tags' and links to /tags", js: true do
-        expect(page).to have_text("Explore")
-        expect(page).to have_selector(:css, 'a[href="/tags"]')
-      end
-    end
-
     context "when rendering < 5 navigation links" do
       let!(:navigation_link_1) do
         create(:navigation_link,