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

fix: The proxy URL is being altered in version 2.20.4, resulting in invalid URL #1058

Merged
merged 17 commits into from
Mar 26, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
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
154 changes: 154 additions & 0 deletions packages/core/src/__tests__/internal/fetchSettings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -281,4 +281,158 @@
expect(setSettingsSpy).not.toHaveBeenCalled();
}
);
describe('getEndpointForSettings', () => {
it.each([
['example.com/v1/', 'https://example.com/v1/'],
['https://example.com/v1/', 'https://example.com/v1/'],
['http://example.com/v1/', 'http://example.com/v1/'],
])(
'should append projects/key/settings if proxy end with / and useSegmentEndpoint is true',
(cdnProxy, expectedBaseURL) => {
const config = {
...clientArgs.config,
useSegmentEndpoints: true,
cdnProxy: cdnProxy,
};
const anotherClient = new SegmentClient({
...clientArgs,
config,
});
const spy = jest.spyOn(
Object.getPrototypeOf(anotherClient),
'getEndpointForSettings'
);
expect(anotherClient['getEndpointForSettings']()).toBe(

Check warning on line 305 in packages/core/src/__tests__/internal/fetchSettings.test.ts

View workflow job for this annotation

GitHub Actions / build-and-test

["getEndpointForSettings"] is better written in dot notation
`${expectedBaseURL}projects/${config.writeKey}/settings`
);
expect(spy).toHaveBeenCalled();
}
);
it.each([
['example.com/v1/projects/', 'https://example.com/v1/projects/'],
['https://example.com/v1/projects/', 'https://example.com/v1/projects/'],
['http://example.com/v1/projects/', 'http://example.com/v1/projects/'],
])(
'should append projects/writeKey/settings if proxy ends with projects/ and useSegmentEndpoint is true',
(cdnProxy, expectedBaseURL) => {
const config = {
...clientArgs.config,
useSegmentEndpoints: true,
cdnProxy: cdnProxy,
};
const anotherClient = new SegmentClient({
...clientArgs,
config,
});

const spy = jest.spyOn(
Object.getPrototypeOf(anotherClient),
'getEndpointForSettings'
);
expect(anotherClient['getEndpointForSettings']()).toBe(

Check warning on line 332 in packages/core/src/__tests__/internal/fetchSettings.test.ts

View workflow job for this annotation

GitHub Actions / build-and-test

["getEndpointForSettings"] is better written in dot notation
`${expectedBaseURL}projects/${config.writeKey}/settings`
);
expect(spy).toHaveBeenCalled();
}
);
it.each([
['example.com/v1/projects', 'https://example.com/v1/projects'],
['https://example.com/v1/projects', 'https://example.com/v1/projects'],
['http://example.com/v1/projects', 'http://example.com/v1/projects'],
])(
'should append /projects/writeKey/settings if proxy ends with /projects and useSegmentEndpoint is true',
(cdnProxy, expectedBaseURL) => {
const config = {
...clientArgs.config,
useSegmentEndpoints: true,
cdnProxy: cdnProxy,
};
const anotherClient = new SegmentClient({
...clientArgs,
config,
});

const spy = jest.spyOn(
Object.getPrototypeOf(anotherClient),
'getEndpointForSettings'
);
expect(anotherClient['getEndpointForSettings']()).toBe(

Check warning on line 359 in packages/core/src/__tests__/internal/fetchSettings.test.ts

View workflow job for this annotation

GitHub Actions / build-and-test

["getEndpointForSettings"] is better written in dot notation
`${expectedBaseURL}/projects/${config.writeKey}/settings`
);
expect(spy).toHaveBeenCalled();
}
);
it.each([
['example.com/v1?params=xx'],
['https://example.com/v1?params=xx'],
['http://example.com/v1?params=xx'],
])(
'should throw an error if proxy comes with query params and useSegmentEndpoint is true',
(cdnProxy) => {
const config = {
...clientArgs.config,
useSegmentEndpoints: true,
cdnProxy: cdnProxy,
};
const anotherClient = new SegmentClient({
...clientArgs,
config,
});

const spy = jest.spyOn(
Object.getPrototypeOf(anotherClient),
'getEndpointForSettings'
);
// Expect the private method to throw an error
expect(anotherClient['getEndpointForSettings']()).toBe(

Check warning on line 387 in packages/core/src/__tests__/internal/fetchSettings.test.ts

View workflow job for this annotation

GitHub Actions / build-and-test

["getEndpointForSettings"] is better written in dot notation
`${settingsCDN}/${config.writeKey}/settings`
);
expect(spy).toHaveBeenCalled();
}
);
it.each([
['example.com/v1/', false],
['example.com/v1/projects/', false],
['example.com/v1/projects', false],
['example.com/v1?params=xx', false],
])(
'should always return identical result if proxy is provided and useSegmentEndpoints is false',
(cdnProxy, useSegmentEndpoints) => {
const config = {
...clientArgs.config,
useSegmentEndpoints: useSegmentEndpoints,
cdnProxy: cdnProxy,
};
const anotherClient = new SegmentClient({
...clientArgs,
config,
});
const spy = jest.spyOn(
Object.getPrototypeOf(anotherClient),
'getEndpointForSettings'
);
const expected = `https://${cdnProxy}`;
expect(anotherClient['getEndpointForSettings']()).toBe(expected);

Check warning on line 415 in packages/core/src/__tests__/internal/fetchSettings.test.ts

View workflow job for this annotation

GitHub Actions / build-and-test

["getEndpointForSettings"] is better written in dot notation
expect(spy).toHaveBeenCalled();
}
);
it('No cdn proxy provided, should return default settings CDN', () => {
const config = {
...clientArgs.config,
useSegmentEndpoints: true, // No matter in this case
};
const anotherClient = new SegmentClient({
...clientArgs,
config,
});
const spy = jest.spyOn(
Object.getPrototypeOf(anotherClient),
'getEndpointForSettings'
);
expect(anotherClient['getEndpointForSettings']()).toBe(

Check warning on line 432 in packages/core/src/__tests__/internal/fetchSettings.test.ts

View workflow job for this annotation

GitHub Actions / build-and-test

["getEndpointForSettings"] is better written in dot notation
`${settingsCDN}/${config.writeKey}/settings`
);
expect(spy).toHaveBeenCalled();
});
});
});
12 changes: 6 additions & 6 deletions packages/core/src/__tests__/util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ describe('getURL function', () => {
});

