Skip to content

Commit e5e0263

Browse files
sunnylqmclaude
andcommitted
fix: land external review findings — hedged endpoints, murmur3, path guards, singleton client
- endpoint: replace the Promise.all fallback round (which waited for the slowest endpoint and blasted every candidate at once) with a hedged race: preferred endpoint first, 250ms stagger, first success wins, losers are cancelled via AbortController; fetchWithTimeout now chains a caller signal - isInRollout: restore the missing k1 assignment in murmurhash3_32_gc; output now matches the canonical murmur3 vectors (rollout buckets reshuffle once) - provider: only __rnPushyVersionHash wraps options.logger, other __rnPushy* payloads no longer hijack it permanently - iOS: exclude Application Support/rctpushy from iCloud backup (NSURLIsExcludedFromBackupKey), including directories created by older versions - iOS/Android/Harmony: server-provided hash/originHash/fileName are validated as safe single path components before touching the filesystem - singleton client: a second different client (appKey/type) or a concurrently mounted UpdateProvider throws SINGLETON_VIOLATION; identical re-creation is idempotent for fast refresh; setOptions bumps optionsVersion and notifies onOptionsChange so runtime option changes reach provider effects Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent ada96c1 commit e5e0263

15 files changed

Lines changed: 562 additions & 94 deletions

File tree

android/src/main/java/cn/reactnative/modules/update/UpdateContext.java

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,12 +211,36 @@ private void enqueue(DownloadTaskParams params) {
211211
executor.execute(new DownloadTask(context, params));
212212
}
213213

214+
// Server-provided identifiers (hash/originHash/fileName) become child
215+
// names under rootDir; anything that could resolve outside of it (path
216+
// separators, "..", ".") must be rejected before touching the filesystem.
217+
static boolean isSafePathComponent(String name) {
218+
return name != null
219+
&& !name.isEmpty()
220+
&& !name.equals(".")
221+
&& !name.equals("..")
222+
&& !name.contains("/")
223+
&& !name.contains("\\")
224+
&& name.indexOf('\0') < 0;
225+
}
226+
227+
private static boolean rejectUnsafeComponent(String name, DownloadFileListener listener) {
228+
if (isSafePathComponent(name)) {
229+
return false;
230+
}
231+
listener.onDownloadFailed(new IllegalArgumentException("Invalid path component: " + name));
232+
return true;
233+
}
234+
214235
public interface DownloadFileListener {
215236
void onDownloadCompleted(DownloadTaskParams params);
216237
void onDownloadFailed(Throwable error);
217238
}
218239

