CENTRAL - NODEAUDIT - POM Cached results never expire

[ejouvin]

Precondition

• I checked the issues list for existing open or closed reports of the same problem.

Describe the bug
When caches are activated for CENTRAL - NODEAUDIT and POM, results are cached but with the internal flag isEternal set to true.

Version of dependency-check used
The problem occurs using version 13.0.0 of the maven plugin, but it affects the core module

Log file

To Reproduce
Steps to reproduce the behavior:

1. Run a first dependency check, on a module containing node module and specially file package-lock.json, with default configuration. Cache should be created, in folder org/owasp/dependency-check-data/11.0/cache under maven respository
2. Wait 24h and expect there is new vulnerability on the module
3. Run again the check
4. The new vulnerability is not thrown

Expected behavior
Cache results should expire after the configuration time

Additional context
Difficult to check with log file or module, as it depends on known vulnerabilities.

The error is in the file dependencycheck-cache.properties

This will apply to "default" configuration

jcs.default.elementattributes.IsEternal=false

But the configuration is not set on region CENTRAL - POM - NODEAUDIT

But in the lib commons-jc3-core lib, default values are not applied as default value on region configuration. It is used when the configuration for the region does not provide the key jcs.region..elementattributes, usually with value org.apache.commons.jcs3.engine.ElementAttributes.

A temporary fix for the moment is to add a system variable in Maven command, like

-Djcs.region.CENTRAL.elementattributes.IsEternal=false
-Djcs.region.POM.elementattributes.IsEternal=false
-Djcs.region.NODEAUDIT.elementattributes.IsEternal=false

But if caches already contain result, the eternal flag is already set to true in cached result. Arguments from command line do not solve the issue.

Second solution, update the file dependencycheck-cache.properties with:

jcs.region.CENTRAL.elementattributes.IsEternal=false
jcs.region.POM.elementattributes.IsEternal=false
jcs.region.NODEAUDIT.elementattributes.IsEternal=false

But it will need to delete the existing cache, for the same reason as previous fix.

[chadlwilson]

This is intentional for CENTRAL and POM insofar as these results should be immutable upstream. NODEAUDIT IIRC it is only caching the discovered/parsed dependencies by a hash of the package.json to avoid reparsing if not changed. (edit: my mistake - mixed up with NodePackageAnalyer - 24h cache of vuln results is intentional)

I dont believe anything documents that CENTRAL or POM expire after 24h and there are not config options exposed for that?

However as I noted at #8737 (comment) it may be caching "not found" results too which would not be desirable.

Can you clarify what problem are you actually trying to solve?

[ejouvin]

But for the package.json, even if dependencies list does not change, new vulnerabilities can be registered.

We faced this kind of issue in our project.
One team member did the analysis on monday, cache is created.
Other team member did the analysis on friday and have critical vulnerabilities.
First team member run again the anaylsis, but as the result is eternal in its cache, critical vulnerabilities are not displayed.

If for CENTRAL and POM it is intentional, fine for me as I did not study / use them.

But in the configuration, we can see that

#30 day cache life for Central
jcs.region.CENTRAL.elementattributes.MaxLife=2592000
#90 day cache life for POM files from Central - this should likely be higher...
jcs.region.POM.elementattributes.MaxLife=7776000
#24 hour default cache life
jcs.region.NODEAUDIT.elementattributes.MaxLife=86400

So why putting configuration + comment to explain the cache life if results are eternal ?

I am preparing a Pull request, support for discussion.

[chadlwilson]

Ok, might have gotten mixed up with the NodePackageAnalyzer so yes, NodeAudit results were intended to be 24-hour cached.

Please be specific about what commands you are running and specifically which example packages you feel were giving stale results alongside your config/integration. If you can supply a debug or info log if what you are seeing, that'd help. Otherwise this is all theoretical.

The caching semantics are likely different depending on whether the project has a package.json or package-lock.json,.for example, so specifics matter.

Anyway, right now, the NODEAUDIT analyzer should be completely broken due to #8422 (unless they resurrected the API again...) so I'm not sure how you are getting any (new) results at all, unless this 2nd team member is actually running via npm audit directly rather than via ODC.

It does not make sense to raise a PR if you don't know what the desired behaviour is or how these caches are intended to work and interact with other analyzers. And given the NodeAuditAnalyzer is at best deprecated and at worst completely broken right now, making changes to it without fixing the core problems.with the analyzer doesn't make sense to me.

[ejouvin]

I will try to provide some input.

