-
Notifications
You must be signed in to change notification settings - Fork 280
fix(harmony): safeguard binary version sync against background-thread resource loading failures #591
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(harmony): safeguard binary version sync against background-thread resource loading failures #591
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,43 @@ | ||||||||||||||||||||||||||||||||||||||||||
| const fs = require("fs"); | ||||||||||||||||||||||||||||||||||||||||||
| const path = require("path"); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| function requireConfigPlugins() { | ||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||
| return require("@expo/config-plugins"); | ||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||
| return require(require.resolve("@expo/config-plugins", { | ||||||||||||||||||||||||||||||||||||||||||
| paths: [process.cwd()], | ||||||||||||||||||||||||||||||||||||||||||
| })); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| const { withDangerousMod } = requireConfigPlugins(); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| const RCT_BRIDGE_IMPORT = "#import <React/RCTBridge.h>"; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| function withPushyRCTBridgeImport(config) { | ||||||||||||||||||||||||||||||||||||||||||
| return withDangerousMod(config, [ | ||||||||||||||||||||||||||||||||||||||||||
| "ios", | ||||||||||||||||||||||||||||||||||||||||||
| (config) => { | ||||||||||||||||||||||||||||||||||||||||||
| const { platformProjectRoot, projectName } = config.modRequest; | ||||||||||||||||||||||||||||||||||||||||||
| const bridgingHeaderPath = path.join( | ||||||||||||||||||||||||||||||||||||||||||
| platformProjectRoot, | ||||||||||||||||||||||||||||||||||||||||||
| projectName, | ||||||||||||||||||||||||||||||||||||||||||
| `${projectName}-Bridging-Header.h` | ||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| const contents = fs.readFileSync(bridgingHeaderPath, "utf8"); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| if (!contents.includes(RCT_BRIDGE_IMPORT)) { | ||||||||||||||||||||||||||||||||||||||||||
| fs.writeFileSync( | ||||||||||||||||||||||||||||||||||||||||||
| bridgingHeaderPath, | ||||||||||||||||||||||||||||||||||||||||||
| `${contents.trimEnd()}\n\n${RCT_BRIDGE_IMPORT}\n` | ||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+29
to
+36
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Guard against a missing bridging header before reading.
🛡️ Proposed guard+ if (!fs.existsSync(bridgingHeaderPath)) {
+ return config;
+ }
+
const contents = fs.readFileSync(bridgingHeaderPath, "utf8");📝 Committable suggestion
Suggested change
🧰 Tools🪛 ast-grep (0.44.0)[warning] 31-34: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use. (detect-non-literal-fs-filename) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| return config; | ||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| module.exports = withPushyRCTBridgeImport; | ||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -26,6 +26,8 @@ export class UpdateContext { | |||||||||||||||||||||||||||
| private static DEBUG: boolean = false; | ||||||||||||||||||||||||||||
| private static isUsingBundleUrl: boolean = false; | ||||||||||||||||||||||||||||
| private static ignoreRollback: boolean = false; | ||||||||||||||||||||||||||||
| private static cachedPackageVersion: string = ''; | ||||||||||||||||||||||||||||
| private static cachedBuildTime: string = ''; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| constructor(context: common.UIAbilityContext) { | ||||||||||||||||||||||||||||
| this.context = context; | ||||||||||||||||||||||||||||
|
|
@@ -60,28 +62,38 @@ export class UpdateContext { | |||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| public getPackageVersion(): string { | ||||||||||||||||||||||||||||
| if (UpdateContext.cachedPackageVersion) { | ||||||||||||||||||||||||||||
| return UpdateContext.cachedPackageVersion; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| const bundleInfo = bundleManager.getBundleInfoForSelfSync( | ||||||||||||||||||||||||||||
| this.getBundleFlags(), | ||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||
| return bundleInfo?.versionName || 'Unknown'; | ||||||||||||||||||||||||||||
| UpdateContext.cachedPackageVersion = bundleInfo?.versionName || 'Unknown'; | ||||||||||||||||||||||||||||
| return UpdateContext.cachedPackageVersion; | ||||||||||||||||||||||||||||
|
Comment on lines
68
to
+73
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Return an empty package version instead of Line 72 turns a failed/empty Proposed fix try {
const bundleInfo = bundleManager.getBundleInfoForSelfSync(
this.getBundleFlags(),
);
- UpdateContext.cachedPackageVersion = bundleInfo?.versionName || 'Unknown';
+ UpdateContext.cachedPackageVersion = bundleInfo?.versionName || '';
return UpdateContext.cachedPackageVersion;
} catch (error) {
console.error('Failed to get bundle info:', error);
return '';
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||
| console.error('Failed to get bundle info:', error); | ||||||||||||||||||||||||||||
| return ''; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| public getBuildTime(): string { | ||||||||||||||||||||||||||||
| if (UpdateContext.cachedBuildTime) { | ||||||||||||||||||||||||||||
| return UpdateContext.cachedBuildTime; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||
| const content = | ||||||||||||||||||||||||||||
| this.context.resourceManager.getRawFileContentSync('meta.json'); | ||||||||||||||||||||||||||||
| const metaData = JSON.parse( | ||||||||||||||||||||||||||||
| new util.TextDecoder().decodeToString(content), | ||||||||||||||||||||||||||||
| ) as Record<string, string | number | boolean | null | undefined>; | ||||||||||||||||||||||||||||
| if (metaData.pushy_build_time) { | ||||||||||||||||||||||||||||
| return String(metaData.pushy_build_time); | ||||||||||||||||||||||||||||
| UpdateContext.cachedBuildTime = String(metaData.pushy_build_time); | ||||||||||||||||||||||||||||
| return UpdateContext.cachedBuildTime; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } catch {} | ||||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||||
| console.error('Failed to read build time from raw file:', error); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| return ''; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
@@ -244,6 +256,9 @@ export class UpdateContext { | |||||||||||||||||||||||||||
| packageVersion: string, | ||||||||||||||||||||||||||||
| buildTime: string, | ||||||||||||||||||||||||||||
| ): void { | ||||||||||||||||||||||||||||
| if (!packageVersion || !buildTime) { | ||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| const currentState = this.getStateSnapshot(); | ||||||||||||||||||||||||||||
| const nextState = NativePatchCore.syncStateWithBinaryVersion( | ||||||||||||||||||||||||||||
| packageVersion, | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,5 +100,4 @@ log('bootup status', { | |
| isFirstTimeDebug, | ||
| isDebugChannel, | ||
| cInfo, | ||
| uuid, | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: reactnativecn/react-native-update
Length of output: 185
🏁 Script executed:
Repository: reactnativecn/react-native-update
Length of output: 3194
🏁 Script executed:
Repository: reactnativecn/react-native-update
Length of output: 50389
🌐 Web query:
Expo config plugin relative path app.plugin.js app.json plugins "./plugins/with-rct-bridge-bridging-header" resolution💡 Result:
In Expo, local config plugins referenced by a relative path in your app.json (e.g., "./plugins/with-rct-bridge-bridging-header") are resolved relative to the project root [1]. When you specify a string path in the plugins array, Expo CLI resolves it using Node's module resolution system [1]. For a local file like "./plugins/with-rct-bridge-bridging-header", Expo will attempt to load the file by appending supported extensions if necessary (such as.js,.ts,.cjs,.mjs, etc., following the latest resolution logic) [2]. Key points regarding this resolution: 1. Local vs. Module Resolution: If the string does not start with a relative path indicator (./ or../) or an absolute path, Expo treats it as an npm package [1]. Using./ ensures it is treated as a local file relative to your project's root directory [1]. 2. Supported Formats: As of May 2026, Expo has aligned config plugin resolution with standard app config resolution, supporting a wider range of formats including.js,.ts,.cjs,.mjs,.cts, and.mts [2]. This allows you to write local plugins in TypeScript or modern JavaScript without requiring manual pre-transpilation in some workflows [2]. 3. Plugin Structure: While npm packages are expected to export the plugin via an app.plugin.js file (or the main field in package.json) [3][4], a local file referenced directly by path is loaded as a module [1]. The exported function from that file will be treated as the config plugin [5][1]. 4. Troubleshooting: If your plugin fails to load, ensure the file exists at the path specified relative to the project root and that it exports the plugin function correctly (usually as the default export or via module.exports) [5][1]. Recent updates have improved error reporting for plugin resolution failures [2].
Citations:
Point the Expo plugin entry at an existing module
Example/expoUsePushy/app.jsonpoints to"./plugins/with-rct-bridge-bridging-header", but there’s no matching module underExample/expoUsePushy/plugins/, and the rootapp.plugin.jswon’t be loaded from that entry. Expo resolves local plugins relative to the project root, so prebuild will fail unless the path is corrected or the plugin file is added.🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 7-9: Avoid require with non-literal values
Context: require(require.resolve("
@expo/config-plugins", {paths: [process.cwd()],
}))
Note: [CWE-829] Inclusion of Functionality from Untrusted Control Sphere (dynamic require).
(detect-non-literal-require)
🤖 Prompt for AI Agents