Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump FQDN regex to allow for 25 characters #4890

Merged
merged 12 commits into from
Apr 5, 2021
4 changes: 3 additions & 1 deletion components/automate-ui/src/app/helpers/auth/regex.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@ export class Regex {
NO_MIXED_WILDCARD_ALLOW_SPECIAL: '^(\\*|[^:*]+)$',

// Allows valid FQDN only
VALID_FQDN: /^(http:\/\/www\.|https:\/\/www\.|http:\/\/|https:\/\/)?[a-zA-Z0-9_]+([\-\.]{1}[a-zA-Z0-9]+)*\.[a-zA-Z]{2,5}(:[0-9]{1,5})?(\/.*)?$/,
// Top level domain is limited to a maximum number of 25 characters
VALID_FQDN: /^(https?:\/\/)?[a-zA-Z0-9_]+([\-\.]{1}[a-zA-Z0-9]+)*\.[a-zA-Z]{2,25}(:[0-9]{1,5})?(\/.*)?$/,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up shortening just the first capture group. When playing around with the sample you provided, I kept running into "Catastrophic Backtracing". Even though this isn't a perfect Regex (as you demonstrated is nearly impossible). Its really a pretty good one for our needs.


// Allows valid IP Address only (ipv4)
VALID_IP_ADDRESS: '^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$',

Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
import { EventEmitter } from '@angular/core';
import { CUSTOM_ELEMENTS_SCHEMA } from '@angular/core';
import { ReactiveFormsModule, FormBuilder } from '@angular/forms';
import { ReactiveFormsModule, FormBuilder, Validators, FormGroup } from '@angular/forms';
import { ComponentFixture, TestBed, waitForAsync } from '@angular/core/testing';
import { MockComponent } from 'ng2-mock-component';
import { Regex } from 'app/helpers/auth/regex';
import { using } from 'app/testing/spec-helpers';

import { CreateChefServerModalComponent } from './create-chef-server-modal.component';

describe('CreateChefServerModalComponent', () => {
let component: CreateChefServerModalComponent;
let fixture: ComponentFixture<CreateChefServerModalComponent>;

let createForm: FormGroup;
let errors = {};

beforeEach( waitForAsync(() => {
TestBed.configureTestingModule({
declarations: [
Expand All @@ -35,18 +40,164 @@ describe('CreateChefServerModalComponent', () => {
beforeEach(() => {
fixture = TestBed.createComponent(CreateChefServerModalComponent);
component = fixture.componentInstance;
// This form must mimic the createForm including Validators
component.createForm = new FormBuilder().group({
id: ['', null],
name: ['', null],
description: ['', null],
fqdn: ['', null],
ip_address: ['', null]
name: ['', [Validators.required, Validators.pattern(Regex.patterns.NON_BLANK)]],
id: ['',
[Validators.required, Validators.pattern(Regex.patterns.ID), Validators.maxLength(64)]],
fqdn: ['', [Validators.required,
Validators.pattern(Regex.patterns.NON_BLANK),
Validators.pattern(Regex.patterns.VALID_FQDN)
]],
ip_address: ['', [Validators.required,
Validators.pattern(Regex.patterns.NON_BLANK),
Validators.pattern(Regex.patterns.VALID_IP_ADDRESS)
]]
});
component.conflictErrorEvent = new EventEmitter();
createForm = component.createForm;
fixture.detectChanges();
});

it('should create', () => {
expect(component).toBeTruthy();
});

describe('form validity', () => {
describe('the form should be invalid', () => {
it('when all inputs are empty', () => {
expect(createForm.valid).toBeFalsy();
});

it('when name is missing', () => {
createForm.controls['id'].setValue('test');
createForm.controls['fqdn'].setValue('test.net');
createForm.controls['ip_address'].setValue('1.2.3.4');

errors = createForm.controls['name'].errors || {};

expect(createForm.valid).toBeFalsy();
expect(errors['required']).toBeTruthy();
});
Comment on lines +72 to +81
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated each of these to test when each individual input is missing.


it('when id is missing', () => {
createForm.controls['name'].setValue('test');
createForm.controls['fqdn'].setValue('test.net');
createForm.controls['ip_address'].setValue('1.2.3.4');

errors = createForm.controls['id'].errors || {};

expect(createForm.valid).toBeFalsy();
expect(errors['required']).toBeTruthy();
});

it('when fqdn is missing', () => {
createForm.controls['name'].setValue('test');
createForm.controls['id'].setValue('test');
createForm.controls['ip_address'].setValue('1.2.3.4');

errors = createForm.controls['fqdn'].errors || {};

expect(createForm.valid).toBeFalsy();
expect(errors['required']).toBeTruthy();
});

it('when ip_address is missing', () => {
createForm.controls['name'].setValue('test');
createForm.controls['id'].setValue('test');
createForm.controls['fqdn'].setValue('test.net');

errors = createForm.controls['ip_address'].errors || {};

expect(createForm.valid).toBeFalsy();
expect(errors['required']).toBeTruthy();
});

it('when the ip_address in invalid', () => {
createForm.controls['name'].setValue('test');
createForm.controls['id'].setValue('test');
createForm.controls['fqdn'].setValue('chef.internal');

createForm.controls['ip_address'].setValue('1.2234.3.4');
errors = createForm.controls['ip_address'].errors || {};

expect(createForm.valid).toBeFalsy();
expect(errors['pattern']).toBeTruthy();
});

// Many testing ideas attributed to https://mathiasbynens.be/demo/url-regex

using([
['is using something other than http or https', 'httpld://www.chef.io'],
['contains two periods', 'chef..internal'],
['there is no TLD suffix', 'http://foo.com.'],
['contains hypens in the TLD', 'chef.this-will-not'],
SEAjamieD marked this conversation as resolved.
Show resolved Hide resolved
['has a TLD that is longer than 25 characters', 'chef.thisisareallylongtldandwontwork'],
['has a TLD that is shorter than 2 characters', 'chef.i'],
['has numbers in the TLD', 'chef.017'],
['has a port number that is too high', 'https://chef.io:987274892'],
['has a colon but no port number', 'https://chef.io:'],
['has a letter in the port', 'https://chef.io:123a'],
['has no domain', 'http://'],
['has no secure domain', 'https://'],
['domain is dots', 'https://..'],
['domain is hash', 'http://#'],
['domain has a space', 'http:// shouldfail.net'],
['contains all numbers', 'http://10.1.1.0']
], function (description: string, input: string) {
it(('when the fqdn ' + description), () => {
createForm.controls['name'].setValue('test');
createForm.controls['id'].setValue('test');
createForm.controls['ip_address'].setValue('1.2.3.4');

createForm.controls['fqdn'].setValue(input);
errors = createForm.controls['fqdn'].errors || {};

expect(createForm.valid).toBeFalsy();
expect(errors['pattern']).toBeTruthy();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree we could have hundreds of tests here, I feel like that is a bit overkill for us, and that these dozen or so are a good cross section.

same feelings for Valid tests below.

});
});
});



describe('the form should be valid', () => {
it('when all 4 inputs are filled and valid', () => {
expect(createForm.valid).toBeFalsy();
createForm.controls['name'].setValue('test');
createForm.controls['id'].setValue('test');
createForm.controls['fqdn'].setValue('chef.internal');
createForm.controls['ip_address'].setValue('1.2.3.4');
expect(createForm.valid).toBeTruthy();
});

using([
['has a TLD that is longer than 2 and less than 25 characters', 'chef.internal'],
['uses https', 'https://chef.io'],
['uses http', 'http://chef.io'],
['omits http or https', 'chef.thisworks'],
['has a port number', 'https://chef.io:123'],
['contains hyphens in the domain', 'new-company-who-dis.nice'],
['contains underscores in the domain', 'new_company_who_dis.chef'],
['contains digits in the domain', '8675309.jenny'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😮 8675309.jenny is valid (this line) but chef.017 (L137) is not; curious...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it sits today, this is by our design.

Currently, the TLD cannot be all numbers, so chef.017 will fail.
Conversely, a TLD of all letters should pass as long as it is between 2 and 25 characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at the Regex though, we certainly could update the TLD portion
from
(https?:\/\/)?[a-zA-Z0-9_]+([\-\.]{1}[a-zA-Z0-9]+)*\.[a-zA-Z]{2,25}(:[0-9]{1,5})?(\/.*)?$

to something like
(https?:\/\/)?[a-zA-Z0-9_]+([\-\.]{1}[a-zA-Z0-9]+)*\.[a-zA-Z change here -> 0-9 <- ]{2,25}(:[0-9]{1,5})?(\/.*)?$

['contains all numbers in the domain', 'http://223.453.739.net'],
['contains a query', 'http://www.chef.io/events/#&product=automate'],
['contains unicode', 'http://foo.com/unicode_(✪)_in_parens'],
['is a citation url', 'http://foo.com/blah_(wikipedia)_blah#cite-1']
], function (description: string, input: string) {
it(('when the fqdn ' + description), () => {
createForm.controls['name'].setValue('test');
createForm.controls['id'].setValue('test');
createForm.controls['ip_address'].setValue('1.2.3.4');

createForm.controls['fqdn'].setValue(input);
errors = createForm.controls['fqdn'].errors || {};

expect(createForm.valid).toBeTruthy();
expect(errors['pattern']).toBeFalsy();
});
});

});
});
});