This simple Maven project contains the plugin definition

    <build>
        <plugins>
            <!-- Configure OWASP vulnerability analysis -->
            <plugin>
                <groupId>org.owasp</groupId>
                <artifactId>dependency-check-maven</artifactId>
                <version>13.0.0</version>
                <configuration>
                    <failBuildOnCVSS>8</failBuildOnCVSS>
                    <nodeAuditSkipDevDependencies>true</nodeAuditSkipDevDependencies>
                    <nodePackageSkipDevDependencies>true</nodePackageSkipDevDependencies>
                    <ossIndexAnalyzerEnabled>false</ossIndexAnalyzerEnabled>
                    <scanSet>
                        <fileSet>
                            <directory>src/main/frontend</directory>
                            <includes>
                                <include>package.json</include>
                                <include>package-lock.json</include>
                            </includes>
                        </fileSet>
                    </scanSet>
                </configuration>
            </plugin>
        </plugins>
    </build>

And there is only files package.json and package-lock.json

The executed Maven command is the following (after configuration for NVD database in settings or using proxy, but out of the scope of the current issue)
dependency-check:aggregate -X

I will focus on the analyzer Node Analyzer, and not the node package analyzer even if caches are created during node package analyzer.
On first execution, we will have those log messages

[DEBUG] Starting Node Audit Analyzer
[DEBUG] Parallel processing with up to 16 threads: Node Audit Analyzer.
[DEBUG] Begin Analysis of 'C:\AmexHub\sources\cache-study\src\main\frontend\package-lock.json' (Node Audit Analyzer)
[DEBUG] Empty dependency of package-lock v2+ removed
[WARNING] dependency skipped: package.json contain an alias for string-width-cjs => string-width@^4.2.0 npm audit doesn't support aliases
[WARNING] dependency skipped: package.json contain an alias for strip-ansi-cjs => strip-ansi@^6.0.1 npm audit doesn't support aliases
[WARNING] dependency skipped: package.json contain an alias for wrap-ansi-cjs => wrap-ansi@^7.0.0 npm audit doesn't support aliases
[DEBUG] Parsing JSON node
[WARNING] Unsupported CVSS vector format in NPM Audit results, please file a feature request at https://github.com/dependency-check/DependencyCheck/issues/new/choose to support vector format '' 
[WARNING] Unsupported CVSS vector format in NPM Audit results, please file a feature request at https://github.com/dependency-check/DependencyCheck/issues/new/choose to support vector format '' 
[WARNING] Unsupported CVSS vector format in NPM Audit results, please file a feature request at https://github.com/dependency-check/DependencyCheck/issues/new/choose to support vector format '' 
[INFO] Finished Node Audit Analyzer (0 seconds)

Caches are created under <MAVEN_REPOSITORY>/org/owasp/dependency-check-data/11.0/cache

For all others executions, message will be

[DEBUG] Starting Node Audit Analyzer
[DEBUG] Parallel processing with up to 16 threads: Node Audit Analyzer.
[DEBUG] Begin Analysis of 'C:\AmexHub\sources\cache-study\src\main\frontend\package-lock.json' (Node Audit Analyzer)
[DEBUG] Empty dependency of package-lock v2+ removed
[WARNING] dependency skipped: package.json contain an alias for string-width-cjs => string-width@^4.2.0 npm audit doesn't support aliases
[WARNING] dependency skipped: package.json contain an alias for strip-ansi-cjs => strip-ansi@^6.0.1 npm audit doesn't support aliases
[WARNING] dependency skipped: package.json contain an alias for wrap-ansi-cjs => wrap-ansi@^7.0.0 npm audit doesn't support aliases
[DEBUG] cache hit for node audit: 4d97ff64c7c07182300f21b2b74db6ebf573877707f87e37544238ebc2f8b461
[INFO] Finished Node Audit Analyzer (0 seconds)

You can see that the cache is hit, and this will be always the case.

In documentation, it is provided this
nodeAuditAnalyzerUseCache, Sets whether the Node Audit Analyzer will cache results. Cached results expire after 24 hours.

In the source code, the file dependencycheck-cache.properties contains this

#24 hour default cache life
jcs.region.NODEAUDIT.elementattributes.MaxLife=86400

So even if issue on NPM audit were faced (not reproduced on my side) the subject is about the fact that documentation announced a time to live of 24h and this is not the case.

The reason is because cache region configuration does not specify the isEternal value. It is set for the "default"

jcs.default.elementattributes.IsEternal=false

But not on region CENTRAL - NODEAUDIT and POM.

