Skip to content

Refactor: Build File Signatures #7629

Merged
DawoudIO merged 3 commits intomasterfrom
feature/integrity-node-signatures-20251123162250
Nov 24, 2025
Merged

Refactor: Build File Signatures #7629
DawoudIO merged 3 commits intomasterfrom
feature/integrity-node-signatures-20251123162250

Conversation

@DawoudIO
Copy link
Contributor

What Changed

  • Replace Grunt generateSignatures with Node script;
  • move signatures to src/admin/data;
  • run integrity checks fresh (remove persistent integrityCheck.json)

Type

  • ✨ Feature
  • 🐛 Bug fix
  • ♻️ Refactor
  • 🏗️ Build/Infrastructure
  • 🔒 Security

Testing

Screenshots

Security Check

  • Introduces new input validation
  • Modifies authentication/authorization
  • Affects data privacy/GDPR

Code Quality

  • Database: Propel ORM only, no raw SQL
  • No deprecated attributes (align, valign, nowrap, border, cellpadding, cellspacing, bgcolor)
  • Bootstrap CSS classes used
  • All CSS bundled via webpack

Pre-Merge

  • Tested locally
  • No new warnings
  • Build passes
  • Backward compatible (or migration documented)

… src/admin/data; run integrity checks fresh (remove persistent integrityCheck.json); update AppIntegrityService and scripts
@DawoudIO DawoudIO added this to the 6.1.0 milestone Nov 24, 2025
Copilot AI review requested due to automatic review settings November 24, 2025 00:24
@DawoudIO DawoudIO requested a review from a team as a code owner November 24, 2025 00:24
@DawoudIO DawoudIO requested review from DAcodedBEAT, MrClever, bigtigerku, grayeul and respencer and removed request for a team November 24, 2025 00:24
Copy link
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

This PR refactors the build signature generation process by replacing the Grunt-based task with a standalone Node.js script, relocating the output file from src/signatures.json to src/admin/data/signatures.json, and modifying how integrity check results are cached in AppIntegrityService.

Key Changes:

  • New Node.js script at scripts/generate-signatures-node.js replaces Grunt generateSignatures task
  • Signature file moved to src/admin/data/signatures.json (from src/signatures.json)
  • AppIntegrityService now runs integrity checks fresh instead of reading from persistent integrityCheck.json cache

Reviewed changes

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

Show a summary per file
File Description
scripts/generate-signatures-node.js New standalone Node.js script for generating SHA1 file signatures
src/ChurchCRM/Service/AppIntegrityService.php Removes persistent cache reading, always runs fresh verification, updates signature file path
package.json Updates deploy script to use Node script instead of Grunt task; reorders dependency
Gruntfile.js Removes deprecated generateSignatures task definition
.gitignore Updates signature file path to new location
.codespellrc Updates signature file path in skip list

const aggregate = crypto.createHash('sha1').update(JSON.stringify(filesArray)).digest('hex');

const signatures = { version: version || null, files: filesArray, sha1: aggregate };

Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The script writes to src/admin/data/signatures.json without ensuring the directory exists first. fs.writeFileSync() will throw an error if the parent directory doesn't exist. Consider adding directory creation before writing:

const outDir = path.dirname(outFile);
if (!fs.existsSync(outDir)) {
  fs.mkdirSync(outDir, { recursive: true });
}
fs.writeFileSync(outFile, JSON.stringify(signatures));
Suggested change
const outDir = path.dirname(outFile);
if (!fs.existsSync(outDir)) {
fs.mkdirSync(outDir, { recursive: true });
}

Copilot uses AI. Check for mistakes.
/^Include\/Config\.php$/,
/^propel\/propel\.php$/,
/^integrityCheck\.json$/,
/^Images\/Person\/thumbnails\//,
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The exclusion pattern for thumbnails has changed from the Grunt version. The old Grunt task excluded only !Images/Person/thumbnails/*.jpg (specific file extension), but this new pattern excludes the entire Images/Person/thumbnails/ directory regardless of file type.

If there are any .php or .js files legitimately stored in this directory, they will now be excluded from integrity checks when they weren't before. Consider matching the original Grunt behavior more precisely:

/^Images\/Person\/thumbnails\/.*\.jpg$/,
Suggested change
/^Images\/Person\/thumbnails\//,
/^Images\/Person\/thumbnails\/.*\.jpg$/,

Copilot uses AI. Check for mistakes.
Comment on lines +40 to +42
// Always run verification fresh - don't use persistent cache files
$verificationResult = AppIntegrityService::verifyApplicationIntegrity();
AppIntegrityService::$IntegrityCheckDetails = (object) $verificationResult;
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

This PR removes the code that reads from integrityCheck.json, but other parts of the codebase still use this file:

  1. src/ChurchCRM/Service/SystemService.php (line 102-104) still writes to integrityCheck.json during background jobs
  2. src/ChurchCRM/tasks/IntegrityCheckTask.php (line 15-21) still reads from integrityCheck.json to determine if a task should be shown

This creates an inconsistency where:

  • Background jobs write to a file that AppIntegrityService no longer reads
  • IntegrityCheckTask depends on a file that may not be reliably populated
  • The change is incomplete - either all components should use the file or none should

Consider either:

  • Reverting this change to keep using the cached file consistently, OR
  • Updating SystemService.php to remove writing the file and updating IntegrityCheckTask.php to call AppIntegrityService directly

Copilot uses AI. Check for mistakes.
@DawoudIO DawoudIO merged commit 64fa5dd into master Nov 24, 2025
7 checks passed
@DawoudIO DawoudIO deleted the feature/integrity-node-signatures-20251123162250 branch November 24, 2025 01:48
@DawoudIO DawoudIO modified the milestones: 6.1.0, 6.2.0 Nov 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants