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.

packages/cli/tests/business-imports.mjs

@@ -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)){
+  for(const spec of specifiers(relative(repo,file),readFileSync(file,'utf8'))){
-   const spec=m[1]??m[2]??m[3],target=resolve(dirname(file),spec);
+   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);
   }

packages/cli/tests/business-imports.test.mjs

@@ -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']);
+});