feat(core): add external package name fallback function to options in generatePolicy, use it in webpack #1109
Conversation
… generatePolicy, use it in webpack
github-actions
bot
added
pkg:lavamoat-core
| */ | ||
| function guessPackageName(requestedName) { | ||
| const isNotPackageName = | ||
| requestedName.startsWith('/') || requestedName.startsWith('.') | ||
| if (isNotPackageName) { | ||
| return `<unknown:${requestedName}>` | ||
| return |
There was a problem hiding this comment.
the LHS of the isNotPackageName assignment feels like it should be in a function exported from aa
There was a problem hiding this comment.
I thought about it too, but note that there's no constraint on using aa anywhere here, it's up to the consumer of inspector to decide how to identify pckages.
| @@ -477,7 +487,8 @@ function aggregateDeps({ packageModules, moduleIdToModuleRecord }) { | |||
| return | |||
| } | |||
| // moduleRecord missing, guess package name | |||
| const packageName = guessPackageName(requestedName) | |||
| const packageName = | |||
There was a problem hiding this comment.
It's bothering me that I don't understand under what circumstances the moduleRecord would be missing. My intuition is that it should not be tolerated, and whatever is consuming the ModuleInspector should ensure it's providing clean data.
It's also bothering me that a) we ultimately fallback to <unknown:${requestedName}> which may (must?) have some consequence when the app runs under Lavamoat, and b) I don't know what that consequence is. My best guess is "useless placeholder".
That said, given we're generating policies here, I appreciate that there's some wiggle room.
Also: if we're providing the user some sort of actionable feedback when we need to throw <unknown:${requestedName}> in the policy, I think I'd feel better about it. If it's not actionable (and I don't see it referenced anywhere else in the repo) and serves no other purpose, why keep it?
There was a problem hiding this comment.
The unknown entry is surfacing a problem to the user. I don't think it could actually be used as a policy entry to make anything work, so it's signal where people actually look (they're much more likely to look there instead of in the terminal output). It's very likely that the overall policy is going to work, so throwing an error would not be a good tradeoff.
I'm also uncomfortable about the fact that there's a mismatch in package entry resolution in webpack apparently, but I wish to fix it if possible (or report if I find a minimal repro). The intention here is to provide a better fallback, because lavamoat overall is perfectly capable of resolving where the dependency belongs.
There's no security impact of this, because we're "rounding up" to the package level.
Also, when we go back to viz, it'll become valuable as a thing we can visualize as a potential problem and help explore. (hopefully after fixing the dummy mismatch in webpack)
This is generally useful for paving over incompatibilities.
In this particular situation, it helps solve a case where
connectionsin webpack contain a module entry that's identified by the path to its index.mjs entrypoint while the index.js entrypoint is actually used in the bundle, so inspector doesn't have a cached entry for this one and without the fallback handler would return<unknown:/...>A fix for this situation with less code reuse would be to replace the whole aggregateDeps logic. The benefit of that would be being able to avoid allocating moduleIdToModuleRecord. What discourages this change is increased reliance on canonicalNameMap and limiting portability of inspector.