-
Notifications
You must be signed in to change notification settings - Fork 427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(cli): [nan-1106] import relative files in syncs/actions #2273
feat(cli): [nan-1106] import relative files in syncs/actions #2273
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have this error when running node ../nango/packages/cli/dist/index.js compile
Error compiling "/Users/samuelbodin/code/nango-integrations/unauthenticated/syncs/trackDeletes.ts":
SyntaxError: Unexpected token '', "��z��" is not valid JSON
at JSON.parse (<anonymous>)
at new AnyMap (/Users/samuelbodin/code/nango/node_modules/@jridgewell/src/any-map.ts:20:37)
at mapSourcePosition (/Users/samuelbodin/code/nango/node_modules/@cspotcode/source-map-support/source-map-support.js:387:14)
at wrapCallSite (/Users/samuelbodin/code/nango/node_modules/@cspotcode/source-map-support/source-map-support.js:592:20)
at Function.prepareStackTrace (/Users/samuelbodin/code/nango/node_modules/@cspotcode/source-map-support/source-map-support.js:671:41)
at prepareStackTraceCallback (node:internal/errors:145:29)
at getStackString (node:internal/util/inspect:1240:16)
at formatError (node:internal/util/inspect:1369:15)
at formatRaw (node:internal/util/inspect:985:14)
at formatValue (node:internal/util/inspect:840:10)
Can you try this again? Should be resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is an inverted condition with the nango-integrations directory detection but other than that it seems to work as expected. I haven't tested very involved or big import though
Question about your PR notes:
Update to download the entire nango-integrations directory instead of just the particular file (v2)
why will we need to do that?
const code = fs.readFileSync(filePath, 'utf-8'); | ||
const ast = parser.parse(code, { sourceType: 'module', plugins: ['typescript'] }); | ||
const importedFiles: string[] = []; | ||
const traverseFn = (traverse as any).default || traverse; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it is unrelated to this PR but I've always wondered why does this .default
thing is doing and why we are not using traverse directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think during tests traverse wasn't available for some reason
|
||
const cwd = process.cwd().split('/').pop(); | ||
const fullPath = path.resolve(importedFilePathWithExtension); | ||
if (cwd === 'nango-integrations' && fullPath.includes(process.cwd())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't it be the negative? ie: if fullPath doesn't includes nango-integration then show error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this was the last thing I added last night without testing thoroughly. Updated and added a test case to support it + improved the error message
So a user could download all the code including helper files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great 👏🏻
I have a question about unintended side-effects, it's now possible to load any npm package inside the build, I don't think it's okay even if it's handy?
…6-import-relative-files-with-cc-or-rollup
Thanks for catching this! Should be resolved by 1980321 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, it now catches the most obvious incorrect import, and execute appropriately.
It's still possible to import node_modules by using relative path (i.e: import {attemptifyAsync} from '../../node_modules/stubborn-fs/src/attemptify';
). It's a bit harder to catch, especially that stuff could live outside node_modules
.
Side note:
- You can't import
.js
file, I'm sure it's not really an issue but it displays the wrong error message in that case. (and it reduces the possibility to import packages) - Because they can import stuff, they'll need a package.json, so we need to check wether the zod/node version is matching what we are using otherwise the types will be wrong and/or expected execution will be slightly different
Yeah, hard to catch these edge cases.
Something to revisit in future iterations perhaps.
Yeah, we should publish in our documentation what versions of things we're using to make sure it aligns |
Some example code for avoiding path traversal attacks -
From https://www.stackhawk.com/blog/node-js-path-traversal-guide-examples-and-prevention/ |
Describe your changes
Use tsup to import relative files in syncs. The error reporting with tsup isn't as nice as ts-node so the tsup transpiles only and ts-node is still used to type check and report nice errors. The result is actually faster than previous. tsup doesn't transpile if there are any compilation errors.
Note that an additional
vite.cli.config
file was added which doesn't use threads because it was interfering withtsup
Edge Cases Handled
crypto
we shouldn't try and type check that filenango-integrations
directoryIssue ticket number and link
NAN-1106
Checklist before requesting a review (skip if just adding/editing APIs & templates)