As a technical study.
In any manner, caches are loaded from class org.owasp.dependencycheck.data.cache.DataCacheFactory, executed from any analyzer.
The file dependencycheck-cache.properties is loaded into a Properties instance, sent to JCS.

Cache instances are retrieved from function getNodeAuditCache, getPomCache or getCentralCache from class org.owasp.dependencycheck.data.cache.DataCacheFactory with a exection like

final CacheAccess<String, List<Advisory>> ca = JCS.getInstance("<REGION_NAME>", attr);

In the JCS class

    public static <K, V> CacheAccess<K, V> getInstance( final String region, final ICompositeCacheAttributes icca )
        throws CacheException
    {
        final CompositeCache<K, V> cache = getCacheManager().getCache( region, icca );
        return new CacheAccess<>( cache );
    }

Let see getCacheManager

    private static CompositeCacheManager getCacheManager() throws CacheException
    {
        synchronized ( JCS.class )
        {
            if ( cacheMgr == null || !cacheMgr.isInitialized())
            {
                if ( configProps != null )
                {
                    cacheMgr = CompositeCacheManager.getUnconfiguredInstance();
                    cacheMgr.configure( configProps );
                }
                else if ( configFilename != null )
                {
                    cacheMgr = CompositeCacheManager.getUnconfiguredInstance();
                    cacheMgr.configure( configFilename );
                }
                else
                {
                    cacheMgr = CompositeCacheManager.getInstance();
                }
            }

            return cacheMgr;
        }
    }

As the variable configProps is not null, it will be used to configure caches. This variable contains definitions from file dependencycheck-cache.properties

    public void configure( final Properties props )
    {
        configure( props, DEFAULT_USE_SYSTEM_PROPERTIES );
    }

It will end in that function, with value true into useSystemProperties (that help us to fix the eternal flag issue)

    public synchronized void configure( final Properties props, final boolean useSystemProperties, final boolean forceReconfiguration )
    {
        if ( props == null )
        {
            log.error( "No properties found. Please configure the cache correctly." );
            return;
        }

        if ( isConfigured )
        {
            if ( !forceReconfiguration )
            {
                log.debug( "Configure called after the manager has been configured.  "
                         + "Force reconfiguration is false. Doing nothing" );
                return;
            }
            log.info( "Configure called after the manager has been configured.  "
                    + "Force reconfiguration is true. Reconfiguring as best we can." );
        }
        if ( useSystemProperties )
        {
            CompositeCacheConfigurator.overrideWithSystemProperties( props );
        }
        doConfigure( props );
    }

Function doConfigure does not provide useful information, just it creates defaultElementAttr and execute configurator.parseRegions to create region caches.

    private synchronized void doConfigure( final Properties properties )
    {
        // We will expose this for managers that need raw properties.
        this.configurationProperties = properties;

        // set the props value and then configure the ThreadPoolManager
        ThreadPoolManager.setProps( properties );
        final ThreadPoolManager poolMgr = ThreadPoolManager.getInstance();
        log.debug( "ThreadPoolManager = {0}", poolMgr);

        // Create event queue
        this.elementEventQueue = new ElementEventQueue();

        // configure the cache
        final CompositeCacheConfigurator configurator = newConfigurator();

        final ElapsedTimer timer = new ElapsedTimer();

        // set default value list
        this.defaultAuxValues = OptionConverter.findAndSubst( CompositeCacheManager.DEFAULT_REGION,
                properties );

        log.info( "Setting default auxiliaries to \"{0}\"", this.defaultAuxValues );

        // set default cache attr
        this.defaultCacheAttr = configurator.parseCompositeCacheAttributes( properties, "",
                new CompositeCacheAttributes(), DEFAULT_REGION );

        log.info( "setting defaultCompositeCacheAttributes to {0}", this.defaultCacheAttr );

        // set default element attr
        this.defaultElementAttr = configurator.parseElementAttributes( properties, "",
                new ElementAttributes(), DEFAULT_REGION );

        log.info( "setting defaultElementAttributes to {0}", this.defaultElementAttr );

        // set up system caches to be used by non system caches
        // need to make sure there is no circularity of reference
        configurator.parseSystemRegions( properties, this );

        // setup preconfigured caches
        configurator.parseRegions( properties, this );

        log.info( "Finished configuration in {0} ms.", timer::getElapsedTime);

        isConfigured = true;
    }

As expected the default element attribute as eternal flag set to false.

In classe org.apache.commons.jcs3.engine.control.CompositeCahceConfigurator, the parseRegions contains this

    protected void parseRegions( final Properties props, final CompositeCacheManager ccm )
    {
        final List<String> regionNames = new ArrayList<>();

        for (final String key : props.stringPropertyNames() )
        {
            if ( key.startsWith( REGION_PREFIX ) && key.indexOf( "attributes" ) == -1 )
            {
                final String regionName = key.substring( REGION_PREFIX.length() );
                regionNames.add( regionName );
                final String auxiliaries = OptionConverter.findAndSubst( key, props );
                final ICache<?, ?> cache;
                synchronized ( regionName )
                {
                    cache = parseRegion( props, ccm, regionName, auxiliaries );
                }
                ccm.addCache( regionName, cache );
            }
        }

        log.info( "Parsed regions {0}", regionNames );
    }

The only objective here is to identify a property jcs.region.REGION_NAME.
Extract region name and execute the function parseRegion.

The function is pretty long.

    protected <K, V> CompositeCache<K, V> parseRegion(
            final Properties props, final CompositeCacheManager ccm, final String regName, final String auxiliaries,
            final ICompositeCacheAttributes cca, final String regionPrefix )
    {
        // First, create or get the cache and element attributes, and create
        // the cache.
        final IElementAttributes ea = parseElementAttributes( props, regName,
                ccm.getDefaultElementAttributes(), regionPrefix );

        final ICompositeCacheAttributes instantiationCca = cca == null
                ? parseCompositeCacheAttributes(props, regName, ccm.getDefaultCacheAttributes(), regionPrefix)
                : cca;
        final CompositeCache<K, V> cache = newCache(instantiationCca, ea);

        // Inject cache manager
        cache.setCompositeCacheManager(ccm);

        // Inject scheduler service
        cache.setScheduledExecutorService(ccm.getScheduledExecutorService());

        // Inject element event queue
        cache.setElementEventQueue(ccm.getElementEventQueue());

        if (auxiliaries != null)
        {
            // Next, create the auxiliaries for the new cache
            final List<AuxiliaryCache<K, V>> auxList = new ArrayList<>();

            log.debug( "Parsing region name \"{0}\", value \"{1}\"", regName, auxiliaries );

            String auxNames[] = auxiliaries.split("\\s*,\\s*");

            // just to be on the safe side...
            if (auxNames.length == 0)
            {
                return null;
            }

            for (String auxName : auxNames)
            {
                if (auxName.isEmpty())
                {
                    log.warn( "Skipping empty auxiliary name" );
                    continue;
                }

                log.debug( "Parsing auxiliary named \"{0}\".", auxName );

                AuxiliaryCache<K, V> auxCache = parseAuxiliary( props, ccm, auxName, regName );

                if ( auxCache != null )
                {
                    if (auxCache instanceof IRequireScheduler)
                    {
                        ((IRequireScheduler) auxCache).setScheduledExecutorService(
                                ccm.getScheduledExecutorService());
                    }

                    auxList.add( auxCache );
                }
            }

            // Associate the auxiliaries with the cache
            cache.setAuxCaches(auxList);
        }

        // Return the new cache
        return cache;
    }

But the important part is how the variable ea is created with function parseElementAttributes

    protected IElementAttributes parseElementAttributes( final Properties props, final String regName,
            final IElementAttributes defaultEAttr, final String regionPrefix )
    {
        IElementAttributes eAttr;

        final String attrName = regionPrefix + regName + CompositeCacheConfigurator.ELEMENT_ATTRIBUTE_PREFIX;

        // auxFactory was not previously initialized.
        // String prefix = regionPrefix + regName + ATTRIBUTE_PREFIX;
        eAttr = OptionConverter.instantiateByKey( props, attrName, null );
        if ( eAttr == null )
        {
            log.info( "No special ElementAttribute class defined for key [{0}], "
                    + "using default class.", attrName );

            eAttr = defaultEAttr;
        }

        log.debug( "Parsing options for \"{0}\"", attrName );

        PropertySetter.setProperties( eAttr, props, attrName + "." );
        // eAttr.setCacheName( regName );

        log.debug( "End of parsing for \"{0}\"", attrName );

        // GET CACHE FROM FACTORY WITH ATTRIBUTES
        // eAttr.setCacheName( regName );
        return eAttr;
    }

For a region, properties starting with jcs.region.REGION_NAME.elementattributes are retrieved and use OptionConverter tools to create an instance of IElementAttributes. In this case, it is something like

And that's where the issue is. You can see that the flag IS_ETERNAL is set to true.
So any result store in the associated cache will be eternal and never refreshed. Even if the configuration is updated after, it will require to delete files associated to the region.