it('should return the root URL when the path is empty', () => {
expect(getURL('www.example.com', '')).toBe('https://www.example.com/');
expect(getURL('www.example.com', '')).toBe('https://www.example.com');
});

it('should handle query parameters correctly in the URL path', () => {
Expand All @@ -188,13 +188,13 @@ describe('getURL function', () => {
});

// Negative Test Cases
it('should handle empty host gracefully', () => {
expect(getURL('', '/home')).toBe('https:///home');
it('should throw an error for empty host', () => {
expect(() => getURL('', '/home')).toThrow('Invalid URL has been passed');
});

it('should handle invalid characters in the host', () => {
expect(getURL('invalid host.com', '/path')).toBe(
'https://invalid host.com/path'
it('should throw an error for invalid characters in the host', () => {
expect(() => getURL('invalid host.com', '/path')).toThrow(
'Invalid URL has been passed'
);
});
});
31 changes: 23 additions & 8 deletions packages/core/src/analytics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -320,22 +320,37 @@ export class SegmentClient {

return map;
}

async fetchSettings() {
const settingsPrefix: string = this.config.cdnProxy ?? settingsCDN;
private getEndpointForSettings(): string {
let settingsPrefix = '';
let settingsEndpoint = '';
const hasProxy = !!(this.config?.cdnProxy ?? '');
const useSegmentEndpoints = Boolean(this.config?.useSegmentEndpoints);
let settingsEndpoint = '';

if (hasProxy) {
settingsPrefix = this.config.cdnProxy ?? '';
if (useSegmentEndpoints) {
settingsEndpoint = `/projects/${this.config.writeKey}/settings`;
} else {
settingsEndpoint = '';
const isCdnProxyEndsWithSlash = settingsPrefix.endsWith('/');
settingsEndpoint = isCdnProxyEndsWithSlash
? `projects/${this.config.writeKey}/settings`
: `/projects/${this.config.writeKey}/settings`;
}
} else {
settingsPrefix = settingsCDN;
settingsEndpoint = `/${this.config.writeKey}/settings`;
}
const settingsURL = getURL(settingsPrefix, settingsEndpoint);
try {
return getURL(settingsPrefix, settingsEndpoint);
} catch (error) {
console.error(
'Error in getEndpointForSettings:',
`fallback to ${settingsCDN}/${this.config.writeKey}/settings`
);
return `${settingsCDN}/${this.config.writeKey}/settings`;
}
}

async fetchSettings() {
const settingsURL = this.getEndpointForSettings();
try {
const res = await fetch(settingsURL, {
headers: {
Expand Down
23 changes: 15 additions & 8 deletions packages/core/src/plugins/SegmentDestination.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,17 +92,24 @@ export class SegmentDestination extends DestinationPlugin {
const config = this.analytics?.getConfig();
const hasProxy = !!(config?.proxy ?? '');
const useSegmentEndpoints = Boolean(config?.useSegmentEndpoints);

let baseURL = '';
let endpoint = '';

if (hasProxy) {
endpoint = useSegmentEndpoints ? '/b' : '';
//baseURL is always config?.proxy if hasProxy
baseURL = config?.proxy ?? '';
if (useSegmentEndpoints) {
const isProxyEndsWithSlash = baseURL.endsWith('/');
endpoint = isProxyEndsWithSlash ? 'b' : '/b';
}
} else {
endpoint = '/b'; // If no proxy, always append '/b'
baseURL = this.apiHost ?? defaultApiHost;
}
try {
return getURL(baseURL, endpoint);
} catch (error) {
console.error('Error in getEndpoint:', `fallback to ${defaultApiHost}`);
return defaultApiHost;
}

const baseURL = config?.proxy ?? this.apiHost ?? defaultApiHost;
return getURL(baseURL, endpoint);
}
configure(analytics: SegmentClient): void {
super.configure(analytics);
Expand All @@ -128,7 +135,7 @@ export class SegmentDestination extends DestinationPlugin {
segmentSettings?.apiHost !== null
) {
//assign the api host from segment settings (domain/v1)
this.apiHost = segmentSettings.apiHost;
this.apiHost = `https://${segmentSettings.apiHost}/b`;
}
this.settingsResolve();
}
Expand Down
Loading
Loading