HarperFast / HarperFast/harper
Application loader security checks (allowedDirectory, allowedBuiltInModules) are skipped on the CommonJS require path
- Dominant language
- JavaScript
- Stars
- 89
- Forks
- 10
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 200
Description
`cjsRequire` in `security/jsLoader.ts:379-396` is the `require` handed to CommonJS component modules under the VM loaders:
```js
const cjsRequire = (spec) => {
const resolvedUrl = resolveModule(spec, url);
if (resolvedUrl === 'harper') return getHarperExports(scope);
if (resolvedUrl.startsWith('file://')) {
if (resolvedUrl.endsWith('.node')) {
checkAllowedModulePath(resolvedUrl, scope.allowedPath); // <-- only here
...
}
const contents = readFileSync(new URL(resolvedUrl)); // <-- no check
return loadCJS(resolvedUrl, contents.toString('utf-8')).exports;
}
return require(resolvedUrl); // <-- Node's real require
};
```
`checkAllowedModulePath` is called only for `.node` native addons. Two consequences:
1. **`allowedDirectory` is not enforced for CommonJS file requires.** A `file://` spec that is not a `.node` addon goes straight to `readFileSync` + `loadCJS`, so `require('../../somewhere/else.js')` loads regardless of `applications.allowedDirectory: app`.
2. **`allowedBuiltInModules` is not enforced for CommonJS.** A bare specifier falls through to Node's real `require` on the last line, which consults neither `ALLOWED_NODE_BUILTIN_MODULES` nor `REPLACED_BUILTIN_MODULES`. So with `allowedBuiltInModules: [path]` configured, `require('fs')` still returns Node's `fs`, and `require('node:child_process')` returns Node's unmodified module rather than Harper's constrained substitute.
The ESM path routes through `checkAllowedModulePath` (`:687`, `:699`, `:752`, `:784`); the CommonJS path largely does not. `moduleLoader: native` and packages selected for native loading by `dependencyLoader: auto` also bypass these checks — that part may be intended, but combined with the above it means neither setting currently constrains an application as a whole.
Suggested fix: route `cjsRequire`'s `file://` and bare-specifier branches through `checkAllowedModulePath` the way the ESM loader does, returning the replacement module when one exists so the constrained `child_process` applies to `require` as well.
Related: #2504 covers a separate defect in `checkAllowedModulePath` itself (raw string-prefix containment).
Found while documenting module loading in HarperFast/documentation#664 (review comments from @kriszyp). That PR scopes both settings to "imports the application module loader handles" and calls them configuration guardrails rather than security boundaries; the wording can be strengthened once this is fixed.
sent with Claude Opus 5
Contributor guide
Research direction
Start in security/jsLoader.ts at cjsRequire (lines 379-396), then compare its file and bare-specifier handling with the ESM checks at lines 687, 699, 752, and 784. Trace the existing loader behavior for allowedDirectory and allowedBuiltInModules; done means CommonJS requires honor those settings, including the constrained child_process replacement, without changing the noted native-loading behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, node.js
- Domain
- backend, security
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100