From 4f008a442a862a94ddae75b3c7900df3f21da292 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Wed, 21 May 2025 13:15:50 -0700 Subject: [PATCH 01/12] Run unit tests under Windows to get some coverage on potential pathing or other issues --- .github/workflows/check.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 88939b6f..bc9a8dbc 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -29,6 +29,18 @@ jobs: - run: npm ci - run: npm run build - run: npm test --workspace gyp-to-cmake --workspace react-native-node-api-cmake --workspace react-native-node-api-modules + test-windows: + name: Run tests on Windows + runs-on: windows-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: lts/jod + - run: npm ci + - run: npm run build + - run: npm test --workspace gyp-to-cmake --workspace react-native-node-api-cmake --workspace react-native-node-api-modules + test-macos: name: Run tests which requires MacOS runs-on: macos-latest From ad9fb3b88fc298c22f17605f4e8ea8283248eac2 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Wed, 21 May 2025 13:30:11 -0700 Subject: [PATCH 02/12] fix win32 incompatibility --- .../react-native-node-api-modules/src/node/path-utils.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/react-native-node-api-modules/src/node/path-utils.ts b/packages/react-native-node-api-modules/src/node/path-utils.ts index 6c99e176..d7582753 100644 --- a/packages/react-native-node-api-modules/src/node/path-utils.ts +++ b/packages/react-native-node-api-modules/src/node/path-utils.ts @@ -247,6 +247,13 @@ export function findNodeApiModulePaths( return []; } const candidatePath = path.join(fromPath, suffix); + // Normalize path separators for consistent pattern matching on all platforms + const normalizedSuffix = suffix.split(path.sep).join('/'); + + if (excludePatterns.some((pattern) => pattern.test(normalizedSuffix))) { + return []; + } + return fs .readdirSync(candidatePath, { withFileTypes: true }) .flatMap((file) => { From 86887281f8dd3a155f488c9db0a8b0d15400a3e7 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Wed, 21 May 2025 13:43:03 -0700 Subject: [PATCH 03/12] permissions on win32 is annoyingly tricky --- .../src/node/path-utils.ts | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/packages/react-native-node-api-modules/src/node/path-utils.ts b/packages/react-native-node-api-modules/src/node/path-utils.ts index d7582753..7d80e35e 100644 --- a/packages/react-native-node-api-modules/src/node/path-utils.ts +++ b/packages/react-native-node-api-modules/src/node/path-utils.ts @@ -45,11 +45,30 @@ export function isNodeApiModule(modulePath: string): boolean { if (!entries.includes(fileName)) { return false; } + + const filePath = path.join(dir, fileName); + try { - fs.accessSync(path.join(dir, fileName), fs.constants.R_OK); - return true; - } catch (cause) { - throw new Error(`Found an unreadable module ${fileName}: ${cause}`); + // First, check if file exists (works the same on all platforms) + fs.accessSync(filePath, fs.constants.F_OK); + + // Then check if it's readable (behavior differs by platform) + if (process.platform === 'win32') { + // On Windows, we need to try to open the file to check read permissions + try { + const fd = fs.openSync(filePath, 'r'); + fs.closeSync(fd); + return true; + } catch (e) { + throw new Error(`Found an unreadable module ${fileName}: ${e}`); + } + } else { + // On Unix-like systems, we can use R_OK to check read permissions + fs.accessSync(filePath, fs.constants.R_OK); + return true; + } + } catch (e) { + throw new Error(`Found an unreadable module ${fileName}: ${e}`); } }); } From 2dcfe92b51219799be2766fb44974dd91f41cce3 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Wed, 21 May 2025 14:01:54 -0700 Subject: [PATCH 04/12] tweaked --- .../src/node/path-utils.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/packages/react-native-node-api-modules/src/node/path-utils.ts b/packages/react-native-node-api-modules/src/node/path-utils.ts index 7d80e35e..c5784c9a 100644 --- a/packages/react-native-node-api-modules/src/node/path-utils.ts +++ b/packages/react-native-node-api-modules/src/node/path-utils.ts @@ -55,13 +55,9 @@ export function isNodeApiModule(modulePath: string): boolean { // Then check if it's readable (behavior differs by platform) if (process.platform === 'win32') { // On Windows, we need to try to open the file to check read permissions - try { - const fd = fs.openSync(filePath, 'r'); - fs.closeSync(fd); - return true; - } catch (e) { - throw new Error(`Found an unreadable module ${fileName}: ${e}`); - } + const fd = fs.openSync(filePath, 'r'); + fs.closeSync(fd); + return true; } else { // On Unix-like systems, we can use R_OK to check read permissions fs.accessSync(filePath, fs.constants.R_OK); From d966cb8104653596bdeb9434294b7e75acc98dbf Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Wed, 21 May 2025 14:56:07 -0700 Subject: [PATCH 05/12] trying lighter weight approach that will work with NTFS ACLs --- .../src/node/path-utils.ts | 31 +++++++++++++------ 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/packages/react-native-node-api-modules/src/node/path-utils.ts b/packages/react-native-node-api-modules/src/node/path-utils.ts index c5784c9a..630072df 100644 --- a/packages/react-native-node-api-modules/src/node/path-utils.ts +++ b/packages/react-native-node-api-modules/src/node/path-utils.ts @@ -53,22 +53,35 @@ export function isNodeApiModule(modulePath: string): boolean { fs.accessSync(filePath, fs.constants.F_OK); // Then check if it's readable (behavior differs by platform) - if (process.platform === 'win32') { - // On Windows, we need to try to open the file to check read permissions - const fd = fs.openSync(filePath, 'r'); - fs.closeSync(fd); - return true; - } else { - // On Unix-like systems, we can use R_OK to check read permissions - fs.accessSync(filePath, fs.constants.R_OK); - return true; + if (!isReadableSync(filePath)) { + throw new Error(`Found an unreadable module ${fileName}`); } } catch (e) { throw new Error(`Found an unreadable module ${fileName}: ${e}`); } + return true; }); } +/** + * Check if a path is readable according to permission bits. + * On Windows, tests store POSIX S_IWUSR bit in stats.mode. + * On Unix-like, uses fs.accessSync for R_OK. + */ +function isReadableSync(p: string): boolean { + try { + if (process.platform === "win32") { + const stats = fs.statSync(p); + return !!(stats.mode & fs.constants.S_IWUSR); + } else { + fs.accessSync(p, fs.constants.R_OK); + return true; + } + } catch { + return false; + } +} + /** * Strip of any platform specific extensions from a module path. */ From bfcb5e8d2feb1a9743842acc552354dcb9586c5c Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Wed, 21 May 2025 15:05:59 -0700 Subject: [PATCH 06/12] another path translation bug --- packages/react-native-node-api-modules/src/node/path-utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-native-node-api-modules/src/node/path-utils.ts b/packages/react-native-node-api-modules/src/node/path-utils.ts index 630072df..b1e5e49c 100644 --- a/packages/react-native-node-api-modules/src/node/path-utils.ts +++ b/packages/react-native-node-api-modules/src/node/path-utils.ts @@ -135,7 +135,7 @@ export function normalizeModulePath(modulePath: string) { const dirname = path.normalize(path.dirname(modulePath)); const basename = path.basename(modulePath); const strippedBasename = stripExtension(basename).replace(/^lib/, ""); - return path.join(dirname, strippedBasename); + return path.join(dirname, strippedBasename).replace(/\\/g, "/"); } export function escapePath(modulePath: string) { From 8eb426d1dc9f882f523e94890175bfb0e061f7be Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Thu, 22 May 2025 15:04:02 -0700 Subject: [PATCH 07/12] settings Windows file/dir permissions in nodejs is not well-worn in the nodejs ecosystem. I'm trying to set the file/dir as offline to simulate it being otherwise inaccessible. --- package-lock.json | 11 +++++ .../package.json | 1 + .../src/node/path-utils.test.ts | 47 ++++++++++++++++--- .../src/node/path-utils.ts | 1 + 4 files changed, 54 insertions(+), 6 deletions(-) diff --git a/package-lock.json b/package-lock.json index 2db6dc1e..968986a1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6043,6 +6043,16 @@ "node": "^8.16.0 || ^10.6.0 || >=11.0.0" } }, + "node_modules/fswin": { + "version": "3.24.829", + "resolved": "https://registry.npmjs.org/fswin/-/fswin-3.24.829.tgz", + "integrity": "sha512-t3KHDNSMHbUzjpzb35c+27dGMLcE5gXvYZ4to5BITvCvPr3dZvX41VUzgEMQ8mVozbn5uiQ9p61/cQVLDEy+ag==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">= 8.0" + } + }, "node_modules/function-bind": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/function-bind/-/function-bind-1.1.2.tgz", @@ -10425,6 +10435,7 @@ "devDependencies": { "@babel/core": "^7.26.10", "@babel/types": "^7.27.0", + "fswin": "^3.24.829", "metro-config": "0.81.1", "node-api-headers": "^1.5.0", "zod": "^3.24.3" diff --git a/packages/react-native-node-api-modules/package.json b/packages/react-native-node-api-modules/package.json index 425ec03c..f6e69214 100644 --- a/packages/react-native-node-api-modules/package.json +++ b/packages/react-native-node-api-modules/package.json @@ -83,6 +83,7 @@ "devDependencies": { "@babel/core": "^7.26.10", "@babel/types": "^7.27.0", + "fswin": "^3.24.829", "metro-config": "0.81.1", "node-api-headers": "^1.5.0", "zod": "^3.24.3" diff --git a/packages/react-native-node-api-modules/src/node/path-utils.test.ts b/packages/react-native-node-api-modules/src/node/path-utils.test.ts index fcd95ad3..eeae3961 100644 --- a/packages/react-native-node-api-modules/src/node/path-utils.test.ts +++ b/packages/react-native-node-api-modules/src/node/path-utils.test.ts @@ -2,6 +2,7 @@ import assert from "node:assert/strict"; import { describe, it } from "node:test"; import path from "node:path"; import fs from "node:fs"; +import fswin from "fswin"; import { determineModuleContext, @@ -13,6 +14,40 @@ import { } from "./path-utils.js"; import { setupTempDirectory } from "./test-utils.js"; +function removeReadPermissions(p: string) { + if (process.platform === "win32") { + // Windows: simulate unreadable by setting file to offline + const attributes = { + IS_READ_ONLY: true, + IS_OFFLINE: true, + IS_TEMPORARY: true, + }; + + const result = fswin.setAttributesSync(p, attributes); + if (!result) console.error('!!!!! can not set attributes'); + } else { + // Unix-like: clear all perms + fs.chmodSync(p, 0); + } +} + +function restoreReadPermissions(p: string) { + if (process.platform === "win32") { + // Windows: simulate unreadable by setting file to offline + const attributes = { + IS_READ_ONLY: false, + IS_OFFLINE: false, + IS_TEMPORARY: false, + }; + + const result = fswin.setAttributesSync(p, attributes); + if (!result) console.error('!!!!! can not set attributes'); + } else { + // Unix-like: clear all perms + fs.chmodSync(p, 0o700); + } +} + describe("isNodeApiModule", () => { it("returns true for .node", (context) => { const tempDirectoryPath = setupTempDirectory(context, { @@ -28,14 +63,14 @@ describe("isNodeApiModule", () => { "addon.android.node": "", }); // remove read permissions on directory - fs.chmodSync(tempDirectoryPath, 0); + removeReadPermissions(tempDirectoryPath); try { assert.equal( isNodeApiModule(path.join(tempDirectoryPath, "addon")), false ); } finally { - fs.chmodSync(tempDirectoryPath, 0o700); + restoreReadPermissions(tempDirectoryPath); } }); @@ -45,14 +80,14 @@ describe("isNodeApiModule", () => { }); const candidate = path.join(tempDirectoryPath, "addon.android.node"); // remove read permission on file - fs.chmodSync(candidate, 0); + removeReadPermissions(candidate); try { assert.throws( () => isNodeApiModule(path.join(tempDirectoryPath, "addon")), /Found an unreadable module addon\.android\.node/ ); } finally { - fs.chmodSync(candidate, 0o600); + restoreReadPermissions(candidate); } }); @@ -81,12 +116,12 @@ describe("isNodeApiModule", () => { }); const unreadable = path.join(tempDirectoryPath, "addon.android.node"); // only android module is unreadable - fs.chmodSync(unreadable, 0); + removeReadPermissions(unreadable); assert.throws( () => isNodeApiModule(path.join(tempDirectoryPath, "addon")), /Found an unreadable module addon\.android\.node/ ); - fs.chmodSync(unreadable, 0o600); + restoreReadPermissions(unreadable); }); }); diff --git a/packages/react-native-node-api-modules/src/node/path-utils.ts b/packages/react-native-node-api-modules/src/node/path-utils.ts index b1e5e49c..1962efde 100644 --- a/packages/react-native-node-api-modules/src/node/path-utils.ts +++ b/packages/react-native-node-api-modules/src/node/path-utils.ts @@ -135,6 +135,7 @@ export function normalizeModulePath(modulePath: string) { const dirname = path.normalize(path.dirname(modulePath)); const basename = path.basename(modulePath); const strippedBasename = stripExtension(basename).replace(/^lib/, ""); + // Replace backslashes with forward slashes for cross-platform compatibility return path.join(dirname, strippedBasename).replace(/\\/g, "/"); } From 1f89e282d6261300b924f324a94da4190ae13e99 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Thu, 22 May 2025 17:22:24 -0700 Subject: [PATCH 08/12] I've tried many, many ways of doing this without shelling out to powershell in some brittle way, and it just doesn't appear feasible for the unreadable directory case. The other unreadable file case is covered. --- .../src/node/path-utils.test.ts | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/packages/react-native-node-api-modules/src/node/path-utils.test.ts b/packages/react-native-node-api-modules/src/node/path-utils.test.ts index eeae3961..b919558d 100644 --- a/packages/react-native-node-api-modules/src/node/path-utils.test.ts +++ b/packages/react-native-node-api-modules/src/node/path-utils.test.ts @@ -15,37 +15,38 @@ import { import { setupTempDirectory } from "./test-utils.js"; function removeReadPermissions(p: string) { - if (process.platform === "win32") { - // Windows: simulate unreadable by setting file to offline - const attributes = { - IS_READ_ONLY: true, - IS_OFFLINE: true, - IS_TEMPORARY: true, - }; - - const result = fswin.setAttributesSync(p, attributes); - if (!result) console.error('!!!!! can not set attributes'); - } else { + if (process.platform !== "win32") { // Unix-like: clear all perms fs.chmodSync(p, 0); + return; } + + // Windows: simulate unreadable by setting file to offline + const attributes = { + IS_READ_ONLY: true, + IS_OFFLINE: true, + IS_TEMPORARY: true, + }; + + const result = fswin.setAttributesSync(p, attributes); + if (!result) console.error('!!!!! can not set attributes to remove read permissions'); } function restoreReadPermissions(p: string) { - if (process.platform === "win32") { - // Windows: simulate unreadable by setting file to offline - const attributes = { - IS_READ_ONLY: false, - IS_OFFLINE: false, - IS_TEMPORARY: false, - }; - - const result = fswin.setAttributesSync(p, attributes); - if (!result) console.error('!!!!! can not set attributes'); - } else { + if (process.platform !== "win32") { // Unix-like: clear all perms fs.chmodSync(p, 0o700); + return; } + + const attributes = { + IS_READ_ONLY: false, + IS_OFFLINE: false, + IS_TEMPORARY: false, + }; + + const result = fswin.setAttributesSync(p, attributes); + if (!result) console.error('!!!!! can not set attributes to restore read permissions'); } describe("isNodeApiModule", () => { @@ -58,11 +59,11 @@ describe("isNodeApiModule", () => { assert(isNodeApiModule(path.join(tempDirectoryPath, "addon.node"))); }); - it("returns false when directory cannot be read due to permissions", (context) => { + // there is no way to set ACLs on directories in Node.js on Windows with brittle powershell commands + (process.platform === "win32" ? it.skip : it)("returns false when directory cannot be read due to permissions", (context) => { const tempDirectoryPath = setupTempDirectory(context, { "addon.android.node": "", }); - // remove read permissions on directory removeReadPermissions(tempDirectoryPath); try { assert.equal( @@ -79,7 +80,6 @@ describe("isNodeApiModule", () => { "addon.android.node": "", }); const candidate = path.join(tempDirectoryPath, "addon.android.node"); - // remove read permission on file removeReadPermissions(candidate); try { assert.throws( From 819c4034f6616e243164435a68f440cc217378ff Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Thu, 22 May 2025 22:21:37 -0700 Subject: [PATCH 09/12] throw if we can't set file attributes so the test fails faster and clearer --- .../react-native-node-api-modules/src/node/path-utils.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-native-node-api-modules/src/node/path-utils.test.ts b/packages/react-native-node-api-modules/src/node/path-utils.test.ts index b919558d..4e374080 100644 --- a/packages/react-native-node-api-modules/src/node/path-utils.test.ts +++ b/packages/react-native-node-api-modules/src/node/path-utils.test.ts @@ -29,7 +29,7 @@ function removeReadPermissions(p: string) { }; const result = fswin.setAttributesSync(p, attributes); - if (!result) console.error('!!!!! can not set attributes to remove read permissions'); + if (!result) throw new Error("can not set attributes to remove read permissions"); } function restoreReadPermissions(p: string) { @@ -46,7 +46,7 @@ function restoreReadPermissions(p: string) { }; const result = fswin.setAttributesSync(p, attributes); - if (!result) console.error('!!!!! can not set attributes to restore read permissions'); + if (!result) throw new Error("can not set attributes to restore read permissions"); } describe("isNodeApiModule", () => { From 6af082f351782b48d211aba4851143dfc006a9b9 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Tue, 27 May 2025 00:51:22 -0700 Subject: [PATCH 10/12] Update packages/react-native-node-api-modules/src/node/path-utils.test.ts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kræn Hansen --- .../react-native-node-api-modules/src/node/path-utils.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-native-node-api-modules/src/node/path-utils.test.ts b/packages/react-native-node-api-modules/src/node/path-utils.test.ts index 4e374080..08a788ff 100644 --- a/packages/react-native-node-api-modules/src/node/path-utils.test.ts +++ b/packages/react-native-node-api-modules/src/node/path-utils.test.ts @@ -60,7 +60,7 @@ describe("isNodeApiModule", () => { }); // there is no way to set ACLs on directories in Node.js on Windows with brittle powershell commands - (process.platform === "win32" ? it.skip : it)("returns false when directory cannot be read due to permissions", (context) => { + it("returns false when directory cannot be read due to permissions", { skip: process.platform === "win32" }, (context) => { const tempDirectoryPath = setupTempDirectory(context, { "addon.android.node": "", }); From 54bb9ab1f2e1f74734dcd87dab5c9c7535c96593 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Tue, 27 May 2025 00:51:36 -0700 Subject: [PATCH 11/12] Update packages/react-native-node-api-modules/src/node/path-utils.ts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kræn Hansen --- packages/react-native-node-api-modules/src/node/path-utils.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-native-node-api-modules/src/node/path-utils.ts b/packages/react-native-node-api-modules/src/node/path-utils.ts index 1962efde..26b808f7 100644 --- a/packages/react-native-node-api-modules/src/node/path-utils.ts +++ b/packages/react-native-node-api-modules/src/node/path-utils.ts @@ -56,8 +56,8 @@ export function isNodeApiModule(modulePath: string): boolean { if (!isReadableSync(filePath)) { throw new Error(`Found an unreadable module ${fileName}`); } - } catch (e) { - throw new Error(`Found an unreadable module ${fileName}: ${e}`); + } catch (err) { + throw new Error(`Found an unreadable module ${fileName}`, { cause: err }); } return true; }); From 935855905ce54bf58301a1e628e2bdc2ba46df72 Mon Sep 17 00:00:00 2001 From: Matt Hargett Date: Tue, 27 May 2025 00:51:46 -0700 Subject: [PATCH 12/12] Update packages/react-native-node-api-modules/src/node/path-utils.ts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kræn Hansen --- packages/react-native-node-api-modules/src/node/path-utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-native-node-api-modules/src/node/path-utils.ts b/packages/react-native-node-api-modules/src/node/path-utils.ts index 26b808f7..e09b31ac 100644 --- a/packages/react-native-node-api-modules/src/node/path-utils.ts +++ b/packages/react-native-node-api-modules/src/node/path-utils.ts @@ -136,7 +136,7 @@ export function normalizeModulePath(modulePath: string) { const basename = path.basename(modulePath); const strippedBasename = stripExtension(basename).replace(/^lib/, ""); // Replace backslashes with forward slashes for cross-platform compatibility - return path.join(dirname, strippedBasename).replace(/\\/g, "/"); + return path.join(dirname, strippedBasename).replaceAll("\\", "/"); } export function escapePath(modulePath: string) {