Skip to content

Commit 262d550

Browse files
andris9claude
andris9
and
claude
committed
fix(mime-node): inherit the access policy from the tree a node hangs in
MimeNode reads disableFileAccess and disableUrlAccess off itself, and createChild builds the child from nothing but the options the caller handed it. A child of a closed root therefore started out open, and a stream stage plugin that reached for the built tree could add a node whose path was read and delivered inside a message on a transporter that had closed file access. _getStream now takes the answer from the node and every node above it, so a subtree assembled before it is attached is covered too. appendChild has to detach first for that to hold, which is what its own doc comment already promised. Without it the node stayed in the previous parent's childNodes and kept streaming as part of that tree while parentNode pointed at the new one, so appending a node of a closed tree to a second open tree answered for the wrong tree. MailComposer passes both flags at every node it builds, so the normal sendMail path is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Dt9FhwVrxHM3HatmiqMdPv
1 parent ab7ef34 commit 262d550

2 files changed

Lines changed: 113 additions & 2 deletions

File tree

‎lib/mime-node/index.js‎

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,14 @@ class MimeNode {
244244
* @return {Object} Appended node object
245245
*/
246246
appendChild(childNode) {
247+
// Take the node out of the tree it is in first. Leaving it there keeps it in that
248+
// parent's childNodes, so it still streams as part of the old tree while parentNode
249+
// already points at the new one, and anything read off the parent chain answers for
250+
// the wrong tree.
251+
if (childNode.parentNode && childNode.parentNode !== this) {
252+
childNode.remove();
253+
}
254+
247255
if (childNode.rootNode !== this.rootNode) {
248256
childNode.rootNode = this.rootNode;
249257
childNode._nodeId = ++this.rootNode.nodeCounter;
@@ -1026,6 +1034,26 @@ class MimeNode {
10261034

10271035
/////// PRIVATE METHODS
10281036

1037+
/**
1038+
* Checks an access policy flag for this node and every node above it. The flags are set
1039+
* from the options the node was built with, and createChild only ever sees the options
1040+
* the caller passed, so a child of a closed tree starts out open. Reading the answer off
1041+
* the parent chain keeps it right whatever order the tree was assembled in.
1042+
*
1043+
* @param {String} flag Either 'disableFileAccess' or 'disableUrlAccess'
1044+
* @return {Boolean} true if this node or an ancestor closed that access
1045+
*/
1046+
_accessDisabled(flag) {
1047+
let node = this;
1048+
while (node) {
1049+
if (node[flag]) {
1050+
return true;
1051+
}
1052+
node = node.parentNode;
1053+
}
1054+
return false;
1055+
}
1056+
10291057
/**
10301058
* Detects and returns handle to a stream related with the content.
10311059
*
@@ -1056,7 +1084,7 @@ class MimeNode {
10561084
}
10571085

10581086
if (content && typeof content.path === 'string' && !content.href) {
1059-
if (this.disableFileAccess) {
1087+
if (this._accessDisabled('disableFileAccess')) {
10601088
contentStream = new PassThrough();
10611089
setImmediate(() => {
10621090
const err = new Error('File access rejected for ' + content.path);
@@ -1070,7 +1098,7 @@ class MimeNode {
10701098
}
10711099

10721100
if (content && typeof content.href === 'string') {
1073-
if (this.disableUrlAccess) {
1101+
if (this._accessDisabled('disableUrlAccess')) {
10741102
contentStream = new PassThrough();
10751103
setImmediate(() => {
10761104
const err = new Error('Url access rejected for ' + content.href);

‎test/mime-node/mime-node-test.js‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ const urlModule = require('url');
66
const MimeNode = require('../../lib/mime-node');
77
const addressparser = require('../../lib/addressparser');
88
const http = require('http');
9+
const fs = require('fs');
910
const stream = require('stream');
1011
const Transform = stream.Transform;
1112
const PassThrough = stream.PassThrough;
@@ -44,6 +45,21 @@ describe('MimeNode Tests', { timeout: 50 * 1000 }, () => {
4445
assert.strictEqual(mb.childNodes.length, 1);
4546
assert.strictEqual(mb.childNodes[0], child);
4647
});
48+
49+
it('should take the node out of the tree it was in', () => {
50+
// leaving it in the old parent keeps it streaming as part of that tree while
51+
// parentNode already points at the new one
52+
let first = new MimeNode('multipart/mixed');
53+
let second = new MimeNode('multipart/mixed');
54+
55+
let child = first.createChild('text/plain');
56+
second.appendChild(child);
57+
58+
assert.strictEqual(first.childNodes.length, 0);
59+
assert.strictEqual(child.parentNode, second);
60+
assert.strictEqual(child.rootNode, second);
61+
assert.strictEqual(second.childNodes[0], child);
62+
});
4763
});
4864

4965
describe('#replace', () => {
@@ -1976,6 +1992,17 @@ describe('MimeNode Tests', { timeout: 50 * 1000 }, () => {
19761992
});
19771993
});
19781994

1995+
it('should reject an URL for a child of a closed root', (t, done) => {
1996+
let mb = new MimeNode('multipart/mixed', { disableUrlAccess: true });
1997+
mb.createChild('text/plain').setContent({ href: 'http://localhost:' + port });
1998+
1999+
mb.build(err => {
2000+
assert.ok(err);
2001+
assert.strictEqual(err.code, 'EURLACCESS');
2002+
done();
2003+
});
2004+
});
2005+
19792006
it('should reject non-http(s) URL attachment', (t, done) => {
19802007
let mb = new MimeNode('text/plain').setContent({
19812008
href: 'file:///etc/passwd'
@@ -2059,6 +2086,62 @@ describe('MimeNode Tests', { timeout: 50 * 1000 }, () => {
20592086
});
20602087
});
20612088

2089+
it('should reject a file for a child of a closed root', (t, done) => {
2090+
// createChild only sees the options the caller hands it, so the child used to
2091+
// start out with file access open under a root that had closed it
2092+
let mb = new MimeNode('multipart/mixed', { disableFileAccess: true });
2093+
mb.createChild('application/octet-stream').setContent({ path: __dirname + '/fixtures/attachment.bin' });
2094+
2095+
mb.build(err => {
2096+
assert.ok(err);
2097+
assert.strictEqual(err.code, 'EFILEACCESS');
2098+
done();
2099+
});
2100+
});
2101+
2102+
it('should reject a file for a grandchild attached before its parent was', (t, done) => {
2103+
// the subtree is assembled before it is hung on the closed root, so the answer
2104+
// has to be read off the parent chain rather than copied down on append
2105+
let mb = new MimeNode('multipart/mixed', { disableFileAccess: true });
2106+
let branch = new MimeNode('multipart/related');
2107+
branch.createChild('application/octet-stream').setContent({ path: __dirname + '/fixtures/attachment.bin' });
2108+
mb.appendChild(branch);
2109+
2110+
mb.build(err => {
2111+
assert.ok(err);
2112+
assert.strictEqual(err.code, 'EFILEACCESS');
2113+
done();
2114+
});
2115+
});
2116+
2117+
it('should not read a file for a child re-parented into a second tree', (t, done) => {
2118+
// appending the node elsewhere used to re-point its parent chain at an open root
2119+
// while the closed root still streamed it
2120+
let mb = new MimeNode('multipart/mixed', { disableFileAccess: true });
2121+
let child = mb.createChild('application/octet-stream');
2122+
child.setContent({ path: __dirname + '/fixtures/attachment.bin' });
2123+
new MimeNode('multipart/mixed').appendChild(child);
2124+
2125+
const encoded = fs.readFileSync(__dirname + '/fixtures/attachment.bin').toString('base64');
2126+
2127+
mb.build((err, msg) => {
2128+
assert.ok(!err);
2129+
assert.ok(!msg.toString().includes(encoded));
2130+
done();
2131+
});
2132+
});
2133+
2134+
it('should resolve a file for a child of an open root', (t, done) => {
2135+
let mb = new MimeNode('multipart/mixed');
2136+
mb.createChild('application/octet-stream').setContent({ path: __dirname + '/fixtures/attachment.bin' });
2137+
2138+
mb.build((err, msg) => {
2139+
assert.ok(!err);
2140+
assert.ok(msg.toString().length);
2141+
done();
2142+
});
2143+
});
2144+
20622145
it('should return an error on invalid file path', (t, done) => {
20632146
let mb = new MimeNode('text/plain').setContent({
20642147
path: '/ASfsdfsdf/Sdgsgdfg/SDFgdfgdfg'

0 commit comments

Comments
 (0)