Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(core): add external package name fallback function to options in generatePolicy, use it in webpack #1109
feat(core): add external package name fallback function to options in generatePolicy, use it in webpack #1109
Changes from 1 commit
fe2e6f4
e38f10c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 theModuleInspector
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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.
the LHS of the
isNotPackageName
assignment feels like it should be in a function exported fromaa
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 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.