Skip to content

Commit

Permalink
Encode TRNs in URLs
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
felixclack committed Oct 13, 2023
1 parent 17d1e0f commit 4e30a8d
Show file tree
Hide file tree
Showing 5 changed files with 101 additions and 2 deletions.
3 changes: 2 additions & 1 deletion app/controllers/check_records/teachers_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
32 changes: 32 additions & 0 deletions app/models/secure_identifier.rb
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion app/views/check_records/search/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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" %>
Expand Down
61 changes: 61 additions & 0 deletions spec/models/secure_identifier_spec.rb
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
and_they_have_no_restrictions

when_i_click_on_the_teacher_record
then_the_trn_is_not_in_the_url
then_i_see_induction_details
then_i_see_qts_details
then_i_see_itt_details
Expand All @@ -38,6 +39,10 @@ def then_i_see_a_teacher_record_in_the_results
expect(page).to have_content "Terry Walsh"
end

def then_the_trn_is_not_in_the_url
expect(page).not_to have_current_path(/teachers\/1234567/)
end

def and_my_search_is_logged
expect(SearchLog.last.last_name).to eq "Walsh"
expect(SearchLog.last.date_of_birth.to_s).to eq "1992-04-05"
Expand Down

0 comments on commit 4e30a8d

Please sign in to comment.