Refactor audit-utils and add bulk advisory fallback (async)#340
Refactor audit-utils and add bulk advisory fallback (async)#340
Conversation
… pnpm audit - Added fallback to npm bulk advisory endpoint when pnpm audit endpoint is retired. - Improved resilience by parsing legacy and new audit outputs uniformly. - Updated audit utilities to fetch installed package versions and query bulk advisories. - Enhanced validate-audit.js to await asynchronous runAuditJson for advisory validation. - Added comprehensive tests for the shared audit helper including bulk advisory fallback. - Updated Makefile audit target to run audit:validate for consistent validation. This enables seamless audit compliance checks despite registry audit endpoint changes. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAttempt Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI / Makefile
participant AuditUtil as audit-utils
participant PnpmAudit as pnpm audit (--json)
participant PnpmLS as pnpm ls (--json --depth Infinity)
participant Registry as Registry / pnpm config
participant BulkAPI as npm bulk advisory endpoint
participant Validator as validate-audit
CLI->>AuditUtil: Invoke runAuditJson()
AuditUtil->>PnpmAudit: Execute pnpm audit --json
alt pnpm audit succeeds
PnpmAudit-->>AuditUtil: JSON advisories + status
AuditUtil->>Validator: Return advisories and status
else pnpm audit retired (ERR_PNPM_AUDIT_BAD_RESPONSE)
PnpmAudit-->>AuditUtil: Retired endpoint error
AuditUtil->>Registry: Resolve registry URL (env or pnpm config)
AuditUtil->>PnpmLS: Execute pnpm ls --json --depth Infinity
PnpmLS-->>AuditUtil: Dependency tree JSON
AuditUtil->>BulkAPI: POST package/version list to bulk endpoint
BulkAPI-->>AuditUtil: Advisory array (or error)
AuditUtil->>AuditUtil: Normalise advisories (extract GHSA IDs, de-duplicate)
AuditUtil->>Validator: Return normalised advisories and status
end
Validator->>Validator: Evaluate and report results
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideRefactors the shared audit utilities to support a bulk advisory fallback when pnpm’s legacy audit endpoint is retired, updates the frontend and security validation flows to use the new async helper, wires the Makefile to the validation entry point, and adds tests and docs around the new behavior. Sequence diagram for audit flow with bulk advisory fallbacksequenceDiagram
actor Developer
participant Makefile
participant FrontendAudit as frontend_run_audit_mjs
participant SecurityValidator as security_validate_audit_js
participant AuditUtils as security_audit_utils_js
participant Pnpm as pnpm_CLI
participant Registry as npm_registry
Developer->>Makefile: make audit
Makefile->>Pnpm: pnpm -r install / run audit
Makefile->>Pnpm: pnpm run audit:validate
Pnpm->>SecurityValidator: execute validate-audit.js
rect rgb(235, 245, 255)
SecurityValidator->>AuditUtils: await runAuditJson()
AuditUtils->>Pnpm: spawnSync pnpm audit --json
Pnpm-->>AuditUtils: stdout, status
AuditUtils->>AuditUtils: parseJsonOutput(stdout, pnpm_audit)
alt legacy_audit_endpoint_retired
AuditUtils->>AuditUtils: collectInstalledPackageVersions()
AuditUtils->>Pnpm: spawnSync pnpm ls --json --depth Infinity
Pnpm-->>AuditUtils: dependency_tree_json
AuditUtils->>AuditUtils: walkDependencyTree / walkDependencySection
AuditUtils->>AuditUtils: readRegistryUrl()
alt registry_env_set
AuditUtils->>AuditUtils: normaliseRegistryUrl(env_registry)
else registry_env_missing
AuditUtils->>Pnpm: execFileSync pnpm config get registry
Pnpm-->>AuditUtils: registry_url
AuditUtils->>AuditUtils: normaliseRegistryUrl(registry_url)
end
AuditUtils->>Registry: POST bulk_advisories (installed_versions)
Registry-->>AuditUtils: bulk_advisories_json
AuditUtils->>AuditUtils: normaliseBulkAdvisories()
AuditUtils-->>SecurityValidator: { json:{advisories}, status }
else legacy_audit_endpoint_available
AuditUtils-->>SecurityValidator: { json, status }
end
end
SecurityValidator->>SecurityValidator: collectAdvisories(json)
SecurityValidator->>SecurityValidator: validateAgainstExceptionLedger()
SecurityValidator-->>Pnpm: exit_code
Pnpm-->>Makefile: exit_code
Makefile-->>Developer: audit result
Note over Developer,Registry: Frontend script frontend_run_audit_mjs uses
Note over Developer,Registry: the same runAuditJson helper and fallback path.
Class diagram for updated audit-utils and consumersclassDiagram
class AuditUtils {
<<module>>
+parseJsonOutput(stdout, commandLabel)
+isRetiredAuditEndpoint(payload)
+isLocalWorkspaceVersion(version)
+shouldSkipPackageVersion(packageName, version)
+addPackageVersion(versionsByPackage, packageName, version)
+processDependencySection(section, versionsByPackage)
+walkDependencySection(section, versionsByPackage)
+walkDependencyTree(node, versionsByPackage)
+collectInstalledPackageVersions()
+normaliseRegistryUrl(rawRegistry)
+readRegistryUrl()
+extractGithubAdvisoryId(advisoryUrl)
+deriveAdvisoryKey(packageName, advisory)
+addPackageAdvisories(packageName, packageAdvisories, advisories)
+normaliseBulkAdvisories(bulkPayload)
+runBulkAdvisoryAudit()
+runAuditJson()
}
class FrontendAuditRunner {
<<script frontend_pwa_scripts_run_audit_mjs>>
+collectAdvisories(json)
+evaluateAudit(payload, options)
+main()
}
class SecurityAuditValidator {
<<script security_validate_audit_js>>
+assertValidSchema(entries)
+assertNoExpired(entries)
+validateAudit(entries, advisories)
}
class PnpmCLI {
<<external>>
+audit_json()
+ls_json_depth_Infinity()
+config_get_registry()
}
class NpmRegistry {
<<external>>
+bulkAdvisoriesEndpoint
}
AuditUtils ..> PnpmCLI : uses_spawnSync_execFileSync
AuditUtils ..> NpmRegistry : uses_bulk_advisory_endpoint
FrontendAuditRunner ..> AuditUtils : uses_runAuditJson
SecurityAuditValidator ..> AuditUtils : uses_runAuditJson
SecurityAuditValidator ..> FrontendAuditRunner : uses_collectAdvisories
note for AuditUtils "Exports asynchronous runAuditJson that encapsulates native pnpm audit and the bulk advisory fallback path."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Bumpy Road Aheadsecurity/audit-utils.js: walkDependencyTree What lead to degradation?walkDependencyTree has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function Why does this problem occur?A Bumpy Road is a function that contains multiple chunks of nested conditional logic inside the same function. The deeper the nesting and the more bumps, the lower the code health. How to fix it?Bumpy Road implementations indicate a lack of encapsulation. Check out the detailed description of the Bumpy Road code health issue. |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Bumpy Road Aheadsecurity/audit-utils.js: normaliseBulkAdvisories What lead to degradation?normaliseBulkAdvisories has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function Why does this problem occur?A Bumpy Road is a function that contains multiple chunks of nested conditional logic inside the same function. The deeper the nesting and the more bumps, the lower the code health. How to fix it?Bumpy Road implementations indicate a lack of encapsulation. Check out the detailed description of the Bumpy Road code health issue. |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Complex Conditionalsecurity/audit-utils.js: addPackageVersion What lead to degradation?addPackageVersion has 1 complex conditionals with 2 branches, threshold = 2 Why does this problem occur?A complex conditional is an expression inside a branch such as an if-statmeent which consists of multiple, logical operations. Example: if (x.started() && y.running()).Complex conditionals make the code even harder to read, and contribute to the Complex Method code smell. Encapsulate them. How to fix it?Apply the DECOMPOSE CONDITIONAL refactoring so that the complex conditional is encapsulated in a separate function with a good name that captures the business rule. Optionally, for simple expressions, introduce a new variable which holds the result of the complex conditional. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# complex_conditional.js
function messageReceived(message, timeReceived) {
- // Ignore all messages which aren't from known customers:
- if (!message.sender &&
- customers.getId(message.name) == null) {
+ // Refactoring #1: encapsulate the business rule in a
+ // function. A clear name replaces the need for the comment:
+ if (!knownCustomer(message)) {
log('spam received -- ignoring');
return;
}
- // Provide an auto-reply when outside business hours:
- if ((timeReceived.getHours() > 17) ||
- (timeReceived.getHours() < 8)) {
+ // Refactoring #2: encapsulate the business rule.
+ // Again, note how a clear function name replaces the
+ // need for a code comment:
+ if (outsideBusinessHours(timeReceived)) {
return autoReplyTo(message);
}
pingAgentFor(message);
+}
+
+function outsideBusinessHours(timeReceived) {
+ // Refactoring #3: replace magic numbers with
+ // symbols that communicate with the code reader:
+ const closingHour = 17;
+ const openingHour = 8;
+
+ const hours = timeReceived.getHours();
+
+ // Refactoring #4: simple conditional rules can
+ // be further clarified by introducing a variable:
+ const afterClosing = hours > closingHour;
+ const beforeOpening = hours < openingHour;
+
+ // Yeah -- look how clear the business rule is now!
+ return afterClosing || beforeOpening;
} |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
… normalization - Introduce DEPENDENCY_SECTION_NAMES constant for consistent dependency sections. - Extract processDependencySection for clearer iteration over dependencies. - Refactor walkDependencyTree and walkDependencySection for clarity and correctness. - Add shouldSkipPackageVersion helper to simplify package version filtering logic. - Extract deriveAdvisoryKey and addPackageAdvisories helper functions to clean up normaliseBulkAdvisories. - Improve comments and examples to clarify APIs and internal logic. These changes improve code readability and maintainability without changing functionality. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="security/audit-utils.js" line_range="204-206" />
<code_context>
+ * addPackageAdvisories('validator', [{ id: 1, url: 'https://github.com/advisories/GHSA-abcd-1234-efgh' }], advisories);
+ * console.log(Object.keys(advisories).length); // 1
+ */
+function addPackageAdvisories(packageName, packageAdvisories, advisories) {
+ if (!Array.isArray(packageAdvisories)) {
+ return;
+ }
+
+ for (const advisory of packageAdvisories) {
+ const { key, githubAdvisoryId } = deriveAdvisoryKey(packageName, advisory);
+
+ if (key in advisories) {
+ continue;
+ }
</code_context>
<issue_to_address>
**suggestion:** Use `Object.hasOwn` instead of `in` when checking advisory map keys.
`key in advisories` will return true for properties on the prototype chain. Even though `advisories` is currently a plain object you control, using `Object.hasOwn(advisories, key)` (or `Object.prototype.hasOwnProperty.call`) makes this check resilient if `advisories` is ever created with a different prototype.
```suggestion
if (Object.hasOwn(advisories, key)) {
continue;
}
```
</issue_to_address>
### Comment 2
<location path="security/audit-utils.js" line_range="71" />
<code_context>
+ versionsByPackage.set(packageName, knownVersions);
+}
+
+function processDependencySection(section, versionsByPackage) {
+ for (const [packageName, dependency] of Object.entries(section)) {
+ if (!dependency || typeof dependency !== 'object') {
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new audit helpers by flattening the dependency traversal, inlining single‑use helpers, and merging small wrapper functions to cut down on indirection and make the control flow easier to follow.
You can keep all new behavior but reduce structural complexity with a few small refactors:
### 1. Flatten dependency traversal
`walkDependencyTree` → `walkDependencySection` → `processDependencySection` can be collapsed into a single traversal that handles sections + recursion in one place:
```js
function walkDependencies(node, versionsByPackage) {
if (!node || typeof node !== 'object') {
return;
}
for (const sectionName of DEPENDENCY_SECTION_NAMES) {
const section = node[sectionName];
if (!section || typeof section !== 'object') continue;
for (const [packageName, dependency] of Object.entries(section)) {
if (!dependency || typeof dependency !== 'object') continue;
const version = typeof dependency.version === 'string' ? dependency.version : undefined;
if (version) {
addPackageVersion(versionsByPackage, packageName, version);
}
walkDependencies(dependency, versionsByPackage);
}
}
}
```
Then use it from `collectInstalledPackageVersions`:
```js
for (const tree of Array.isArray(packageTrees) ? packageTrees : [packageTrees]) {
walkDependencies(tree, versionsByPackage);
}
```
You can then remove `walkDependencySection` and `processDependencySection`.
### 2. Inline `shouldSkipPackageVersion` into `addPackageVersion`
`shouldSkipPackageVersion` is only used once and just wraps a couple of checks. You can inline it to cut a layer without losing readability:
```js
function addPackageVersion(versionsByPackage, packageName, version) {
const isMissing = !packageName || !version;
if (isMissing || isLocalWorkspaceVersion(version)) {
return;
}
const knownVersions = versionsByPackage.get(packageName) ?? new Set();
knownVersions.add(version);
versionsByPackage.set(packageName, knownVersions);
}
```
This keeps `isLocalWorkspaceVersion` as the meaningful semantic check, but removes an extra function hop.
### 3. Merge `normaliseBulkAdvisories` and `addPackageAdvisories`
You can keep `deriveAdvisoryKey` (it carries useful intent) but inline `addPackageAdvisories` into `normaliseBulkAdvisories`:
```js
function normaliseBulkAdvisories(bulkPayload) {
const advisories = {};
for (const [packageName, packageAdvisories] of Object.entries(bulkPayload ?? {})) {
if (!Array.isArray(packageAdvisories)) continue;
for (const advisory of packageAdvisories) {
const { key, githubAdvisoryId } = deriveAdvisoryKey(packageName, advisory);
if (key in advisories) continue;
advisories[key] = {
...advisory,
github_advisory_id: githubAdvisoryId,
package_name: packageName,
};
}
}
return advisories;
}
```
This keeps the behavior the same while reducing one level of indirection.
### 4. Clarify `parseJsonOutput` argument naming
To make its dual use (spawn vs HTTP) clearer, you can rename the parameter and adjust call sites:
```js
function parseJsonOutput(payloadText, commandLabel) {
const text = payloadText?.trim?.() ?? '';
if (!text) return {};
try {
return JSON.parse(text);
} catch (error) {
error.message = `Failed to parse ${commandLabel} JSON: ${error.message}`;
throw error;
}
}
```
Usage:
```js
const packageTrees = parseJsonOutput(result.stdout, 'pnpm ls');
const bulkPayload = parseJsonOutput(responseText, 'bulk advisory audit');
const json = parseJsonOutput(stdout, 'pnpm audit');
```
These small consolidations should reduce the mental overhead of following the code paths without changing any functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4da648d53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…t responses - Updated advisory ID extraction to preserve original casing instead of forcing uppercase. - Enhanced parseJsonOutput to optionally require non-empty JSON strings, adding validation for empty audit responses. - Refactored bulk advisory normalization logic to prevent advisory ID case normalization. - Added tests to ensure advisory casing is preserved from bulk payload URL. - Added tests to verify rejection of blank bulk advisory responses. - Improved dependency tree walking function for clarity and maintainability. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend-pwa/scripts/audit-utils.test.mjs`:
- Around line 73-269: Extract the repeated two-step pnpm retired-endpoint stub
into a helper (e.g., setupPnpmAuditRetired or createRetiredPnpmFixture) that
calls spawnSyncMock.mockReturnValueOnce with the ERR_PNPM_AUDIT_BAD_RESPONSE
result and a second mockReturnValueOnce with the pnpm ls result (using existing
createPnpmResult), then replace the duplicated blocks in the tests that
currently call spawnSyncMock.mockReturnValueOnce(...) (the blocks around the
first and second mockReturnValueOnce in the tests using runAuditJson) with a
single call to that helper; keep references to spawnSyncMock and
createPnpmResult inside the helper so the tests otherwise remain unchanged.
In `@security/audit-utils.js`:
- Around line 27-233: Add JSDoc comments with compact outcome-driven `@example`
blocks to every helper function introduced in this diff (parseJsonOutput,
isRetiredAuditEndpoint, isLocalWorkspaceVersion, addPackageVersion,
walkDependencies, collectInstalledPackageVersions, normaliseRegistryUrl,
readRegistryUrl, extractGithubAdvisoryId, deriveAdvisoryKey,
normaliseBulkAdvisories, runBulkAdvisoryAudit): for each function document its
purpose, parameters and return value and include a minimal `@example` showing a
concrete input and the expected output (e.g., parseJsonOutput showing input JSON
string → parsed object, walkDependencies showing a small dependency tree →
versions map, extractGithubAdvisoryId showing URL → GHSA id,
normaliseBulkAdvisories showing sample bulk payload → normalized advisories,
runBulkAdvisoryAudit showing successful response → { json, status }), keeping
examples terse and outcome-focused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33f09b4f-d8e5-4c42-8f80-b4ce54845f89
📒 Files selected for processing (2)
frontend-pwa/scripts/audit-utils.test.mjssecurity/audit-utils.js
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. frontend-pwa/scripts/audit-utils.test.mjs Comment on lines +173 to +203 ❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on lines +85 to +108 function walkDependencies(node, versionsByPackage) {
if (!node || typeof node !== 'object') {
return;
}
for (const sectionName of DEPENDENCY_SECTION_NAMES) {
const section = node[sectionName];
if (!section || typeof section !== 'object') {
continue;
}
for (const [packageName, dependency] of Object.entries(section)) {
if (!dependency || typeof dependency !== 'object') {
continue;
}
if (typeof dependency.version === 'string') {
addPackageVersion(versionsByPackage, packageName, dependency.version);
}
walkDependencies(dependency, versionsByPackage);
}
}
}❌ New issue: Bumpy Road Ahead |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on lines +179 to +203 function normaliseBulkAdvisories(bulkPayload) {
const advisories = {};
for (const [packageName, packageAdvisories] of Object.entries(bulkPayload ?? {})) {
if (!Array.isArray(packageAdvisories)) {
continue;
}
for (const advisory of packageAdvisories) {
const { key, githubAdvisoryId } = deriveAdvisoryKey(packageName, advisory);
if (Object.hasOwn(advisories, key)) {
continue;
}
advisories[key] = {
...advisory,
github_advisory_id: githubAdvisoryId,
package_name: packageName,
};
}
}
return advisories;
}❌ New issue: Bumpy Road Ahead |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for the following concern and provide an AI coding agent prompt for the fix:
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Extract version-map construction and advisory line formatting\nhelpers in security/audit-utils.js while preserving behaviour.\n\nThis complements the earlier dependency-section extraction and brings\nthe file's measured mean cyclomatic complexity below 4 without\npushing any individual function above 9.\n\nValidated with:\n- make check-fmt\n- make lint\n- make test\n- AST-based complexity check (mean 3.95, max 7)
Document the audit-utils test helpers with terse JSDoc so their\nmocking contracts stay explicit.\n\nValidate bulk advisory entries before spreading them into the\naccumulator, and add a fetch timeout to the bulk advisory fallback\nso the audit flow fails clearly instead of hanging indefinitely.\n\nAlso complete the addPackageAdvisories JSDoc with an explicit\n tag and outcome-focused example while keeping the file\nunder the repository line-limit rule.\n\nValidated with:\n- make check-fmt\n- make lint\n- make test
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@security/audit-utils.js`:
- Around line 238-242: The JSDoc comment for the function
normalizeBulkAdvisories uses British spelling "Normalise" while the function
name uses American spelling "normalize"; update the JSDoc summary and any other
occurrences in that comment block to "Normalize" so the wording matches the
function name normalizeBulkAdvisories and remains consistent across the file.
- Around line 158-167: The function name and JSDoc use the en-GB "normalise"
spelling which conflicts with the project's en-GB-oxendict policy (and with
other functions like normalizeBulkAdvisories); rename the function
normaliseRegistryUrl to normalizeRegistryUrl, update its JSDoc/comment to
"Normalize", and update all call sites and any exports/imports that reference
normaliseRegistryUrl to use normalizeRegistryUrl so symbol names remain
consistent across the module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 126c5b72-27e0-41d7-ad36-e6b6f67995f8
📒 Files selected for processing (2)
frontend-pwa/scripts/audit-utils.test.mjssecurity/audit-utils.js
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on lines +259 to +265 async function runBulkAdvisoryAudit() {
const registryUrl = readRegistryUrl();
const endpoint = new URL(BULK_ADVISORY_PATH, registryUrl);
const controller = new AbortController();
const timeoutId = setTimeout(() => controller.abort(), BULK_AUDIT_TIMEOUT_MS);
let response;
let responseText;❌ New issue: Complex Method |
This comment was marked as resolved.
This comment was marked as resolved.
Rename `normaliseRegistryUrl` to `normalizeRegistryUrl` and update its local call sites so the helper spelling matches the rest of the module. Also align the `normalizeBulkAdvisories` JSDoc summary with the function name. Validated with: - make check-fmt - make lint - make test
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Extract the bulk advisory fetch and result shaping logic out of `runBulkAdvisoryAudit` so the fallback audit path stays easier to read and remains below the repository complexity thresholds. Validated with: - make check-fmt - make lint - make test - AST-based complexity check (mean 3.56, max 7)
Fail closed when normalized bulk advisory payload entries are not arrays, reject empty `pnpm ls` output before parsing, and add the requested outcome-focused examples to the remaining helper JSDoc blocks. Validated with: - make check-fmt - make lint - make test
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
security/audit-utils.js (1)
240-245:⚠️ Potential issue | 🟠 MajorValidate the bulk payload object before iterating it.
A primitive or array payload still falls through
Object.entries(...)and turns into{}, which hides a broken registry response as a clean audit result.🛠️ Proposed fix
function normalizeBulkAdvisories(bulkPayload) { + const isPlainObject = + typeof bulkPayload === 'object' && bulkPayload !== null && !Array.isArray(bulkPayload); + if (!isPlainObject) { + throw new TypeError( + 'Invalid bulk advisory payload: expected an object keyed by package name.', + ); + } + const advisories = {}; - for (const [packageName, packageAdvisories] of Object.entries(bulkPayload ?? {})) { + for (const [packageName, packageAdvisories] of Object.entries(bulkPayload)) { if (!Array.isArray(packageAdvisories)) { throw new TypeError(`Invalid bulk advisory entry for package ${packageName}: expected array, received ${JSON.stringify(packageAdvisories)}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@security/audit-utils.js` around lines 240 - 245, In normalizeBulkAdvisories, validate that bulkPayload is a plain object before iterating: check typeof bulkPayload === 'object' && bulkPayload !== null && !Array.isArray(bulkPayload), and if that check fails throw a TypeError like "Invalid bulk advisory payload: expected an object keyed by package name."; then remove the fallback Object.entries(bulkPayload ?? {}) and iterate Object.entries(bulkPayload) so primitive/array inputs no longer silently become {}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@security/audit-utils.js`:
- Around line 254-257: Add outcome-driven `@example` blocks to the JSDoc for
fetchBulkAdvisories and toAdvisoryResult: update the fetchBulkAdvisories comment
(function name fetchBulkAdvisories) to include an example showing calling it
with a registry URL and a sample packageVersions input and the expected returned
structure (e.g., { response, responseText: '{}' } when registry returns {}), and
update the toAdvisoryResult comment (function name toAdvisoryResult) to include
a simple example demonstrating input {} producing { json: { advisories: {} },
status: 0 }; keep examples concise and outcome-focused and place them in the
existing JSDoc blocks around the function signatures.
- Around line 120-123: Reject malformed pnpm ls JSON in buildVersionMap by
validating inputs before walking dependencies: add an isDependencyTreeNode check
that ensures packageTrees (or each element when an array) is a non-null object
and not an array, throw a TypeError('pnpm ls returned an invalid dependency tree
payload.') if validation fails, then assign trees = Array.isArray(packageTrees)
? packageTrees : [packageTrees] and iterate trees with walkDependencies to
populate versionsByPackage (instead of collapsing scalars to an empty map).
Apply the same validation logic to the other place where you currently
iterate/normalize packageTrees (the second occurrence mentioned) so both code
paths fail closed on malformed payloads.
---
Duplicate comments:
In `@security/audit-utils.js`:
- Around line 240-245: In normalizeBulkAdvisories, validate that bulkPayload is
a plain object before iterating: check typeof bulkPayload === 'object' &&
bulkPayload !== null && !Array.isArray(bulkPayload), and if that check fails
throw a TypeError like "Invalid bulk advisory payload: expected an object keyed
by package name."; then remove the fallback Object.entries(bulkPayload ?? {})
and iterate Object.entries(bulkPayload) so primitive/array inputs no longer
silently become {}.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a425acf7-4693-4c7c-b705-2156c47b5395
📒 Files selected for processing (1)
security/audit-utils.js
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on file */
export function runAuditJson() {
const result = spawnSync('pnpm', ['audit', '--json'], {
/** @file Shared helpers for dependency audits and advisory filtering. */❌ New issue: Overall Code Complexity |
This comment was marked as resolved.
This comment was marked as resolved.
Validate bulk advisory and pnpm dependency-tree payloads before\nprocessing them so malformed upstream JSON fails closed instead of\nquietly degrading to empty results.\n\nAdd the missing outcome-focused JSDoc examples for the extracted bulk\naudit helpers while keeping the file within the repository line limit.
Add three private predicate helpers in `security/audit-utils.js` to reduce per-function branching and lower the module's mean cyclomatic complexity. This keeps behaviour unchanged while making the registry, advisory-shape, and allow-list checks reusable and easier to scan. A code-only complexity check puts the file at a mean of 3.58 with a maximum individual function complexity of 7.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Overall Code Complexitysecurity/audit-utils.js: Complex Conditionalsecurity/audit-utils.js: normalizeBulkAdvisories What lead to degradation?normalizeBulkAdvisories has 1 complex conditionals with 2 branches, threshold = 2 Why does this problem occur?A complex conditional is an expression inside a branch such as an if-statmeent which consists of multiple, logical operations. Example: if (x.started() && y.running()).Complex conditionals make the code even harder to read, and contribute to the Complex Method code smell. Encapsulate them. How to fix it?Apply the DECOMPOSE CONDITIONAL refactoring so that the complex conditional is encapsulated in a separate function with a good name that captures the business rule. Optionally, for simple expressions, introduce a new variable which holds the result of the complex conditional. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# complex_conditional.js
function messageReceived(message, timeReceived) {
- // Ignore all messages which aren't from known customers:
- if (!message.sender &&
- customers.getId(message.name) == null) {
+ // Refactoring #1: encapsulate the business rule in a
+ // function. A clear name replaces the need for the comment:
+ if (!knownCustomer(message)) {
log('spam received -- ignoring');
return;
}
- // Provide an auto-reply when outside business hours:
- if ((timeReceived.getHours() > 17) ||
- (timeReceived.getHours() < 8)) {
+ // Refactoring #2: encapsulate the business rule.
+ // Again, note how a clear function name replaces the
+ // need for a code comment:
+ if (outsideBusinessHours(timeReceived)) {
return autoReplyTo(message);
}
pingAgentFor(message);
+}
+
+function outsideBusinessHours(timeReceived) {
+ // Refactoring #3: replace magic numbers with
+ // symbols that communicate with the code reader:
+ const closingHour = 17;
+ const openingHour = 8;
+
+ const hours = timeReceived.getHours();
+
+ // Refactoring #4: simple conditional rules can
+ // be further clarified by introducing a variable:
+ const afterClosing = hours > closingHour;
+ const beforeOpening = hours < openingHour;
+
+ // Yeah -- look how clear the business rule is now!
+ return afterClosing || beforeOpening;
} |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai pause |
Replace the inline bulk advisory payload object check with the existing `isPlainAdvisoryObject` helper. This keeps the behaviour unchanged while reducing duplication in `normalizeBulkAdvisories`. Repo formatting, lint, and test gates all passed before commit.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@security/audit-utils.js`:
- Around line 314-316: The JSDoc for the async function runAuditJson incorrectly
declares a plain object return; update the `@returns` annotation to a Promise of
that object (e.g. change `@returns` {{ json: { advisories?: Record<string,
unknown> }, status: number }} to `@returns` {Promise<{ json: { advisories?:
Record<string, unknown> }, status: number }>} ) so editor hints and the awaited
example match the async runAuditJson() implementation.
- Around line 153-156: Add concise JSDoc `@example` blocks to the predicate
helpers isValidRegistryValue, isPlainAdvisoryObject, and isExpectedAdvisory
showing minimal input and the expected outcome; for each function include a one-
or two-line example that calls the function with representative input and shows
the return value (e.g., isValidRegistryValue('https://registry') -> true), place
the `@example` directly in the existing JSDoc above each function, and ensure
examples are outcome-driven (input followed by the expected returned value) and
consistent with the function's documented behavior.
- Around line 191-196: The extractGithubAdvisoryId function returns GHSA IDs
preserving original URL casing which breaks case-sensitive allowlist lookups;
change the matching to capture the hex groups and return a canonical lowercase
GHSA string (e.g., build "GHSA-" + capturedGroups.toLowerCase()) so extracted
IDs always match audit-exceptions.schema.json and isExpectedAdvisory Set
lookups; update any tests that asserted preserved casing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19f095fc-8b1b-4dd8-99a0-9ae8d0b0a331
📒 Files selected for processing (1)
security/audit-utils.js
✅ Actions performedReviews paused. |
Canonicalize GHSA IDs extracted from bulk advisory URLs so\nallowlist lookups are case-stable.\n\nAlso fix the async runAuditJson() JSDoc return type and add\noutcome-driven examples to the predicate helper documentation.\n\nGates passed:\n- make check-fmt\n- make lint\n- make test
1 participant
Summary
Changes
Core audit/fallback logic
Frontend tests
CI and tooling
Documentation
Test plan
How to test locally
Notes
◳ Generated by DevBoxer ◰
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/1c7c3f6e-9984-4c5a-86f2-940376afcd9c
📎 Task: https://www.devboxer.com/task/eff8ed2d-7627-4c32-951b-f2f88f065a7e
Summary by Sourcery
Introduce a shared audit helper that supports pnpm’s native audit JSON output and a bulk advisory fallback, and update consumers to use the new async flow.
New Features:
Enhancements:
Build:
Documentation:
Tests:
📎 Task: https://www.devboxer.com/task/a1973a97-54b0-4120-bcbb-9fb0a437bef0