Skip to content

Commit 00b53a8

Browse files
fix: 8 code-review fixes — ESM detection, volume paths, fd leak, path traversal, more
- Fix environment detection: use process.versions.node instead of typeof module (works in both CJS and ESM contexts) - Fix JMS volume path resolution relative to metadata file, not CWD - Fix NodeDataLoader: close file descriptors after read, handle short reads - Fix browser progress callbacks: check callbacks array, not onprogress - Add Node guard to getFileURL() to match getHTMLDataURIFile() - Add JPAK2 magic type support (packer.py format now compatible with loader) - Fix Python unpacker path traversal: reject paths with '..' escapes - Fix bytesToString to prefer TextDecoder for UTF-8 correctness - Update AGENTS.md: document node:test framework 🤖 Generated with Mister Maluco Co-Authored-By: MisterMal <teskeslab@lucasteske.dev>
1 parent 983b7e4 commit 00b53a8

8 files changed

Lines changed: 56 additions & 19 deletions

File tree

AGENTS.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ python3 tools/unpacker.py file.jpak # extract any JPAK file
4848
## CI / Publishing
4949

5050
- `ci.yml` — runs lint + test on push/PR to master (Node 18/20/22 matrix)
51-
- `publish.yml` — triggered by `v*` tags or manual dispatch; runs full validation then `npm publish`
51+
- `publish.yml` — triggered by `v*` tags or manual dispatch; runs full validation then `npm publish` via OIDC trusted publisher
5252

5353
Requires `NPM_TOKEN` secret set in GitHub repo settings (from npmjs.com Access Tokens, with publish permission scoped to `@teskevirtualsystem/jpak`).
5454

@@ -69,10 +69,11 @@ git push --follow-tags # triggers publish workflow
6969
## Testing
7070

7171
```sh
72-
node test/test_node.js # loads test/packtest.jms via Node loader
72+
node --test test/test.js # run 47 automated tests via node:test (zero dependencies)
7373
```
7474

75-
No test framework. Manual browser tests at `test/test_jpak.html` and `test_jms.html`.
75+
Six test suites: namespace, utilities, classes, JMS loader, JPAK 1.0 loader, error handling.
76+
Manual browser tests at `test/test_jpak.html` and `test_jms.html`.
7677

7778
## Conventions
7879

jssrc/alpha.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ const JPAK = {
4343

4444
(function() {
4545

46-
const inNode = (typeof module !== 'undefined' && typeof module.exports !== 'undefined');
46+
const inNode = (typeof process !== 'undefined' && process.versions && process.versions.node);
4747

4848
if (inNode) {
4949
const { Buffer } = require('buffer');
@@ -71,6 +71,7 @@ const JPAK = {
7171
* Convert a Uint8Array to a string (replaces old Uint8Array.prototype.asString)
7272
*/
7373
JPAK.Tools.bytesToString = function(bytes) {
74+
if (typeof TextDecoder !== 'undefined') return new TextDecoder().decode(bytes);
7475
let o = "";
7576
for (let i = 0; i < bytes.byteLength; i++)
7677
o += String.fromCharCode(bytes[i]);
@@ -260,6 +261,7 @@ const JPAK = {
260261

261262
JPAK.Constants.MAGIC_TYPE = {
262263
"JPAK1": 0,
264+
"JPAK2": 0,
263265
"JMS1": 1,
264266
"JDS1": 2
265267
};

jssrc/data-loader.js

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
3333

3434
(function() {
3535

36-
const inNode = (typeof module !== 'undefined' && typeof module.exports !== 'undefined');
36+
const inNode = (typeof process !== 'undefined' && process.versions && process.versions.node);
3737

3838
if (!inNode) {
3939
class DataLoader {
@@ -54,7 +54,7 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
5454
};
5555

5656
this.xhr.onprogress = (e) => {
57-
if (e.lengthComputable && this.onprogress !== undefined) {
57+
if (e.lengthComputable) {
5858
const percentComplete = (( (e.loaded / e.total)*10000 ) >> 0)/100;
5959
this._reportProgress({"loaded":e.loaded,"total":e.total,"percent": percentComplete});
6060
}
@@ -180,15 +180,18 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
180180
return;
181181
}
182182

183+
const close = () => { try { fs.close(fd, () => {}); } catch (e) {} };
184+
183185
const getSize = () => {
184186
return new Promise((sizeResolve, sizeReject) => {
185187
if (_this.partial) {
186188
sizeResolve(_this.partialTo - _this.partialFrom + 1);
187189
} else {
188190
fs.fstat(fd, (err, stats) => {
189-
if (err)
191+
if (err) {
192+
close();
190193
sizeReject(err);
191-
else
194+
} else
192195
sizeResolve(stats.size);
193196
});
194197
}
@@ -199,12 +202,27 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
199202
return new Promise((loadResolve, loadReject) => {
200203
const buffer = Buffer.alloc(readsize);
201204
const position = _this.partial ? _this.partialFrom : 0;
202-
fs.read(fd, buffer, 0, readsize, position, (err) => {
203-
if (err)
204-
loadReject(err);
205-
else
206-
loadResolve(buffer);
207-
});
205+
206+
const readChunk = (buf, offset, remaining) => {
207+
if (remaining <= 0) {
208+
close();
209+
loadResolve(buf);
210+
return;
211+
}
212+
fs.read(fd, buf, offset, remaining, position + offset, (err, bytesRead) => {
213+
if (err) {
214+
close();
215+
loadReject(err);
216+
} else if (bytesRead <= 0) {
217+
close();
218+
loadResolve(buf);
219+
} else {
220+
readChunk(buf, offset + bytesRead, remaining - bytesRead);
221+
}
222+
});
223+
};
224+
225+
readChunk(buffer, 0, readsize);
208226
});
209227
}).then((data) => {
210228
_this._reportLoad(JPAK.Tools.toArrayBuffer(data));

jssrc/directory-entry.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
3333

3434
(function() {
3535

36-
const inNode = (typeof module !== 'undefined' && typeof module.exports !== 'undefined');
36+
const inNode = (typeof process !== 'undefined' && process.versions && process.versions.node);
3737

3838
class JPKDirectoryEntry {
3939
constructor(name, path, numfiles, directories, files, aeskey) {

jssrc/jds.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
3333

3434
(function() {
3535

36-
const inNode = (typeof module !== 'undefined' && typeof module.exports !== 'undefined');
36+
const inNode = (typeof process !== 'undefined' && process.versions && process.versions.node);
3737
if (inNode) {
3838

3939
const fs = require("fs");

jssrc/jms.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
3333

3434
(function() {
3535

36-
const inNode = (typeof module !== 'undefined' && typeof module.exports !== 'undefined');
36+
const inNode = (typeof process !== 'undefined' && process.versions && process.versions.node);
3737

3838
class JMS {
3939
constructor(volumeTable, fileTable, producerId, flags, userflags) {

jssrc/loader.js

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,13 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
3333

3434
(function() {
3535

36-
const inNode = (typeof module !== 'undefined' && typeof module.exports !== 'undefined');
36+
const inNode = (typeof process !== 'undefined' && process.versions && process.versions.node);
37+
38+
let fs, p;
39+
if (inNode) {
40+
fs = require('fs');
41+
p = require('path');
42+
}
3743

3844
class Loader {
3945
constructor(parameters) {
@@ -176,6 +182,7 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
176182
}
177183

178184
getFileURL(path, mimeType) {
185+
if (inNode) return Promise.reject(new Error("getFileURL is only available in browser"));
179186
return this.getFile(path, mimeType).then((blob) => {
180187
if (blob !== undefined)
181188
return URL.createObjectURL(blob);
@@ -307,7 +314,10 @@ CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
307314
len = len || file.size;
308315

309316
if (file.volume in this.volumeTable) {
310-
const volumePath = this.volumeTable[file.volume].filename;
317+
let volumePath = this.volumeTable[file.volume].filename;
318+
if (inNode) {
319+
volumePath = p.resolve(p.dirname(this.jpakfile), volumePath);
320+
}
311321
const fileLoader = new JPAK.Tools.DataLoader({
312322
url: volumePath,
313323
partial: true,

tools/jpaktool.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,9 @@ def ProcessFiles(entry, volume, root, version=2, rootkey=None, compress=False):
315315
if version == 1:
316316
for file in entry:
317317
filepath = os.path.join(root, entry[file]["name"])
318+
if os.path.realpath(filepath) != filepath:
319+
print("Skipping unsafe path: %s" % filepath)
320+
continue
318321
print("Extracting file %s" % filepath)
319322
f = open(filepath, "wb")
320323
volume.seek(entry[file]["offset"])
@@ -324,6 +327,9 @@ def ProcessFiles(entry, volume, root, version=2, rootkey=None, compress=False):
324327
elif version == 2:
325328
for file in entry:
326329
filepath = os.path.join(root, entry[file]["name"])
330+
if os.path.realpath(filepath) != filepath:
331+
print("Skipping unsafe path: %s" % filepath)
332+
return
327333
if entry[file]["key"] is True:
328334
print("Cannot decrypt file %s" % filepath)
329335
return

0 commit comments

Comments
 (0)