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.

Sign up for GitHub

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

Jump to bottom

chore: correct port validation #7077

Merged
merged 2 commits into from
May 14, 2024
Merged

chore: correct port validation #7077

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f71e1ba
chore: correct port validation
vzhukovs May 6, 2024
7c5590e
chore: correct port validation
vzhukovs May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions packages/main/src/plugin/util/port.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,14 +133,16 @@ test.each(hosts)(
},
);

test('fails with range error if trying to get a port which is over upper range', async () => {
await expect(port.getFreePort(200000)).rejects.toThrowError(/options.port should be >= 0 and < 65536/);
});

test('fails with range error if value is over upper range', async () => {
await expect(port.isFreePort(200000)).rejects.toThrowError(/options.port should be >= 0 and < 65536/);
await expect(port.isFreePort(200000)).rejects.toThrowError(
/The port must have an integer value within the range from 1025 to 65535./,
);
});

test('fails with range error if value is less lower range', async () => {
await expect(port.isFreePort(-1)).rejects.toThrowError(/options.port should be >= 0 and < 65536/);
await expect(port.isFreePort(-1)).rejects.toThrowError(/The port must be greater than 1024./);
});

test('should return message that user is trying to check unprivileged port', async () => {
await expect(port.isFreePort(1)).rejects.toThrowError(/The port must be greater than 1024./);
});
38 changes: 27 additions & 11 deletions packages/main/src/plugin/util/port.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@ export async function getFreePort(port = 0): Promise<number> {
}
let isFree = false;
while (!isFree) {
isFree = await isFreePort(port);
if (!isFree) {
try {
await isFreePort(port);
isFree = true;
} catch (e) {
port++;
}
}
Expand All @@ -45,9 +47,10 @@ export async function getFreePortRange(rangeSize: number): Promise<string> {
let startPort = port;

do {
if (await isFreePort(port)) {
try {
await isFreePort(port);
++port;
} else {
} catch (e) {
++port;
startPort = port;
}
Expand All @@ -60,20 +63,33 @@ function isFreeAddressPort(address: string, port: number): Promise<boolean> {
const server = net.createServer();
return new Promise((resolve, reject) =>
server
.on('error', (error: NodeJS.ErrnoException) => (error.code === 'EADDRINUSE' ? resolve(false) : reject(error)))
.on('error', (error: NodeJS.ErrnoException) => {
if (error.code === 'EADDRINUSE') {
reject(new Error(`Port ${port} is already in use.`));
} else if (error.code === 'EACCES') {
reject(new Error('Operation require administrative privileges.'));
} else {
reject(new Error(`Failed to check port status: ${error}`));
}
})
.on('listening', () => server.close(() => resolve(true)))
.listen(port, address),
);
}

export async function isFreePort(port: number): Promise<boolean> {
const intfs = getIPv4Interfaces();
// Test this special interface separately, or it will interfere with other tests done in parallel
if (!(await isFreeAddressPort('0.0.0.0', port))) {
return false;
if (isNaN(port) || port > 65535) {
throw new Error('The port must have an integer value within the range from 1025 to 65535.');
} else if (port < 1024) {
throw new Error('The port must be greater than 1024.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice having custom message 👍

}
const checkInterfaces = await Promise.all(intfs.map(intf => isFreeAddressPort(intf, port)));
return checkInterfaces.every(element => element === true);

const intfs = getIPv4Interfaces();

await isFreeAddressPort('0.0.0.0', port);
await Promise.all(intfs.map(intf => isFreeAddressPort(intf, port)));

return true;
}

function getIPv4Interfaces(): string[] {
Expand Down
116 changes: 6 additions & 110 deletions packages/renderer/src/lib/image/RunImage.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ describe('RunImage', () => {
});

test('Expect to see an error if the container/host ranges have different size', async () => {
(window.isFreePort as Mock).mockResolvedValue(true);
router.goto('/basic');

await createRunImage(undefined, ['command1', 'command2']);
Expand All @@ -352,6 +353,9 @@ describe('RunImage', () => {
await userEvent.clear(containerInput);
await userEvent.keyboard('9000-9003');

// wait onPortInputTimeout (500ms) triggers
await new Promise(resolve => setTimeout(resolve, 600));

const button = screen.getByRole('button', { name: 'Start Container' });

await fireEvent.click(button);
Expand Down Expand Up @@ -464,86 +468,8 @@ describe('RunImage', () => {
);
});

test('Expect "start container" button to be disabled if user adds a port which is invalid (lower than 0)', async () => {
router.goto('/basic');

await createRunImage(undefined, ['command1', 'command2']);

const link1 = screen.getByRole('link', { name: 'Basic' });
await fireEvent.click(link1);

const customMappingButton = screen.getByRole('button', { name: 'Add custom port mapping' });
await fireEvent.click(customMappingButton);

const hostInput = screen.getByLabelText('host port');
await userEvent.click(hostInput);
await userEvent.clear(hostInput);
// adds a negative port
await userEvent.keyboard('-1');

const containerInput = screen.getByLabelText('container port');
await userEvent.click(containerInput);
await userEvent.clear(containerInput);
await userEvent.keyboard('80');

const button = screen.getByRole('button', { name: 'Start Container' });
expect((button as HTMLButtonElement).disabled).toBeTruthy();
});

test('Expect "start container" button to be disabled if user adds a port which is invalid (over upper limit > 65535)', async () => {
router.goto('/basic');

await createRunImage(undefined, ['command1', 'command2']);

const link1 = screen.getByRole('link', { name: 'Basic' });
await fireEvent.click(link1);

const customMappingButton = screen.getByRole('button', { name: 'Add custom port mapping' });
await fireEvent.click(customMappingButton);

const hostInput = screen.getByLabelText('host port');
await userEvent.click(hostInput);
await userEvent.clear(hostInput);
// adds a negative port
await userEvent.keyboard('71000');

const containerInput = screen.getByLabelText('container port');
await userEvent.click(containerInput);
await userEvent.clear(containerInput);
await userEvent.keyboard('80');

const button = screen.getByRole('button', { name: 'Start Container' });
expect((button as HTMLButtonElement).disabled).toBeTruthy();
});

test('Expect "start container" button to be disabled if user adds a port which is invalid (isNaN)', async () => {
router.goto('/basic');

await createRunImage(undefined, ['command1', 'command2']);

const link1 = screen.getByRole('link', { name: 'Basic' });
await fireEvent.click(link1);

const customMappingButton = screen.getByRole('button', { name: 'Add custom port mapping' });
await fireEvent.click(customMappingButton);

const hostInput = screen.getByLabelText('host port');
await userEvent.click(hostInput);
await userEvent.clear(hostInput);
// adds a negative port
await userEvent.keyboard('test');

const containerInput = screen.getByLabelText('container port');
await userEvent.click(containerInput);
await userEvent.clear(containerInput);
await userEvent.keyboard('80');

const button = screen.getByRole('button', { name: 'Start Container' });
expect((button as HTMLButtonElement).disabled).toBeTruthy();
});

test('Expect "start container" button to be disabled if user adds a port which is NOT free', async () => {
(window.isFreePort as Mock).mockResolvedValue(false);
test('Expect "start container" button to be disabled when port is not free', async () => {
(window.isFreePort as Mock).mockRejectedValue(new Error('Error Message'));
router.goto('/basic');

await createRunImage(undefined, ['command1', 'command2']);
Expand Down Expand Up @@ -571,34 +497,4 @@ describe('RunImage', () => {
const button = screen.getByRole('button', { name: 'Start Container' });
expect((button as HTMLButtonElement).disabled).toBeTruthy();
});

test('Expect "start container" button to be enabled if user adds a port which is valid and free', async () => {
(window.isFreePort as Mock).mockResolvedValue(true);
router.goto('/basic');

await createRunImage(undefined, ['command1', 'command2']);

const link1 = screen.getByRole('link', { name: 'Basic' });
await fireEvent.click(link1);

const customMappingButton = screen.getByRole('button', { name: 'Add custom port mapping' });
await fireEvent.click(customMappingButton);

const hostInput = screen.getByLabelText('host port');
await userEvent.click(hostInput);
await userEvent.clear(hostInput);
// adds a negative port
await userEvent.keyboard('8080');

const containerInput = screen.getByLabelText('container port');
await userEvent.click(containerInput);
await userEvent.clear(containerInput);
await userEvent.keyboard('80');

// wait onPortInputTimeout (500ms) triggers
await new Promise(resolve => setTimeout(resolve, 600));

const button = screen.getByRole('button', { name: 'Start Container' });
expect((button as HTMLButtonElement).disabled).toBeFalsy();
});
});
35 changes: 12 additions & 23 deletions packages/renderer/src/lib/image/RunImage.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,33 +598,22 @@ function onPortInput(event: Event, portInfo: PortInfo, updateUI: () => void) {
const target = event.currentTarget as HTMLInputElement;
// convert string to number
const _value: number = Number(target.value);
// if number is not valid (NaN or outside the value range), set the error
if (isNaN(_value) || _value < 0 || _value > 65535) {
portInfo.error = 'port should be >= 0 and < 65536';
updateUI();
return;
}
onPortInputTimeout = setTimeout(() => {
isPortFree(_value).then(isFree => {
portInfo.error = isFree;
updateUI();
});
window
.isFreePort(_value)
.then(_ => {
portInfo.error = '';
updateUI();
})
.catch((error: unknown) => {
if (error && typeof error === 'object' && 'message' in error) {
portInfo.error = (error as { message: string }).message;
}
updateUI();
});
}, 500);
}

function isPortFree(port: number): Promise<string> {
return window
.isFreePort(port)
.then(isFree => {
if (!isFree) {
return `Port ${port} is already in use`;
} else {
return '';
}
})
.catch((_: unknown) => `Port ${port} is already in use`);
}

async function assertAllPortAreValid(): Promise<void> {
const invalidHostPorts = hostContainerPortMappings.filter(pair => pair.hostPort.error);
const invalidContainerPortMapping = containerPortMapping?.filter(port => port.error) || [];
Expand Down