Skip to content

fix: CodeQL CDS extractor support for private @sap/cds-indexer package installs via .npmrc#347

Merged
data-douser merged 12 commits intomainfrom
dd/cds-extractor/2
Mar 30, 2026
Merged

fix: CodeQL CDS extractor support for private @sap/cds-indexer package installs via .npmrc#347
data-douser merged 12 commits intomainfrom
dd/cds-extractor/2

Conversation

@data-douser
Copy link
Copy Markdown
Collaborator

@data-douser data-douser commented Mar 30, 2026

What This PR Contributes

Addresses a bug in the CDS extractor where .npmrc config files were not being copied or respected for CDS extractor installs of scanned CAP project dependencies, meaning that our "best effort" support for @sap/cds-indexer packages was not working as the CDS extractor was only trying to download packages from npmjs.org -- regardless of a project's .npmrc config.

Summary of Changes

This pull request introduces improvements to the CDS package manager's dependency installation process and enhances the JavaScript extractor's pre-finalization script. The main focus is on ensuring that custom npm configuration is respected during dependency installation and on expanding file indexing for better CodeQL analysis.

Outline of Changes

Dependency installation robustness:

  • Added findNearestNpmrc and copyNpmrcToCache functions in cacheInstaller.ts to locate and copy the nearest .npmrc file into the cache directory. This ensures that npm install in the cache respects custom registry settings, authentication tokens, and SSL configurations.
  • Updated the cacheInstallDependencies function to invoke copyNpmrcToCache before installing dependencies, improving support for projects with custom npm configurations.
  • Exported the new helper functions from index.ts for use in other modules.
  • Added comprehensive unit tests for findNearestNpmrc and copyNpmrcToCache (including error handling when the copy fails), and for the .npmrc propagation path in cacheInstallDependencies.

Extractor pre-finalization enhancements:

  • Updated pre-finalize.cmd to add informative logging around the indexing of .cds files and to include new steps for indexing UI5 .view.xml and .fragment.xml files, ensuring these are included in the CodeQL database.
  • Fixed pre-finalize.cmd to check ERRORLEVEL after each codeql database index-files invocation and short-circuit with a non-zero exit code if any step fails, so a failure in CDS indexing is no longer masked by subsequent UI5 indexing.

Future Works


📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.

