Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(core): add external package name fallback function to options in generatePolicy, use it in webpack #1109

Merged
merged 2 commits into from Mar 20, 2024
Merged

feat(core): add external package name fallback function to options in generatePolicy, use it in webpack #1109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe2e6f4
feat(core): add external package name fallback function to options in…
naugtur Mar 18, 2024
e38f10c
chore(core): improved types
naugtur Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 33 additions & 17 deletions packages/core/src/generatePolicy.js
View file
Open in desktop
Expand Up @@ -348,7 +348,11 @@ function createModuleInspector(opts) {
/**
* @type {GeneratePolicyFn}
*/
function generatePolicy({ policyOverride, includeDebugInfo = false }) {
function generatePolicy({
policyOverride,
includeDebugInfo = false,
moduleToPackageFallback,
}) {
/** @type {import('./schema').Resources} */
const resources = {}
/**
Expand All @@ -375,6 +379,7 @@ function createModuleInspector(opts) {
const packageDeps = aggregateDeps({
packageModules,
moduleIdToModuleRecord,
moduleToPackageFallback,
})
if (packageDeps.length) {
packages = Object.fromEntries(
Expand Down Expand Up @@ -444,19 +449,27 @@ function createModuleInspector(opts) {
}

/**
* @param {{
* packageModules: Record<
* string,
* import('./moduleRecord').LavamoatModuleRecord
* >
* moduleIdToModuleRecord: Map<
* string,
* import('./moduleRecord').LavamoatModuleRecord
* >
* }} opts
* @returns
* @callback ModuleToPackageFallbackFn
* @param {string} requestedName
* @returns {string | undefined}
*/

/**
* @typedef {Object} AggregateDepsOptions
* @property {Record<string, import('./moduleRecord').LavamoatModuleRecord>} packageModules
* @property {Map<string, import('./moduleRecord').LavamoatModuleRecord>} moduleIdToModuleRecord
* @property {ModuleToPackageFallbackFn} [moduleToPackageFallback]
*/
function aggregateDeps({ packageModules, moduleIdToModuleRecord }) {

/**
* @param {AggregateDepsOptions} opts
* @returns {string[]}
*/
function aggregateDeps({
packageModules,
moduleIdToModuleRecord,
moduleToPackageFallback = guessPackageName,
}) {
const deps = new Set()
// get all dep package from the "packageModules" collection of modules
Object.values(packageModules).forEach((moduleRecord) => {
Expand All @@ -477,7 +490,8 @@ function aggregateDeps({ packageModules, moduleIdToModuleRecord }) {
return
}
// moduleRecord missing, guess package name
const packageName = guessPackageName(requestedName)
const packageName =
Copy link
Contributor

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 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?

Copy link
Member Author

@naugtur naugtur Mar 20, 2024 •
edited

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)

moduleToPackageFallback(requestedName) || `<unknown:${requestedName}>`
deps.add(packageName)
}
)
Expand All @@ -493,14 +507,13 @@ function aggregateDeps({ packageModules, moduleIdToModuleRecord }) {
* For when you encounter a `requestedName` that was not inspected, likely
* because resolution was skipped for that module
*
* @param {string} requestedName
* @returns {string}
* @type {ModuleToPackageFallbackFn}
*/
function guessPackageName(requestedName) {
const isNotPackageName =
requestedName.startsWith('/') || requestedName.startsWith('.')
if (isNotPackageName) {
return `<unknown:${requestedName}>`
return
Copy link
Contributor

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 from aa

Copy link
Member Author

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.

}
// resolving is skipped so guess package name
const pathParts = requestedName.split('/')
Expand Down Expand Up @@ -530,7 +543,9 @@ function getDefaultPaths(policyName) {
* @callback GeneratePolicyFn
* @param {Partial<ModuleInspectorOptions> & {
* policyOverride?: import('./schema').LavaMoatPolicyOverrides
* moduleToPackageFallback?: (value: string) => string | undefined
* }} opts
*
* @returns {import('./schema').LavaMoatPolicy
* | import('./schema').LavaMoatPolicyDebug}
*/
Expand All @@ -545,6 +560,7 @@ function getDefaultPaths(policyName) {
* @typedef ModuleInspectorOptions
* @property {(value: string) => boolean} isBuiltin
* @property {boolean} [includeDebugInfo]
* @property {(specifier: string) => string | undefined} [moduleToPackageFallback]
*/

/**
Expand Down
10 changes: 8 additions & 2 deletions packages/webpack/src/buildtime/policyGenerator.js
View file
Open in desktop
Expand Up @@ -52,7 +52,7 @@ module.exports = {
if (policyFromOptions) {
// TODO: avoid loading the policy file if policyFromOptions is present
final = policyFromOptions
} else {
} else if (policy) {
final = applyOverride(policy)
}
if (emit) {
Expand All @@ -67,12 +67,18 @@ module.exports = {

// load policy file
// load overrides
// generate policy if requested and write to file
// generate policy if requested and write to fileWe're not aware of any
// merge result with overrides
// return that and emit snapshot
const moduleInspector = createModuleInspector({
isBuiltin: () => false,
includeDebugInfo: false,
// If the specifier is requested as a dependency in importMap but was never passed to inspectModule, its package name will be looked up here.
// This is a workaround to inconsistencies in how webpack represents connections.
// Specifically what happened to surface this issue: `connections` in 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:/...>
// There should be no security implications of this, since the package is already resolved clearly and this is only a part of policy generation, not runtime.
moduleToPackageFallback: (specifier) =>
getPackageNameForModulePath(canonicalNameMap, specifier),
})

return {
Expand Down