test(row 333): harden the business-imports helper and test it on small trees (Vikunja #779)
The helper read imports with a regex that treated a commented import as live, missed `import (` written with a space, skipped a missing target inside packages/business/src before checking it existed, and resolved targets lexically. It now blanks comments (and ignores import forms inside strings and regular expressions) before matching, counts a spaced or line-broken dynamic import, throws on a dynamic import whose argument is not a plain string and on a file with an unterminated string, comment, template or regular expression, checks every target exists and is a regular file before the inside-own-directory skip, and refuses a target whose real path is outside the real repository root. The output for the real tree is unchanged. business-imports.test.mjs builds small trees under a private temp directory.
This commit is contained in:
jason.woltje committed
1 parent
38cfa72335
commit
0e7e203db8
2 files changed
+236
-7
No files matched your search
@@ -3,21 +3,96 @@
|
||||
// a plug-in source file (row 333's model-quota.mjs) does not break the release tests the way it did
|
||||
// when each fixture named kinds.mjs alone. A real copy ships all of packages/plugins (scripts/
|
||||
// stack-copy.sh COPY_PATHS); a fixture models a smaller one and takes only what the loader reads.
|
||||
import {existsSync,readdirSync,readFileSync} from 'node:fs';
|
||||
//
|
||||
// Vikunja #779: the helper reads imports with a regex, not a parser, so it stays strict about what
|
||||
// it cannot read. Comments are blanked before matching, so an import in a comment is not one, and an
|
||||
// import form inside a string or a regular expression is not one either. A dynamic import written
|
||||
// with a space, `import ('./x.mjs')`, counts. These throw: a dynamic import whose argument is not a
|
||||
// plain string, a file with an unterminated string, comment, template or regular expression, a
|
||||
// target that is missing or not a regular file, and a target whose real path leaves the repository.
|
||||
// The scanner knows line and block comments, quoted strings, template literals, regular expression
|
||||
// literals (by the usual guess from the previous token) and a hashbang. A template literal with a
|
||||
// backtick nested inside its ${} is outside that, and so is a regular expression it guesses wrong.
|
||||
import {readdirSync,readFileSync,realpathSync,statSync} from 'node:fs';
|
||||
import {dirname,join,relative,resolve,sep} from 'node:path';
|
||||
|
||||
const IMPORT=/(?:import|export)\s[^'"`;]*?\bfrom\s*['"](\.{1,2}\/[^'"]+)['"]|\bimport\s*['"](\.{1,2}\/[^'"]+)['"]|\bimport\(\s*['"](\.{1,2}\/[^'"]+)['"]\s*\)/g;
|
||||
const IMPORT=/(?:import|export)\s[^'"`;]*?\bfrom\s*['"](\.{1,2}\/[^'"]+)['"]|\bimport\s*['"](\.{1,2}\/[^'"]+)['"]|(?<![.\w$])import\s*\(\s*['"](\.{1,2}\/[^'"]+)['"]\s*\)/g;
|
||||
const DYNAMIC=/(?<![.\w$])import\s*\(/g;
|
||||
const DYNAMIC_LITERAL=/^import\s*\(\s*(['"])[^'"\n]*\1\s*\)/;
|
||||
const REGEX_AFTER='(,=:[!&|?{};+-*%<>~^';
|
||||
const sources=(dir)=>readdirSync(dir,{withFileTypes:true}).flatMap((e)=>e.isDirectory()?sources(join(dir,e.name)):e.name.endsWith('.mjs')?[join(dir,e.name)]:[]);
|
||||
|
||||
// The text with every comment blanked and every template literal emptied, and the ranges of the
|
||||
// quoted strings and regular expressions left in it, as [from,to) offsets into the returned code.
|
||||
// Throws on a string, comment, template or regular expression that never ends.
|
||||
function scan(text){
|
||||
let code='',i=0,previous='';
|
||||
const literals=[];
|
||||
const fail=(what)=>{throw new Error(`unterminated ${what}`);};
|
||||
const keep=(end,last)=>{literals.push([code.length,code.length+end-i]);code+=text.slice(i,end);i=end;previous=last;};
|
||||
if(text.startsWith('#!')){const end=text.indexOf('\n');i=end<0?text.length:end;}
|
||||
while(i<text.length){
|
||||
const c=text[i],next=text[i+1];
|
||||
if(c==='/'&&next==='/'){const end=text.indexOf('\n',i);code+=' ';i=end<0?text.length:end;continue;}
|
||||
if(c==='/'&&next==='*'){const end=text.indexOf('*/',i+2);if(end<0)fail('comment');code+=' ';i=end+2;continue;}
|
||||
if(c==='"'||c==="'"){
|
||||
let j=i+1;
|
||||
for(;j<text.length&&text[j]!==c;j++){if(text[j]==='\\')j++;else if(text[j]==='\n')fail('string');}
|
||||
if(j>=text.length)fail('string');
|
||||
keep(j+1,c);continue;
|
||||
}
|
||||
if(c==='`'){
|
||||
let j=i+1;
|
||||
for(;j<text.length&&text[j]!=='`';j++)if(text[j]==='\\')j++;
|
||||
if(j>=text.length)fail('template');
|
||||
code+='``';i=j+1;previous='`';continue;
|
||||
}
|
||||
if(c==='/'&&(previous===''||REGEX_AFTER.includes(previous))){
|
||||
let j=i+1,inClass=false;
|
||||
for(;j<text.length&&(inClass||text[j]!=='/');j++){
|
||||
if(text[j]==='\\')j++;else if(text[j]==='\n')fail('regular expression');
|
||||
else if(text[j]==='[')inClass=true;else if(text[j]===']')inClass=false;
|
||||
}
|
||||
if(j>=text.length)fail('regular expression');
|
||||
keep(j+1,'/');continue;
|
||||
}
|
||||
code+=c;i++;
|
||||
if(!/\s/.test(c))previous=c;
|
||||
}
|
||||
return {code,literals};
|
||||
}
|
||||
|
||||
// The specifiers a source file imports by a relative path. A dynamic import of anything but a plain
|
||||
// string throws, as does a file the scanner cannot read to its end.
|
||||
function specifiers(file,text){
|
||||
let scanned;
|
||||
try{scanned=scan(text);}
|
||||
catch(error){throw new Error(`${file} cannot be read for imports: ${error.message}`);}
|
||||
const {code,literals}=scanned,live=(m)=>!literals.some(([from,to])=>m.index>=from&&m.index<to);
|
||||
for(const m of code.matchAll(DYNAMIC)){
|
||||
if(live(m)&&!DYNAMIC_LITERAL.test(code.slice(m.index,m.index+512)))throw new Error(`${file} has a dynamic import that is not a plain string literal`);
|
||||
}
|
||||
return [...code.matchAll(IMPORT)].filter(live).map((m)=>m[1]??m[2]??m[3]);
|
||||
}
|
||||
|
||||
// Repo-relative paths, sorted: every file that a relative import from packages/business/src reaches
|
||||
// outside it, following the imports of those files in turn. An import of a missing file throws.
|
||||
// outside it, following the imports of those files in turn. An import of a missing file, of anything
|
||||
// but a regular file, or of a file whose real path is outside the repository throws, wherever the
|
||||
// importing file sits, so an import of a missing file inside packages/business/src throws too.
|
||||
export function businessImportedFiles(repo){
|
||||
const own=join(repo,'packages','business','src')+sep,found=new Set(),todo=sources(own);
|
||||
const own=join(repo,'packages','business','src')+sep,found=new Set(),checked=new Set(),todo=sources(own),root=realpathSync(repo)+sep;
|
||||
for(let file;(file=todo.pop())!==undefined;){
|
||||
for(const m of readFileSync(file,'utf8').matchAll(IMPORT)){
|
||||
const spec=m[1]??m[2]??m[3],target=resolve(dirname(file),spec);
|
||||
for(const spec of specifiers(relative(repo,file),readFileSync(file,'utf8'))){
|
||||
const target=resolve(dirname(file),spec),from=`${relative(repo,file)} imports ${spec}`;
|
||||
if(!checked.has(target)){
|
||||
let info,real;
|
||||
try{info=statSync(target);real=realpathSync(target);}
|
||||
catch(error){throw error.code==='ENOENT'||error.code==='ENOTDIR'?new Error(`${from}, which does not exist`):error;}
|
||||
if(!info.isFile())throw new Error(`${from}, which is not a regular file`);
|
||||
if(!real.startsWith(root))throw new Error(`${from}, which is outside the repository`);
|
||||
checked.add(target);
|
||||
}
|
||||
if(target.startsWith(own)||found.has(target))continue;
|
||||
if(!existsSync(target))throw new Error(`${relative(repo,file)} imports ${spec}, which does not exist`);
|
||||
found.add(target);
|
||||
if(target.endsWith('.mjs'))todo.push(target);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,154 @@
|
||||
// Vikunja #779: the business-imports helper on synthetic trees. Every tree is built under a private
|
||||
// temp directory with the repository one level down, so a test can put a file outside the repository.
|
||||
import test, {after} from 'node:test';
|
||||
import assert from 'node:assert/strict';
|
||||
import {mkdirSync,mkdtempSync,rmSync,symlinkSync,writeFileSync} from 'node:fs';
|
||||
import {tmpdir} from 'node:os';
|
||||
import {dirname,join} from 'node:path';
|
||||
import {fileURLToPath} from 'node:url';
|
||||
import {businessImportedFiles} from './business-imports.mjs';
|
||||
|
||||
const ROOT=fileURLToPath(new URL('../../..',import.meta.url)).replace(/\/$/,'');
|
||||
const base=mkdtempSync(join(tmpdir(),'bi-'));
|
||||
after(()=>rmSync(base,{recursive:true,force:true}));
|
||||
const BIZ='repo/packages/business/src/',EXT='repo/packages/other/src/',EMPTY='export {};\n';
|
||||
let count=0;
|
||||
// files: path under the tree's top directory -> text. links: path -> target path, both under it.
|
||||
function tree(files,links={}){
|
||||
const top=join(base,`t${++count}`);
|
||||
for(const [path,text] of Object.entries(files)){mkdirSync(dirname(join(top,path)),{recursive:true});writeFileSync(join(top,path),text);}
|
||||
for(const [path,target] of Object.entries(links)){mkdirSync(dirname(join(top,path)),{recursive:true});symlinkSync(join(top,target),join(top,path));}
|
||||
return join(top,'repo');
|
||||
}
|
||||
const withBusiness=(text,extra={})=>tree({[`${BIZ}a.mjs`]:text,...extra});
|
||||
|
||||
test('the real tree gives the plug-in files the loader reads', ()=>{
|
||||
const found=businessImportedFiles(ROOT);
|
||||
assert.ok(found.includes('packages/plugins/src/kinds.mjs'),found.join(' '));
|
||||
assert.ok(found.includes('packages/plugins/src/model-quota.mjs'),found.join(' '));
|
||||
assert.deepEqual(found,[...found].sort());
|
||||
});
|
||||
|
||||
test('every static form and a literal dynamic import is found, and the imports of what it finds in turn', ()=>{
|
||||
const repo=withBusiness([
|
||||
"import x from '../../other/src/from.mjs';",
|
||||
"import {\n y,\n z,\n} from '../../other/src/multi.mjs';",
|
||||
"import '../../other/src/bare.mjs';",
|
||||
"export * from '../../other/src/reexport.mjs';",
|
||||
"export {w} from '../../other/src/named.mjs';",
|
||||
"const d=await import('../../other/src/dynamic.mjs');",
|
||||
"const n=await import(\n '../../other/src/newline.mjs'\n);",
|
||||
].join('\n'),{
|
||||
[`${EXT}from.mjs`]:"import '../../third/src/deep.mjs';\n",
|
||||
'repo/packages/third/src/deep.mjs':EMPTY,
|
||||
...Object.fromEntries(['multi','bare','reexport','named','dynamic','newline'].map(n=>[`${EXT}${n}.mjs`,EMPTY])),
|
||||
});
|
||||
assert.deepEqual(businessImportedFiles(repo),[
|
||||
...['bare','dynamic','from','multi','named','newline','reexport'].map(n=>`packages/other/src/${n}.mjs`),
|
||||
'packages/third/src/deep.mjs',
|
||||
]);
|
||||
});
|
||||
|
||||
test('#779 a dynamic import written with a space or a line break before the bracket is found', ()=>{
|
||||
const repo=withBusiness("const s=await import ('../../other/src/spaced.mjs');\nconst t=await import\n ('../../other/src/broken.mjs');\n",
|
||||
{[`${EXT}spaced.mjs`]:EMPTY,[`${EXT}broken.mjs`]:EMPTY});
|
||||
assert.deepEqual(businessImportedFiles(repo),['packages/other/src/broken.mjs','packages/other/src/spaced.mjs']);
|
||||
});
|
||||
|
||||
test('#779 an import in a line or block comment is not an import, and one after a comment is', ()=>{
|
||||
const repo=withBusiness([
|
||||
"// import '../../other/src/missing1.mjs';",
|
||||
"/* import x from '../../other/src/missing2.mjs'; */",
|
||||
"/*\n import('../../other/src/missing3.mjs')\n*/",
|
||||
"const x=1; // import '../../other/src/missing4.mjs'",
|
||||
"import '../../other/src/real.mjs'; // import('../../other/src/missing5.mjs')",
|
||||
"// import(name)",
|
||||
"/* import(name) */ import '../../other/src/after.mjs';",
|
||||
].join('\n'),{[`${EXT}real.mjs`]:EMPTY,[`${EXT}after.mjs`]:EMPTY});
|
||||
assert.deepEqual(businessImportedFiles(repo),['packages/other/src/after.mjs','packages/other/src/real.mjs']);
|
||||
});
|
||||
|
||||
test('#779 comment markers inside strings, templates and regular expressions do not hide an import', ()=>{
|
||||
const repo=withBusiness([
|
||||
"const url='http://example.invalid/a'; import '../../other/src/after-url.mjs';",
|
||||
"const glob=\"/* open\"; import '../../other/src/after-glob.mjs';",
|
||||
"const tpl=`// not a comment ${1}`; import '../../other/src/after-template.mjs';",
|
||||
"const re=/['\"]\\/\\/x/; import '../../other/src/after-regex.mjs';",
|
||||
"const half=a / 2;\nconst mid=(a+b) / 2;\nimport '../../other/src/after-divide.mjs';",
|
||||
].join('\n'),Object.fromEntries(['url','glob','template','regex','divide'].map(n=>[`${EXT}after-${n}.mjs`,EMPTY])));
|
||||
assert.deepEqual(businessImportedFiles(repo),['url','glob','template','regex','divide'].map(n=>`packages/other/src/after-${n}.mjs`).sort());
|
||||
});
|
||||
|
||||
test('#779 an import form inside a string or a regular expression is not an import', ()=>{
|
||||
const repo=withBusiness([
|
||||
"const a=\"import '../../other/src/missing1.mjs'\";",
|
||||
"const b='import(name)';",
|
||||
"const c=/import (name)/;",
|
||||
"const d=\"export {x} from '../../other/src/missing2.mjs'\";",
|
||||
"import '../../other/src/real.mjs';",
|
||||
].join('\n'),{[`${EXT}real.mjs`]:EMPTY});
|
||||
assert.deepEqual(businessImportedFiles(repo),['packages/other/src/real.mjs']);
|
||||
});
|
||||
|
||||
test('#779 a hashbang line is skipped and a bare slash in it does not start a regular expression', ()=>{
|
||||
const repo=withBusiness("#!/usr/bin/env node\nimport '../../other/src/real.mjs';\n",{[`${EXT}real.mjs`]:EMPTY});
|
||||
assert.deepEqual(businessImportedFiles(repo),['packages/other/src/real.mjs']);
|
||||
const bare=withBusiness("#!/odd\nimport '../../other/src/real.mjs';\n",{[`${EXT}real.mjs`]:EMPTY});
|
||||
assert.deepEqual(businessImportedFiles(bare),['packages/other/src/real.mjs']);
|
||||
});
|
||||
|
||||
test('#779 a dynamic import of anything but a plain string throws, and what only looks like one does not', ()=>{
|
||||
for(const argument of ['name',"'../../other/src/' + name",'`../../other/src/${name}.mjs`',"'../../other/src/a.mjs', {with:{type:'json'}}","'../../other/src/a.mjs' + ''"]){
|
||||
const repo=withBusiness(`const m=await import(${argument});\n`,{[`${EXT}a.mjs`]:EMPTY});
|
||||
assert.throws(()=>businessImportedFiles(repo),/business\/src\/a\.mjs has a dynamic import that is not a plain string literal/,argument);
|
||||
}
|
||||
const repo=withBusiness([
|
||||
'const u=import.meta.url;','loader.import(thing);','reimport(thing);','$import(thing);',
|
||||
"const fs=await import('node:fs');","const s='import(name)';",
|
||||
].join('\n'));
|
||||
assert.deepEqual(businessImportedFiles(repo),[]);
|
||||
});
|
||||
|
||||
test('#779 a file the scanner cannot read to its end throws and names the file, business or external', ()=>{
|
||||
for(const [what,text] of [['string',"const a='open;\n"],['string','const a="open'],['comment',"/* open\nimport '../../other/src/a.mjs';\n"],['template','const a=`open'],['regular expression','const a=/open\n']]){
|
||||
const repo=withBusiness(text,{[`${EXT}a.mjs`]:EMPTY});
|
||||
assert.throws(()=>businessImportedFiles(repo),new RegExp(`packages/business/src/a\\.mjs cannot be read for imports: unterminated ${what}`),text);
|
||||
}
|
||||
const repo=withBusiness("import '../../other/src/a.mjs';\n",{[`${EXT}a.mjs`]:"/* never closed\n"});
|
||||
assert.throws(()=>businessImportedFiles(repo),/packages\/other\/src\/a\.mjs cannot be read for imports: unterminated comment/);
|
||||
});
|
||||
|
||||
test('#779 an import of a file that is missing throws, inside packages/business/src or out of it', ()=>{
|
||||
const inside=withBusiness("import './missing.mjs';\n");
|
||||
assert.throws(()=>businessImportedFiles(inside),/packages\/business\/src\/a\.mjs imports \.\/missing\.mjs, which does not exist/);
|
||||
const outside=withBusiness("import '../../other/src/missing.mjs';\n");
|
||||
assert.throws(()=>businessImportedFiles(outside),/imports \.\.\/\.\.\/other\/src\/missing\.mjs, which does not exist/);
|
||||
const broken=tree({[`${BIZ}a.mjs`]:"import '../../other/src/link.mjs';\n"},{[`${EXT}link.mjs`]:'gone.mjs'});
|
||||
assert.throws(()=>businessImportedFiles(broken),/which does not exist/);
|
||||
const beyond=withBusiness("import '../../other/src/a.mjs/b.mjs';\n",{[`${EXT}a.mjs`]:EMPTY});
|
||||
assert.throws(()=>businessImportedFiles(beyond),/which does not exist/);
|
||||
});
|
||||
|
||||
test('#779 an import of a directory throws', ()=>{
|
||||
const repo=withBusiness("import '../../other/src/dir';\n",{[`${EXT}dir/index.mjs`]:EMPTY});
|
||||
assert.throws(()=>businessImportedFiles(repo),/imports \.\.\/\.\.\/other\/src\/dir, which is not a regular file/);
|
||||
});
|
||||
|
||||
test('#779 a target outside the repository throws, by a path that climbs out or by a symlink', ()=>{
|
||||
const climbs=tree({[`${BIZ}a.mjs`]:"import '../../../../outside.mjs';\n",'outside.mjs':EMPTY});
|
||||
assert.throws(()=>businessImportedFiles(climbs),/imports \.\.\/\.\.\/\.\.\/\.\.\/outside\.mjs, which is outside the repository/);
|
||||
const sibling=tree({[`${BIZ}a.mjs`]:"import '../../../../repo2/x.mjs';\n",'repo2/x.mjs':EMPTY});
|
||||
assert.throws(()=>businessImportedFiles(sibling),/which is outside the repository/);
|
||||
const linked=tree({[`${BIZ}a.mjs`]:"import '../../other/src/link.mjs';\n",'outside.mjs':EMPTY},{[`${EXT}link.mjs`]:'outside.mjs'});
|
||||
assert.throws(()=>businessImportedFiles(linked),/imports \.\.\/\.\.\/other\/src\/link\.mjs, which is outside the repository/);
|
||||
const folder=tree({[`${BIZ}a.mjs`]:"import '../../other/src/out/x.mjs';\n",'outside/x.mjs':EMPTY},{[`${EXT}out`]:'outside'});
|
||||
assert.throws(()=>businessImportedFiles(folder),/which is outside the repository/);
|
||||
});
|
||||
|
||||
test('#779 a symlink that stays inside the repository, and a repository reached by a symlink, are fine', ()=>{
|
||||
const repo=tree({[`${BIZ}a.mjs`]:"import '../../other/src/link.mjs';\n",'repo/packages/third/real.mjs':EMPTY},{[`${EXT}link.mjs`]:'repo/packages/third/real.mjs'});
|
||||
assert.deepEqual(businessImportedFiles(repo),['packages/other/src/link.mjs']);
|
||||
const via=join(base,`via${++count}`);
|
||||
symlinkSync(repo,via);
|
||||
assert.deepEqual(businessImportedFiles(via),['packages/other/src/link.mjs']);
|
||||
});
|
||||
Reference in new issue
Block a user