[wrangler] Add dependency metadata to worker uploads#14591
[wrangler] Add dependency metadata to worker uploads#14591dario-piotrowicz wants to merge 6 commits into
Conversation
🦋 Changeset detectedLatest commit: 7ea3c18 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
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 |
workers-devprod
requested review from
a team and
jamesopstad
and removed request for
a team
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
✅ All changesets look good |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dario-piotrowicz
changed the title
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@cloudflare/autoconfig
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
This comment was marked as resolved.
This comment was marked as resolved.
dario-piotrowicz
force-pushed
the
dario/DEVX-2621/upload-packages-instrumentation-data
branch
from
43c2fbc to
6103512
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dario-piotrowicz
force-pushed
the
dario/DEVX-2621/upload-packages-instrumentation-data
branch
2 times, most recently
from
21e45a2 to
8d63eaa
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dario-piotrowicz
force-pushed
the
dario/DEVX-2621/upload-packages-instrumentation-data
branch
from
8d63eaa to
9a414f5
Compare
dario-piotrowicz
force-pushed
the
dario/DEVX-2621/upload-packages-instrumentation-data
branch
from
9a414f5 to
c1c1703
Compare
There was a problem hiding this comment.
I assume this has basically been copied verbatim from autoconfig?
There was a problem hiding this comment.
yes this is the code from autoconfig, but it's not been copied but actually completely moved and autoconfig uses this code from workers-uitls now
| if (isPackagePrivate(dependencyName, projectPath)) { | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm slightly concerned about the potential performance impact here. This will be multiple filesystem calls for every installed package on a deploy.
What is this mitigating?
There was a problem hiding this comment.
Many of the concerns raised in the original PR: #12708.
I imagine if this is now directly tied to an account, and treated as private account data similar to my worker contents, rather than general telemetry analytics, this would be less of a concern.
There was a problem hiding this comment.
I don't think filtering out packages marked with private: true is actually going to limit uploading the names of private packages. I imagine most internal packages will be published to private registries, but not necessarily marked as private: true? Because of that, I don't think this check adds enough value for the cost.
(Also, these package names will probably be included as comments by esbuild in the Worker bundle that's uploaded anyway)
There was a problem hiding this comment.
lots of orgs do use npm private packages on the public registry, so this does add value in that case. But yes - if this is data that's inherently tied to a user account and under the same protections like my worker code, vs the previous implementation as "anonymous" telemetry, then this seems okay.
There was a problem hiding this comment.
@penalosa yes this logic is best effort we did attempt a more airtight solution in 12708 but it has its own issues
I feel like this is probably better than nothing though and given that project are not going to have dependencies in the order of the thousands I think the performance impact here should be quite limited
Do you strongly think we should remove this logic?
| * @returns An array of package dependency entries, or `undefined` if package.json | ||
| * cannot be read or no valid dependencies are found | ||
| */ | ||
| export function collectPackageDependencies( |
There was a problem hiding this comment.
I think this should be async, and we should make all the filesystem operations async. It doesn't matter loads in Wrangler though, I suppose
There was a problem hiding this comment.
yeah I suspect that it won't make too much of a difference in wrangler since that even if this was async it'd still block wrangler from progressing anyways (and the code is simpler as sync)
but I'll see if it can be converted without too many issues 👍
There was a problem hiding this comment.
I converted the function to async (16dd01e)
however it doesn't change too much since this function reuses a bunch of autoconfig code that is sync right now, to convert the whole thing sync would require a bunch of refactoring 😓 , I think this is something we might want to do as a followup, what do you think? 🙂
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
4 participants
Fixes https://jira.cfdata.org/browse/DEVX-2621
This PR updates Wrangler to collect limited information about the public npm packages a built project depends on (dependency's name, the version range declared in package.json, and the exact version installed) at deploy/version-upload time and includes it in the upload metadata.
This lets us understand what versions of what packages are being deployed to us, so that we can identify ways to better protect customers given the recent wave of supply-chain attacks in the JS ecosystem, and add appropriate WAF rules (eg. https://developers.cloudflare.com/changelog/post/2026-05-06-react-nextjs-vulnerabilities/)
Users can opt out of by setting
dependencies_instrumentationtofalsein their Wrangler config file.A picture of a cute animal (not mandatory, but encouraged)