feat(clerk-js): Add bundle-check script #4199
Conversation
🦋 Changeset detectedLatest commit: 5401e68 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } else { | ||
| const filePath = `./dist${req.url}`; | ||
| const extname = path.extname(filePath); | ||
| if (fs.existsSync(filePath) && (extname === '.js' || extname === '.css')) { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 3 hours ago
To fix the problem, we need to ensure that the constructed filePath is contained within a safe root directory. We can achieve this by normalizing the path using path.resolve and then checking that the normalized path starts with the root directory. This will prevent path traversal attacks by ensuring that the file path does not escape the intended directory.
- Define a root directory for the files.
- Normalize the
filePathusingpath.resolve. - Check that the normalized path starts with the root directory.
- If the check fails, return a 403 Forbidden response.
| @@ -131,3 +131,9 @@ | ||
| } else { | ||
| const filePath = `./dist${req.url}`; | ||
| const ROOT = path.resolve('./dist'); | ||
| let filePath = path.resolve(ROOT, `.${req.url}`); | ||
| if (!filePath.startsWith(ROOT)) { | ||
| res.writeHead(403, { 'content-type': 'text/plain' }); | ||
| res.end('403 Forbidden\n'); | ||
| return; | ||
| } | ||
| const extname = path.extname(filePath); |
|
|
||
| function signIn() { | ||
| const script = ` | ||
| window.Clerk.load({ router: window.VIRTUAL_ROUTER }).then(() => { |
There was a problem hiding this comment.
This router option is not actually used in this version of clerk-js. It's being introduced in #4114 but it doesn't hurt anything to have it here now. Happy to remove it if it gives anyone heartburn!
| <script | ||
| type="text/javascript" | ||
| src="/clerk.browser.js" | ||
| data-clerk-publishable-key="${process.env.CLERK_PUBLISHABLE_KEY}" |
There was a problem hiding this comment.
CLERK_PUBLISHABLE_KEY is not exported from some frameworks such as Next.js, right? In this case, would be NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY - how do we forward it to clerk-js?
There was a problem hiding this comment.
This script is purely for our own use, and doesn't come into play for any runtime behaviors. We'll be manually running it like node bundle-check.mjs. I honestly want to just embed a key into the file, but we've never done that (and the few that are embedded point to lclclerk.dev domains, so the AIO components won't render with them). I don't know of a reason why embedding a publishable key would be bad, but I wanted to get confirmation before I did that. I would strongly prefer we not have to set an environment variable to run the script.
| if (req.url && req.url in routes) { | ||
| res.writeHead(200, { 'content-type': 'text/html' }); | ||
| res.end(routes[req.url]); | ||
| } else { |
There was a problem hiding this comment.
We're not checking if req.url is defined here - It might be an impossible case to lead to issues since fs.existsSync(filePath) would fail, but maybe it's better to safeguard for it.
There was a problem hiding this comment.
Yeah this is actually because the TypeScript types are not great. req.url is always defined for incoming messages received by the HTTP server. But I agree that there's no harm in still checking! I'll add that.
Co-authored-by: Laura Beatris <[email protected]>
Description
This PR adds a new script,
bundle-check.mjs, toclerk-js. This allows us to confirm which chunks are dynamically loaded during a particular invocation of Clerk, and to see their gzipped sizes. This will be useful when comparing the total loaded JavaScript sizes of the new versus the old components when they're both in theclerk-jspackage.Example output:
Checklist
npm testruns as expected.npm run buildruns as expected.Type of change