docs(reference): give module loading an owner in reference - #664
Conversation
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-664 This preview will update automatically when you push new commits. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Module Loading' documentation page, updates the 'JavaScript Environment' and 'Configuration Options' pages, and refactors the v5 migration guide to detail the module loading configurations and modes in Harper v5.0.0. The review feedback suggests correcting the configuration file name to harper-config.yaml and formatting a critical security warning regarding the compartment mode's bypass of the constrained child_process using a caution block for better visibility.
|
|
||
| The loader is also what makes application context work. It gives each application a `harper` module scoped to that application: the `logger` it exports is tagged with the application name, and `config` reflects that application's own configuration. Under the VM loaders it additionally substitutes a constrained [`child_process`](./javascript-environment.md#child-processes) module. | ||
|
|
||
| Everything on this page is controlled by the `applications` section of `harperdb-config.yaml`: |
There was a problem hiding this comment.
The configuration file name is referred to as harperdb-config.yaml here, but it has been rebranded to harper-config.yaml in v5. Please update this reference to ensure consistency with the rest of the documentation.
|
|
||
| SES `Compartment`-based loading, using the [`ses`](https://www.npmjs.com/package/ses) implementation of the proposed Compartment API. One compartment per application, created on demand because it is considerably heavier than the other modes. | ||
|
|
||
| Advanced; only needed for specialized sandboxing requirements. Note that compartments resolve built-in modules through Node directly, so Harper's constrained `child_process` is bypassed entirely under this mode — the spawn allowlist, the mandatory `name` option, the single-process lock, and the `execSync` block all disappear together. |
There was a problem hiding this comment.
This critical security warning regarding the bypass of the constrained child_process under compartment mode is currently combined with other concepts. To make it prominent and easily scannable, please format it using a :::caution block and present the warning in separate, distinct sentences.
References
- When documenting critical caveats, potential runtime errors, or limitations, format them using a :::caution block (or appropriate admonition) to ensure they are prominent and visible, while maintaining consistency with the document's existing formatting patterns.
- Ensure critical security warnings, such as unauthorized access risks or fallback behaviors, are presented in separate, distinct sentences rather than being combined with other concepts or buried behind semicolons, so that readers scanning the documentation can easily find them.
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
This is really good! Excellent documentation! I included codex suggestions, but I'm not sure we even should follow them; as the edge cases and bugs it names aren't necessarily worth the doc complexity/confusion of adding them (in particular, I think we should file bugs, not document them).
🤖 Reviewed with Codex
|
|
||
| `allowedDirectory` restricts where application modules may be loaded from. | ||
|
|
||
| - `app` (default) — an application may only load modules from within its own directory tree. Loading from outside it throws `Can not load module at <path> outside of allowed path <path>`. |
There was a problem hiding this comment.
Please avoid presenting this as a complete directory boundary in current Harper releases. A VM-loaded CommonJS component can require() a file outside its application without invoking the path check; native and ordinary compartment file loads also bypass it. Even checked ESM paths use a raw string-prefix comparison, so an application rooted at /components/foo can load /components/foo-other/file.js. The root fix belongs in Harper core: apply a separator-aware path.relative() containment check on every file-loading path. Until supported releases contain that fix, document the exact enforcement matrix and these bypasses instead of promising access only within the application's tree.
There was a problem hiding this comment.
Agreed on both counts, and I took your point in the top-level review that these are bugs to file rather than document. Two issues:
- allowedDirectory containment uses a raw string prefix, so a sibling directory sharing a name prefix passes the check harper#2504 — the raw
startsWithcontainment, so/components/foopasses for/components/foo-other/file.js. Suggested the separator-awarepath.relative()check. - Application loader security checks (allowedDirectory, allowedBuiltInModules) are skipped on the CommonJS require path harper#2505 —
cjsRequireonly callscheckAllowedModulePathfor.nodeaddons, so a plainfile://require skips it entirely.
In the docs I stopped short of the enforcement matrix and just scoped the claim: the check applies to imports the application module loader handles, and it is a configuration guardrail rather than a security boundary. That reads correctly both now and after the fixes land, so it should not need another revision.
sent with Claude Opus 5
|
|
||
| ## Allowed Built-in Modules | ||
|
|
||
| `allowedBuiltInModules` restricts which Node.js built-ins applications may import. If it is omitted, all built-ins are allowed — which is the default. |
There was a problem hiding this comment.
This allowlist is not currently application-wide. Under the default VM loader, CommonJS require('fs') returns Node's module before the allowlist check; moduleLoader: native skips the check entirely; and packages selected for native loading by dependencyLoader: auto can import any built-in. Consequently, allowedBuiltInModules: [path] does not prevent application code from reaching fs. If this is intended as a security boundary, enforce it consistently in Harper core. Otherwise, qualify this as applying only to imports handled by the application loader and include the bypass matrix here.
There was a problem hiding this comment.
Right — same root cause, filed as HarperFast/harper#2505. The last line of cjsRequire falls through to Node’s real require, which consults neither ALLOWED_NODE_BUILTIN_MODULES nor REPLACED_BUILTIN_MODULES, so allowedBuiltInModules: [path] does not stop require("fs") and require("node:child_process") returns the unmodified module.
Same treatment as above: scoped to imports the application loader handles, described as a guardrail rather than a boundary, no bypass matrix.
sent with Claude Opus 5
|
|
||
| ### Constrained `fetch` | ||
|
|
||
| Under `lockdown: ses`, the modes that build a custom global object (`vm` and `compartment`) also install an https-only `fetch` in that global. Under `vm-current-context` and `native`, application code uses the standard global `fetch`. |
There was a problem hiding this comment.
The constrained fetch is not usable with the standard string form currently described. Shipped core derives the URL with typeof resource === 'string' || resource.url; for fetch('https://example.com') that produces boolean true, and new URL(true) throws TypeError: Invalid URL before checking the protocol. A URL object also produces undefined; only a Request-like object with .url works. Please fix and test the expression in Harper core, or document the current input limitation until that fix ships.
There was a problem hiding this comment.
Confirmed — filed as HarperFast/harper#2503. const url = typeof resource === "string" || resource.url looks like a ternary that lost its ?, so a string argument yields boolean true and new URL(true) throws before the protocol check; a URL instance yields undefined and throws too.
Rather than document a broken feature, I removed the constrained fetch section outright. It comes back once the fix ships.
sent with Claude Opus 5
| ```yaml | ||
| applications: | ||
| lockdown: freeze-after-load # freeze-after-load (default) | freeze | ses | none | ||
| moduleLoader: vm-current-context # vm-current-context (default) | vm | native | compartment |
There was a problem hiding this comment.
The v5.0.0 page badge makes these default labels appear valid throughout v5, but the tagged core configurations differ: v5.0.0 defaulted to lockdown: freeze and moduleLoader: vm; freeze-after-load became the default in v5.0.2; allowedDirectory appeared in v5.0.4; and vm-current-context became the default in v5.1.0. Please add the required changed-version annotation for v5.1.0 and state the earlier defaults and patch-level availability explicitly, so v5.0 users do not reason from the wrong isolation model.
There was a problem hiding this comment.
Verified all four against the tags and you were right on every one, so this is now a "Defaults changed during v5.0" note plus a <VersionBadge type="changed" version="v5.1.0" /> on the default-mode section and v5.0.4 on Allowed Directory.
| Setting | v5.0.0 | Current |
|---|---|---|
lockdown |
freeze |
freeze-after-load, since v5.0.2 |
moduleLoader |
vm |
vm-current-context, since v5.1.0 |
allowedDirectory |
not available | app, since v5.0.4 |
The moduleLoader one was the real trap: a v5.0.x reader would have concluded they were on vm-current-context sharing intrinsics with Harper, when they were on vm — the exact mode my own troubleshooting section tells you to move away from when instanceof fails.
sent with Claude Opus 5
|
|
||
| The loader is also what makes application context work. It gives each application a `harper` module scoped to that application: the `logger` it exports is tagged with the application name, and `config` reflects that application's own configuration. Under the VM loaders it additionally substitutes a constrained [`child_process`](./javascript-environment.md#child-processes) module. | ||
|
|
||
| Everything on this page is controlled by the `applications` section of `harperdb-config.yaml`: |
There was a problem hiding this comment.
Please use the canonical v5 filename, harper-config.yaml. origin/main now standardizes the v5 reference on that name and explains separately that upgraded installations may still point settings_path at legacy harperdb-config.yaml. This new page otherwise reintroduces the legacy name as if it were canonical; align it while rebasing onto current main.
There was a problem hiding this comment.
Fixed, and rebased onto current main — which had already corrected the same name in the migration guide.
sent with Claude Opus 5
| Harper runs as a single process. Every co-located application shares that process and its worker threads, so it is worth being precise about what is isolated between applications and what is not. | ||
|
|
||
| - **Module contexts are isolated.** Harper loads each application's JavaScript in its own module context using Node.js's VM module loader, giving every application a distinct module cache. One application's modules, imports, and module-scoped state are not visible to another, so two applications can depend on different packages—or different versions of the same package—without colliding. | ||
| - **Module contexts are isolated.** Harper loads each application's JavaScript in its own module context using Node.js's VM module loader, giving every application a distinct module cache. One application's modules, imports, and module-scoped state are not visible to another, so two applications can depend on different packages—or different versions of the same package—without colliding. This is the default (`moduleLoader: vm-current-context`) and it is configurable—see [Module Loading](/reference/v5/components/module-loading) for the other modes, including `native`, which drops the per-application module cache entirely. |
There was a problem hiding this comment.
High — Module-cache isolation is overstated. With the default dependencyLoader: auto, packages that do not depend on harper use Node's native loader; when two applications resolve the same package file, they share Node's cache and singleton state, contrary to this unconditional guarantee. Qualify the claim to modules handled by the application loader and note that native-loaded dependencies may share cache and state.
—
Reviewed f6a97c3
There was a problem hiding this comment.
Good catch — this was overstated, and it is intended behavior rather than a bug, so I fixed it in the docs.
The bullet now leads with "Application source is isolated; some dependencies are not" and spells out that under the default dependencyLoader: auto, packages that do not declare harper as a dependency load through Node's loader and share its process-wide cache, so two applications resolving the same package file get the same instance and the same singleton state.
The new reference page carries the same qualification: the mode-comparison table now footnotes the "Per app" module-cache column, and the opening paragraph names the exception.
sent with Claude Opus 5
Application module loading was documented only in the v5 release notes. The reference page that should own it (`components/javascript-environment`) used `## Module Loading` for an unrelated topic (ESM vs CJS, importing from `harper`), the configuration reference linked outward to the migration guide for `lockdown` and `moduleLoader`, and the only per-mode behavior matrix was buried under `## Child Processes`. Two options -- `allowedDirectory` and `allowedBuiltInModules` -- were absent from the configuration reference entirely. - Add `reference/components/module-loading.md` covering the `moduleLoader` modes, dependency loading, intrinsic lockdown, allowed directory, allowed built-in modules, and how to choose a mode. - Rename `javascript-environment`'s `## Module Loading` to `## Module Formats` so the heading stops colliding, and scope its intro and process-globals claim to the loaders they actually describe. - Document `allowedDirectory` and `allowedBuiltInModules` in the configuration reference, and repoint `lockdown`/`moduleLoader` from the release notes to the new page. - Trim the migration guide to migration-grade content, keeping the `#module-loader-modes` and `#intrinsic-lockdown` anchors. - Fix `applications.allowedShellCommands` in the 5.0 notes; that option does not exist, the real name is `allowedSpawnCommands`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Use the canonical v5 filename `harper-config.yaml`. - Document that the defaults changed during v5.0: `lockdown` was `freeze` until v5.0.2, `moduleLoader` was `vm` until v5.1.0, and `allowedDirectory` did not exist until v5.0.4. A v5.0.x reader following the previous text would believe they were on `vm-current-context` sharing intrinsics with Harper when they were actually on `vm`. - Qualify module-cache isolation: it covers modules the application loader handles, not dependencies that `dependencyLoader: auto` routes to Node's loader, which share its process-wide cache and singleton state. - Scope `allowedDirectory` and `allowedBuiltInModules` to imports the application loader handles, and call them configuration guardrails rather than security boundaries (HarperFast/harper#2504, HarperFast/harper#2505). - Drop the constrained `fetch` section; the feature does not work as described (HarperFast/harper#2503). It returns once that ships. - Promote the compartment `child_process` bypass to a warning admonition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
f6a97c3 to
396eb5f
Compare
|
Pushed review fixes (rebased onto current main, so the diff is against Taking @kriszyp's framing — file bugs, don't document them — the defects the review surfaced went to Harper rather than into the page:
What changed in the docs:
Build and sent with Claude Opus 5 |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-664 This preview will update automatically when you push new commits. |
🧹 Preview CleanupThe preview deployment for this PR has been removed. |
3 participants
Problem
Harper's v5 application module loading is documented almost entirely in the release notes, and the reference pages that should own it point outward at them.
reference/components/javascript-environmenthas a## Module Loadingheading, but it covers ESM vs CJS, importing fromharper, and Vite SSR externals. The stringmoduleLoadernever appears in that section. It's the heading you'd click, and it's about something else.reference/configuration/options.md#applicationsdocumentslockdownandmoduleLoaderby linking to/release-notes/v5-lincoln/v5-migration#.... That's an evergreen reference page depending on a frozen point-in-time migration doc — ifmoduleLoadergains a mode, the only full description of the modes lives in a document that by definition describes v4→v5.## Child Processes. You have to be researching sidecar processes to find the clearest statement of what each loader does to your imports.allowedDirectoryandallowedBuiltInModuleswere documented only in the migration guide, and were missing from the configuration reference entirely — even thoughallowedDirectory: appships in core'sstatic/defaultConfig.yaml.Three separate pages also asserted the default loader's behavior as though it were the only behavior, which made
javascript-environment("seeded from the same process globals") read as a flat contradiction of5.0.md("its own global object"). They describe different modes; nothing said so.Changes
New page —
reference/components/module-loading.md, in the Components sidebar after JavaScript Environment. Covers the fourmoduleLoadermodes with a comparison table (module cache / intrinsics / global object / application context / constrainedchild_process),dependencyLoader,lockdown,allowedDirectory,allowedBuiltInModules, the constrainedfetch, and a symptom-driven "Choosing a Mode" section.javascript-environment.md— renamed## Module Loadingto## Module Formatsso the heading stops colliding; scoped the intro and the process-globals claim to the loaders they actually describe; the child-process substitution table now links to the loader reference instead of re-explaining modes.configuration/options.md— documentedallowedDirectoryandallowedBuiltInModules; repointedlockdownandmoduleLoaderat the new page.v5-migration.md— trimmed from 66 lines to 38. Kept what breaks and how to cope; moved the mode-by-mode detail, lockdown mode list,allowedBuiltinModulesblock, anddependencyLoadersection into reference. Deliberately kept#module-loader-modesand#intrinsic-lockdownas real sections so those anchors survive — they were the two linked from elsewhere.5.0.md— fixedapplications.allowedShellCommands. That option does not exist; the real name isallowedSpawnCommands. Anyone who followed that release note wrote a key Harper silently ignores, then had their spawn blocked with no explanation.learn/developers/multiple-applications.mdx— notes the VM loader is the default and configurable.Verification
Everything on the new page is checked against
harperorigin/mainrather than carried over on faith:allowedDirectory: anyoverride —utility/install/installer.ts:67node:strip + first-segment match for built-ins —security/jsLoader.ts:1202-1216REPLACED_BUILTIN_MODULEScontains onlychild_process, so an allowlisted built-in still gets the substitute —security/jsLoader.ts:986allowedShellCommandsreturns zero hits anywhere in core;allowedSpawnCommandsis instatic/defaultConfig.yaml:32andutility/hdbTerms.ts:519One inherited claim corrected along the way: the migration guide said the https-only
fetchunderlockdown: sesapplies "only invmmode". It's installed on the custom global object (security/jsLoader.ts:856), which bothvmandcompartmentbuild, socompartmentgets it too.npm run buildis clean (onBrokenLinks: 'throw', no anchor warnings) andformat:checkpasses.Note for reviewers
The trim drops two anchors from the migration guide:
#allowed-built-in-modulesand#dependency-loading. Nothing in-repo linked to them, but external links (support threads, blog posts) would break silently. Happy to add stub headings if that's a concern.sent with Claude Opus 5