Add migration to purge page URL query strings

Author: rsmithlal

app/models/better_together/metrics/page_view.rb

@@ -4,6 +4,8 @@
 module BetterTogether
   module Metrics
     class PageView < ApplicationRecord # rubocop:todo Style/Documentation
+      SENSITIVE_QUERY_PARAMS = %w[token password secret].freeze
+
       belongs_to :pageable, polymorphic: true
 
       # Validations
@@ -14,16 +16,38 @@ class PageView < ApplicationRecord # rubocop:todo Style/Documentation
 
       # Add a method to set the page_url automatically if the pageable responds to a `url` method
       before_validation :set_page_url
+      validate :page_url_without_sensitive_parameters
 
       private
 
+      attr_reader :page_url_query
+
       # Set the page_url if the pageable object doesn't respond to :url
-      def set_page_url
-        if pageable.respond_to?(:url)
-          self.page_url = pageable.becomes(pageable.class.base_class).url
-        elsif pageable.present? && page_url.blank?
-          self.page_url = generate_url_for_pageable
-        end
+      def set_page_url # rubocop:todo Metrics/AbcSize, Metrics/MethodLength
+        url = if pageable.respond_to?(:url)
+                pageable.becomes(pageable.class.base_class).url
+              elsif pageable.present? && page_url.blank?
+                generate_url_for_pageable
+              else
+                page_url
+              end
+
+        return if url.blank?
+
+        uri = URI.parse(url)
+        @page_url_query = uri.query
+        self.page_url = uri.path
+      rescue URI::InvalidURIError
+        errors.add(:page_url, 'is invalid')
+      end
+
+      def page_url_without_sensitive_parameters
+        return if page_url_query.blank?
+
+        params = Rack::Utils.parse_nested_query(page_url_query)
+        return unless params.keys.intersect?(SENSITIVE_QUERY_PARAMS)
+
+        errors.add(:page_url, 'contains sensitive parameters')
       end
 
       # Generate the URL for the pageable using `url_for`

db/migrate/20250301000000_purge_query_strings_from_page_urls.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+# Removes query strings from existing page_url entries
+class PurgeQueryStringsFromPageUrls < ActiveRecord::Migration[7.1]
+  class PageView < ApplicationRecord # rubocop:todo Style/Documentation
+    self.table_name = 'better_together_metrics_page_views'
+  end
+
+  def up
+    PageView.where("page_url LIKE '%?%'").find_each do |pv|
+      uri = URI.parse(pv.page_url)
+      PageView.where(id: pv.id).update_all(page_url: uri.path)
+    rescue URI::InvalidURIError
+      PageView.where(id: pv.id).update_all(page_url: nil)
+    end
+  end
+
+  def down
+    # irreversible
+  end
+end

spec/models/better_together/metrics/page_view_spec.rb

@@ -4,8 +4,29 @@
 
 module BetterTogether
   RSpec.describe Metrics::PageView, type: :model do
-    it 'exists' do
-      expect(described_class).to be
+    let(:viewed_at) { Time.zone.now }
+    let(:locale) { 'en' }
+
+    it 'normalizes page_url to exclude query strings' do
+      page_view = described_class.new(
+        page_url: 'https://example.com/path?foo=bar',
+        viewed_at: viewed_at,
+        locale: locale
+      )
+
+      expect(page_view).to be_valid
+      expect(page_view.page_url).to eq('/path')
+    end
+
+    it 'rejects URLs containing sensitive parameters' do
+      page_view = described_class.new(
+        page_url: 'https://example.com/path?token=abc',
+        viewed_at: viewed_at,
+        locale: locale
+      )
+
+      expect(page_view).not_to be_valid
+      expect(page_view.errors[:page_url]).to include('contains sensitive parameters')
     end
   end
 end