219240
public void downloadFullUpdate(String url, String hash, DownloadFileListener listener) {
241+
if (rejectUnsafeComponent(hash, listener)) {
242+
return;
243+
}
220244
DownloadTaskParams params = new DownloadTaskParams();
221245
params.type = DownloadTaskParams.TASK_TYPE_PATCH_FULL;
222246
params.url = url;
@@ -228,6 +252,9 @@ public void downloadFullUpdate(String url, String hash, DownloadFileListener lis
228252
}
229253

230254
public void downloadFile(String url, String hash, String fileName, DownloadFileListener listener) {
255+
if (rejectUnsafeComponent(fileName, listener)) {
256+
return;
257+
}
231258
DownloadTaskParams params = new DownloadTaskParams();
232259
params.type = DownloadTaskParams.TASK_TYPE_PLAIN_DOWNLOAD;
233260
params.url = url;
@@ -246,6 +273,9 @@ public void downloadFile(String url, String hash, String fileName, DownloadFileL
246273
}
247274

248275
public void downloadPatchFromApk(String url, String hash, DownloadFileListener listener) {
276+
if (rejectUnsafeComponent(hash, listener)) {
277+
return;
278+
}
249279
DownloadTaskParams params = new DownloadTaskParams();
250280
params.type = DownloadTaskParams.TASK_TYPE_PATCH_FROM_APK;
251281
params.url = url;
@@ -257,6 +287,9 @@ public void downloadPatchFromApk(String url, String hash, DownloadFileListener l
257287
}
258288

259289
public void downloadPatchFromPpk(String url, String hash, String originHash, DownloadFileListener listener) {
290+
if (rejectUnsafeComponent(hash, listener) || rejectUnsafeComponent(originHash, listener)) {
291+
return;
292+
}
260293
DownloadTaskParams params = new DownloadTaskParams();
261294
params.type = DownloadTaskParams.TASK_TYPE_PATCH_FROM_PPK;
262295
params.url = url;
@@ -312,6 +345,9 @@ private void persistEditor(SharedPreferences.Editor editor, String reason) {
312345
}
313346

314347
public void switchVersion(String hash) {
348+
if (!isSafePathComponent(hash)) {
349+
throw new IllegalArgumentException("Invalid hash: " + hash);
350+
}
315351
if (!new File(rootDir, hash+"/index.bundlejs").exists()) {
316352
throw new IllegalStateException("Bundle version " + hash + " not found.");
317353
}

harmony/pushy/src/main/ets/UpdateContext.ts

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,27 @@ type FlushablePreferences = preferences.Preferences & {
2020
flushSync?: () => void;
2121
};
2222

23+
// 服务端下发的 hash/originHash/fileName 会拼进 rootDir 作为子路径;凡是可能
24+
// 逃出 rootDir 的值(路径分隔符、".."、".")必须在触碰文件系统前拒绝。
25+
export function isSafePathComponent(name: string): boolean {
26+
return (
27+
typeof name === 'string' &&
28+
name.length > 0 &&
29+
name !== '.' &&
30+
name !== '..' &&
31+
!name.includes('/') &&
32+
!name.includes('\\') &&
33+
!name.includes('\0')
34+
);
35+
}
36+
37+
function assertSafePathComponent(name: string): string {
38+
if (!isSafePathComponent(name)) {
39+
throw Error(`Invalid path component: ${name}`);
40+
}
41+
return name;
42+
}
43+
2344
export class UpdateContext {
2445
private context: common.UIAbilityContext;
2546
private rootDir: string;
@@ -296,7 +317,7 @@ export class UpdateContext {
296317
const params = new DownloadTaskParams();
297318
params.type = type;
298319
params.url = url;
299-
params.hash = hash;
320+
params.hash = assertSafePathComponent(hash);
300321
return params;
301322
}
302323

@@ -474,7 +495,7 @@ export class UpdateContext {
474495
url,
475496
hash,
476497
);
477-
params.targetFile = this.rootDir + '/' + fileName;
498+
params.targetFile = this.rootDir + '/' + assertSafePathComponent(fileName);
478499
await this.executeTask(params);
479500
}
480501

@@ -488,7 +509,7 @@ export class UpdateContext {
488509
url,
489510
hash,
490511
);
491-
params.originHash = originHash;
512+
params.originHash = assertSafePathComponent(originHash);
492513
params.targetFile = `${this.rootDir}/${originHash}_${hash}.ppk.patch`;
493514
params.unzipDirectory = `${this.rootDir}/${hash}`;
494515
params.originDirectory = `${this.rootDir}/${params.originHash}`;
@@ -516,7 +537,7 @@ export class UpdateContext {
516537

517538
public switchVersion(hash: string): void {
518539
try {
519-
const bundlePath = this.getBundlePath(hash);
540+
const bundlePath = this.getBundlePath(assertSafePathComponent(hash));
520541
if (!fileIo.accessSync(bundlePath)) {
521542
throw Error(`Bundle version ${hash} not found.`);
522543
}

harmony/pushy/src/test/Validation.test.ets

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,28 @@
11
import { describe, it, expect } from '@ohos/hypium';
22
import { validateHashInfo, getErrorMessage } from '../main/ets/PushyTurboModule';
3+
import { isSafePathComponent } from '../main/ets/UpdateContext';
34

45
export default function validationTest() {
6+
describe('isSafePathComponent', () => {
7+
it('accepts a plain hash-like name', 0, () => {
8+
expect(isSafePathComponent('a1b2c3d4')).assertEqual(true);
9+
expect(isSafePathComponent('origin_hash-1.ppk')).assertEqual(true);
10+
});
11+
12+
it('rejects empty and dot names', 0, () => {
13+
expect(isSafePathComponent('')).assertEqual(false);
14+
expect(isSafePathComponent('.')).assertEqual(false);
15+
expect(isSafePathComponent('..')).assertEqual(false);
16+
});
17+
18+
it('rejects names containing path separators', 0, () => {
19+
expect(isSafePathComponent('../etc')).assertEqual(false);
20+
expect(isSafePathComponent('a/b')).assertEqual(false);
21+
expect(isSafePathComponent('a\\b')).assertEqual(false);
22+
expect(isSafePathComponent('a\0b')).assertEqual(false);
23+
});
24+
});
25+
526
describe('validateHashInfo', () => {
627
it('accepts valid JSON object', 0, () => {
728
let threw = false;

ios/RCTPushy/RCTPushy.mm

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,25 @@ static BOOL PushyStringIsBlank(NSString *value) {
139139
return [[value stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]] length] == 0;
140140
}
141141

142+
// Server-provided identifiers (hash/originHash) are used as child names under
143+
// the download root; anything that could resolve outside of it (path
144+
// separators, "..", ".") must be rejected before touching the filesystem.
145+
static BOOL PushyIsSafePathComponent(NSString *value) {
146+
if (PushyStringIsBlank(value)) {
147+
return NO;
148+
}
149+
if ([value isEqualToString:@"."] || [value isEqualToString:@".."]) {
150+
return NO;
151+
}
152+
if ([value containsString:@"/"] || [value containsString:@"\\"]) {
153+
return NO;
154+
}
155+
if ([value rangeOfString:@"\0"].location != NSNotFound) {
156+
return NO;
157+
}
158+
return YES;
159+
}
160+
142161
static void PushyRejectError(RCTPromiseRejectBlock reject, NSError *error) {
143162
// Prefer the stable cross-platform code (error_codes.h); fall back to the
144163
// numeric NSError code for system errors that were not classified.
@@ -248,6 +267,7 @@ - (void)applyPatchForHash:(NSString *)hash
248267
callback:(void (^)(NSError *error))callback;
249268
- (BOOL)switchVersion:(NSString *)hash error:(NSError **)error;
250269
- (BOOL)ensureDirectoryExistsAtPath:(NSString *)path;
270+
+ (void)excludeFromBackup:(NSString *)path;
251271
- (void)unzipFileAtPath:(NSString *)path
252272
toDestination:(NSString *)destination
253273
completionHandler:(void (^)(NSError *error))completionHandler;
@@ -694,12 +714,12 @@ - (void)performUpdate:(PushyType)type options:(NSDictionary *)options callback:(
694714
NSString *updateUrl = PushyOptionString(options, @"updateUrl");
695715
NSString *hash = PushyOptionString(options, @"hash");
696716

697-
if (PushyStringIsBlank(updateUrl) || PushyStringIsBlank(hash)) {
717+
if (PushyStringIsBlank(updateUrl) || !PushyIsSafePathComponent(hash)) {
698718
callback(PushyErrorWithCode(pushy::error_codes::kInvalidOptions, ERROR_OPTIONS));
699719
return;
700720
}
701721
NSString *originHash = PushyOptionString(options, @"originHash");
702-
if (type == PushyTypePatchFromPpk && PushyStringIsBlank(originHash)) {
722+
if (type == PushyTypePatchFromPpk && !PushyIsSafePathComponent(originHash)) {
703723
callback(PushyErrorWithCode(pushy::error_codes::kInvalidOptions, ERROR_OPTIONS));
704724
return;
705725
}
@@ -893,7 +913,7 @@ - (void)applyPatchForHash:(NSString *)hash
893913

894914
- (BOOL)switchVersion:(NSString *)hash error:(NSError **)error
895915
{
896-
if (PushyStringIsBlank(hash)) {
916+
if (!PushyIsSafePathComponent(hash)) {
897917
if (error != NULL) {
898918
*error = PushyErrorWithCode(pushy::error_codes::kInvalidOptions, ERROR_OPTIONS);
899919
}
@@ -922,6 +942,10 @@ - (BOOL)ensureDirectoryExistsAtPath:(NSString *)path
922942
NSFileManager *fileManager = [NSFileManager defaultManager];
923943
BOOL isDirectory = NO;
924944
if ([fileManager fileExistsAtPath:path isDirectory:&isDirectory]) {
945+
if (isDirectory) {
946+
// Directories created by older versions never got the flag.
947+
[RCTPushy excludeFromBackup:path];
948+
}
925949
return isDirectory;
926950
}
927951

@@ -933,10 +957,27 @@ - (BOOL)ensureDirectoryExistsAtPath:(NSString *)path
933957
if (!success && error != nil) {
934958
RCTLogWarn(@"Pushy create directory error: %@", error.localizedDescription);
935959
}
960+
if (success) {
961+
[RCTPushy excludeFromBackup:path];
962+
}
936963

937964
return success;
938965
}
939966

967+
// Everything under rctpushy is re-downloadable, and Application Support is
968+
// backed up to iCloud by default — Apple requires such content to be
969+
// excluded from backups.
970+
+ (void)excludeFromBackup:(NSString *)path
971+
{
972+
NSURL *url = [NSURL fileURLWithPath:path isDirectory:YES];
973+
NSError *error = nil;
974+
if (![url setResourceValue:@YES
975+
forKey:NSURLIsExcludedFromBackupKey
976+
error:&error]) {
977+
RCTLogWarn(@"Pushy exclude from backup error: %@", error.localizedDescription);
978+
}
979+
}
980+
940981
- (void)unzipFileAtPath:(NSString *)path
941982
toDestination:(NSString *)destination
942983
completionHandler:(void (^)(NSError *error))completionHandler

src/__tests__/client.test.ts

Lines changed: 74 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1276,21 +1276,84 @@ describe('downloadAndInstallApk apkStatus (JS-3)', () => {
12761276
});
12771277
});
12781278

1279-
describe('options isolation across instances', () => {
1280-
test('mutating options on one instance does not affect another instance', async () => {
1279+
describe('client singleton', () => {
1280+
test('a second client with a different appKey throws SINGLETON_VIOLATION', async () => {
12811281
setupClientMocks();
1282-
const { Pushy } = await importFreshClient('options-isolation');
1283-
const client1 = new Pushy({ appKey: 'app-1', debug: true });
1284-
const client2 = new Pushy({ appKey: 'app-2', debug: false });
1282+
const { Pushy } = await importFreshClient('singleton-different-appkey');
1283+
const client1 = new Pushy({ appKey: 'app-1' });
1284+
expect(client1.options.appKey).toBe('app-1');
12851285

1286-
client1.setOptions({ updateStrategy: 'alwaysAlert' });
1286+
let error: any;
1287+
try {
1288+
new Pushy({ appKey: 'app-2' });
1289+
} catch (e) {
1290+
error = e;
1291+
}
1292+
expect(error?.code).toBe('SINGLETON_VIOLATION');
1293+
});
12871294

1288-
expect(client1.options.appKey).toBe('app-1');
1295+
test('a Cresc client after a Pushy client throws SINGLETON_VIOLATION', async () => {
1296+
setupClientMocks();
1297+
const { Pushy, Cresc } = await importFreshClient('singleton-cross-type');
1298+
new Pushy({ appKey: 'app-1' });
1299+
1300+
let error: any;
1301+
try {
1302+
new Cresc({ appKey: 'app-1' });
1303+
} catch (e) {
1304+
error = e;
1305+
}
1306+
expect(error?.code).toBe('SINGLETON_VIOLATION');
1307+
});
1308+
1309+
test('re-creating the same client is idempotent and applies the new options', async () => {
1310+
setupClientMocks();
1311+
const { Pushy } = await importFreshClient('singleton-idempotent');
1312+
const client1 = new Pushy({ appKey: 'app-1', debug: true });
1313+
// e.g. dev fast refresh re-running the module that builds the client
1314+
const client2 = new Pushy({
1315+
appKey: 'app-1',
1316+
updateStrategy: 'silentAndNow',
1317+
});
1318+
1319+
expect(client2).toBe(client1);
12891320
expect(client1.options.debug).toBe(true);
1290-
expect(client1.options.updateStrategy).toBe('alwaysAlert');
1321+
expect(client1.options.updateStrategy).toBe('silentAndNow');
1322+
});
1323+
1324+
test('setOptions bumps optionsVersion and notifies subscribers', async () => {
1325+
setupClientMocks();
1326+
const { Pushy } = await importFreshClient('singleton-options-version');
1327+
const client = new Pushy({ appKey: 'app-1' });
1328+
const initialVersion = client.optionsVersion;
1329+
const listener = mock(() => {});
1330+
const unsubscribe = client.onOptionsChange(listener);
12911331

1292-
expect(client2.options.appKey).toBe('app-2');
1293-
expect(client2.options.debug).toBe(false);
1294-
expect(client2.options.updateStrategy).not.toBe('alwaysAlert');
1332+
client.setOptions({ updateStrategy: 'alwaysAlert' });
1333+
expect(client.optionsVersion).toBe(initialVersion + 1);
1334+
expect(listener).toHaveBeenCalledTimes(1);
1335+
1336+
unsubscribe();
1337+
client.setOptions({ updateStrategy: 'silentAndNow' });
1338+
expect(listener).toHaveBeenCalledTimes(1);
1339+
});
1340+
1341+
test('a second concurrent provider mount claim throws SINGLETON_VIOLATION', async () => {
1342+
setupClientMocks();
1343+
const { Pushy } = await importFreshClient('singleton-provider-claim');
1344+
const client = new Pushy({ appKey: 'app-1' });
1345+
1346+
const release = client.claimProviderMount();
1347+
let error: any;
1348+
try {
1349+
client.claimProviderMount();
1350+
} catch (e) {
1351+
error = e;
1352+
}
1353+
expect(error?.code).toBe('SINGLETON_VIOLATION');
1354+
1355+
// After unmount the claim is released and a new provider may mount.
1356+
release();
1357+
expect(() => client.claimProviderMount()).not.toThrow();
12951358
});
12961359
});

0 commit comments

Comments
 (0)