Improve SSH key format validation

Changelog: other

app/assets/javascripts/pages/profiles/keys/index.js

@@ -7,11 +7,13 @@ function initSshKeyValidation() {
   const input = document.querySelector('.js-add-ssh-key-validation-input');
   if (!input) return;
 
+  const supportedAlgorithms = JSON.parse(input.dataset.supportedAlgorithms);
   const warning = document.querySelector('.js-add-ssh-key-validation-warning');
   const originalSubmit = input.form.querySelector('.js-add-ssh-key-validation-original-submit');
   const confirmSubmit = warning.querySelector('.js-add-ssh-key-validation-confirm-submit');
 
   const addSshKeyValidation = new AddSshKeyValidation(
+    supportedAlgorithms,
     input,
     warning,
     originalSubmit,

app/assets/javascripts/profile/add_ssh_key_validation.js

 export default class AddSshKeyValidation {
-  constructor(inputElement, warningElement, originalSubmitElement, confirmSubmitElement) {
+  constructor(
+    supportedAlgorithms,
+    inputElement,
+    warningElement,
+    originalSubmitElement,
+    confirmSubmitElement,
+  ) {
     this.inputElement = inputElement;
     this.form = inputElement.form;
 
+    this.supportedAlgorithms = supportedAlgorithms;
+    this.publicKeyRegExp = new RegExp(`^(${this.supportedAlgorithms.join('|')})`);
+
     this.warningElement = warningElement;
 
     this.originalSubmitElement = originalSubmitElement;
@@ -23,7 +32,7 @@ export default class AddSshKeyValidation {
   }
 
   submit(event) {
-    this.isValid = AddSshKeyValidation.isPublicKey(this.inputElement.value);
+    this.isValid = this.isPublicKey(this.inputElement.value);
 
     if (this.isValid) return true;
 
@@ -37,7 +46,7 @@ export default class AddSshKeyValidation {
     this.originalSubmitElement.classList.toggle('hide', isVisible);
   }
 
-  static isPublicKey(value) {
-    return /^(ssh|ecdsa-sha2)-/.test(value);
+  isPublicKey(value) {
+    return this.publicKeyRegExp.test(value);
   }
 }

app/models/key.rb

@@ -22,7 +22,7 @@ class Key < ApplicationRecord
   validates :key,
     presence: true,
     length: { maximum: 5000 },
-    format: { with: /\A(ssh|ecdsa)-.*\Z/ }
+    format: { with: /\A(#{Gitlab::SSHPublicKey.supported_algorithms.join('|')})/ }
 
   validates :fingerprint,
     uniqueness: true,

app/views/profiles/keys/_form.html.haml

@@ -5,7 +5,7 @@
 
     .form-group
       = f.label :key, s_('Profiles|Key'), class: 'label-bold'
-      = f.text_area :key, class: "form-control gl-form-input js-add-ssh-key-validation-input qa-key-public-key-field", rows: 8, required: true
+      = f.text_area :key, class: "form-control gl-form-input js-add-ssh-key-validation-input qa-key-public-key-field", rows: 8, required: true, data: { supported_algorithms: Gitlab::SSHPublicKey.supported_algorithms }
       %p.form-text.text-muted= s_('Profiles|Begins with %{ssh_key_algorithms}.') % { ssh_key_algorithms: ssh_key_allowed_algorithms }
     .form-row
       .col.form-group

spec/features/profiles/keys_spec.rb

@@ -32,10 +32,10 @@ RSpec.describe 'Profile > SSH Keys' do
       expect(find('.breadcrumbs-sub-title')).to have_link(attrs[:title])
     end
 
-    it 'shows a confirmable warning if the key does not start with ssh-' do
+    it 'shows a confirmable warning if the key begins with an algorithm name that is unsupported' do
       attrs = attributes_for(:key)
 
-      fill_in('Key', with: 'invalid-key')
+      fill_in('Key', with: 'unsupported-ssh-rsa key')
       fill_in('Title', with: attrs[:title])
       click_button('Add key')
 

spec/frontend/profile/add_ssh_key_validation_spec.js

@@ -3,18 +3,18 @@ import AddSshKeyValidation from '../../../app/assets/javascripts/profile/add_ssh
 describe('AddSshKeyValidation', () => {
   describe('submit', () => {
     it('returns true if isValid is true', () => {
-      const addSshKeyValidation = new AddSshKeyValidation({});
-      jest.spyOn(AddSshKeyValidation, 'isPublicKey').mockReturnValue(true);
+      const addSshKeyValidation = new AddSshKeyValidation([], {});
+      jest.spyOn(addSshKeyValidation, 'isPublicKey').mockReturnValue(true);
 
-      expect(addSshKeyValidation.submit()).toBeTruthy();
+      expect(addSshKeyValidation.submit()).toBe(true);
     });
 
     it('calls preventDefault and toggleWarning if isValid is false', () => {
-      const addSshKeyValidation = new AddSshKeyValidation({});
+      const addSshKeyValidation = new AddSshKeyValidation([], {});
       const event = {
         preventDefault: jest.fn(),
       };
-      jest.spyOn(AddSshKeyValidation, 'isPublicKey').mockReturnValue(false);
+      jest.spyOn(addSshKeyValidation, 'isPublicKey').mockReturnValue(false);
       jest.spyOn(addSshKeyValidation, 'toggleWarning').mockImplementation(() => {});
 
       addSshKeyValidation.submit(event);
@@ -31,14 +31,15 @@ describe('AddSshKeyValidation', () => {
       warningElement.classList.add('hide');
 
       const addSshKeyValidation = new AddSshKeyValidation(
+        [],
         {},
         warningElement,
         originalSubmitElement,
       );
       addSshKeyValidation.toggleWarning(true);
 
-      expect(warningElement.classList.contains('hide')).toBeFalsy();
-      expect(originalSubmitElement.classList.contains('hide')).toBeTruthy();
+      expect(warningElement.classList.contains('hide')).toBe(false);
+      expect(originalSubmitElement.classList.contains('hide')).toBe(true);
     });
 
     it('hides warningElement and shows originalSubmitElement if isVisible is false', () => {
@@ -47,25 +48,32 @@ describe('AddSshKeyValidation', () => {
       originalSubmitElement.classList.add('hide');
 
       const addSshKeyValidation = new AddSshKeyValidation(
+        [],
         {},
         warningElement,
         originalSubmitElement,
       );
       addSshKeyValidation.toggleWarning(false);
 
-      expect(warningElement.classList.contains('hide')).toBeTruthy();
-      expect(originalSubmitElement.classList.contains('hide')).toBeFalsy();
+      expect(warningElement.classList.contains('hide')).toBe(true);
+      expect(originalSubmitElement.classList.contains('hide')).toBe(false);
     });
   });
 
   describe('isPublicKey', () => {
-    it('returns false if probably invalid public ssh key', () => {
-      expect(AddSshKeyValidation.isPublicKey('nope')).toBeFalsy();
+    it('returns false if value begins with an algorithm name that is unsupported', () => {
+      const addSshKeyValidation = new AddSshKeyValidation(['ssh-rsa', 'ssh-algorithm'], {});
+
+      expect(addSshKeyValidation.isPublicKey('nope key')).toBe(false);
+      expect(addSshKeyValidation.isPublicKey('ssh- key')).toBe(false);
+      expect(addSshKeyValidation.isPublicKey('unsupported-ssh-rsa key')).toBe(false);
     });
 
-    it('returns true if probably valid public ssh key', () => {
-      expect(AddSshKeyValidation.isPublicKey('ssh-')).toBeTruthy();
-      expect(AddSshKeyValidation.isPublicKey('ecdsa-sha2-')).toBeTruthy();
+    it('returns true if value begins with an algorithm name that is supported', () => {
+      const addSshKeyValidation = new AddSshKeyValidation(['ssh-rsa', 'ssh-algorithm'], {});
+
+      expect(addSshKeyValidation.isPublicKey('ssh-rsa key')).toBe(true);
+      expect(addSshKeyValidation.isPublicKey('ssh-algorithm key')).toBe(true);
     });
   });
 });

spec/models/key_spec.rb

@@ -21,6 +21,28 @@ RSpec.describe Key, :mailer do
     it { is_expected.to allow_value(attributes_for(:ecdsa_key_256)[:key]).for(:key) }
     it { is_expected.to allow_value(attributes_for(:ed25519_key_256)[:key]).for(:key) }
     it { is_expected.not_to allow_value('foo-bar').for(:key) }
+
+    context 'key format' do
+      let(:key) { build(:key) }
+
+      it 'does not allow the key that begins with an algorithm name that is unsupported' do
+        key.key = 'unsupported-ssh-rsa key'
+
+        key.valid?
+
+        expect(key.errors.of_kind?(:key, :invalid)).to eq(true)
+      end
+
+      Gitlab::SSHPublicKey.supported_algorithms.each do |supported_algorithm|
+        it "allows the key that begins with supported algorithm name '#{supported_algorithm}'" do
+          key.key = "#{supported_algorithm} key"
+
+          key.valid?
+
+          expect(key.errors.of_kind?(:key, :invalid)).to eq(false)
+        end
+      end
+    end
   end
 
   describe "Methods" do