Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,18 @@ describe('parseNoProxyRules', () => {

it('parses host:port', () => {
const rules = parseNoProxyRules('example.com:8080');
expect(rules).toEqual([{ host: 'example.com', port: 8080, wildcard: false }]);
expect(rules).toEqual([
{ host: 'example.com', port: 8080, wildcard: false },
]);
});

it('does not parse malformed port suffixes', () => {
expect(parseNoProxyRules('example.com:443abc')).toEqual([
{ host: 'example.com:443abc', wildcard: false },
]);
expect(parseNoProxyRules('[::1]:abc')).toEqual([
{ host: '[::1]:abc', wildcard: false },
]);
});
});

Expand All @@ -55,7 +66,9 @@ describe('shouldBypassProxy', () => {
it('respects port-scoped rules', () => {
const rules = parseNoProxyRules('example.com:8080');
expect(shouldBypassProxy('http://example.com:8080/path', rules)).toBe(true);
expect(shouldBypassProxy('http://example.com:9090/path', rules)).toBe(false);
expect(shouldBypassProxy('http://example.com:9090/path', rules)).toBe(
false,
);
});

it('matches implicit default ports', () => {
Expand All @@ -71,7 +84,9 @@ describe('shouldBypassProxy', () => {

// Explicit port that differs from rule should not match
const rules8080 = parseNoProxyRules('example.com:8080');
expect(shouldBypassProxy('https://example.com/path', rules8080)).toBe(false);
expect(shouldBypassProxy('https://example.com/path', rules8080)).toBe(
false,
);
});

it('wildcard bypasses everything', () => {
Expand All @@ -84,6 +99,27 @@ describe('shouldBypassProxy', () => {
expect(shouldBypassProxy('http://[::1]:3000/path', rules)).toBe(true);
});

it('respects IPv6 port-scoped rules', () => {
const rules = parseNoProxyRules('[::1]:3000');
expect(shouldBypassProxy('http://[::1]:3000/path', rules)).toBe(true);
expect(shouldBypassProxy('http://[::1]:3001/path', rules)).toBe(false);
});

it('does not bypass for malformed port suffixes', () => {
expect(
shouldBypassProxy(
'https://example.com/path',
parseNoProxyRules('example.com:443abc'),
),
).toBe(false);
expect(
shouldBypassProxy(
'http://[::1]:3000/path',
parseNoProxyRules('[::1]:abc'),
),
).toBe(false);
});

it('matches exact IP literal', () => {
const rules = parseNoProxyRules('192.168.1.1');
expect(shouldBypassProxy('http://192.168.1.1/', rules)).toBe(true);
Expand Down
39 changes: 29 additions & 10 deletions packages/desktop/apps/electron/src/main/network-proxy-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@
/** Split a comma-separated string into trimmed, non-empty entries. */
export function splitCommaSeparated(str: string | undefined): string[] {
if (!str) return [];
return str.split(',').map(s => s.trim()).filter(Boolean);
return str
.split(',')
.map((s) => s.trim())
.filter(Boolean);
}

export interface NoProxyRule {
Expand All @@ -19,6 +22,14 @@ export interface NoProxyRule {
wildcard: boolean;
}

function parsePort(raw: string): number | undefined {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] After /^\d+$/ regex validation, Number(raw) always produces a non-negative integer — Number.isInteger() and port >= 0 are guaranteed true. Only port <= 65535 carries decision value.

Suggested change
function parsePort(raw: string): number | undefined {
return port <= 65535 ? port : undefined;

— DeepSeek/deepseek-v4-pro via Qwen Code /review

if (!/^\d+$/.test(raw)) return undefined;
const port = Number(raw);
return Number.isInteger(port) && port >= 0 && port <= 65535
? port
: undefined;
}

/**
* Parse a comma-separated NO_PROXY string into structured rules.
*
Expand All @@ -33,8 +44,8 @@ export function parseNoProxyRules(noProxy: string | undefined): NoProxyRule[] {
if (!noProxy) return [];

return splitCommaSeparated(noProxy)
.map(entry => entry.toLowerCase())
.map(entry => {
.map((entry) => entry.toLowerCase())
.map((entry) => {
if (entry === '*') {
return { host: '*', wildcard: true };
}
Expand All @@ -49,21 +60,24 @@ export function parseNoProxyRules(noProxy: string | undefined): NoProxyRule[] {
const ipv6Host = cleaned.slice(1, closeBracket);
const afterBracket = cleaned.slice(closeBracket + 1);
if (afterBracket.startsWith(':')) {
const port = parseInt(afterBracket.slice(1), 10);
if (!isNaN(port)) {
const port = parsePort(afterBracket.slice(1));
if (port !== undefined) {
return { host: ipv6Host, port, wildcard: false };
}
}
return { host: ipv6Host, wildcard: false };
if (afterBracket === '') {
return { host: ipv6Host, wildcard: false };
}
return { host: cleaned, wildcard: false };
}
}

// Check for port (non-IPv6)
const lastColon = cleaned.lastIndexOf(':');
if (lastColon > 0) {
const host = cleaned.slice(0, lastColon);
const port = parseInt(cleaned.slice(lastColon + 1), 10);
if (!isNaN(port)) {
const port = parsePort(cleaned.slice(lastColon + 1));
if (port !== undefined) {
return { host, port, wildcard: false };
}
}
Expand All @@ -78,14 +92,19 @@ export function parseNoProxyRules(noProxy: string | undefined): NoProxyRule[] {
/** Default ports by protocol, used when URL omits an explicit port. */
const DEFAULT_PORTS: Record<string, number> = { 'http:': 80, 'https:': 443 };

export function shouldBypassProxy(url: string | URL, rules: NoProxyRule[]): boolean {
export function shouldBypassProxy(
url: string | URL,
rules: NoProxyRule[],
): boolean {
if (rules.length === 0) return false;

const parsed = typeof url === 'string' ? new URL(url) : url;
const hostname = parsed.hostname.toLowerCase();
// Strip brackets from IPv6
const host = hostname.startsWith('[') ? hostname.slice(1, -1) : hostname;
const port = parsed.port ? parseInt(parsed.port, 10) : DEFAULT_PORTS[parsed.protocol];
const port = parsed.port
? parseInt(parsed.port, 10)
: DEFAULT_PORTS[parsed.protocol];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The PR converts both parseInt sites in parseNoProxyRules to the strict parsePort(), but shouldBypassProxy still uses bare parseInt(parsed.port, 10). While safe today (URL constructor validates ports at construction time), the inconsistency invites future copy-paste regressions. Consider replacing with parsePort() or adding a comment explaining why parseInt is safe here.

— DeepSeek/deepseek-v4-pro via Qwen Code /review

for (const rule of rules) {
if (rule.wildcard) return true;
Expand Down
Loading