data-douser and others added 7 commits March 27, 2026 21:12
Adds a full complement of .github/** files for leveraging
AI agents with the ql-mcp server for more efficient development
of features and troubleshooting of problems related to SAP
frameworks such as CAP / CDS, UI5, and XSJS.

Sets up the 'advanced-security/codeql-sap-js' repo for ongoing
"agentic maintenance", aka

Resolves #259
…nd UI5 prompt outer fences (#336)

* Initial plan

* Address review feedback: fix yamllint commands, agent structure diagram, and UI5 prompt fences

Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>
Agent-Logs-Url: https://github.com/advanced-security/codeql-sap-js/sessions/404b2862-c982-4730-894b-d434ad2bb1ed

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>
…e in copilot-setup-steps workflow

Agent-Logs-Url: https://github.com/advanced-security/codeql-sap-js/sessions/baa532ae-dce3-46f4-8f65-9200ae19135c

Co-authored-by: mbaluda <5237080+mbaluda@users.noreply.github.com>
Base automatically changed from dd/copilot-setup/2 to main March 30, 2026 01:56
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes CDS extractor dependency installation so private @sap/cds-indexer installs can respect project npm configuration (e.g., custom registries via .npmrc), and extends JS extractor pre-finalization indexing to include common UI5 XML view/fragment files.

Changes:

  • Add .npmrc discovery/copy helpers and integrate them into CDS dependency cache installation.
  • Export the new helpers from the CDS package manager entrypoint.
  • Extend pre-finalize.cmd to index UI5 .view.xml / .fragment.xml files (in addition to .cds indexing).

Reviewed changes

Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.

File Description
extractors/javascript/tools/pre-finalize.cmd Adds UI5 XML indexing and extra logging around index-files steps.
extractors/cds/tools/src/packageManager/index.ts Re-exports new .npmrc helper utilities from the package manager module.
extractors/cds/tools/src/packageManager/cacheInstaller.ts Implements .npmrc lookup/copy and wires it into cache-based dependency installation.
extractors/cds/tools/dist/cds-extractor.bundle.js Regenerated bundle reflecting the package manager changes.
Comments suppressed due to low confidence (3)

extractors/cds/tools/src/packageManager/cacheInstaller.ts:76

  • findNearestNpmrc walks all the way up to the filesystem root. Since it’s called with a path under sourceRoot, this can unintentionally pick up an .npmrc outside the analyzed project (e.g., a parent directory or user home), which may contain credentials and different registry settings. Consider bounding the search to sourceRoot (or another explicit stop directory) and ignore any .npmrc found outside that boundary.
export function findNearestNpmrc(startDir: string): string | undefined {
  let current = resolve(startDir);

  // Walk up the directory tree until we find an .npmrc or reach the root

  while (true) {
    const candidate = join(current, '.npmrc');
    if (existsSync(candidate)) {
      return candidate;
    }
    const parent = dirname(current);
    if (parent === current) {
      // Reached filesystem root without finding .npmrc
      return undefined;
    }
    current = parent;
  }

extractors/cds/tools/src/packageManager/cacheInstaller.ts:103

  • Copying a project .npmrc into the cache directory can persist authentication tokens under ${sourceRoot}/.cds-extractor-cache/... beyond the install, increasing the risk of credential exposure (e.g., via workspace artifacts or later inspection). Prefer passing npm config via npm --userconfig / NPM_CONFIG_USERCONFIG, or ensure the copied .npmrc is removed immediately after the install completes (success or failure).
export function copyNpmrcToCache(cacheDir: string, projectDir: string): void {
  const npmrcPath = findNearestNpmrc(projectDir);
  if (!npmrcPath) {
    return;
  }

  const dest = join(cacheDir, '.npmrc');
  try {
    copyFileSync(npmrcPath, dest);
    cdsExtractorLog('info', `Copied .npmrc from '${npmrcPath}' to cache directory '${cacheDir}'`);
  } catch (err) {
    cdsExtractorLog(
      'warn',
      `Failed to copy .npmrc to cache directory: ${err instanceof Error ? err.message : String(err)}`,
    );
  }

extractors/cds/tools/src/packageManager/cacheInstaller.ts:268

  • copyNpmrcToCache is only called when the cache directory is first created. If the cache directory already exists but node_modules is missing/partial (e.g., previous failed run), the subsequent npm install will run without the intended .npmrc and can still fail to reach private registries. Invoke the npmrc setup right before running npm install (when nodeModulesExists is false), not only during initial directory creation.
      // Copy the project's .npmrc (if any) so npm respects custom registries
      const firstProjectDir = Array.from(dependencyGraph.projects.keys())[0];
      if (firstProjectDir) {
        copyNpmrcToCache(cacheDir, join(sourceRoot, firstProjectDir));
      }
    }

Comment thread extractors/cds/tools/src/packageManager/cacheInstaller.ts
Comment thread extractors/javascript/tools/pre-finalize.cmd
@data-douser
Copy link
Copy Markdown
Collaborator Author

@copilot apply changes based on the comments in this thread

Copilot AI and others added 3 commits March 30, 2026 02:20
Agent-Logs-Url: https://github.com/advanced-security/codeql-sap-js/sessions/8906d85d-2830-47c2-b48a-447b735a4538

Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>
…ve accidentally committed .codeql-version

Agent-Logs-Url: https://github.com/advanced-security/codeql-sap-js/sessions/8906d85d-2830-47c2-b48a-447b735a4538

Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 30, 2026

@copilot apply changes based on the comments in this thread

Done in commits 5fda254 and 7dac699:

cacheInstaller.ts tests (5fda254): Added three new test suites covering all requested cases:

  • findNearestNpmrc: verifies it finds .npmrc in the start directory, a parent directory, and returns undefined when none exists
  • copyNpmrcToCache: verifies successful copy, no-op when no .npmrc found, and graceful warning (no throw) when copyFileSync fails
  • cacheInstallDependencies .npmrc propagation: verifies .npmrc is copied before npm install, npm install still runs when the copy fails, and copyFileSync is not called when no .npmrc is found

pre-finalize.cmd (5fda254 / 7dac699): Added if %ERRORLEVEL% neq 0 (exit /b %ERRORLEVEL%) checks after each codeql database index-files invocation (CDS and UI5) so any failure short-circuits the script with a non-zero exit code. The final exit /b %ERRORLEVEL% now reflects the last command's result rather than unconditionally returning 0.

@data-douser data-douser reopened this Mar 30, 2026
@data-douser data-douser marked this pull request as ready for review March 30, 2026 12:16
@data-douser data-douser self-assigned this Mar 30, 2026
@data-douser data-douser added bug Something isn't working dependencies Pull requests that update a dependency file javascript Pull requests that update javascript code labels Mar 30, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated 2 comments.

Comment thread extractors/javascript/tools/pre-finalize.cmd Outdated
Comment thread extractors/cds/tools/src/packageManager/cacheInstaller.ts Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.

Comment thread extractors/cds/tools/test/src/packageManager/cacheInstaller.test.ts
@data-douser data-douser disabled auto-merge March 30, 2026 14:34
@data-douser data-douser merged commit 89bd0bf into main Mar 30, 2026
20 checks passed
@data-douser data-douser deleted the dd/cds-extractor/2 branch March 30, 2026 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working dependencies Pull requests that update a dependency file javascript Pull requests that update javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants