FIX: never attempt to log invalid post numbers

FIX: never attempt to log invalid post numbers

Previously in some cases we would queue logging of invalid post numbers

The impact would be we would miss logging an incoming link and would leak
an error.

From 20268385a5e6a902b1dfabdf4171215d1fa977e3 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Wed, 21 Nov 2018 11:58:47 +1100
Subject: [PATCH] FIX: never attempt to log invalid post numbers

Previously in some cases we would queue logging of invalid post numbers

The impact would be we would miss logging an incoming link and would leak
an error.

diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index 6bc4fa4..12c1e99 100644
--- a/app/controllers/topics_controller.rb
+++ b/app/controllers/topics_controller.rb
@@ -806,7 +806,7 @@ class TopicsController < ApplicationController
         host: request.host,
         current_user: current_user,
         topic_id: @topic_view.topic.id,
-        post_number: params[:post_number],
+        post_number: @topic_view.current_post_number,
         username: request['u'],
         ip_address: request.remote_ip
       }
diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb
index a14dd9f..0702366 100644
--- a/app/serializers/topic_view_serializer.rb
+++ b/app/serializers/topic_view_serializer.rb
@@ -174,11 +174,11 @@ class TopicViewSerializer < ApplicationSerializer
   end
 
   def current_post_number
-    [object.post_number, object.highest_post_number].min
+    object.current_post_number
   end
 
   def include_current_post_number?
-    object.highest_post_number.present?
+    object.current_post_number.present?
   end
 
   def highest_post_number
diff --git a/lib/topic_view.rb b/lib/topic_view.rb
index 139ff65..8edfed9 100644
--- a/lib/topic_view.rb
+++ b/lib/topic_view.rb
@@ -481,6 +481,14 @@ class TopicView
     @filtered_posts.order(sort_order: :desc).limit(1).pluck(:id).first
   end
 
+  def current_post_number
+    if highest_post_number.present?
+      post_number > highest_post_number ? highest_post_number : post_number
+    else
+      nil
+    end
+  end
+
   protected
 
   def read_posts_set
diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb
index 0c6b6f3..93c628f 100644
--- a/spec/requests/topics_controller_spec.rb
+++ b/spec/requests/topics_controller_spec.rb
@@ -1095,6 +1095,18 @@ RSpec.describe TopicsController do
       end.to change(TopicViewItem, :count).by(1)
     end
 
+    it 'records a view to invalid post_number' do
+      user = Fabricate(:user)
+
+      expect do
+        get "/t/#{topic.slug}/#{topic.id}/#{256**4}", params: {
+          u: user.username
+        }
+        expect(response.status).to eq(200)
+      end.to change { IncomingLink.count }.by(1)
+
+    end
+
     it 'records incoming links' do
       user = Fabricate(:user)

GitHub

1 Like

Minor but the else condition is not required here.

2 Likes

No probs followed this up. Thanks!

1 Like