Commit da34ccb2 authored by Imre Farkas's avatar Imre Farkas

Merge branch 'update-ssh-key-format-validation' into 'master'

Improve SSH key format validation

See merge request gitlab-org/gitlab!77996
parents fbf50135 053b079d
......@@ -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,
......
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);
}
}
......@@ -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,
......
......@@ -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
......
......@@ -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')
......
......@@ -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);
});
});
});
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment