Skip to content

Commit 3bb3d54

Browse files
committed
Simplify redirect whitelist validation per PR review
- Remove isValidWildcardPattern function (provided false sense of security) - Trust whitelist as configured, only reject literal "*" - Add default whitelist value (*.kbase.us) for local development - Clarify .env header comment about config.json overwriting values
1 parent d9766dc commit 3bb3d54

3 files changed

Lines changed: 17 additions & 44 deletions

File tree

.env

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
# Includes env defaults for local development,
2-
# for deploy enviroment configurations, see config.json
1+
# Defaults for local development only.
2+
# These values are overwritten by config.json when deployed.
33

44
# Base URL path for the enviroment
55
PUBLIC_URL = '/dev/'
@@ -16,7 +16,7 @@ REACT_APP_KBASE_BACKUP_COOKIE_NAME = 'test_kbase_backup_session'
1616
REACT_APP_KBASE_BACKUP_COOKIE_DOMAIN = 'localhost'
1717

1818
# Comma-separated list of allowed external redirect domains (supports wildcards like *.berdl.kbase.us)
19-
REACT_APP_REDIRECT_WHITELIST = ''
19+
REACT_APP_REDIRECT_WHITELIST = '*.kbase.us'
2020

2121
EXTEND_ESLINT=true
2222
SKIP_PREFLIGHT_CHECK=true

src/common/utils/redirectValidation.test.ts

Lines changed: 7 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import {
22
isExternalUrl,
3-
isValidWildcardPattern,
43
matchesWildcard,
54
getRedirectWhitelist,
65
isWhitelistedExternalUrl,
@@ -33,25 +32,6 @@ describe('isExternalUrl', () => {
3332
});
3433
});
3534

36-
describe('isValidWildcardPattern', () => {
37-
test('accepts exact domains', () => {
38-
expect(isValidWildcardPattern('hub.berdl.kbase.us')).toBe(true);
39-
expect(isValidWildcardPattern('example.com')).toBe(true);
40-
});
41-
42-
test('accepts wildcards with 2+ domain parts', () => {
43-
expect(isValidWildcardPattern('*.berdl.kbase.us')).toBe(true);
44-
expect(isValidWildcardPattern('*.kbase.us')).toBe(true);
45-
expect(isValidWildcardPattern('*.example.com')).toBe(true);
46-
});
47-
48-
test('rejects TLD-only wildcards', () => {
49-
expect(isValidWildcardPattern('*.com')).toBe(false);
50-
expect(isValidWildcardPattern('*.us')).toBe(false);
51-
expect(isValidWildcardPattern('*.org')).toBe(false);
52-
});
53-
});
54-
5535
describe('matchesWildcard', () => {
5636
test('matches exact domains', () => {
5737
expect(matchesWildcard('example.com', 'example.com')).toBe(true);
@@ -174,9 +154,14 @@ describe('isWhitelistedExternalUrl', () => {
174154
expect(isWhitelistedExternalUrl('')).toBe(false);
175155
});
176156

177-
test('returns false when pattern is too broad', () => {
157+
test('returns false when whitelist contains literal asterisk', () => {
158+
process.env.REACT_APP_REDIRECT_WHITELIST = '*';
159+
expect(isWhitelistedExternalUrl('https://anything.com')).toBe(false);
160+
});
161+
162+
test('allows broad wildcards like *.com when explicitly configured', () => {
178163
process.env.REACT_APP_REDIRECT_WHITELIST = '*.com';
179-
expect(isWhitelistedExternalUrl('https://evil.com')).toBe(false);
164+
expect(isWhitelistedExternalUrl('https://anything.com')).toBe(true);
180165
});
181166

182167
test('works with multiple whitelist entries', () => {

src/common/utils/redirectValidation.ts

Lines changed: 7 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,7 @@
22
* Utilities for validating external redirect URLs.
33
*
44
* External URLs must be HTTPS and match a whitelisted domain pattern.
5-
* Wildcards like *.berdl.kbase.us are supported, but TLD-only
6-
* wildcards (*.com, *.us) are rejected for security.
5+
* Wildcards like *.berdl.kbase.us are supported. Literal "*" is rejected.
76
*/
87

98
export const getRedirectWhitelist = (): string[] => {
@@ -14,18 +13,6 @@ export const getRedirectWhitelist = (): string[] => {
1413
.filter(Boolean);
1514
};
1615

17-
/**
18-
* Validates that a wildcard pattern is not too broad.
19-
* Rejects patterns like *.com or *.co.uk that would match any domain.
20-
* Requires at least 2 domain parts after the wildcard.
21-
*/
22-
export const isValidWildcardPattern = (pattern: string): boolean => {
23-
if (!pattern.startsWith('*.')) return true; // exact domain, always valid
24-
const withoutWildcard = pattern.slice(2);
25-
const parts = withoutWildcard.split('.');
26-
return parts.length >= 2; // e.g., *.berdl.kbase.us has 3 parts
27-
};
28-
2916
/**
3017
* Checks if a hostname matches a domain pattern.
3118
* Supports wildcards: *.berdl.kbase.us matches hub.berdl.kbase.us
@@ -42,7 +29,7 @@ export const matchesWildcard = (hostname: string, pattern: string): boolean => {
4229
* Validates if a URL is whitelisted for external redirect.
4330
* - Must be HTTPS
4431
* - Must match a pattern in the whitelist
45-
* - Wildcard patterns must not be too broad
32+
* - Rejects if whitelist contains literal "*"
4633
*/
4734
export const isWhitelistedExternalUrl = (url: string): boolean => {
4835
try {
@@ -52,10 +39,11 @@ export const isWhitelistedExternalUrl = (url: string): boolean => {
5239
if (parsed.protocol !== 'https:') return false;
5340

5441
const whitelist = getRedirectWhitelist();
55-
return whitelist.some(
56-
(pattern) =>
57-
isValidWildcardPattern(pattern) &&
58-
matchesWildcard(parsed.hostname, pattern)
42+
43+
if (whitelist.includes('*')) return false;
44+
45+
return whitelist.some((pattern) =>
46+
matchesWildcard(parsed.hostname, pattern)
5947
);
6048
} catch {
6149
return false;

0 commit comments

Comments
 (0)