From 0726d08f049f233f7005a7bc7bdb2e29d39c7439 Mon Sep 17 00:00:00 2001
From: Felix Clack <felixclack@gmail.com>
Date: Fri, 13 Oct 2023 11:33:56 +0100
Subject: [PATCH] Encode TRNs in URLs

Exposing a TRN in the URL opens us up to someone being able to enumerate
through the values and capture data on people.

We want to prevent this by encoding the values passed in the URL. This
ensures that the UI is the only way to access someone's record.

I opted to use a symmetric encoding process to allow us to encode/decode
using a secret that we control.

This is an easy to understand encryption that is as secure as our secret
key.
---
 .../check_records/teachers_controller.rb      |  3 +-
 app/models/secure_identifier.rb               | 32 ++++++++++
 app/views/check_records/search/show.html.erb  |  2 +-
 spec/models/secure_identifier_spec.rb         | 61 +++++++++++++++++++
 4 files changed, 96 insertions(+), 2 deletions(-)
 create mode 100644 app/models/secure_identifier.rb
 create mode 100644 spec/models/secure_identifier_spec.rb

diff --git a/app/controllers/check_records/teachers_controller.rb b/app/controllers/check_records/teachers_controller.rb
index 2ff0907d..2bcfa104 100644
--- a/app/controllers/check_records/teachers_controller.rb
+++ b/app/controllers/check_records/teachers_controller.rb
@@ -2,7 +2,8 @@ module CheckRecords
   class TeachersController < CheckRecordsController
     def show
       client = QualificationsApi::Client.new(token: ENV["QUALIFICATIONS_API_FIXED_TOKEN"])
-      @teacher = client.teacher(trn: params[:id])
+      trn = SecureIdentifier.decode(params[:id])
+      @teacher = client.teacher(trn:)
       @npqs = @teacher.qualifications.filter(&:npq?)
       @other_qualifications = @teacher.qualifications.filter { |qualification| !qualification.npq? }
     rescue QualificationsApi::TeacherNotFoundError
diff --git a/app/models/secure_identifier.rb b/app/models/secure_identifier.rb
new file mode 100644
index 00000000..c04c8973
--- /dev/null
+++ b/app/models/secure_identifier.rb
@@ -0,0 +1,32 @@
+class SecureIdentifier
+  SECRET_KEY = Rails.application.credentials.secret_key_base.byteslice(0..31)
+  CIPHER = 'aes-256-cbc'
+
+  def self.encode(value)
+    raise ArgumentError, "value cannot be nil" if value.nil?
+    return value if value.empty?
+
+    cipher = OpenSSL::Cipher.new(CIPHER)
+    cipher.encrypt
+    cipher.key = SECRET_KEY
+
+    encrypted = cipher.update(value) + cipher.final
+    Base64.urlsafe_encode64(encrypted).strip
+  end
+
+  def self.decode(value)
+    raise ArgumentError, "value cannot be nil" if value.nil?
+    return value if value.empty?
+
+    decipher = OpenSSL::Cipher.new(CIPHER)
+    decipher.decrypt
+    decipher.key = SECRET_KEY
+  
+    begin
+      decoded = Base64.urlsafe_decode64(value)
+      decipher.update(decoded) + decipher.final
+    rescue ArgumentError => e
+      return value
+    end
+  end
+end
\ No newline at end of file
diff --git a/app/views/check_records/search/show.html.erb b/app/views/check_records/search/show.html.erb
index 3f93871f..d1468a7f 100644
--- a/app/views/check_records/search/show.html.erb
+++ b/app/views/check_records/search/show.html.erb
@@ -14,7 +14,7 @@
         <% @teachers.each do |teacher| %>
           <div class="search-results__item">
             <h2 class="govuk-heading-s">
-              <%= link_to teacher.name, check_records_teacher_path(teacher.trn), class: "govuk-link--no-visited-state" %>
+              <%= link_to teacher.name, check_records_teacher_path(SecureIdentifier.encode(teacher.trn)), class: "govuk-link--no-visited-state" %>
             </h2>
             <strong class="govuk-tag govuk-tag--<%= teacher.sanctions.any? ? "red" : "green" %>"%>
               <%= teacher.sanctions.any? ? "Restrictions" : "No restrictions" %>
diff --git a/spec/models/secure_identifier_spec.rb b/spec/models/secure_identifier_spec.rb
new file mode 100644
index 00000000..db7d2dea
--- /dev/null
+++ b/spec/models/secure_identifier_spec.rb
@@ -0,0 +1,61 @@
+require 'rails_helper'
+
+RSpec.describe SecureIdentifier do
+  let(:plain_text) { 'Hello, World!' }
+
+  describe '.encode' do
+    subject { described_class.encode(plain_text) }
+
+    it { is_expected.not_to eq(plain_text) }
+
+    it 'returns a Base64 encoded string' do
+      is_expected.to eq(
+        Base64.strict_encode64(Base64.strict_decode64(SecureIdentifier.encode(plain_text)))
+      )
+    end
+
+    context 'when given an empty string' do
+      let(:plain_text) { '' }
+
+      it { is_expected.to eq(plain_text) }
+    end
+
+    context 'when given nil' do
+      let(:plain_text) { nil }
+
+      it 'raises an ArgumentError' do
+        expect { subject }.to raise_error(ArgumentError)
+      end
+    end
+  end
+
+  describe '.decode' do
+    subject { described_class.decode(encoded_text) }
+
+    let(:encoded_text) { described_class.encode(plain_text) }
+
+    it { is_expected.to eq(plain_text) }
+
+    context 'when given an invalid Base64 string' do
+      let(:encoded_text) { 'invalid' }
+
+      it 'returns the passed value' do
+        is_expected.to eq(encoded_text)
+      end
+    end
+
+    context 'when given an empty string' do
+      let(:encoded_text) { '' }
+
+      it { is_expected.to eq(encoded_text) }
+    end
+
+    context 'when given nil' do
+      let(:encoded_text) { nil }
+
+      it 'raises an ArgumentError' do
+        expect { subject }.to raise_error(ArgumentError)
+      end
+    end
+  end
+end
\ No newline at end of file