For us, a possible fix was to introduce three argument in the Maven command and make sure cache files do not exist before the first execution.

-Djcs.region.CENTRAL.elementattributes.IsEternal=false -Djcs.region.NODEAUDIT.elementattributes.IsEternal=false -Djcs.region.POM.elementattributes.IsEternal=false

The initial goal of this issue and the pull request is to only solve that point. No matter if NPM audit is "deprecated" or not, as this analysis is applied to the three caches CENTRAL, NPM and POM.

Hope it will help.

cache-demo.zip

[chadlwilson]

Thanks for the extra details - but you're missing the point. I'm not disputing that there may be a problem/bug.

I was asking you to start with your observed problem and how to reproduce it via standard ODC commands, not your detailed diagnosis of the current code and how to fix it. This is pretty basic "how to raise an issue with software on the internet" stuff. The demo somewhat helps, but it really shouldn't take so many messages to get there.

Here's why:

1. In fix: Add eternal configuration for NODEAUDIT cache results. #8758 you have raised a PR, but with zero tests to prevent regression. We want to avoid regressing behaviour again, right? This was probably working at some point, but then JCS or ODC changed the semantics of whether configuration for cache regions is correctly inherited. The code you show for CompositeCacheConfigurator shows that it's supposed to take default attributes in some sense; the intention is that these apply, including IsEternal.

DependencyCheck/core/src/main/resources/dependencycheck-cache.properties

Lines 1 to 18 in 4aacdb1

	# DEFAULT CACHE REGION
	jcs.default=ODC
	jcs.default.cacheattributes=org.apache.commons.jcs3.engine.CompositeCacheAttributes
	jcs.default.cacheattributes.MemoryCacheName=org.apache.commons.jcs3.engine.memory.lru.LRUMemoryCache
	jcs.default.cacheattributes.UseMemoryShrinker=false
	jcs.default.cacheattributes.MaxMemoryIdleTimeSeconds=3600
	jcs.default.cacheattributes.ShrinkerIntervalSeconds=60
	jcs.default.elementattributes=org.apache.commons.jcs3.engine.ElementAttributes
	jcs.default.elementattributes.IsEternal=false
	# use zero max objects with an update pattern to force disk caching
	jcs.default.cacheattributes.MaxObjects=0
	jcs.default.cacheattributes.DiskUsagePattern=UPDATE
	#24 hour default cache life
	jcs.default.elementattributes.MaxLife=86400
	jcs.default.elementattributes.IdleTime=1800
	jcs.default.elementattributes.IsSpool=true
	jcs.default.elementattributes.IsRemote=false
	jcs.default.elementattributes.IsLateral=false

Maybe that's not working as intended which is why there is these what looks like hacks to override config:

DependencyCheck/core/src/main/java/org/owasp/dependencycheck/data/cache/DataCacheFactory.java

Lines 132 to 134 in d1d8b99

	attr.setUseDisk(true);
	attr.setUseLateral(false);
	attr.setUseRemote(false);

So we should try and fix it properly, and ensure no regressions.

The initial goal of this issue and the pull request is to only solve that point. No matter if NPM audit is "deprecated" or not, as this analysis is applied to the three caches CENTRAL, NPM and POM.

OK, but maintainers and triagers have other things to worry about. It does in fact "matter" to me, OK? And your PR only address NODEAUDIT, so does not in fact deal with CENTRAL or POM caches, meaning the PR and issue are not scoped consistently with one another.

2. We have to do significant refactoring to resolve Remove legacy NodeAuditAnalyzer dependency on NPM audit legacy /quick API #8422 and we need to make sure the user-facing semantics are retained - NOT whether or not it uses JCS to cache results. There is a PR with work-in-progress to do so I've already spent time on at fix: reinstate the NodeAuditAnalyzer via use of npm audit via CLI #8725 but we have to re-assess whether we can/should even be caching the results; since we will likely be relying on npm CLI to do the work.

That means any caching semantics based on file hashes needs to be totally re-evaluated and is quite likely a bad idea, and is quite likely to be removed.

• PNPM and Yarn have no such caches
• the new NPM API is a bulk query that is unlikely to need caching
• intention is to delegate to npm CLI, so caching will be brittle and couple ODC to underlying semantics of NPM CLI)

Thus spending time reviewing or testing this now could be a waste of maintainer time, and should be re-evaluated afterwards. But that depends on properly understanding what you are seeing.

[ejouvin]

Fine for me.
Do with your backlog and planned updates and let cancel the pull request.