Skip to content

Commit 3826d88

Browse files
authored
Merge pull request #958 from evershopcommerce/fix_928
fix: unauthenticated SSRF + unauthenticated IDOR on all customer endp…
2 parents b633377 + af6e6fd commit 3826d88

22 files changed

Lines changed: 1043 additions & 40 deletions

File tree

package-lock.json

Lines changed: 521 additions & 8 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/evershop/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,7 @@
249249
"tailwindcss": "^4.1.18",
250250
"touch": "^3.1.1",
251251
"tw-animate-css": "^1.4.0",
252+
"undici": "^7.25.0",
252253
"uniqid": "^5.3.0",
253254
"urql": "^3.0.3",
254255
"uuid": "^9.0.0",

packages/evershop/src/components/frontStore/customer/CustomerContext.tsx

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -270,10 +270,10 @@ interface CustomerDispatchContextValue {
270270
addressData: Omit<ExtendedCustomerAddress, 'id'>
271271
) => Promise<ExtendedCustomerAddress>;
272272
updateAddress: (
273-
addressId: string | number,
273+
uuid: string,
274274
addressData: Partial<ExtendedCustomerAddress>
275275
) => Promise<ExtendedCustomerAddress>;
276-
deleteAddress: (addressId: string | number) => Promise<void>;
276+
deleteAddress: (uuid: string) => Promise<void>;
277277
updateProfile: (data: {
278278
full_name?: string;
279279
email?: string;
@@ -507,11 +507,11 @@ export function CustomerProvider({
507507
// Update address function
508508
const updateAddress = useCallback(
509509
async (
510-
addressId: string | number,
510+
uuid: string,
511511
addressData: Partial<ExtendedCustomerAddress>
512512
): Promise<ExtendedCustomerAddress> => {
513513
const address = state.customer?.addresses?.find(
514-
(addr) => addr.addressId === addressId
514+
(addr) => addr.uuid === uuid
515515
);
516516
if (!address?.updateApi) {
517517
throw new Error(_('Update address API not available'));
@@ -553,9 +553,9 @@ export function CustomerProvider({
553553

554554
// Delete address function
555555
const deleteAddress = useCallback(
556-
async (addressId: string | number): Promise<void> => {
556+
async (uuid: string): Promise<void> => {
557557
const address = state.customer?.addresses?.find(
558-
(addr) => addr.addressId === addressId
558+
(addr) => addr.uuid === uuid
559559
);
560560
if (!address?.deleteApi) {
561561
throw new Error(_('Delete address API not available'));

packages/evershop/src/components/frontStore/customer/MyAddresses.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ const Address: React.FC<{
5454
method="PATCH"
5555
onSubmit={async (data) => {
5656
try {
57-
await updateAddress(address.addressId, data);
57+
await updateAddress(address.uuid as string, data);
5858
setDialogOpen(false);
5959
toast.success(_('Address has been updated successfully!'));
6060
} catch (error) {
@@ -67,6 +67,7 @@ const Address: React.FC<{
6767
<CheckboxField
6868
label={_('Set as default')}
6969
defaultChecked={address.isDefault}
70+
defaultValue={address.isDefault}
7071
name="is_default"
7172
/>
7273
</div>
@@ -78,7 +79,7 @@ const Address: React.FC<{
7879
onClick={async (e) => {
7980
e.preventDefault();
8081
try {
81-
await deleteAddress(address.addressId);
82+
await deleteAddress(address.uuid as string);
8283
toast.success(_('Address has been deleted successfully!'));
8384
} catch (error) {
8485
toast.error(error.message);
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
import { Agent, buildConnector, fetch as undiciFetch } from 'undici';
2+
3+
const REQUEST_TIMEOUT_MS = 15000;
4+
5+
/**
6+
* Hosts the image proxy (`/images?src=…`) is allowed to fetch from, read from
7+
* the `IMAGE_ALLOWED_HOSTS` environment variable (comma-separated). This is a
8+
* strict allowlist: any host not listed — public or private — is refused. It
9+
* both blocks SSRF (loopback, link-local metadata, private ranges, …) and stops
10+
* the endpoint from being abused as a free image-optimization proxy for
11+
* arbitrary third-party images. When unset/empty, no external host is allowed
12+
* (only local media/public/theme images are processed).
13+
*/
14+
export function getAllowedImageHosts(): string[] {
15+
return (process.env.IMAGE_ALLOWED_HOSTS || '')
16+
.split(',')
17+
.map((host) => host.trim().toLowerCase())
18+
.filter(Boolean);
19+
}
20+
21+
function isAllowedHost(host: string | undefined | null): boolean {
22+
if (!host) {
23+
return false;
24+
}
25+
return getAllowedImageHosts().includes(host.toLowerCase());
26+
}
27+
28+
const baseConnector = buildConnector({});
29+
30+
type Connector = ReturnType<typeof buildConnector>;
31+
type ConnectorOptions = Parameters<Connector>[0];
32+
type ConnectorCallback = Parameters<Connector>[1];
33+
34+
/**
35+
* Refuse to open a connection to any host that is not on the allowlist. Because
36+
* this runs for every connection — including each redirect hop — a redirect
37+
* from an allowed host to a non-allowed one (e.g. an open redirect pointed at an
38+
* internal address) is still blocked.
39+
*/
40+
const guardedConnector = (
41+
options: ConnectorOptions,
42+
callback: ConnectorCallback
43+
): void => {
44+
if (!isAllowedHost(options.hostname)) {
45+
callback(
46+
new Error(
47+
`Blocked image fetch from non-allowlisted host: ${options.hostname}`
48+
),
49+
null
50+
);
51+
return;
52+
}
53+
baseConnector(options, callback);
54+
};
55+
56+
const guardedAgent = new Agent({ connect: guardedConnector });
57+
58+
/**
59+
* Parse and validate an untrusted image URL: http/https only, and the host must
60+
* be on the `IMAGE_ALLOWED_HOSTS` allowlist. Returns the parsed URL.
61+
*/
62+
export function assertAllowedUrl(rawUrl: string): URL {
63+
let url: URL;
64+
try {
65+
url = new URL(rawUrl);
66+
} catch {
67+
throw new Error('Invalid URL');
68+
}
69+
if (url.protocol !== 'http:' && url.protocol !== 'https:') {
70+
throw new Error('Only http and https URLs are allowed');
71+
}
72+
const host = url.hostname.replace(/^\[/, '').replace(/\]$/, '');
73+
if (!isAllowedHost(host)) {
74+
throw new Error(
75+
`Image host "${host}" is not allowed. Add it to the IMAGE_ALLOWED_HOSTS environment variable.`
76+
);
77+
}
78+
return url;
79+
}
80+
81+
/**
82+
* `fetch` for untrusted, user-supplied image URLs. Only hosts on the
83+
* `IMAGE_ALLOWED_HOSTS` allowlist may be reached (every redirect hop included),
84+
* restricted to http(s), with a request timeout to guard against hanging hosts.
85+
*/
86+
export async function secureFetch(
87+
rawUrl: string,
88+
init: Parameters<typeof undiciFetch>[1] = {}
89+
): Promise<Awaited<ReturnType<typeof undiciFetch>>> {
90+
const url = assertAllowedUrl(rawUrl);
91+
const controller = new AbortController();
92+
const timer = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS);
93+
try {
94+
return await undiciFetch(url, {
95+
...init,
96+
signal: controller.signal,
97+
dispatcher: guardedAgent
98+
});
99+
} finally {
100+
clearTimeout(timer);
101+
}
102+
}
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
import http from 'http';
2+
import { assertAllowedUrl, secureFetch } from '../../secureFetch.js';
3+
4+
describe('secureFetch image-host allowlist', () => {
5+
const ORIGINAL = process.env.IMAGE_ALLOWED_HOSTS;
6+
afterAll(() => {
7+
if (ORIGINAL === undefined) {
8+
delete process.env.IMAGE_ALLOWED_HOSTS;
9+
} else {
10+
process.env.IMAGE_ALLOWED_HOSTS = ORIGINAL;
11+
}
12+
});
13+
14+
describe('assertAllowedUrl', () => {
15+
beforeEach(() => {
16+
process.env.IMAGE_ALLOWED_HOSTS = 'cdn.example.com, 127.0.0.1';
17+
});
18+
19+
it('rejects non-http(s) schemes', () => {
20+
expect(() => assertAllowedUrl('ftp://cdn.example.com/x')).toThrow();
21+
expect(() => assertAllowedUrl('file:///etc/passwd')).toThrow();
22+
});
23+
24+
it('rejects any host not on the allowlist (public or private)', () => {
25+
expect(() => assertAllowedUrl('http://evil.com/x')).toThrow(/not allowed/);
26+
expect(() =>
27+
assertAllowedUrl('http://169.254.169.254/latest/meta-data/')
28+
).toThrow(/not allowed/);
29+
expect(() => assertAllowedUrl('http://10.0.0.5/')).toThrow(/not allowed/);
30+
});
31+
32+
it('allows hosts on the allowlist', () => {
33+
expect(assertAllowedUrl('https://cdn.example.com/a.jpg').hostname).toBe(
34+
'cdn.example.com'
35+
);
36+
expect(assertAllowedUrl('http://127.0.0.1:9000/a.jpg').hostname).toBe(
37+
'127.0.0.1'
38+
);
39+
});
40+
41+
it('rejects everything when the allowlist is empty', () => {
42+
process.env.IMAGE_ALLOWED_HOSTS = '';
43+
expect(() => assertAllowedUrl('https://cdn.example.com/a.jpg')).toThrow(
44+
/not allowed/
45+
);
46+
});
47+
});
48+
49+
describe('secureFetch', () => {
50+
let server;
51+
let port;
52+
53+
beforeAll(async () => {
54+
server = http.createServer((req, res) => {
55+
res.writeHead(200, { 'Content-Type': 'text/plain' });
56+
res.end('ok');
57+
});
58+
await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve));
59+
port = server.address().port;
60+
});
61+
62+
afterAll(async () => {
63+
await new Promise((resolve) => server.close(resolve));
64+
});
65+
66+
it('fetches from a host on the allowlist', async () => {
67+
process.env.IMAGE_ALLOWED_HOSTS = '127.0.0.1';
68+
const res = await secureFetch(`http://127.0.0.1:${port}/`);
69+
expect(res.status).toBe(200);
70+
});
71+
72+
it('refuses a host that is not on the allowlist', async () => {
73+
process.env.IMAGE_ALLOWED_HOSTS = 'cdn.example.com';
74+
await expect(
75+
secureFetch(`http://127.0.0.1:${port}/`)
76+
).rejects.toThrow();
77+
});
78+
});
79+
});

packages/evershop/src/modules/cms/bootstrap.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import path from 'path';
22
import { JSONSchemaType } from 'ajv';
33
import config from 'config';
44
import { CONSTANTS } from '../../lib/helpers.js';
5+
import { warning } from '../../lib/log/logger.js';
56
import { defaultPaginationFilters } from '../../lib/util/defaultPaginationFilters.js';
67
import { merge } from '../../lib/util/merge.js';
78
import { addProcessor } from '../../lib/util/registry.js';
@@ -10,7 +11,19 @@ import { registerDefaultPageCollectionFilters } from '../../modules/cms/services
1011
import { registerDefaultWidgetCollectionFilters } from '../../modules/cms/services/registerDefaultWidgetCollectionFilters.js';
1112
import { Route } from '../../types/route.js';
1213

13-
export default () => {
14+
export default (context: { command?: string } = {}) => {
15+
// Warn (non-blocking) at server boot if the image proxy allowlist is unset.
16+
// Without IMAGE_ALLOWED_HOSTS the /images endpoint will not fetch any external
17+
// image — only local media/public/theme images are processed.
18+
if (
19+
(context.command === 'start' || context.command === 'dev') &&
20+
!process.env.IMAGE_ALLOWED_HOSTS?.trim()
21+
) {
22+
warning(
23+
'IMAGE_ALLOWED_HOSTS is not set. The /images endpoint will not optimize external images — only local media/public/theme images are processed. Set IMAGE_ALLOWED_HOSTS to a comma-separated list of trusted hosts (e.g. "cdn.example.com,images.internal") to allow fetching external images.'
24+
);
25+
}
26+
1427
addProcessor('configurationSchema', (schema) => {
1528
merge(schema, {
1629
properties: {

packages/evershop/src/modules/cms/services/imageProcessor.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import path from 'path';
33
import sharp from 'sharp';
44
import { CONSTANTS } from '../../../lib/helpers.js';
55
import { debug } from '../../../lib/log/logger.js';
6+
import { secureFetch } from '../../../lib/util/secureFetch.js';
67

78
const hasTransparency = async (imageBuffer: Buffer): Promise<boolean> => {
89
try {
@@ -203,9 +204,11 @@ export const imageProcessor = async (
203204
// Not in cache, fetch from external URL
204205
}
205206

206-
// Fetch image from external URL
207+
// Fetch image from external URL. `secureFetch` blocks SSRF to internal /
208+
// non-public addresses (loopback, link-local metadata, private ranges,
209+
// DNS rebinding, and redirects to any of those).
207210
try {
208-
const response = await fetch(src);
211+
const response = await secureFetch(src);
209212
if (!response.ok) {
210213
throw new Error(
211214
`Failed to fetch image: ${response.status} ${response.statusText}`

packages/evershop/src/modules/cms/services/tests/unit/imageProcessor.test.js

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,15 @@ await jest.unstable_mockModule('../../../../../lib/helpers.js', () => ({
5454
}
5555
}));
5656

57+
// Mock the secureFetch module so external-URL tests don't make real requests.
58+
// imageProcessor fetches external images through secureFetch (the SSRF guard).
59+
await jest.unstable_mockModule('../../../../../lib/util/secureFetch.js', () => ({
60+
secureFetch: jest.fn(),
61+
assertAllowedUrl: jest.fn(),
62+
getAllowedImageHosts: jest.fn(() => [])
63+
}));
64+
const { secureFetch } = await import('../../../../../lib/util/secureFetch.js');
65+
5766
// Import imageProcessor only after mocking helpers
5867
const { imageProcessor } = await import('../../imageProcessor.js');
5968

@@ -694,8 +703,8 @@ describe('Cache key generation', () => {
694703
});
695704

696705
test('Should handle external URLs in cache key', async () => {
697-
// Mock fetch for external URLs
698-
global.fetch = jest.fn().mockResolvedValue({
706+
// Mock secureFetch for external URLs
707+
secureFetch.mockResolvedValue({
699708
ok: true,
700709
arrayBuffer: () => sharp(testImagePath).toBuffer()
701710
});
@@ -711,8 +720,8 @@ describe('Cache key generation', () => {
711720
const fileExistsResult = await fileExists(result.path);
712721
expect(fileExistsResult).toBe(true);
713722

714-
// Clean up the global mock
715-
delete global.fetch;
723+
// Clean up the mock
724+
secureFetch.mockReset();
716725
});
717726
});
718727

@@ -725,17 +734,17 @@ describe('Error handling', () => {
725734
});
726735

727736
test('Should handle external URL fetch errors gracefully', async () => {
728-
global.fetch = jest.fn().mockRejectedValue(new Error('Network error'));
737+
secureFetch.mockRejectedValue(new Error('Network error'));
729738

730739
await expect(
731740
imageProcessor('https://example.com/image.jpg', 200, 80)
732741
).rejects.toThrow('Image not found or processing failed');
733742

734-
delete global.fetch;
743+
secureFetch.mockReset();
735744
});
736745

737746
test('Should handle unsuccessful HTTP response', async () => {
738-
global.fetch = jest.fn().mockResolvedValue({
747+
secureFetch.mockResolvedValue({
739748
ok: false,
740749
status: 404,
741750
statusText: 'Not Found'
@@ -745,7 +754,7 @@ describe('Error handling', () => {
745754
imageProcessor('https://example.com/image.jpg', 200, 80)
746755
).rejects.toThrow('Image not found or processing failed');
747756

748-
delete global.fetch;
757+
secureFetch.mockReset();
749758
});
750759

751760
test('Should handle invalid URL format', async () => {

packages/evershop/src/modules/customer/api/createCustomerAddress/[bodyParser]createCustomerAddress.js renamed to packages/evershop/src/modules/customer/api/createCustomerAddress/createCustomerAddress.js

File renamed without changes.

0 commit comments

Comments
 (0)