Add extra fields (client_number, etc) and functions to client and quotes and invoices #1029
Conversation
Added client_number to ip_clients table Added to ip_invoices table: invoice_quote_number, invoice_work_order, invoice_agreement
…e_agreement Added new fields: invoice_quote_number, invoice_work_order, invoice_agreement, and resized css styles to accommodate extra fields
Added labels for: client_number quote_number work_order agreement
Added field client_number to list of quotes
Added entry for related_quote_number
Added related_quote_number instead of quote_number
Changed layout and added quote_work_order and quote_agreement text fields
Alter ip_quotes table to add quote_work_order and quote_agreement
In function validation_rules added quote_work_order quote_agreement
In function validation_rules added invoice_quote_number invoice_work_order invoice_agreement
Added quote_work_order and quote_agreement fields
Added quote_work_order and quote_agreement fields
Added quote_work_order and quote_agreement fields
Added invoice_quote_number, invoice_work_order and invoice_agreement
Added invoice_quote_number, invoice_work_order and invoice_agreement
Added option to select related quote number
Added an option to select related quote in invoice module
Added an option to select related quote number in invoice module
Added an option to select related quote number in invoice module
![accesslint[bot]](https://avatars.githubusercontent.com/in/1502?s=60&v=4){kind=small}
novafirst
changed the title
Added a few options for newly created fields
|
Hey @novafirst since I released version 1.6.3 some merge conflicts would occur while dealing with your pull-request. I could merge it into 1.7, however in version 2.0 of InvoicePlane the Customer Number is offered out of the box. |
nielsdrost7
changed the title
…client-and-quotes-and-invoices
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application/modules/upload/controllers/Upload.php (1)
172-193: Avoid logging raw attacker‑supplied filenames.
Prefer hashing to reduce PII exposure and log‑injection risk.🔧 Suggested change
- log_message('error', 'Path traversal attempt detected in filename: ' . $filename); + $filenameHash = hash_for_logging($filename); + log_message('error', 'Path traversal attempt detected in filename (hash: ' . $filenameHash . ')');
🤖 Fix all issues with AI agents
In @.github/workflows/composer-update.yml:
- Around line 77-84: The "Check for changes" step (id: check-changes) only
inspects composer.lock via `git diff --quiet composer.lock` so it misses updates
to composer.json; update the check to detect changes to both composer.lock and
composer.json (e.g., test diffs or git status for composer.json composer.lock)
and set GITHUB_OUTPUT changes_detected accordingly so PRs are opened when either
file was modified.
In @.github/workflows/crowdin-sync.yml:
- Around line 1-105: The workflow is using the wrong branch name for Crowdin
operations: update the two occurrences of localization_branch_name currently set
to "master" to "development" inside the "Upload sources to Crowdin" and
"Download translations from Crowdin" steps (look for the
localization_branch_name key in those steps and replace its value), so Crowdin
actions target the repository's default branch.
In @.github/workflows/docker-phpmyadmin.yml:
- Around line 1-5: The workflow's triggers were changed to only
workflow_dispatch, removing automatic builds; update the top-level "on" block to
include workflow_dispatch plus automatic triggers by adding a "push" event for
the development branch and a "push" with "tags: ['v*.*.*']" (i.e., restore
push/branches and tag pushes) so the workflow runs both manually and
automatically upon branch pushes and semver tags; ensure the "on" block contains
workflow_dispatch, push.branches: [development], and push.tags: ['v*.*.*'] to
restore the original behavior.
In @.github/workflows/phpunit.yml:
- Around line 1-49: The workflow uses Laravel-specific files and commands (steps
named "Prepare Laravel environment" and "Run PHPUnit" that call php artisan and
reference .env.testing.example) which don't exist in this CodeIgniter 3.3 repo;
replace those steps with CodeIgniter-appropriate test setup: install PHPUnit via
Composer (ensure composer.json includes phpunit), add or reference a
phpunit.xml/phpunit.xml.dist suited to this project, remove php artisan
key:generate/.env.testing.example usage, and change the Run PHPUnit step to
invoke vendor/bin/phpunit (or the configured test command) and any necessary DB
setup (create sqlite or mysql service and migration/seeding commands if you add
CI test helpers). Ensure the workflow's "Setup PHP with Composer" step provides
required PHP extensions and that the final test step runs the repository's
actual test runner instead of php artisan.
In @.github/workflows/release.yml:
- Line 427: Update the GitHub Actions step that references
softprops/action-gh-release by changing the action version from v1 to v2
(replace softprops/action-gh-release@v1 with softprops/action-gh-release@v2) so
the workflow runs on Node 20; ensure any inputs or outputs used with that step
remain compatible after the version bump and adjust them if the v2 action
introduced any breaking changes.
In @.github/workflows/setup.yml:
- Around line 196-223: The workflow step with id key_generate currently runs the
Laravel command "php artisan key:generate", but this is a CodeIgniter 3 project;
replace that call with a CI-appropriate key setup: remove "php artisan
key:generate" and instead generate a random encryption key (for example via a
short PHP one-liner or openssl) and write it into your CodeIgniter config (set
$config['encryption_key'] in application/config/config.php or write to your .env
if you use one); update the step id key_generate to echo success/failure exactly
as before based on that command’s exit code. Also scan other workflow steps that
call "php artisan migrate" or "php artisan db:seed" and replace them with your
project's migration/seed approach (invoke your custom CLI controllers via "php
index.php ControllerName method" or run your project-specific migration
scripts).
In @.github/workflows/test-php.yml:
- Around line 3-15: Update the GitHub Actions workflow triggers and matrix so CI
runs on PRs and includes the project-required minimum PHP version: add
pull_request to the top-level on: triggers (in addition to workflow_dispatch)
and expand the strategy.matrix.php-versions array to include '8.1' alongside
'8.2'; confirm the matrix key names operating-system and php-versions remain
unchanged so the run job picks up the new entries.
In @.github/workflows/yarn-update.yml:
- Around line 203-211: The workflow step with id updated-packages invokes the
missing script generate-package-update-report.cjs; either add that script under
the repository (implement the logic that generates updated-packages.txt and
exits non-zero on errors) or change the step to point to the existing script
path/name that produces updated-packages.txt; update the run block to call the
correct script name and ensure the script writes updated-packages.txt before the
echo to GITHUB_OUTPUT so the "Get updated packages" step succeeds.
In `@application/helpers/file_security_helper.php`:
- Line 177: The code calls a non-existent mb_rtrim when building $fullPath in
file_security_helper.php; replace the mb_rtrim usage with PHP's rtrim() so
$fullPath = rtrim($baseDirectory, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR .
$basename['filename']; (i.e., update the expression that constructs $fullPath to
use rtrim instead of mb_rtrim).
- Line 87: The code calls a non-existent mb_rtrim which will cause a fatal
error; replace the mb_rtrim call with the standard rtrim when building
$realBaseWithSep from $realBase and DIRECTORY_SEPARATOR (e.g., update the
expression that sets $realBaseWithSep to use rtrim($realBase,
DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR) because DIRECTORY_SEPARATOR is a
single-byte ASCII char and multibyte handling is unnecessary.
In `@application/modules/clients/Enums/ClientTitleEnum.php`:
- Around line 20-31: The tryFrom method currently uses array_search on
self::getValues() and checks if ($searchResult), which skips the first enum
(index 0) and returns the index rather than the matched string; change the
lookup to perform a strict search (e.g., use array_search($value, $values, true)
or use in_array with strict true) and when a match is found return the matched
value string (the element from $values), not the numeric index; ensure tryFrom
returns null (or the expected empty/false value) when no strict match exists.
In `@application/modules/invoices/views/view.php`:
- Around line 479-498: The echoed input values for invoice_quote_number,
invoice_work_order, and invoice_agreement are not escaped and can cause XSS;
update the value outputs in the view to escape user content (e.g., wrap the
properties with htmlsc() or htmlspecialchars()) when echoing inside the value
attribute for the inputs in view.php (references: invoice_quote_number,
invoice_work_order, invoice_agreement) so the rendered value attribute uses the
escaped string.
In `@application/modules/quotes/controllers/Ajax.php`:
- Around line 21-43: In modal_quote_lookups ensure $data is always defined
before calling $this->layout->load_view to avoid the undefined variable when
$client_id is empty: either initialize $data = array() (or with default keys
'related_quotes','client_id','quote_statuses') before the if block, or add an
early return/conditional so load_view is only called when $data is set;
reference the modal_quote_lookups method, the $this->mdl_quotes->... calls, and
the final $this->layout->load_view('quotes/modal_quote_lookups', $data) when
applying the fix.
- Around line 144-146: The POST-extracted variables $quote_work_order and
$quote_agreement are never added to the record being saved; update the
controller method in application/modules/quotes/controllers/Ajax.php to include
these keys in the $db_array (e.g., 'quote_work_order' => $quote_work_order and
'quote_agreement' => $quote_agreement) before the save/insert call so the values
persist to the DB; ensure the $db_array used by the existing save/update
function contains those two fields and run existing migrations/column names to
match.
In `@application/modules/quotes/views/modal_quote_lookups.php`:
- Around line 9-29: Validate that a quote is selected in the
.select-items-confirm click handler by checking quote_id (from
$("input[name='quote_id']:checked").val()) and aborting with a user-facing
message if invalid; wrap the $.post callback parsing in a try/catch around
JSON.parse(data) and handle parse or AJAX errors by logging and showing an error
to the user; replace the for..in loop over items with a deterministic iteration
(e.g., use items[0] or a for loop over numeric indices) so all fields are set
correctly and move $('#modal-choose-quote').modal('hide') outside the loop so
the modal is closed only after processing completes; ensure the error-handling
branches do not attempt to use the parsed items when parsing fails.
In `@application/modules/quotes/views/view.php`:
- Around line 349-362: The quote_work_order and quote_agreement values are
echoed into input value attributes without escaping, enabling XSS; update the
view (inputs with id "quote_work_order" and "quote_agreement" in view.php) to
output escaped values using a safe HTML-escaping function (e.g.
htmlspecialchars($quote->quote_work_order, ENT_QUOTES, 'UTF-8') and
htmlspecialchars($quote->quote_agreement, ENT_QUOTES, 'UTF-8') or your
framework's html_escape helper) and always render the value attribute (even when
empty) to avoid malformed HTML.
In `@application/modules/setup/sql/041_1.7.0.sql`:
- Line 1: The migration file 041_1.7.0.sql currently contains only a comment;
add ALTER TABLE statements to create the missing columns used by the app: add
client_number (e.g., VARCHAR(255) DEFAULT NULL) to ip_clients, add
invoice_quote_number (TEXT DEFAULT NULL), invoice_work_order (VARCHAR(255)
DEFAULT NULL) and invoice_agreement (VARCHAR(255) DEFAULT NULL) to ip_invoices,
and add quote_work_order (VARCHAR(255) DEFAULT NULL) and quote_agreement
(VARCHAR(255) DEFAULT NULL) to ip_quotes; ensure the statements are idempotent
for your DB flavor (or wrapped in a safe conditional/transaction) and follow the
repository’s naming and datatype conventions so functions referencing these
fields (client_number, invoice_quote_number, invoice_work_order,
invoice_agreement, quote_work_order, quote_agreement) will find the columns at
runtime.
In `@application/views/quote_templates/public/InvoicePlane_Web.php`:
- Around line 79-83: Escape the user-supplied VAT/tax ID and attachment name
outputs to prevent stored XSS by replacing direct echoes of $quote->user_vat_id
and $quote->user_tax_code (and any attachment name echoes around the other
mentioned lines) with an HTML-escaped variant; use the project's escaping helper
(e.g., html_escape()) or PHP's htmlspecialchars(..., ENT_QUOTES, 'UTF-8') when
outputting these variables so all printed values are properly encoded before
echoing.
In `@assets/core/js/jquery-ui.js`:
- Around line 888-894: The .position() widget is vulnerable to XSS when a string
passed to options.of is handed directly to $() — change handling so that when
options.of is a string you call $(document).find(options.of) (ensuring strings
are treated only as selectors) and otherwise continue to pass non-string values
(DOM element, jQuery object, Event, window/document) directly to $(); update the
branch around options.of and target to reflect this secure pattern and consider
replacing the vendored assets/core/js/jquery-ui.js with jQuery UI 1.13.0+ which
includes this fix upstream.
In `@assets/core/js/scripts.js`:
- Around line 76-77: The preview currently uses .text() in
update_email_template_preview(), which escapes HTML and shows raw tags; change
it to write sanitized HTML into the iframe/body (use .html() on
$('#email-template-preview').contents().find('body')) after running the template
content through your sanitization function (e.g., sanitizeHtml or a safeSanitize
helper) to prevent XSS while allowing tags to render; update references to
$('.email-template-body').val() as the input to the sanitizer before injecting.
♻️ Duplicate comments (2)
application/modules/invoices/views/view.php (1)
468-499: Accessibility: Input fields are missing accessible labels.The
<label>elements are not properly associated with their corresponding<input>elements viaforattributes. This makes the form less accessible for screen readers and voice control users.Suggested fix
<div class="col-xs-12 col-md-4"> <div class="invoice-properties"> - <label><?php _trans('related_quote_number'); ?></label> + <label for="invoice_quote_number"><?php _trans('related_quote_number'); ?></label> ... </div> <div class="invoice-properties"> - <label><?php _trans('work_order'); ?></label> + <label for="invoice_work_order"><?php _trans('work_order'); ?></label> ... </div> <div class="invoice-properties"> - <label><?php _trans('agreement'); ?></label> + <label for="invoice_agreement"><?php _trans('agreement'); ?></label> ... </div> </div>application/modules/quotes/views/view.php (1)
349-350: Accessibility: Input fields missing associated labelforattribute.The labels for
quote_work_orderandquote_agreementinputs don't have aforattribute linking them to their respective inputs, which impacts screen reader users.
🟡 Minor comments (9)
application/helpers/client_helper.php-45-56 (1)
45-56: Add null safety and use strict comparison.The function lacks null safety checks that
format_client()above implements. If$clientis null or missing theclient_numberproperty, this will throw an error.Additionally, consider using strict comparison (
!== "") orempty()for consistency with PHP best practices.Proposed fix with null safety
/** * `@param` object $client * `@return` string */ -function format_client_number($client) +function format_client_number($client): string { - if ($client->client_number != "") { + if (!empty($client->client_number)) { return $client->client_number; } return "00-00000"; }.github/workflows/README.md-221-226 (1)
221-226: Minor inconsistency in PHP version documentation.The documentation mentions "PHP 8.2" but the actual workflow (
pint.yml) uses PHP 8.4. Consider updating for consistency.**What it does:** 1. **Checks out the PR branch** - Gets the latest code from the pull request -2. **Sets up PHP environment** - Installs PHP 8.2 with required extensions +2. **Sets up PHP environment** - Installs PHP 8.4 with required extensions.github/workflows/phpstan.yml-65-93 (1)
65-93: Dead code: PR comment step will never execute.Since this workflow only triggers on
workflow_dispatch, the conditiongithub.event_name == 'pull_request'will never be true. Either addpull_requestto the triggers or remove this step.If you intend to support PR comments in the future, add the trigger:
on: workflow_dispatch: # Manual only — does not run automatically + pull_request: + branches: + - master + - developOtherwise, remove the dead code step (lines 65-93).
.github/workflows/setup.yml-371-374 (1)
371-374: Unreachable code:echo "::endgroup::"will never execute.The
exit 0andexit 1statements on lines 350 and 371 terminate the script before the::endgroup::echo can be reached.Suggested fix - move endgroup before exit statements
exit 0 fi - - echo "::endgroup::"The
::endgroup::at line 374 can be safely removed since the::group::at line 287 will be implicitly closed when the step ends.application/views/quote_templates/public/InvoicePlane_Web.php-215-216 (1)
215-216: Use$colspanfor discount rows to keep column alignment.The hardcoded colspan causes misalignment when item discounts are hidden.
🧩 Suggested fix
- <td class="no-bottom-border" colspan="4"></td> + <td class="no-bottom-border" colspan="<?php echo $colspan; ?>"></td> ... - <td class="no-bottom-border" colspan="4"></td> + <td class="no-bottom-border" colspan="<?php echo $colspan; ?>"></td>Also applies to: 267-268
.github/workflows/composer-update.yml-50-58 (1)
50-58: Composer audit output check always flags vulnerabilities.The
[ -s audit-report.json ]check is unreliable. Even when no vulnerabilities are found,composer audit --format=jsonoutputs valid JSON with an empty advisories object (e.g.,{"advisories": {}}), which is still a non-empty file. This causes the flag to always be set to true.Parse the JSON instead to check if the
advisoriesobject actually contains findings.Suggested fix
- if [ -s audit-report.json ]; then - echo "vulnerabilities_found=true" >> $GITHUB_OUTPUT - else - echo "vulnerabilities_found=false" >> $GITHUB_OUTPUT - fi + php -r '$data=json_decode(file_get_contents("audit-report.json"), true) ?: []; $has = !empty($data["advisories"]); echo "vulnerabilities_found=" . ($has ? "true" : "false") . PHP_EOL;' >> $GITHUB_OUTPUTapplication/modules/quotes/controllers/Ajax.php-52-53 (1)
52-53: Ineffectivestatuses()call inmodal_quote_lookups_select.Same issue as above - the
statuses()return value is discarded.🐛 Remove the ineffective call
if (!empty($quote_id)) { $this->mdl_quotes->is_related_quote($quote_id); - $this->mdl_quotes->statuses(); $quote = $this->mdl_quotes->get()->result(); echo json_encode($quote); }application/modules/quotes/views/modal_quote_lookups.php-42-42 (1)
42-42: Fix ARIA attributes for accessibility.The
aria-labelledbyattribute should reference theidof the element containing the modal's title, not the modal's own id. Also, the modal should havetabindex="-1"for proper focus management.♿ Proposed fix
-<div id="modal-choose-quote" class="modal col-xs-12 col-sm-10 col-sm-offset-1" role="dialog" aria-labelledby="modal-choose-quote" aria-hidden="true"> +<div id="modal-choose-quote" class="modal col-xs-12 col-sm-10 col-sm-offset-1" role="dialog" aria-labelledby="modal-choose-quote-title" aria-hidden="true" tabindex="-1"> <form class="modal-content"> <div class="modal-header"> <button type="button" class="close" data-dismiss="modal"><i class="fa fa-close"></i></button> - <h4 class="panel-title"><?php _trans('select_related_quote'); ?></h4> + <h4 class="panel-title" id="modal-choose-quote-title"><?php _trans('select_related_quote'); ?></h4> </div>application/modules/quotes/controllers/Ajax.php-29-30 (1)
29-30: Ineffectivestatuses()call - return value not used.The
statuses()method returns an array but the return value is not assigned to anything. This call has no effect.🐛 Remove the ineffective call
$this->mdl_quotes->by_client($client_id); $this->mdl_quotes->related_is_open(); - $this->mdl_quotes->statuses(); $quotes = $this->mdl_quotes->get()->result();
🧹 Nitpick comments (11)
application/third_party/MX/Controller.php (1)
55-55: Simplify the no-op string replacement.The
str_replace('', '', get_class($this))call replaces an empty string with an empty string, which is effectively a no-op. This makes the code less readable.♻️ Simplify to direct assignment
- $class = str_replace('', '', get_class($this)); + $class = get_class($this);.github/workflows/docker.yml (1)
23-27: Preferdocker composeor ensuredocker-composeis installed.Ubuntu runners have moved to Compose v2; the
docker-composebinary may be missing. Consider switching todocker composeor adding an install step.♻️ Proposed update
- - name: Build and start containers (if docker-compose.yml is ready) - run: docker-compose up -d --build + - name: Build and start containers (if docker-compose.yml is ready) + run: docker compose up -d --build - - name: Check running services - run: docker-compose ps + - name: Check running services + run: docker compose pscomposer.json (1)
20-20: Consider using a caret constraint forpocketarc/codeigniter.The fixed version
"3.3"won't receive patch updates automatically, unlike other dependencies using caret constraints (e.g.,^2.6). Consider using"^3.3"to allow patch-level updates while maintaining compatibility.Suggested change
- "pocketarc/codeigniter": "3.3", + "pocketarc/codeigniter": "^3.3",.github/workflows/pint.yml (2)
70-76: Parse error detection may have false positives.Grepping for
"!"in the output is too broad and may match legitimate content (e.g., file paths, output messages). Consider using a more specific pattern or relying solely on the exit code and "Parse error" string.Suggested improvement
# Check if there were parse errors - if grep -q "Parse error" pint_output.log || grep -q "!" pint_output.log; then + if grep -qE "Parse error|^\s+!" pint_output.log; then echo "parse_errors=true" >> $GITHUB_OUTPUT echo "WARNING: Parse errors detected in one or more files" elseOr alternatively, remove the
"!"check entirely and rely on the more specific "Parse error" pattern.
93-107: Consider handling push failures gracefully.If the
git pushfails (e.g., due to branch protection rules or concurrent updates), the workflow will fail without clear guidance. Consider adding error handling.Suggested improvement
git config --local user.email "github-actions[bot]@users.noreply.github.com" git config --local user.name "github-actions[bot]" git add -A git commit -m "style: apply Laravel Pint fixes" --no-verify - git push + git push || { + echo "::warning::Failed to push changes. This may be due to branch protection rules or concurrent updates." + echo "Please pull the latest changes and run Pint locally." + exit 1 + }.github/workflows/setup.yml (1)
102-108: Minor: Missing newline before Step 1 comment.There's a missing blank line between the initialization step output and the Step 1 comment, which slightly reduces readability.
echo "errors_found=false" >> $GITHUB_OUTPUT echo "::notice::Starting setup with error tracking enabled" + # Step 1: Yarn Install.github/workflows/phpstan.yml (1)
50-54: Fail step could be more informative.Consider outputting a summary of the error count or directing users to the artifacts for details.
Suggested improvement
- name: Check for errors and fail if found if: steps.phpstan-analysis.outcome == 'failure' run: | echo "PHPStan found errors in the codebase" + echo "See the uploaded artifacts for detailed results" + echo "Download phpstan-report.md for a formatted summary" exit 1.github/workflows/yarn-update.yml (2)
219-220: Consider enabling automatic branch deletion.Setting
delete-branch: falsewill leave automated branches (e.g.,automated/yarn-update-*) in the repository after the PR is merged. Over time, this can lead to branch clutter.💡 Suggested change
branch: automated/yarn-update-${{ github.run_number }} - delete-branch: false + delete-branch: true
107-136: Security update logic relies on parsing audit output format.The security update step parses
audit-report.jsonusinggrepto extract vulnerable package names. This approach is brittle and may break if Yarn's audit JSON format changes. Additionally, upgrading vulnerable packages to--latestmay introduce breaking changes.Consider pinning to compatible versions rather than always using
--latest:for pkg in $VULNERABLE_PACKAGES; do echo " Upgrading $pkg..." - yarn upgrade "$pkg" --latest || true + yarn upgrade "$pkg" || true doneapplication/modules/quotes/models/Mdl_quotes.php (1)
345-352: Consider adding type hint for$quote_idparameter.The method follows existing patterns in this file, but adding a type hint would improve type safety and documentation.
💡 Suggested improvement
/** + * `@param` int $quote_id * `@return` $this */ - public function is_related_quote($quote_id) + public function is_related_quote(int $quote_id) { $this->filter_where('ip_quotes.quote_id', $quote_id); return $this; }application/modules/quotes/views/modal_quote_lookups.php (1)
13-14: Remove commented-out debug code.These commented lines appear to be leftover from development and should be removed.
🧹 Suggested cleanup
- //$.post("<?php echo site_url('quotes/ajax/process_product_selections'); ?>", { - //$.post("<?php echo site_url('quotes/ajax/modal_quote_lookups_select'); ?>", { - $.post("<?php echo site_url('quotes/ajax/modal_quote_lookups_select'); ?>", {
| - name: Check for changes | ||
| id: check-changes | ||
| run: | | ||
| if git diff --quiet composer.lock; then | ||
| echo "changes_detected=false" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "changes_detected=true" >> $GITHUB_OUTPUT | ||
| fi |
There was a problem hiding this comment.
Detect composer.json changes when deciding to open a PR.
composer bump can update composer.json without a lockfile change; the current check would skip PR creation.
🛠️ Suggested fix
- if git diff --quiet composer.lock; then
+ if git diff --quiet composer.lock composer.json; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Check for changes | |
| id: check-changes | |
| run: | | |
| if git diff --quiet composer.lock; then | |
| echo "changes_detected=false" >> $GITHUB_OUTPUT | |
| else | |
| echo "changes_detected=true" >> $GITHUB_OUTPUT | |
| fi | |
| - name: Check for changes | |
| id: check-changes | |
| run: | | |
| if git diff --quiet composer.lock composer.json; then | |
| echo "changes_detected=false" >> $GITHUB_OUTPUT | |
| else | |
| echo "changes_detected=true" >> $GITHUB_OUTPUT | |
| fi |
🤖 Prompt for AI Agents
In @.github/workflows/composer-update.yml around lines 77 - 84, The "Check for
changes" step (id: check-changes) only inspects composer.lock via `git diff
--quiet composer.lock` so it misses updates to composer.json; update the check
to detect changes to both composer.lock and composer.json (e.g., test diffs or
git status for composer.json composer.lock) and set GITHUB_OUTPUT
changes_detected accordingly so PRs are opened when either file was modified.
| name: Crowdin Translation Sync | ||
|
|
||
| on: | ||
| workflow_dispatch: | ||
| inputs: | ||
| action: | ||
| description: 'Action to perform' | ||
| required: true | ||
| type: choice | ||
| options: | ||
| - upload-sources | ||
| - download-translations | ||
| - sync-bidirectional | ||
| default: 'download-translations' | ||
| schedule: | ||
| # Run weekly on Sundays at 2:00 AM UTC | ||
| - cron: '0 2 * * 0' | ||
|
|
||
| permissions: | ||
| contents: write | ||
| pull-requests: write | ||
|
|
||
| # Note: This workflow requires a Personal Access Token (PAT) to create pull requests. | ||
| # The default GITHUB_TOKEN has restricted permissions and cannot create PRs that trigger other workflows. | ||
| # | ||
| # To configure the required secret: | ||
| # 1. Create a Personal Access Token (classic) with 'repo' and 'workflow' scopes | ||
| # at https://github.com/settings/tokens | ||
| # 2. Add the token as a repository secret named 'PAT_TOKEN' | ||
| # at https://github.com/OWNER/REPO/settings/secrets/actions | ||
| # | ||
| # See: https://github.com/crowdin/github-action | ||
|
|
||
| jobs: | ||
| crowdin-sync: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Upload sources to Crowdin | ||
| if: github.event.inputs.action == 'upload-sources' || github.event.inputs.action == 'sync-bidirectional' | ||
| uses: crowdin/github-action@v2 | ||
| with: | ||
| upload_sources: true | ||
| upload_translations: false | ||
| download_translations: false | ||
| localization_branch_name: master | ||
| config: 'crowdin.yml' | ||
| project_id: ${{ secrets.CROWDIN_PROJECT_ID }} | ||
| token: ${{ secrets.CROWDIN_PERSONAL_TOKEN }} | ||
|
|
||
| - name: Download translations from Crowdin | ||
| if: github.event.inputs.action == 'download-translations' || github.event.inputs.action == 'sync-bidirectional' || github.event_name == 'schedule' | ||
| uses: crowdin/github-action@v2 | ||
| with: | ||
| upload_sources: false | ||
| upload_translations: false | ||
| download_translations: true | ||
| localization_branch_name: master | ||
| create_pull_request: true | ||
| pull_request_title: 'chore(i18n): update translations from Crowdin' | ||
| pull_request_body: | | ||
| ## Translation Update | ||
| This PR updates translations downloaded from Crowdin. | ||
| **Triggered by:** ${{ github.event_name }} | ||
| **Action:** ${{ github.event.inputs.action || 'download-translations' }} | ||
| ### Review Checklist | ||
| - [ ] Review translation changes for accuracy | ||
| - [ ] Check for any formatting issues | ||
| - [ ] Verify no code is affected | ||
| - [ ] Test translations in UI if possible | ||
| --- | ||
| *This PR was automatically created by the Crowdin Sync workflow.* | ||
| pull_request_labels: | | ||
| i18n | ||
| translations | ||
| crowdin | ||
| automated-pr | ||
| config: 'crowdin.yml' | ||
| project_id: ${{ secrets.CROWDIN_PROJECT_ID }} | ||
| token: ${{ secrets.CROWDIN_PERSONAL_TOKEN }} | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.PAT_TOKEN }} | ||
|
|
||
| - name: Workflow summary | ||
| run: | | ||
| echo "## Crowdin Sync Complete" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "**Action:** ${{ github.event.inputs.action || 'download-translations' }}" >> $GITHUB_STEP_SUMMARY | ||
| echo "**Triggered by:** ${{ github.event_name }}" >> $GITHUB_STEP_SUMMARY | ||
| echo "" >> $GITHUB_STEP_SUMMARY | ||
| echo "### Next Steps" >> $GITHUB_STEP_SUMMARY | ||
| echo "- Review the created pull request (if any)" >> $GITHUB_STEP_SUMMARY | ||
| echo "- Verify translation changes" >> $GITHUB_STEP_SUMMARY | ||
| echo "- Merge when ready" >> $GITHUB_STEP_SUMMARY |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check available branches
echo "=== Available Branches ==="
git branch -a 2>/dev/null || echo "No git branches available"
# Check localization_branch_name references in the workflow
echo -e "\n=== localization_branch_name in workflow ==="
rg -n 'localization_branch_name' .github/workflows/crowdin-sync.yml
# Check for crowdin.yml or crowdin.yaml config file
echo -e "\n=== Crowdin config files ==="
fd -a '(crowdin\.ya?ml|crowdin\.json)$' . -x cat {}
# Check default branch in repo config
echo -e "\n=== Repository Default Branch ==="
cat .git/config 2>/dev/null | grep -A5 'branch' || echo "Git config not available"
# Check common branch name patterns in repository
echo -e "\n=== Branch name patterns in README/docs ==="
fd -a 'README.md' . -x rg -i 'main branch|default branch|development|main|master' {} || echo "No README matches"Repository: InvoicePlane/InvoicePlane
Length of output: 1265
Change localization_branch_name from master to development.
The repository's default branch is development, not master. The hardcoded localization_branch_name: master on lines 51 and 63 references a non-existent branch, which will cause the workflow to fail when attempting to create pull requests. Update both instances to localization_branch_name: development.
🤖 Prompt for AI Agents
In @.github/workflows/crowdin-sync.yml around lines 1 - 105, The workflow is
using the wrong branch name for Crowdin operations: update the two occurrences
of localization_branch_name currently set to "master" to "development" inside
the "Upload sources to Crowdin" and "Download translations from Crowdin" steps
(look for the localization_branch_name key in those steps and replace its
value), so Crowdin actions target the repository's default branch.
| name: Build & Push PHPMyAdmin Docker Image | ||
|
|
||
| on: | ||
| push: | ||
| branches: | ||
| - development | ||
| paths: | ||
| - 'resources/docker/phpmyadmin/Dockerfile' | ||
| push: | ||
| tags: | ||
| - 'v*.*.*' | ||
| workflow_dispatch: | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/docker-phpmyadmin.yml | head -20Repository: InvoicePlane/InvoicePlane
Length of output: 602
🏁 Script executed:
git log --oneline -n 20 -- .github/workflows/docker-phpmyadmin.ymlRepository: InvoicePlane/InvoicePlane
Length of output: 177
🏁 Script executed:
git diff HEAD~5 HEAD -- .github/workflows/docker-phpmyadmin.ymlRepository: InvoicePlane/InvoicePlane
Length of output: 99
🏁 Script executed:
git log --all --oneline -- .github/workflows/docker-phpmyadmin.ymlRepository: InvoicePlane/InvoicePlane
Length of output: 198
🏁 Script executed:
git show a71ce175:.github/workflows/docker-phpmyadmin.yml 2>/dev/null | head -20Repository: InvoicePlane/InvoicePlane
Length of output: 462
🏁 Script executed:
git log --all --format="%H %s" -- .github/workflows/docker-phpmyadmin.ymlRepository: InvoicePlane/InvoicePlane
Length of output: 262
🏁 Script executed:
git show 3879f70d:.github/workflows/docker-phpmyadmin.yml 2>/dev/null | head -20Repository: InvoicePlane/InvoicePlane
Length of output: 411
🏁 Script executed:
git diff 3879f70d a71ce175 -- .github/workflows/docker-phpmyadmin.ymlRepository: InvoicePlane/InvoicePlane
Length of output: 551
Automatic image builds removed; workflow now manual-only. The push triggers on development branch and v*.*.* tags were replaced with workflow_dispatch, meaning builds won't trigger automatically on code changes or releases. If automatic builds are needed, restore the push and tag triggers alongside manual dispatch.
✅ Suggested trigger config (manual + automatic)
on:
+ push:
+ branches:
+ - development
+ paths:
+ - 'resources/docker/phpmyadmin/Dockerfile'
+ push:
+ tags:
+ - 'v*.*.*'
workflow_dispatch:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: Build & Push PHPMyAdmin Docker Image | |
| on: | |
| push: | |
| branches: | |
| - development | |
| paths: | |
| - 'resources/docker/phpmyadmin/Dockerfile' | |
| push: | |
| tags: | |
| - 'v*.*.*' | |
| workflow_dispatch: | |
| name: Build & Push PHPMyAdmin Docker Image | |
| on: | |
| push: | |
| branches: | |
| - development | |
| tags: | |
| - 'v*.*.*' | |
| paths: | |
| - 'resources/docker/phpmyadmin/Dockerfile' | |
| workflow_dispatch: | |
🤖 Prompt for AI Agents
In @.github/workflows/docker-phpmyadmin.yml around lines 1 - 5, The workflow's
triggers were changed to only workflow_dispatch, removing automatic builds;
update the top-level "on" block to include workflow_dispatch plus automatic
triggers by adding a "push" event for the development branch and a "push" with
"tags: ['v*.*.*']" (i.e., restore push/branches and tag pushes) so the workflow
runs both manually and automatically upon branch pushes and semver tags; ensure
the "on" block contains workflow_dispatch, push.branches: [development], and
push.tags: ['v*.*.*'] to restore the original behavior.
.github/workflows/phpunit.yml
Outdated
| name: Run PHPUnit Tests | ||
|
|
||
| on: | ||
| workflow_dispatch: # Manual only — does not run automatically | ||
|
|
||
| jobs: | ||
| phpunit: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
|
|
||
| # MySQL service commented out - tests use SQLite in-memory instead | ||
| # services: | ||
| # mysql: | ||
| # image: mysql:8 | ||
| # env: | ||
| # MYSQL_ROOT_PASSWORD: root | ||
| # MYSQL_DATABASE: testing | ||
| # ports: | ||
| # - 3306:3306 | ||
| # options: >- | ||
| # --health-cmd="mysqladmin ping --silent" | ||
| # --health-interval=10s | ||
| # --health-timeout=5s | ||
| # --health-retries=3 | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup PHP with Composer | ||
| uses: ./.github/actions/setup-php-composer | ||
| with: | ||
| php-version: '8.3' | ||
| php-extensions: 'mbstring, bcmath, pdo_mysql' | ||
| composer-flags: '--no-progress --prefer-dist --optimize-autoloader' | ||
|
|
||
| - name: Prepare Laravel environment | ||
| run: | | ||
| cp .env.testing.example .env.testing | ||
| php artisan key:generate --env=testing | ||
| #- name: Run Laravel migrations | ||
| # run: php artisan migrate --force --env=testing | ||
|
|
||
| #- name: Run Laravel seeds (optional) | ||
| # run: php artisan db:seed --force --env=testing | ||
|
|
||
| - name: Run PHPUnit | ||
| run: php artisan test --env=testing |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Laravel artifacts expected by the workflow
echo "=== Checking for artisan file ==="
fd -a '^artisan$'
echo "=== Checking for .env.testing.example file ==="
fd -a '\.env\.testing\.example$'
echo "=== Checking for laravel/framework in composer.json ==="
rg -n '"laravel/framework"' composer.jsonRepository: InvoicePlane/InvoicePlane
Length of output: 205
🏁 Script executed:
#!/bin/bash
# Identify the actual project structure
echo "=== Root directory files ==="
ls -la | head -20
echo "=== Checking for composer.json ==="
test -f composer.json && wc -l composer.json || echo "No composer.json"
echo "=== Checking for phpunit.xml ==="
fd -a 'phpunit\.xml'
echo "=== Checking for test directory ==="
fd -type d -name tests | head -5
echo "=== Checking for vendor/bin/phpunit ==="
test -f vendor/bin/phpunit && echo "phpunit found in vendor" || echo "No vendor/bin/phpunit"
echo "=== Looking at composer.json (first 50 lines) ==="
head -50 composer.json 2>/dev/null || echo "No composer.json"Repository: InvoicePlane/InvoicePlane
Length of output: 3263
🏁 Script executed:
#!/bin/bash
# Check for test-related files and environment setup
echo "=== Looking for any test files or directories ==="
find . -type f \( -name "*.test.php" -o -name "*Test.php" -o -name "test.php" \) 2>/dev/null | head -10
echo "=== Checking for .env files ==="
find . -name ".env*" -type f 2>/dev/null
echo "=== Checking application directory structure ==="
ls -la application/ | head -20
echo "=== Looking for phpunit configuration anywhere ==="
find . -name "phpunit*" 2>/dev/null | head -10
echo "=== Checking if tests folder exists anywhere ==="
find . -type d -name "*test*" 2>/dev/null | head -10Repository: InvoicePlane/InvoicePlane
Length of output: 1299
Rewrite phpunit.yml workflow—this project uses CodeIgniter, not Laravel.
The workflow assumes Laravel commands (php artisan, key:generate) and files (.env.testing.example), but this repository is a CodeIgniter 3.3 project with no test infrastructure. Commands like php artisan test, php artisan key:generate, and php artisan migrate do not exist in CodeIgniter. Either reconfigure this workflow for CodeIgniter testing or establish an appropriate testing framework and infrastructure for this project.
🤖 Prompt for AI Agents
In @.github/workflows/phpunit.yml around lines 1 - 49, The workflow uses
Laravel-specific files and commands (steps named "Prepare Laravel environment"
and "Run PHPUnit" that call php artisan and reference .env.testing.example)
which don't exist in this CodeIgniter 3.3 repo; replace those steps with
CodeIgniter-appropriate test setup: install PHPUnit via Composer (ensure
composer.json includes phpunit), add or reference a phpunit.xml/phpunit.xml.dist
suited to this project, remove php artisan key:generate/.env.testing.example
usage, and change the Run PHPUnit step to invoke vendor/bin/phpunit (or the
configured test command) and any necessary DB setup (create sqlite or mysql
service and migration/seeding commands if you add CI test helpers). Ensure the
workflow's "Setup PHP with Composer" step provides required PHP extensions and
that the final test step runs the repository's actual test runner instead of php
artisan.
.github/workflows/release.yml
Outdated
|
|
||
| # Step 9: Create GitHub Draft Pre-Release | ||
| - name: Create GitHub Draft Pre-Release | ||
| uses: softprops/action-gh-release@v1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the workflow file around line 427
cat -n .github/workflows/release.yml | sed -n '420,435p'Repository: InvoicePlane/InvoicePlane
Length of output: 664
🌐 Web query:
softprops/action-gh-release v1 deprecated runtime node
💡 Result:
Short answer: the v1 tag of softprops/action-gh-release runs on an older Node runtime (Node 16) and will trigger GitHub Actions deprecation warnings; upgrade your workflow to use the v2 release (which declares node20) to stop the warning.
What to do:
- Replace uses: softprops/action-gh-release@v1 with softprops/action-gh-release@v2 (or newer v2.x). [1][2]
- If you see other failures after upgrading, check the action changelog / issues for breaking changes or fixes. [3]
Sources:
- action-gh-release repo (usage & releases). [1]
- Example reporting of the Node.js 16 deprecation mentioning softprops/action-gh-release@v1. [2]
- action-gh-release changelog noting v2 upgrade to node20. [3]
Upgrade softprops/action-gh-release from v1 to v2.
The v1 release uses Node 16, which GitHub Actions has deprecated. Upgrading to v2 (Node 20) prevents workflow failures.
🔧 Suggested fix
- uses: softprops/action-gh-release@v1
+ uses: softprops/action-gh-release@v2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: softprops/action-gh-release@v1 | |
| uses: softprops/action-gh-release@v2 |
🧰 Tools
🪛 actionlint (1.7.10)
427-427: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In @.github/workflows/release.yml at line 427, Update the GitHub Actions step
that references softprops/action-gh-release by changing the action version from
v1 to v2 (replace softprops/action-gh-release@v1 with
softprops/action-gh-release@v2) so the workflow runs on Node 20; ensure any
inputs or outputs used with that step remain compatible after the version bump
and adjust them if the v2 action introduced any breaking changes.
| <label><?php _trans('work_order'); ?></label> | ||
| <input type="text" id="quote_work_order" class="form-control input-sm" | ||
| <?php if ($quote->quote_work_order) : ?> | ||
| value="<?php echo $quote->quote_work_order; ?>" | ||
| <?php endif; ?>> | ||
|
|
||
| </div> | ||
| <div class="quote-properties"> | ||
| <label><?php _trans('agreement'); ?></label> | ||
| <input type="text" id="quote_agreement" class="form-control input-sm" | ||
| <?php if ($quote->quote_agreement) : ?> | ||
| value="<?php echo $quote->quote_agreement; ?>" | ||
| <?php endif; ?>> | ||
| </div> |
There was a problem hiding this comment.
Missing XSS protection on output values.
The quote_work_order and quote_agreement values are output without HTML escaping. If these values contain malicious content, it could lead to XSS attacks.
🔒 Proposed fix
<input type="text" id="quote_work_order" class="form-control input-sm"
<?php if ($quote->quote_work_order) : ?>
- value="<?php echo $quote->quote_work_order; ?>"
+ value="<?php _htmlsc($quote->quote_work_order); ?>"
<?php endif; ?>>
</div>
<div class="quote-properties">
<label><?php _trans('agreement'); ?></label>
<input type="text" id="quote_agreement" class="form-control input-sm"
<?php if ($quote->quote_agreement) : ?>
- value="<?php echo $quote->quote_agreement; ?>"
+ value="<?php _htmlsc($quote->quote_agreement); ?>"
<?php endif; ?>>🤖 Prompt for AI Agents
In `@application/modules/quotes/views/view.php` around lines 349 - 362, The
quote_work_order and quote_agreement values are echoed into input value
attributes without escaping, enabling XSS; update the view (inputs with id
"quote_work_order" and "quote_agreement" in view.php) to output escaped values
using a safe HTML-escaping function (e.g.
htmlspecialchars($quote->quote_work_order, ENT_QUOTES, 'UTF-8') and
htmlspecialchars($quote->quote_agreement, ENT_QUOTES, 'UTF-8') or your
framework's html_escape helper) and always render the value attribute (even when
empty) to avoid malformed HTML.
| @@ -0,0 +1 @@ | |||
| # Added for versioning | |||
There was a problem hiding this comment.
Critical: Missing database schema changes for new fields.
This SQL migration file contains only a comment but no actual schema modifications. Based on the PR objectives, this file should include ALTER TABLE statements to add the following fields:
Clients table:
client_number
Invoices table:
invoice_quote_number(text field)invoice_work_order(imported from quote when associated)invoice_agreement(imported from quote when associated)
Quotes table:
quote_work_orderquote_agreement
Without these schema changes, the application code expecting these fields will fail at runtime with database errors.
📝 Example schema changes needed
# Added for versioning
# Add client_number to clients table
ALTER TABLE `ip_clients` ADD COLUMN `client_number` VARCHAR(255) DEFAULT NULL AFTER `client_id`;
# Add new fields to invoices table
ALTER TABLE `ip_invoices` ADD COLUMN `invoice_quote_number` TEXT DEFAULT NULL;
ALTER TABLE `ip_invoices` ADD COLUMN `invoice_work_order` VARCHAR(255) DEFAULT NULL;
ALTER TABLE `ip_invoices` ADD COLUMN `invoice_agreement` VARCHAR(255) DEFAULT NULL;
# Add new fields to quotes table
ALTER TABLE `ip_quotes` ADD COLUMN `quote_work_order` VARCHAR(255) DEFAULT NULL;
ALTER TABLE `ip_quotes` ADD COLUMN `quote_agreement` VARCHAR(255) DEFAULT NULL;Note: Column names, data types, and positions should be verified against the actual application code and existing schema conventions.
🤖 Prompt for AI Agents
In `@application/modules/setup/sql/041_1.7.0.sql` at line 1, The migration file
041_1.7.0.sql currently contains only a comment; add ALTER TABLE statements to
create the missing columns used by the app: add client_number (e.g.,
VARCHAR(255) DEFAULT NULL) to ip_clients, add invoice_quote_number (TEXT DEFAULT
NULL), invoice_work_order (VARCHAR(255) DEFAULT NULL) and invoice_agreement
(VARCHAR(255) DEFAULT NULL) to ip_invoices, and add quote_work_order
(VARCHAR(255) DEFAULT NULL) and quote_agreement (VARCHAR(255) DEFAULT NULL) to
ip_quotes; ensure the statements are idempotent for your DB flavor (or wrapped
in a safe conditional/transaction) and follow the repository’s naming and
datatype conventions so functions referencing these fields (client_number,
invoice_quote_number, invoice_work_order, invoice_agreement, quote_work_order,
quote_agreement) will find the columns at runtime.
| if ($quote->user_vat_id) { | ||
| echo lang('vat_id_short') . ': ' . $quote->user_vat_id . '<br>'; | ||
| } | ||
| if ($quote->user_tax_code) { | ||
| echo lang('tax_code_short') . ': ' . $quote->user_tax_code . '<br>'; |
There was a problem hiding this comment.
Escape VAT/tax IDs and attachment names in the public view.
These fields can contain user-supplied values; rendering them unescaped risks stored XSS.
🛠️ Suggested fix
- echo lang('vat_id_short') . ': ' . $quote->user_vat_id . '<br>';
+ echo lang('vat_id_short') . ': ' . htmlsc($quote->user_vat_id) . '<br>';
...
- echo lang('tax_code_short') . ': ' . $quote->user_tax_code . '<br>';
+ echo lang('tax_code_short') . ': ' . htmlsc($quote->user_tax_code) . '<br>';
...
- echo ': ' . $quote->client_vat_id . '<br>';
+ echo ': ' . htmlsc($quote->client_vat_id) . '<br>';
...
- echo ': ' . $quote->client_tax_code . '<br>';
+ echo ': ' . htmlsc($quote->client_tax_code) . '<br>';
...
- <td><?php echo $attachment['name']; ?></td>
+ <td><?php echo htmlsc($attachment['name']); ?></td>Also applies to: 116-122, 316-316
🤖 Prompt for AI Agents
In `@application/views/quote_templates/public/InvoicePlane_Web.php` around lines
79 - 83, Escape the user-supplied VAT/tax ID and attachment name outputs to
prevent stored XSS by replacing direct echoes of $quote->user_vat_id and
$quote->user_tax_code (and any attachment name echoes around the other mentioned
lines) with an HTML-escaped variant; use the project's escaping helper (e.g.,
html_escape()) or PHP's htmlspecialchars(..., ENT_QUOTES, 'UTF-8') when
outputting these variables so all printed values are properly encoded before
echoing.
| // Ensure that string values for options.of are treated strictly as selectors | ||
| // and are not interpreted as HTML by the jQuery constructor. | ||
| if ( typeof options.of === "string" ) { | ||
| target = $( document ).find( options.of ); | ||
| } else { | ||
| target = $( options.of ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CVE-2022-31160 jQuery UI XSS vulnerability
💡 Result:
Short summary — CVE-2022-31160 (jQuery UI)
-
What it is: a reflected/stored XSS in jQuery UI's checkboxradio widget. When a checkboxradio is initialized on an input inside a label, calling .checkboxradio("refresh") can incorrectly decode encoded HTML entities in the label text and thereby execute JavaScript. [1][2]
-
Affected versions: jquery-ui < 1.13.2. Patched in jQuery UI 1.13.2. [1][2]
-
Severity / impact: CVSSv3 ~6.1 (Medium); network-exploitable but requires user interaction in many scenarios. [1][3]
-
Mitigation / fix: upgrade to jQuery UI 1.13.2 (or later). As a workaround, wrap non-input contents of the label in a to prevent the unsafe decoding. [1][2][4]
References
- NVD / CVE-2022-31160.
- GitHub Advisory (jquery-ui) — GHSA-h6gj-6jjq-h8g9.
- Debian / Ubuntu security tracker entries.
- Vendor/third‑party advisories describing the mitigation and patch.
🌐 Web query:
jQuery UI 1.13.0 security patches CVE position widget
💡 Result:
Short answer — yes: jQuery UI 1.13.0 fixed multiple XSS issues (including the .position() "of" option). Specifically:
- CVE-2021-41184 — XSS in the "of" option of the .position() util; fixed in 1.13.0. [1][2]
- CVE-2021-41182 — XSS in Datepicker altField; fixed in 1.13.0. [1][3]
- CVE-2021-41183 — XSS in Datepicker *Text options; fixed in 1.13.0. [1][3]
Note: a later XSS (CVE-2022-31160) affecting checkboxradio.refresh was fixed in jQuery UI 1.13.2. [4]
Sources:
[1] jquery/jquery-ui security advisories (GitHub).
[2] CVE-2021-41184 advisories / OSV / vendor pages.
[3] CVE-2021-41182 / CVE-2021-41183 entries (NVD/OSV).
[4] CVE-2022-31160 (package advisories / Rapid7 / OSV).
🌐 Web query:
jQuery UI 1.12.1 to 1.13.0 upgrade security fixes
💡 Result:
Summary — what changed (security fixes)
- jQuery UI 1.13.0 fixed three XSS vulnerabilities present in 1.12.1:
- CVE-2021-41182 — XSS via Datepicker altField. [1][3]
- CVE-2021-41183 — XSS via various Datepicker "*Text" options (closeText, currentText, prev/nextText, buttonText, appendText). [1][3]
- CVE-2021-41184 — XSS via the of option of the .position() utility. [1][6]
Recommendation
- Upgrade any jQuery UI < 1.13.0 to 1.13.0 or later (patches 1.13.1/1.13.2 followed and included further security/regression fixes). [2][5][4]
Sources
- jQuery UI 1.13.0 release notes / upgrade guide. [1][2]
- Snyk vulnerability page for jquery-ui 1.12.1. [3]
- Tenable advisory summarizing the three CVEs. [4]
- jQuery UI blog posts for 1.13.x patch releases. [5]
Security fix for XSS vulnerability in .position() widget — verify alignment with official patches.
This change correctly mitigates CVE-2021-41184, a known jQuery UI XSS vulnerability where passing a string directly to $() in the .position() widget's of option could interpret it as HTML instead of a selector. Using $(document).find(options.of) ensures string inputs are treated strictly as selectors.
Regarding the CodeQL warning on line 893: this appears to be a false positive. The else branch only executes when options.of is not a string (e.g., DOM element, jQuery object, Event object, or window/document), all of which are safe to pass to $().
Consider upgrading to jQuery UI 1.13.0+ instead of maintaining manual patches to this vendored file, as 1.13.0 includes this fix natively.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 893-893: Unsafe jQuery plugin
Potential XSS vulnerability in the '$.fn.position' plugin.
🤖 Prompt for AI Agents
In `@assets/core/js/jquery-ui.js` around lines 888 - 894, The .position() widget
is vulnerable to XSS when a string passed to options.of is handed directly to
$() — change handling so that when options.of is a string you call
$(document).find(options.of) (ensuring strings are treated only as selectors)
and otherwise continue to pass non-string values (DOM element, jQuery object,
Event, window/document) directly to $(); update the branch around options.of and
target to reflect this secure pattern and consider replacing the vendored
assets/core/js/jquery-ui.js with jQuery UI 1.13.0+ which includes this fix
upstream.
assets/core/js/scripts.js
Outdated
| function update_email_template_preview() { | ||
| $('#email-template-preview').contents().find("body").html($('.email-template-body').val()); | ||
| $('#email-template-preview').contents().find("body").text($('.email-template-body').val()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check git diff to see the original code before the change
cd assets/core/js && git diff HEAD scripts.js | head -100Repository: InvoicePlane/InvoicePlane
Length of output: 51
🏁 Script executed:
# Also check the broader context of the change
git log --oneline -20 -- assets/core/js/scripts.jsRepository: InvoicePlane/InvoicePlane
Length of output: 177
🏁 Script executed:
# Get the full function to understand context better
git show HEAD:assets/core/js/scripts.js | sed -n '76,85p'Repository: InvoicePlane/InvoicePlane
Length of output: 472
🏁 Script executed:
# Check if there's a specific commit that changed this function
git log -p --follow -S "update_email_template_preview" -- assets/core/js/scripts.js | head -200Repository: InvoicePlane/InvoicePlane
Length of output: 51
🏁 Script executed:
# Use git blame to find when .text() was introduced
git blame assets/core/js/scripts.js | grep -A2 -B2 "update_email_template_preview"Repository: InvoicePlane/InvoicePlane
Length of output: 3304
🏁 Script executed:
# Search for any previous versions of this function in git history
git log --all -p -- assets/core/js/scripts.js | grep -A10 -B5 "update_email_template_preview" | head -150Repository: InvoicePlane/InvoicePlane
Length of output: 5635
🏁 Script executed:
# Check for related security issues or PRs
git log --all --oneline | grep -i "email\|template\|security\|xss" | head -20Repository: InvoicePlane/InvoicePlane
Length of output: 51
🏁 Script executed:
# Look for any comments or TODOs related to email template in the file
grep -n -B3 -A3 "email.template" assets/core/js/scripts.jsRepository: InvoicePlane/InvoicePlane
Length of output: 2660
HTML email preview will show raw tags instead of rendering.
Using .text() escapes HTML, so the preview breaks—users will see plain-text markup like <b>text</b> instead of rendered output. If the intent was security hardening, sanitize the input first then render with .html().
🔧 Suggested fix
-function update_email_template_preview() {
- $('#email-template-preview').contents().find("body").text($('.email-template-body').val());
-}
+function update_email_template_preview() {
+ const html = $('.email-template-body').val();
+ // If needed, sanitize here to prevent XSS.
+ $('#email-template-preview').contents().find("body").html(html);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function update_email_template_preview() { | |
| $('#email-template-preview').contents().find("body").html($('.email-template-body').val()); | |
| $('#email-template-preview').contents().find("body").text($('.email-template-body').val()); | |
| function update_email_template_preview() { | |
| const html = $('.email-template-body').val(); | |
| // If needed, sanitize here to prevent XSS. | |
| $('#email-template-preview').contents().find("body").html(html); | |
| } |
🤖 Prompt for AI Agents
In `@assets/core/js/scripts.js` around lines 76 - 77, The preview currently uses
.text() in update_email_template_preview(), which escapes HTML and shows raw
tags; change it to write sanitized HTML into the iframe/body (use .html() on
$('#email-template-preview').contents().find('body')) after running the template
content through your sanitization function (e.g., sanitizeHtml or a safeSanitize
helper) to prevent XSS while allowing tags to render; update references to
$('.email-template-body').val() as the input to the sanitizer before injecting.
nielsdrost7
left a comment
nielsdrost7
left a comment
There was a problem hiding this comment.
In @.github/workflows/composer-update.yml:
- Around line 77-84: The "Check for changes" step (id: check-changes) only
inspects composer.lock viagit diff --quiet composer.lockso it misses updates
to composer.json; update the check to detect changes to both composer.lock and
composer.json (e.g., test diffs or git status for composer.json composer.lock)
and set GITHUB_OUTPUT changes_detected accordingly so PRs are opened when either
file was modified.
In @.github/workflows/crowdin-sync.yml:
- Around line 1-105: The workflow is using the wrong branch name for Crowdin
operations: update the two occurrences of localization_branch_name currently set
to "master" to "development" inside the "Upload sources to Crowdin" and
"Download translations from Crowdin" steps (look for the
localization_branch_name key in those steps and replace its value), so Crowdin
actions target the repository's default branch.
In @.github/workflows/docker-phpmyadmin.yml:
- Around line 1-5: The workflow's triggers were changed to only
workflow_dispatch, removing automatic builds; update the top-level "on" block to
include workflow_dispatch plus automatic triggers by adding a "push" event for
the development branch and a "push" with "tags: ['v*..']" (i.e., restore
push/branches and tag pushes) so the workflow runs both manually and
automatically upon branch pushes and semver tags; ensure the "on" block contains
workflow_dispatch, push.branches: [development], and push.tags: ['v*..'] to
restore the original behavior.
In @.github/workflows/phpunit.yml:
- Around line 1-49: The workflow uses Laravel-specific files and commands (steps
named "Prepare Laravel environment" and "Run PHPUnit" that call php artisan and
reference .env.testing.example) which don't exist in this CodeIgniter 3.3 repo;
replace those steps with CodeIgniter-appropriate test setup: install PHPUnit via
Composer (ensure composer.json includes phpunit), add or reference a
phpunit.xml/phpunit.xml.dist suited to this project, remove php artisan
key:generate/.env.testing.example usage, and change the Run PHPUnit step to
invoke vendor/bin/phpunit (or the configured test command) and any necessary DB
setup (create sqlite or mysql service and migration/seeding commands if you add
CI test helpers). Ensure the workflow's "Setup PHP with Composer" step provides
required PHP extensions and that the final test step runs the repository's
actual test runner instead of php artisan.
In @.github/workflows/release.yml:
- Line 427: Update the GitHub Actions step that references
softprops/action-gh-release by changing the action version from v1 to v2
(replace softprops/action-gh-release@v1 with softprops/action-gh-release@v2) so
the workflow runs on Node 20; ensure any inputs or outputs used with that step
remain compatible after the version bump and adjust them if the v2 action
introduced any breaking changes.
In @.github/workflows/setup.yml:
- Around line 196-223: The workflow step with id key_generate currently runs the
Laravel command "php artisan key:generate", but this is a CodeIgniter 3 project;
replace that call with a CI-appropriate key setup: remove "php artisan
key:generate" and instead generate a random encryption key (for example via a
short PHP one-liner or openssl) and write it into your CodeIgniter config (set
$config['encryption_key'] in application/config/config.php or write to your .env
if you use one); update the step id key_generate to echo success/failure exactly
as before based on that command’s exit code. Also scan other workflow steps that
call "php artisan migrate" or "php artisan db:seed" and replace them with your
project's migration/seed approach (invoke your custom CLI controllers via "php
index.php ControllerName method" or run your project-specific migration
scripts).
In @.github/workflows/test-php.yml:
- Around line 3-15: Update the GitHub Actions workflow triggers and matrix so CI
runs on PRs and includes the project-required minimum PHP version: add
pull_request to the top-level on: triggers (in addition to workflow_dispatch)
and expand the strategy.matrix.php-versions array to include '8.1' alongside
'8.2'; confirm the matrix key names operating-system and php-versions remain
unchanged so the run job picks up the new entries.
In @.github/workflows/yarn-update.yml:
- Around line 203-211: The workflow step with id updated-packages invokes the
missing script generate-package-update-report.cjs; either add that script under
the repository (implement the logic that generates updated-packages.txt and
exits non-zero on errors) or change the step to point to the existing script
path/name that produces updated-packages.txt; update the run block to call the
correct script name and ensure the script writes updated-packages.txt before the
echo to GITHUB_OUTPUT so the "Get updated packages" step succeeds.
In @application/helpers/file_security_helper.php:
- Line 177: The code calls a non-existent mb_rtrim when building $fullPath in
file_security_helper.php; replace the mb_rtrim usage with PHP's rtrim() so
$fullPath = rtrim($baseDirectory, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR .
$basename['filename']; (i.e., update the expression that constructs $fullPath to
use rtrim instead of mb_rtrim). - Line 87: The code calls a non-existent mb_rtrim which will cause a fatal
error; replace the mb_rtrim call with the standard rtrim when building
$realBaseWithSep from $realBase and DIRECTORY_SEPARATOR (e.g., update the
expression that sets $realBaseWithSep to use rtrim($realBase,
DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR) because DIRECTORY_SEPARATOR is a
single-byte ASCII char and multibyte handling is unnecessary.
In @application/modules/clients/Enums/ClientTitleEnum.php:
- Around line 20-31: The tryFrom method currently uses array_search on
self::getValues() and checks if ($searchResult), which skips the first enum
(index 0) and returns the index rather than the matched string; change the
lookup to perform a strict search (e.g., use array_search($value, $values, true)
or use in_array with strict true) and when a match is found return the matched
value string (the element from $values), not the numeric index; ensure tryFrom
returns null (or the expected empty/false value) when no strict match exists.
In @application/modules/invoices/views/view.php:
- Around line 479-498: The echoed input values for invoice_quote_number,
invoice_work_order, and invoice_agreement are not escaped and can cause XSS;
update the value outputs in the view to escape user content (e.g., wrap the
properties with htmlsc() or htmlspecialchars()) when echoing inside the value
attribute for the inputs in view.php (references: invoice_quote_number,
invoice_work_order, invoice_agreement) so the rendered value attribute uses the
escaped string.
In @application/modules/quotes/controllers/Ajax.php:
- Around line 21-43: In modal_quote_lookups ensure $data is always defined
before calling $this->layout->load_view to avoid the undefined variable when
$client_id is empty: either initialize $data = array() (or with default keys
'related_quotes','client_id','quote_statuses') before the if block, or add an
early return/conditional so load_view is only called when $data is set;
reference the modal_quote_lookups method, the $this->mdl_quotes->... calls, and
the final $this->layout->load_view('quotes/modal_quote_lookups', $data) when
applying the fix. - Around line 144-146: The POST-extracted variables $quote_work_order and
$quote_agreement are never added to the record being saved; update the
controller method in application/modules/quotes/controllers/Ajax.php to include
these keys in the $db_array (e.g., 'quote_work_order' => $quote_work_order and
'quote_agreement' => $quote_agreement) before the save/insert call so the values
persist to the DB; ensure the $db_array used by the existing save/update
function contains those two fields and run existing migrations/column names to
match.
In @application/modules/quotes/views/modal_quote_lookups.php:
- Around line 9-29: Validate that a quote is selected in the
.select-items-confirm click handler by checking quote_id (from
$("input[name='quote_id']:checked").val()) and aborting with a user-facing
message if invalid; wrap the $.post callback parsing in a try/catch around
JSON.parse(data) and handle parse or AJAX errors by logging and showing an error
to the user; replace the for..in loop over items with a deterministic iteration
(e.g., use items[0] or a for loop over numeric indices) so all fields are set
correctly and move $('#modal-choose-quote').modal('hide') outside the loop so
the modal is closed only after processing completes; ensure the error-handling
branches do not attempt to use the parsed items when parsing fails.
In @application/modules/quotes/views/view.php:
- Around line 349-362: The quote_work_order and quote_agreement values are
echoed into input value attributes without escaping, enabling XSS; update the
view (inputs with id "quote_work_order" and "quote_agreement" in view.php) to
output escaped values using a safe HTML-escaping function (e.g.
htmlspecialchars($quote->quote_work_order, ENT_QUOTES, 'UTF-8') and
htmlspecialchars($quote->quote_agreement, ENT_QUOTES, 'UTF-8') or your
framework's html_escape helper) and always render the value attribute (even when
empty) to avoid malformed HTML.
In @application/modules/setup/sql/041_1.7.0.sql:
- Line 1: The migration file 041_1.7.0.sql currently contains only a comment;
add ALTER TABLE statements to create the missing columns used by the app: add
client_number (e.g., VARCHAR(255) DEFAULT NULL) to ip_clients, add
invoice_quote_number (TEXT DEFAULT NULL), invoice_work_order (VARCHAR(255)
DEFAULT NULL) and invoice_agreement (VARCHAR(255) DEFAULT NULL) to ip_invoices,
and add quote_work_order (VARCHAR(255) DEFAULT NULL) and quote_agreement
(VARCHAR(255) DEFAULT NULL) to ip_quotes; ensure the statements are idempotent
for your DB flavor (or wrapped in a safe conditional/transaction) and follow the
repository’s naming and datatype conventions so functions referencing these
fields (client_number, invoice_quote_number, invoice_work_order,
invoice_agreement, quote_work_order, quote_agreement) will find the columns at
runtime.
In @application/views/quote_templates/public/InvoicePlane_Web.php:
- Around line 79-83: Escape the user-supplied VAT/tax ID and attachment name
outputs to prevent stored XSS by replacing direct echoes of $quote->user_vat_id
and $quote->user_tax_code (and any attachment name echoes around the other
mentioned lines) with an HTML-escaped variant; use the project's escaping helper
(e.g., html_escape()) or PHP's htmlspecialchars(..., ENT_QUOTES, 'UTF-8') when
outputting these variables so all printed values are properly encoded before
echoing.
In @assets/core/js/jquery-ui.js:
- Around line 888-894: The .position() widget is vulnerable to XSS when a string
passed to options.of is handed directly to $() — change handling so that when
options.of is a string you call $(document).find(options.of) (ensuring strings
are treated only as selectors) and otherwise continue to pass non-string values
(DOM element, jQuery object, Event, window/document) directly to $(); update the
branch around options.of and target to reflect this secure pattern and consider
replacing the vendored assets/core/js/jquery-ui.js with jQuery UI 1.13.0+ which
includes this fix upstream.
In @assets/core/js/scripts.js:
- Around line 76-77: The preview currently uses .text() in
update_email_template_preview(), which escapes HTML and shows raw tags; change
it to write sanitized HTML into the iframe/body (use .html() on
$('#email-template-preview').contents().find('body')) after running the template
content through your sanitization function (e.g., sanitizeHtml or a safeSanitize
helper) to prevent XSS while allowing tags to render; update references to
$('.email-template-body').val() as the input to the sanitizer before injecting.
|
@nielsdrost7 I've opened a new pull request, #1394, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ity issues (#1394) * Initial plan * Fix GitHub workflow issues: triggers, commands, and scripts Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com> * Fix PHP security issues: XSS, undefined variables, and logic bugs Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com> * Fix JavaScript XSS vulnerability in email template preview Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com> * Address code review feedback: improve URL validation and data handling Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR adds comprehensive GitHub Actions CI/CD infrastructure, implements quote-related features (work order and agreement fields), migrates from codeigniter/framework to pocketarc/codeigniter, and applies widespread code formatting improvements. It includes new database migrations, a quote lookup modal UI, security fixes for HTML sanitization, and configuration updates across build and deployment workflows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
application/modules/sessions/controllers/Sessions.php (1)
451-556: Add explicit PHP version constraint to composer.json.The codebase uses
str_contains()andstr_starts_with(), which require PHP 8.0+. While the Docker configuration specifies PHP 8.1,composer.jsonlacks an explicit PHP version requirement in the "require" section. This allows installations without the required runtime, causing fatal errors. Add"php": ">=8.1"to composer.json to enforce the minimum version requirement.
🤖 Fix all issues with AI agents
In @.github/scripts/generate-package-update-report.cjs:
- Around line 26-33: The current pattern in the package extraction block (where
updatedPackages is populated) is too loose and picks up non-package keys like
"__metadata"; update the parsing to only consider yarn.lock entry header lines
that end with a colon and then extract the package descriptor/name from that
header (i.e., match lines like 'package@version:'), rejecting keys without
trailing ':'; modify the two regex checks around the line.match(...) and the
descriptor extraction (the match(...) used to populate updatedPackages) to first
assert the line ends with ':' and then capture the package identifier (including
scoped names and potential slash) from the descriptor before adding to
updatedPackages.
In @.github/workflows/docker-publish.yml:
- Around line 3-4: The workflow currently only uses the workflow_dispatch
trigger, so automatic Docker builds on version tag pushes were removed; restore
the push.tags trigger by updating the workflow's triggers to include a push
event filtering tags (e.g., add a push: tags: ['v*'] entry alongside
workflow_dispatch) so that when version tags like "v1.7.0" are pushed the Docker
publish job runs automatically; modify the "on:" block in
.github/workflows/docker-publish.yml (where workflow_dispatch is declared) to
include the push.tags pattern.
In @.github/workflows/phpstan.yml:
- Around line 65-93: The "Comment on PR (if available)" step is unreachable
because the workflow only triggers on workflow_dispatch while the step checks
github.event_name == 'pull_request'; either remove this step entirely if PR
comments aren't needed, or update the workflow triggers to include pull_request
so the condition can be true — adjust the top-level "on:" entries to add
pull_request (or change the step's if to match an existing trigger) and keep the
step's existing fs/readFileSync and github.rest.issues.createComment logic
intact.
In @.github/workflows/pint.yml:
- Around line 14-18: Update the pull_request trigger filter under the workflow's
pull_request block so it includes the repository's actual default branch name;
replace or add the correct branch 'development' alongside 'master' and any other
branches listed (currently 'develop') so the Pint workflow runs for PRs
targeting 'development' — modify the branches array in the pull_request YAML
block to include 'development' (and remove 'develop' if that was a typo).
- Around line 93-101: The push step "Commit and Push Changes" should skip when
the PR comes from a fork; update its if condition to only run when changes exist
and the workflow is running in the same repository (not a fork). Replace the
current condition (steps.verify_diff.outputs.has_changes == 'true') with a
compound check such as: steps.verify_diff.outputs.has_changes == 'true' &&
(github.event.pull_request == null ||
github.event.pull_request.head.repo.full_name == github.repository) so the push
runs for non-PR runs or PRs originating from the same repo but is skipped for
forked PRs.
In @.github/workflows/quickstart.yml:
- Around line 59-68: The workflow currently runs Laravel-only commands (steps
named "Prepare Laravel environment", "Run database migrations", "Run smoke
tests") using `php artisan`, which don't exist in this CodeIgniter 3.x repo;
remove or replace those steps: either delete this workflow if no CI tests are
needed, or update the steps to CodeIgniter-compatible actions such as installing
PHP dependencies (composer install), copying the repository's CodeIgniter
env/example file into the runtime env, setting up any DB fixtures/migrations
using the project's migration script or SQL seed files, and running tests via
the project's configured test runner (e.g., phpunit) instead of `php artisan
test`; adjust step names ("Prepare Laravel environment", "Run database
migrations", "Run smoke tests") accordingly so they reflect CodeIgniter
commands.
In @.github/workflows/README.md:
- Around line 623-644: Remove the hard-coded line numbers in the README and
update the instructions to reference searchable identifiers instead: instruct
users to edit the release.yml keys (php-version, node-version, retention-days)
by locating those keys in release.yml and to modify the "Create release zip"
step by its step name ("Create release zip") when changing ZIP exclusions;
replace each outdated line-number instruction with guidance to find and edit the
specific key or step name so the docs remain correct across file edits.
- Line 40: Update the README line that currently says "Automatically runs on
every push to the `master` branch" to accurately describe the actual trigger
used by the workflow: reference the workflow file (release.yml) and state it
uses workflow_dispatch (manual trigger) and requires selecting a release type;
replace the sentence accordingly so it mentions manual dispatch and release type
selection rather than automatic pushes to master.
In @.github/workflows/setup.yml:
- Around line 34-51: Update the workflow input descriptions to remove "php
artisan" and reference CodeIgniter's CLI instead: replace the description string
'Run php artisan key:generate' with something like 'Run CodeIgniter CLI (php
spark) key generation' (or 'Run php spark key:generate'), change 'Run php
artisan migrate' (input run_migrate) to 'Run CodeIgniter migration (php spark
migrate)' and change 'Run php artisan db:seed' (input run_seed) to 'Run
CodeIgniter seeder (php spark db:seed)'; ensure you only update the description
values for these inputs so the dispatch UI labels are accurate for a CodeIgniter
workflow.
In @.github/workflows/test-frontend.yml:
- Around line 6-7: The workflow currently only uses the on: workflow_dispatch
trigger which disables automatic PR testing; update the on: section to include
pull_request (e.g., change from only workflow_dispatch to on: { pull_request:
{}, workflow_dispatch: {} }) so frontend tests run automatically on PRs as well
as allowing manual dispatch; ensure the pull_request entry is present and
configured with any branch filters you need.
In `@application/modules/guest/controllers/gateways/Stripe.php`:
- Line 122: The code is accessing nonexistent Checkout Session properties
($session->cancel, $session->cancellation_reason, $session->last_payment_error);
update the logic that builds the error/explanation string to use
$session->status or $session->payment_status instead and remove uses of
$session->cancel/cancellation_reason, and when you need detailed payment errors
expand or retrieve the session with 'payment_intent' and read
$session->payment_intent->last_payment_error as the source of payment failure
info; adjust the fallback plumbing so the expression that previously referenced
($session->cancel ? $session->cancellation_reason :
$session->last_payment_error) instead checks status/payment_status and, if
necessary, uses the expanded payment_intent->last_payment_error for detailed
messages.
In `@application/modules/invoices/views/view.php`:
- Around line 468-492: These three text inputs (invoice_quote_number,
invoice_work_order, invoice_agreement) are editable even when the invoice is
read-only; update their markup to emit the same read-only/disabled attribute
used by other invoice fields by conditionally adding readonly/disabled when the
invoice is read-only (use the same condition/variable other fields use, e.g.
$invoice->is_read_only or the existing invoice-status check), so the inputs are
non-editable in read-only state.
In `@application/modules/quotes/controllers/Ajax.php`:
- Around line 49-62: The modal_quote_lookups_select method currently returns
nothing when $quote_id is empty which breaks client-side JSON.parse; update
modal_quote_lookups_select to validate $quote_id and return a consistent JSON
error response (and appropriate HTTP status) instead of silence — e.g., when
$quote_id is empty respond with a JSON object like {"error":"quote_id required"}
using the same output mechanism used elsewhere (use
$this->output->set_content_type('application/json')->set_output(json_encode(...))
or equivalent) so callers parsing JSON won't fail; keep the existing logic that
loads $this->mdl_quotes and returns the quote when $quote_id is present.
In `@application/modules/quotes/views/modal_quote_lookups.php`:
- Around line 101-103: The current echo of
$quote_statuses[$quote->quote_status_id]['label'] can raise an undefined index
when $quote->quote_status_id is missing or not present in $quote_statuses;
update the view to check that isset($quote_statuses[$quote->quote_status_id])
(and optionally isset(['label'])) before accessing it and render a safe fallback
(e.g. a default label like "Unknown" or an empty string) when the lookup fails
so the template doesn't trigger notices; locate the usage in
modal_quote_lookups.php where $quote_statuses and $quote->quote_status_id are
referenced and add the conditional fallback logic there.
In `@application/modules/sessions/controllers/Sessions.php`:
- Around line 323-444: The current session-based rate limiting in
_is_ip_rate_limited_password_reset, _record_password_reset_attempt,
_is_email_rate_limited_password_reset and _record_email_password_reset_attempt
is bypassable by clearing cookies; replace session storage with a server-side
store (Redis or DB) keyed by IP/email with a TTL (e.g. keys like
password_reset:ip:{md5(ip)} and password_reset:email:{md5(email)}), implement
atomic increment or append-with-expiry and window-based trimming on read,
enforce counts against env-configured limits, persist attempts with expiry
instead of session->userdata/session->set_userdata, and update logging and tests
to reflect the new storage mechanism.
In `@application/modules/setup/sql/038_1.6.1.sql`:
- Around line 1-6: The migration duplicates columns already added in
041_1.7.0.sql causing "Duplicate column name" errors; update 038_1.6.1.sql to
avoid re-adding the same columns by either removing the six ALTER TABLE
statements for client_number (ip_clients), invoice_quote_number,
invoice_work_order, invoice_agreement (ip_invoices), and quote_work_order,
quote_agreement (ip_quotes), or replace each direct ALTER with conditional logic
that first checks for the column's existence (e.g., query
INFORMATION_SCHEMA/COLUMN_NAME or use CREATE/ALTER ... IF NOT EXISTS where
supported) and only runs the ALTER if the column is missing; ensure the chosen
fix preserves the intended data type/position consistency with 041_1.7.0.sql
(VARCHAR(255)/TEXT and AFTER positions) so sequential upgrades do not conflict.
In `@application/modules/setup/sql/041_1.7.0.sql`:
- Around line 1-17: The migrations create the same columns with conflicting
types—ensure a single canonical type and remove duplicate creations: update
041_1.7.0.sql to use the same types as 038_1.6.1.sql (set client_number,
invoice_quote_number, invoice_work_order, invoice_agreement, quote_work_order,
quote_agreement to VARCHAR(100) instead of VARCHAR(255)/TEXT) or conversely
consolidate by removing the duplicate column ALTERs from 038_1.6.1.sql and
standardizing on VARCHAR(255)/TEXT across both files; pick one schema, update
the ALTER TABLE statements in ip_clients/ip_invoices/ip_quotes (referencing
client_number, invoice_quote_number, invoice_work_order, invoice_agreement,
quote_work_order, quote_agreement) to that type, and delete the
duplicate/contradictory ALTER in the other migration so only one migration adds
each column.
In `@assets/core/js/scripts.js`:
- Around line 76-114: The anchor-href regex in sanitizeHtml incorrectly uses a
character class for " which breaks matching; update the replacement that
handles <a> tags (the escaped variable replacement block) to use a non-greedy
capture like /<a\s+href="(.*?)">/gi (keeping the same href
validation logic and callback signature), so the entire href value between the
escaped "..." is captured reliably; ensure you update the
corresponding replace call on escaped (inside sanitizeHtml) to use that new
regex and preserve the existing URL validation checks.
In `@package.json`:
- Around line 38-40: package.json is pinning "sass": "^1.97" which introduces
breaking changes; before upgrading, search the codebase for deprecated/changed
Sass patterns and update them: locate any "@function type" definitions and
rename them, replace all "@import" usages with "@use" / "@forward" modules,
remove or replace "meta.feature-exists()" calls, audit any "@use"/"@forward ...
with" that depend on private variables and convert to public APIs, and scan for
nested rules/declarations whose emitted order may change and adjust
selectors/mixins accordingly; run the test build (and compare generated CSS)
after updating to validate outputs and adjust styles where order-dependent
behavior changed.
♻️ Duplicate comments (3)
assets/core/js/jquery-ui.js (1)
888-894: Security fix for XSS vulnerability correctly applied.This change properly mitigates CVE-2021-41184 by using
$(document).find(options.of)for string inputs, ensuring they're treated strictly as selectors rather than potentially interpreted as HTML. Theelsebranch correctly handles non-string types (DOM elements, jQuery objects, Events, window/document) which are safe to pass directly to$().As noted in previous reviews, consider upgrading to jQuery UI 1.13.0+ which includes this fix natively, reducing the maintenance burden of vendored patches.
application/views/quote_templates/public/InvoicePlane_Web.php (1)
114-123: Escape client VAT/tax IDs and attachment names to prevent stored XSS.These fields can contain user-supplied values and should be HTML-escaped before output.
🛠️ Suggested fix
- echo ': ' . $quote->client_vat_id . '<br>'; + echo ': ' . htmlsc($quote->client_vat_id) . '<br>'; ... - echo ': ' . $quote->client_tax_code . '<br>'; + echo ': ' . htmlsc($quote->client_tax_code) . '<br>'; ... - <td><?php echo $attachment['name']; ?></td> + <td><?php echo htmlsc($attachment['name']); ?></td>Also applies to: 315-317
.github/workflows/composer-update.yml (1)
86-92: Smoke-test step still calls Laravel Artisan.If this is CodeIgniter,
artisanwon’t exist and this step will fail or be skipped unintentionally. Please switch to the project’s actual test runner or guard the command.🛠️ Safer guard (replace with project-specific command as needed)
- php artisan key:generate --env=testing - php artisan test --configuration=phpunit.smoke.xml --env=testing + if [ -f artisan ]; then + php artisan key:generate --env=testing + php artisan test --configuration=phpunit.smoke.xml --env=testing + elif [ -x vendor/bin/phpunit ]; then + vendor/bin/phpunit --configuration=phpunit.smoke.xml + else + echo "::warning::No test runner found; skipping smoke tests." + fi#!/bin/bash # Verify whether Laravel's artisan or a phpunit runner exists ls -la artisan 2>/dev/null || true fd -a 'phpunit\.smoke\.xml|phpunit\.xml' . fd -a 'phpunit' vendor/bin 2>/dev/null || true
🧹 Nitpick comments (9)
application/modules/settings/models/Mdl_versions.php (1)
44-44: Prefer explicitreturn null;for clarity.Using
return;implicitly returnsnull, but an explicitreturn null;would be more consistent with the docblock (@return string|null) and makes the intent clearer for maintainability.✨ Suggested change
if ($result->query->num_rows() === 0) { - return; + return null; }.github/workflows/test-frontend.yml (1)
14-15: Node 14 and 16 are end-of-life."Production applications should only use Active LTS or Maintenance LTS releases." Node.js 18 maintenance LTS ends April 2025, Node.js 20 ends April 2026, and Node.js 22 ends April 2027. Node 14 and 16 reached EOL in April 2023 and September 2023 respectively.Testing against EOL versions provides limited value and may produce false positives on deprecated APIs.
♻️ Suggested update to supported Node.js versions
matrix: operating-system: [ubuntu-latest] - node: ['14', '16', '18'] + node: ['20', '22']application/helpers/client_helper.php (1)
49-56: Add null safety check for$clientparameter.Accessing
$client->client_numberwithout checking if$clientis null could cause an error when called with invalid data. Also, consider using strict comparison orempty()for consistency with the rest of the codebase.♻️ Suggested improvement
function format_client_number($client) { - if ($client->client_number != "") { + if (!empty($client) && !empty($client->client_number)) { return $client->client_number; } return "00-00000"; }.github/workflows/docker.yml (2)
19-32: Consider adding a cleanup step to ensure containers are stopped.The workflow builds and starts containers but doesn't clean them up. While GitHub-hosted runners are ephemeral, adding a cleanup step is a good practice for consistency and in case the workflow is ever run on self-hosted runners.
♻️ Suggested cleanup step
- name: Check running services run: docker-compose ps `#-` name: # Optional: Run health checks or tests # run: # echo "TODO: insert test commands after containers are up" + + - name: Cleanup + if: always() + run: docker-compose down --volumes --remove-orphans
23-24: Usedocker composeinstead of the deprecateddocker-composecommand.Docker Compose V2 is integrated into Docker CLI and the standalone
docker-composecommand is deprecated. Docker recommends usingdocker compose(with a space) for forward compatibility and to benefit from active maintenance. Thedocker-composecommand continues to work through aliasing but will receive only critical and security fixes.Consider updating line 24 from
docker-compose up -d --buildtodocker compose up -d --build..github/workflows/phpstan.yml (1)
25-30: Consider capturing PHPStan exit code separately.When PHPStan finds errors, it exits non-zero, but
continue-on-error: truemasks this. The subsequent "Check for errors" step (lines 50-54) correctly fails, but capturing the exit code explicitly would be cleaner and more debuggable.Suggested improvement
- name: Run PHPStan (JSON output) id: phpstan-analysis run: | - vendor/bin/phpstan analyse --memory-limit=1G --error-format=json > phpstan.json - cat phpstan.json + set +e + vendor/bin/phpstan analyse --memory-limit=1G --error-format=json > phpstan.json + echo "exit_code=$?" >> $GITHUB_OUTPUT + set -e + cat phpstan.json continue-on-error: trueapplication/modules/quotes/models/Mdl_quotes.php (1)
345-361: Use short array syntax for consistency.
The rest of the model already uses[].♻️ Suggested refactor
- $this->filter_where_in('quote_status_id', array(2, 3, 4)); + $this->filter_where_in('quote_status_id', [2, 3, 4]);application/modules/quotes/views/view.php (1)
347-359: XSS protection correctly applied.The
htmlsc()function properly escapes output values. The null coalescing (?? '') handles missing values gracefully.For improved accessibility, consider adding
forattributes to the labels to formally associate them with the inputs.♿ Accessibility improvement
<div class="col-xs-12 col-md-4"> <div class="quote-properties"> - <label><?php _trans('work_order'); ?></label> + <label for="quote_work_order"><?php _trans('work_order'); ?></label> <input type="text" id="quote_work_order" class="form-control input-sm" value="<?php echo htmlsc($quote->quote_work_order ?? ''); ?>"> </div> <div class="quote-properties"> - <label><?php _trans('agreement'); ?></label> + <label for="quote_agreement"><?php _trans('agreement'); ?></label> <input type="text" id="quote_agreement" class="form-control input-sm" value="<?php echo htmlsc($quote->quote_agreement ?? ''); ?>"> </div> </div>application/modules/quotes/controllers/Ajax.php (1)
21-47: Previous undefined$dataissue resolved.The
$dataarray is now properly initialized with default values (lines 27-32) before conditional population.However, line 37 calls
$this->mdl_quotes->statuses()without using the return value - this call is redundant sincestatuses()is called again on line 40.♻️ Remove redundant call
if (!empty($client_id)) { $this->mdl_quotes->by_client($client_id); $this->mdl_quotes->related_is_open(); - $this->mdl_quotes->statuses(); $quotes = $this->mdl_quotes->get()->result(); $quote_statuses = $this->mdl_quotes->statuses();
| // Look for package name lines (they typically start with package names after +/-) | ||
| if (line.match(/^[-+]"?[@a-zA-Z0-9_-]+/)) { | ||
| // Extract package name | ||
| const match = line.match(/^[-+]"?(@?[a-zA-Z0-9_-]+(?:\/[a-zA-Z0-9_-]+)?)/); | ||
| if (match && match[1]) { | ||
| updatedPackages.add(match[1]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Tighten yarn.lock entry parsing to avoid false positives (e.g., "__metadata").
The current regex can capture non-package keys in Yarn v2+/v3 lockfiles, which pollutes the report. Prefer matching only entry header lines that end with : and extracting the package name from the descriptor.
🔧 Proposed fix (more precise entry parsing)
- if (line.match(/^[-+]"?[`@a-zA-Z0-9_-`]+/)) {
- // Extract package name
- const match = line.match(/^[-+]"?(@?[a-zA-Z0-9_-]+(?:\/[a-zA-Z0-9_-]+)?)/);
- if (match && match[1]) {
- updatedPackages.add(match[1]);
- }
- }
+ const entryMatch = line.match(/^[-+]"?(.+?)":$/);
+ if (entryMatch) {
+ const descriptor = entryMatch[1];
+ const nameMatch = descriptor.match(/^(@?[^@]+(?:\/[^@]+)?)@/);
+ if (nameMatch && nameMatch[1]) {
+ updatedPackages.add(nameMatch[1]);
+ }
+ }🤖 Prompt for AI Agents
In @.github/scripts/generate-package-update-report.cjs around lines 26 - 33, The
current pattern in the package extraction block (where updatedPackages is
populated) is too loose and picks up non-package keys like "__metadata"; update
the parsing to only consider yarn.lock entry header lines that end with a colon
and then extract the package descriptor/name from that header (i.e., match lines
like 'package@version:'), rejecting keys without trailing ':'; modify the two
regex checks around the line.match(...) and the descriptor extraction (the
match(...) used to populate updatedPackages) to first assert the line ends with
':' and then capture the package identifier (including scoped names and
potential slash) from the descriptor before adding to updatedPackages.
| on: | ||
| push: | ||
| tags: [ 'v*.*.*' ] | ||
| workflow_dispatch: |
There was a problem hiding this comment.
Automatic Docker image builds removed for releases.
Removing the push.tags trigger means Docker images won't automatically build and publish when version tags (e.g., v1.7.0) are pushed. This requires manual workflow dispatch for every release.
If automatic release builds are desired, consider restoring the tag trigger:
🔧 Suggested fix to restore automatic release builds
on:
workflow_dispatch:
+ push:
+ tags:
+ - 'v*.*.*'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| on: | |
| push: | |
| tags: [ 'v*.*.*' ] | |
| workflow_dispatch: | |
| on: | |
| workflow_dispatch: | |
| push: | |
| tags: | |
| - 'v*.*.*' |
🤖 Prompt for AI Agents
In @.github/workflows/docker-publish.yml around lines 3 - 4, The workflow
currently only uses the workflow_dispatch trigger, so automatic Docker builds on
version tag pushes were removed; restore the push.tags trigger by updating the
workflow's triggers to include a push event filtering tags (e.g., add a push:
tags: ['v*'] entry alongside workflow_dispatch) so that when version tags like
"v1.7.0" are pushed the Docker publish job runs automatically; modify the "on:"
block in .github/workflows/docker-publish.yml (where workflow_dispatch is
declared) to include the push.tags pattern.
| - name: Comment on PR (if available) | ||
| if: github.event_name == 'pull_request' && always() | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const fs = require('fs'); | ||
| try { | ||
| if (!fs.existsSync('phpstan-report.md')) { | ||
| console.log('PHPStan report file "phpstan-report.md" not found. Skipping PR comment.'); | ||
| return; | ||
| } | ||
| const report = fs.readFileSync('phpstan-report.md', 'utf8'); | ||
| if (!report || report.trim().length === 0) { | ||
| console.log('PHPStan report is empty. Skipping PR comment.'); | ||
| return; | ||
| } | ||
| await github.rest.issues.createComment({ | ||
| issue_number: context.issue.number, | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| body: report | ||
| }); | ||
| } catch (error) { | ||
| console.log('Failed to create PHPStan PR comment:', error && error.message ? error.message : error); | ||
| } |
There was a problem hiding this comment.
PR comment step will never execute.
The workflow only triggers on workflow_dispatch (line 4), but this step's condition checks for github.event_name == 'pull_request' (line 66). This condition will always be false, making this step unreachable dead code.
Either:
- Remove this step if PR comments aren't needed, or
- Add
pull_requestto the workflow triggers if you want PR comments
Option: Add pull_request trigger
on:
workflow_dispatch: # Manual only — does not run automatically
+ pull_request:
+ paths:
+ - '**.php'🤖 Prompt for AI Agents
In @.github/workflows/phpstan.yml around lines 65 - 93, The "Comment on PR (if
available)" step is unreachable because the workflow only triggers on
workflow_dispatch while the step checks github.event_name == 'pull_request';
either remove this step entirely if PR comments aren't needed, or update the
workflow triggers to include pull_request so the condition can be true — adjust
the top-level "on:" entries to add pull_request (or change the step's if to
match an existing trigger) and keep the step's existing fs/readFileSync and
github.rest.issues.createComment logic intact.
| pull_request: | ||
| branches: | ||
| - master | ||
| - develop | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/pint.yml | head -25Repository: InvoicePlane/InvoicePlane
Length of output: 681
🏁 Script executed:
git branch -a 2>/dev/null || echo "No branches info available"
git for-each-ref --format='%(refname:short)' refs/heads/ 2>/dev/null || echo "Unable to list branches"Repository: InvoicePlane/InvoicePlane
Length of output: 233
Add development branch to PR trigger filters; workflow will not run on PRs targeting the default branch.
The repository's default branch is development (not develop), but the workflow only filters for master and develop. This means Pint checks will be skipped on PRs targeting the main development branch.
Suggested fix
pull_request:
branches:
- master
- develop
+ - development📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pull_request: | |
| branches: | |
| - master | |
| - develop | |
| pull_request: | |
| branches: | |
| - master | |
| - develop | |
| - development | |
🤖 Prompt for AI Agents
In @.github/workflows/pint.yml around lines 14 - 18, Update the pull_request
trigger filter under the workflow's pull_request block so it includes the
repository's actual default branch name; replace or add the correct branch
'development' alongside 'master' and any other branches listed (currently
'develop') so the Pint workflow runs for PRs targeting 'development' — modify
the branches array in the pull_request YAML block to include 'development' (and
remove 'develop' if that was a typo).
| - name: Commit and Push Changes | ||
| if: steps.verify_diff.outputs.has_changes == 'true' | ||
| run: | | ||
| git config --local user.email "github-actions[bot]@users.noreply.github.com" | ||
| git config --local user.name "github-actions[bot]" | ||
| git add -A | ||
| git commit -m "style: apply Laravel Pint fixes" --no-verify | ||
| git push | ||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/pint.ymlRepository: InvoicePlane/InvoicePlane
Length of output: 5216
Guard auto-push to prevent failures on forked PRs.
Forked pull requests have limited token permissions and cannot push to the base repository. Add a condition to skip the push step for forked PRs.
✅ Add fork check to push condition
- name: Commit and Push Changes
- if: steps.verify_diff.outputs.has_changes == 'true'
+ if: steps.verify_diff.outputs.has_changes == 'true' && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository)
run: |
git config --local user.email "github-actions[bot]@users.noreply.github.com"
git config --local user.name "github-actions[bot]"
git add -A
git commit -m "style: apply Laravel Pint fixes" --no-verify
git push📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Commit and Push Changes | |
| if: steps.verify_diff.outputs.has_changes == 'true' | |
| run: | | |
| git config --local user.email "github-actions[bot]@users.noreply.github.com" | |
| git config --local user.name "github-actions[bot]" | |
| git add -A | |
| git commit -m "style: apply Laravel Pint fixes" --no-verify | |
| git push | |
| - name: Commit and Push Changes | |
| if: steps.verify_diff.outputs.has_changes == 'true' && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) | |
| run: | | |
| git config --local user.email "github-actions[bot]@users.noreply.github.com" | |
| git config --local user.name "github-actions[bot]" | |
| git add -A | |
| git commit -m "style: apply Laravel Pint fixes" --no-verify | |
| git push | |
🤖 Prompt for AI Agents
In @.github/workflows/pint.yml around lines 93 - 101, The push step "Commit and
Push Changes" should skip when the PR comes from a fork; update its if condition
to only run when changes exist and the workflow is running in the same
repository (not a fork). Replace the current condition
(steps.verify_diff.outputs.has_changes == 'true') with a compound check such as:
steps.verify_diff.outputs.has_changes == 'true' && (github.event.pull_request ==
null || github.event.pull_request.head.repo.full_name == github.repository) so
the push runs for non-PR runs or PRs originating from the same repo but is
skipped for forked PRs.
| /** | ||
| * Check if IP address has exceeded rate limit for password resets using session storage | ||
| * Check if IP address has exceeded rate limit for password resets using session storage. | ||
| * | ||
| * @param int $max_attempts Maximum attempts allowed per hour | ||
| * @param int $max_attempts Maximum attempts allowed per hour | ||
| * @param int $window_minutes Time window in minutes | ||
| * | ||
| * @return bool True if rate limited, false otherwise | ||
| */ | ||
| private function _is_ip_rate_limited_password_reset() | ||
| { | ||
| $max_attempts = env('PASSWORD_RESET_IP_MAX_ATTEMPTS', 5); | ||
| $max_attempts = env('PASSWORD_RESET_IP_MAX_ATTEMPTS', 5); | ||
| $window_minutes = env('PASSWORD_RESET_IP_WINDOW_MINUTES', 60); | ||
| $ip_address = $this->input->ip_address(); | ||
|
|
||
| $ip_address = $this->input->ip_address(); | ||
| $session_key = 'password_reset_attempts_' . md5($ip_address); | ||
|
|
||
| // Get current attempts from session | ||
| $attempts = $this->session->userdata($session_key); | ||
| if (!$attempts) { | ||
|
|
||
| if ( ! $attempts) { | ||
| $attempts = []; | ||
| } | ||
|
|
||
| // Clean up old attempts outside the time window | ||
| $cutoff_time = time() - ($window_minutes * 60); | ||
| $attempts = array_filter($attempts, function($timestamp) use ($cutoff_time) { | ||
| $attempts = array_filter($attempts, function ($timestamp) use ($cutoff_time) { | ||
| return $timestamp > $cutoff_time; | ||
| }); | ||
|
|
||
| // Check if rate limited | ||
| if (count($attempts) >= $max_attempts) { | ||
| log_message('info', trans('log_ip_rate_limit_check') . ': ' . count($attempts) . ' attempts from IP: ' . $ip_address); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Record a password reset attempt for the current IP | ||
| * Record a password reset attempt for the current IP. | ||
| */ | ||
| private function _record_password_reset_attempt() | ||
| { | ||
| $ip_address = $this->input->ip_address(); | ||
| $ip_address = $this->input->ip_address(); | ||
| $session_key = 'password_reset_attempts_' . md5($ip_address); | ||
|
|
||
| // Get current attempts from session | ||
| $attempts = $this->session->userdata($session_key); | ||
| if (!$attempts) { | ||
|
|
||
| if ( ! $attempts) { | ||
| $attempts = []; | ||
| } | ||
|
|
||
| // Add current timestamp | ||
| $attempts[] = time(); | ||
|
|
||
| // Store back to session | ||
| $this->session->set_userdata($session_key, $attempts); | ||
| } | ||
|
|
||
| /** | ||
| * Check if email-based rate limit exceeded for password resets using session storage | ||
| * Check if email-based rate limit exceeded for password resets using session storage. | ||
| * | ||
| * @param string $email Email address to check | ||
| * @param int $max_attempts Maximum attempts allowed | ||
| * @param int $window_hours Time window in hours | ||
| * @param string $email Email address to check | ||
| * @param int $max_attempts Maximum attempts allowed | ||
| * @param int $window_hours Time window in hours | ||
| * | ||
| * @return bool True if rate limited, false otherwise | ||
| */ | ||
| private function _is_email_rate_limited_password_reset($email) | ||
| { | ||
| $max_attempts = env('PASSWORD_RESET_EMAIL_MAX_ATTEMPTS', 3); | ||
| $window_hours = env('PASSWORD_RESET_EMAIL_WINDOW_HOURS', 1); | ||
|
|
||
| $session_key = 'password_reset_email_' . md5($email); | ||
|
|
||
| // Get current attempts from session | ||
| $attempts = $this->session->userdata($session_key); | ||
| if (!$attempts) { | ||
|
|
||
| if ( ! $attempts) { | ||
| $attempts = []; | ||
| } | ||
|
|
||
| // Clean up old attempts outside the time window | ||
| $cutoff_time = time() - ($window_hours * 3600); | ||
| $attempts = array_filter($attempts, function($timestamp) use ($cutoff_time) { | ||
| $attempts = array_filter($attempts, function ($timestamp) use ($cutoff_time) { | ||
| return $timestamp > $cutoff_time; | ||
| }); | ||
|
|
||
| // Check if rate limited | ||
| if (count($attempts) >= $max_attempts) { | ||
| log_message('info', trans('log_email_rate_limit_check') . ': ' . count($attempts) . ' attempts for email: ' . $email); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Record a password reset attempt for a specific email | ||
| * Record a password reset attempt for a specific email. | ||
| * | ||
| * @param string $email Email address | ||
| */ | ||
| private function _record_email_password_reset_attempt($email) | ||
| { | ||
| $session_key = 'password_reset_email_' . md5($email); | ||
|
|
||
| // Get current attempts from session | ||
| $attempts = $this->session->userdata($session_key); | ||
| if (!$attempts) { | ||
|
|
||
| if ( ! $attempts) { | ||
| $attempts = []; | ||
| } | ||
|
|
||
| // Add current timestamp | ||
| $attempts[] = time(); | ||
|
|
||
| // Store back to session | ||
| $this->session->set_userdata($session_key, $attempts); | ||
| } |
There was a problem hiding this comment.
Session-scoped reset limits are bypassable.
Because attempts are stored in the session, a requester can clear cookies/start a new session and reset the counters. For password resets, this defeats rate limiting. Consider persisting attempts server-side (DB/cache) keyed by IP/email with TTL.
🤖 Prompt for AI Agents
In `@application/modules/sessions/controllers/Sessions.php` around lines 323 -
444, The current session-based rate limiting in
_is_ip_rate_limited_password_reset, _record_password_reset_attempt,
_is_email_rate_limited_password_reset and _record_email_password_reset_attempt
is bypassable by clearing cookies; replace session storage with a server-side
store (Redis or DB) keyed by IP/email with a TTL (e.g. keys like
password_reset:ip:{md5(ip)} and password_reset:email:{md5(email)}), implement
atomic increment or append-with-expiry and window-based trimming on read,
enforce counts against env-configured limits, persist attempts with expiry
instead of session->userdata/session->set_userdata, and update logging and tests
to reflect the new storage mechanism.
| ALTER TABLE `ip_clients` ADD client_number VARCHAR(100) NULL DEFAULT NULL AFTER `client_id`; | ||
| ALTER TABLE `ip_invoices` ADD invoice_quote_number VARCHAR(100) NULL DEFAULT NULL AFTER `invoice_id`; | ||
| ALTER TABLE `ip_invoices` ADD invoice_work_order VARCHAR(100) NULL DEFAULT NULL AFTER `invoice_quote_number`; | ||
| ALTER TABLE `ip_invoices` ADD invoice_agreement VARCHAR(100) NULL DEFAULT NULL AFTER `invoice_work_order`; | ||
| ALTER TABLE `ip_quotes` ADD quote_work_order VARCHAR(100) NULL DEFAULT NULL AFTER `quote_id`; | ||
| ALTER TABLE `ip_quotes` ADD quote_agreement VARCHAR(100) NULL DEFAULT NULL AFTER `quote_work_order`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Checking 038_1.6.1.sql ==="
if [ -f application/modules/setup/sql/038_1.6.1.sql ]; then
cat -n application/modules/setup/sql/038_1.6.1.sql
else
echo "File not found at application/modules/setup/sql/038_1.6.1.sql"
fi
echo -e "\n=== Checking 041_1.7.0.sql ==="
if [ -f application/modules/setup/sql/041_1.7.0.sql ]; then
cat -n application/modules/setup/sql/041_1.7.0.sql
else
echo "File not found at application/modules/setup/sql/041_1.7.0.sql"
fiRepository: InvoicePlane/InvoicePlane
Length of output: 1735
Critical: Duplicate column definitions will cause sequential upgrade failures.
This migration (038_1.6.1.sql) adds six columns that are also defined in 041_1.7.0.sql:
client_numberonip_clientsinvoice_quote_number,invoice_work_order,invoice_agreementonip_invoicesquote_work_order,quote_agreementonip_quotes
When users upgrade sequentially through both versions, 041_1.7.0.sql will fail with "Duplicate column name" errors. Additionally, the column definitions conflict between migrations:
- Data types differ: 038 uses VARCHAR(100), while 041 uses VARCHAR(255) or TEXT
- AFTER positions differ: e.g.,
invoice_quote_numberis positioned AFTERinvoice_idin 038 but AFTERinvoice_numberin 041
Remove the duplicate definitions from one migration file, or use conditional logic to skip columns already present.
🤖 Prompt for AI Agents
In `@application/modules/setup/sql/038_1.6.1.sql` around lines 1 - 6, The
migration duplicates columns already added in 041_1.7.0.sql causing "Duplicate
column name" errors; update 038_1.6.1.sql to avoid re-adding the same columns by
either removing the six ALTER TABLE statements for client_number (ip_clients),
invoice_quote_number, invoice_work_order, invoice_agreement (ip_invoices), and
quote_work_order, quote_agreement (ip_quotes), or replace each direct ALTER with
conditional logic that first checks for the column's existence (e.g., query
INFORMATION_SCHEMA/COLUMN_NAME or use CREATE/ALTER ... IF NOT EXISTS where
supported) and only runs the ALTER if the column is missing; ensure the chosen
fix preserves the intended data type/position consistency with 041_1.7.0.sql
(VARCHAR(255)/TEXT and AFTER positions) so sequential upgrades do not conflict.
| # Added for versioning and new fields for client_number, invoice fields, and quote fields | ||
|
|
||
| # Add client_number to ip_clients | ||
| ALTER TABLE `ip_clients` | ||
| ADD COLUMN client_number VARCHAR(255) DEFAULT NULL AFTER `client_id`; | ||
|
|
||
| # Add invoice_quote_number, invoice_work_order, and invoice_agreement to ip_invoices | ||
| ALTER TABLE `ip_invoices` | ||
| ADD COLUMN invoice_quote_number TEXT DEFAULT NULL AFTER `invoice_number`, | ||
| ADD COLUMN invoice_work_order VARCHAR(255) DEFAULT NULL AFTER `invoice_quote_number`, | ||
| ADD COLUMN invoice_agreement VARCHAR(255) DEFAULT NULL AFTER `invoice_work_order`; | ||
|
|
||
| # Add quote_work_order and quote_agreement to ip_quotes | ||
| ALTER TABLE `ip_quotes` | ||
| ADD COLUMN quote_work_order VARCHAR(255) DEFAULT NULL AFTER `quote_number`, | ||
| ADD COLUMN quote_agreement VARCHAR(255) DEFAULT NULL AFTER `quote_work_order`; | ||
|
|
There was a problem hiding this comment.
Schema changes added, but data type inconsistencies with 038_1.6.1.sql.
The migration now includes the required ALTER TABLE statements. However, there are data type mismatches with 038_1.6.1.sql:
| Column | 038_1.6.1.sql | 041_1.7.0.sql |
|---|---|---|
client_number |
VARCHAR(100) | VARCHAR(255) |
invoice_quote_number |
VARCHAR(100) | TEXT |
invoice_work_order |
VARCHAR(100) | VARCHAR(255) |
invoice_agreement |
VARCHAR(100) | VARCHAR(255) |
quote_work_order |
VARCHAR(100) | VARCHAR(255) |
quote_agreement |
VARCHAR(100) | VARCHAR(255) |
These inconsistencies, combined with the duplicate column issue flagged in 038_1.6.1.sql, indicate these migrations need consolidation. Align on a single data type per column and remove duplicates.
🤖 Prompt for AI Agents
In `@application/modules/setup/sql/041_1.7.0.sql` around lines 1 - 17, The
migrations create the same columns with conflicting types—ensure a single
canonical type and remove duplicate creations: update 041_1.7.0.sql to use the
same types as 038_1.6.1.sql (set client_number, invoice_quote_number,
invoice_work_order, invoice_agreement, quote_work_order, quote_agreement to
VARCHAR(100) instead of VARCHAR(255)/TEXT) or conversely consolidate by removing
the duplicate column ALTERs from 038_1.6.1.sql and standardizing on
VARCHAR(255)/TEXT across both files; pick one schema, update the ALTER TABLE
statements in ip_clients/ip_invoices/ip_quotes (referencing client_number,
invoice_quote_number, invoice_work_order, invoice_agreement, quote_work_order,
quote_agreement) to that type, and delete the duplicate/contradictory ALTER in
the other migration so only one migration adds each column.
| // Sanitize HTML content for safe rendering in email template preview | ||
| function sanitizeHtml(html) { | ||
| // Create a temporary div to parse HTML | ||
| var temp = document.createElement('div'); | ||
| temp.textContent = html; // This escapes all HTML | ||
| var escaped = temp.innerHTML; | ||
|
|
||
| // Allowed safe tags that should render as HTML | ||
| var allowedTags = { | ||
| 'b': true, 'strong': true, 'i': true, 'em': true, 'u': true, | ||
| 'p': true, 'br': true, 'h1': true, 'h2': true, 'h3': true, 'h4': true, | ||
| 'ul': true, 'ol': true, 'li': true, 'a': true, 'span': true, 'div': true, | ||
| 'table': true, 'tr': true, 'td': true, 'th': true, 'thead': true, 'tbody': true | ||
| }; | ||
|
|
||
| // Replace escaped versions of allowed tags back to HTML | ||
| // But only for closing tags and simple opening tags without attributes | ||
| escaped = escaped.replace(/<(\/?)(b|strong|i|em|u|p|br|h1|h2|h3|h4|ul|ol|li|span|div|table|tr|td|th|thead|tbody)>/gi, | ||
| function(match, slash, tag) { | ||
| if (allowedTags[tag.toLowerCase()]) { | ||
| return '<' + slash + tag + '>'; | ||
| } | ||
| return match; | ||
| }); | ||
|
|
||
| // Handle <a> tags specially to allow href but sanitize it | ||
| escaped = escaped.replace(/<a\s+href="([^"]+)">/gi, function(match, href) { | ||
| // Only allow http, https, and mailto URLs with proper validation | ||
| // Ensure javascript: or data: URIs are not allowed | ||
| if (href.match(/^https?:\/\/[a-zA-Z0-9\-._~:/?#\[\]@!$&'()*+,;=%]+$/i) || | ||
| href.match(/^mailto:[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/i)) { | ||
| return '<a href="' + href + '">'; | ||
| } | ||
| return '<a href="' + href + '">'; | ||
| }); | ||
| escaped = escaped.replace(/<\/a>/gi, '</a>'); | ||
|
|
||
| return escaped; | ||
| } |
There was a problem hiding this comment.
Good sanitization implementation, but verify the escaped quote matching in the <a> tag regex.
The sanitization approach is sound—escaping everything first, then selectively re-enabling safe tags. However, the regex on line 102 may have issues matching escaped quotes:
When textContent escapes HTML, quotes become ". The regex pattern "([^"]+)" is interpreted as "([^&]+)quot; because [^"] is a character class excluding &, q, u, o, t, ; individually, not the literal string.
🔧 Suggested fix for escaped quote matching
- escaped = escaped.replace(/<a\s+href="([^"]+)">/gi, function(match, href) {
+ escaped = escaped.replace(/<a\s+href="([^&]+?)">/gi, function(match, href) {Or use a more explicit non-greedy pattern:
- escaped = escaped.replace(/<a\s+href="([^"]+)">/gi, function(match, href) {
+ escaped = escaped.replace(/<a\s+href="(.+?)">/gi, function(match, href) {🤖 Prompt for AI Agents
In `@assets/core/js/scripts.js` around lines 76 - 114, The anchor-href regex in
sanitizeHtml incorrectly uses a character class for " which breaks
matching; update the replacement that handles <a> tags (the escaped variable
replacement block) to use a non-greedy capture like
/<a\s+href="(.*?)">/gi (keeping the same href validation logic
and callback signature), so the entire href value between the escaped
"..." is captured reliably; ensure you update the corresponding
replace call on escaped (inside sanitizeHtml) to use that new regex and preserve
the existing URL validation checks.
| "postcss": "8.4", | ||
| "sass": "^1.89.2", | ||
| "sass": "^1.97", | ||
| "select2": "4.1.0-rc.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and inspect package.json to see the complete dependencies
git ls-files | grep -E '(package\.json|Gruntfile|grunt)' | head -20Repository: InvoicePlane/InvoicePlane
Length of output: 94
🏁 Script executed:
# Read package.json to understand the full dependency set
cat -n package.json | grep -A 30 -B 5 "sass"Repository: InvoicePlane/InvoicePlane
Length of output: 1054
🌐 Web query:
grunt-sass 3.1 Dart Sass 1.97 compatibility breaking changes
💡 Result:
Short answer: grunt-sass itself is just a Grunt wrapper that calls whatever Sass implementation you pass in, so “breaking” behavior with Dart Sass 1.97 comes from Dart Sass 1.97’s language/CLI changes — not from grunt-sass. See the grunt-sass README that you must pass the Sass implementation to options. [1]
Relevant Dart Sass 1.97+ changes to watch for (from the Dart Sass changelog and releases):
- Declarations, childless at‑rules, and comments are now emitted in source order even when interleaved with nested rules (breaking change / obsoletes mixed-decls). [2]
- The function name type() is reserved for the plain CSS function;
@functionnamed type now errors and calls parse as special function strings. [2] - Configuring private variables via
@use... with,@forward... with, and meta.load-css(..., $with: ...) is deprecated (encourages module encapsulation). [2] - Deprecation/removal of
@import(migration to@use/@forward was announced and@importwill be removed in Dart Sass 3.0). This may surface as deprecation warnings now and break when you move to later Dart Sass majors. [3] - New color-space support (display-p3-linear) and related parsing/serialization changes (feature additions that can affect output). [4]
If your project uses grunt-sass + Dart Sass 1.97:
- Expect any of the above Sass-language/serialization changes to affect compilation output or cause errors in previously-valid
@function/@use/@import patterns. These are Dart Sass changes — update your Sass code (replace@importwith@use/@forward, avoid@functiontype, adjust private-variable expectations, and check nested-rule ordering) to remain compatible. [2][3][4]
Sources:
[1] grunt-sass README/npm (usage and implementation option).
[2] Dart Sass changelog / releases (breaking changes: mixed-decls ordering; reserved type() name; private variable deprecation).
[3] Dart Sass deprecation/@import removal discussion.
[4] Dart Sass 1.97 release notes (display-p3-linear support).
🌐 Web query:
Sass 1.97 release notes breaking changes deprecations
💡 Result:
Here are the breaking changes and deprecations called out in the Dart Sass 1.97 release notes:
- Declarations, childless at-rules, and comments are now emitted in the order they appear in source even when interleaved with nested rules — this is a breaking change and it obsoletes the previous mixed-decls deprecation. [1]
- The function name type() is now reserved for the plain CSS function:
@functiondefinitions named type will error, and calls parse as special function strings. [1][2] - Configuring private variables via
@use... with,@forward... with, and meta.load-css(..., $with: ...) is deprecated (private variables are now treated as fully encapsulated). [1] - meta.feature-exists() has been deprecated (to be removed in Dart Sass 2.0.0); use other detection functions or remove usages. [3]
- Miscellaneous bugfix:
@extendrules loaded through mixed@import/@use could fail to apply — fixed. [1] - CLI: in --watch mode, the source map is deleted when its source file is deleted. [1]
Sources: Dart Sass release notes and breaking-change docs.
[1] Dart Sass release (1.97) changelog. [2] Breaking change: type() function. [3] Breaking change: meta.feature-exists().
Audit Sass code for breaking changes in 1.97 before upgrading.
grunt-sass 3.1 will work with Sass 1.97, but Dart Sass 1.97 introduces breaking changes that require code updates:
- Nested rules/declarations now emit in source order (may change CSS output)
@function typeis reserved; rename any existing@function typedefinitions@use/@forward ... withfor private variables is deprecated@importis deprecated (removes in Sass 3.0); migrate to@use/@forwardmeta.feature-exists()is deprecated
Review your Sass codebase for these patterns and update before merging.
🤖 Prompt for AI Agents
In `@package.json` around lines 38 - 40, package.json is pinning "sass": "^1.97"
which introduces breaking changes; before upgrading, search the codebase for
deprecated/changed Sass patterns and update them: locate any "@function type"
definitions and rename them, replace all "@import" usages with "@use" /
"@forward" modules, remove or replace "meta.feature-exists()" calls, audit any
"@use"/"@forward ... with" that depend on private variables and convert to
public APIs, and scan for nested rules/declarations whose emitted order may
change and adjust selectors/mixins accordingly; run the test build (and compare
generated CSS) after updating to validate outputs and adjust styles where
order-dependent behavior changed.
Description
Added a few new fields and additional functions to clients, invoices and quotes modules
List of all new fields:
client_number
invoice_quote_number
invoice_work_order
invoice_agreement
quote_work_order
quote_agreement
Related Issue
https://community.invoiceplane.com/t/topic/10662
Motivation and Context
This is an improvement to already great application. Having ability to have client_number for each client is sometimes mandatory for a larger size company or when billing requires that number.
So in Clients module, added one additional field:
client_number
For Invoices three additional fields and a new function:
invoice_quote_number
invoice_work_order
invoice_agreement
invoice_quote_number -> This is a text field, you can type in a related quote number or using a new function you can select related quote from a list by clicking on Change Related Quote. This will associate the invoice with specific quote and import quote_work_order and quote_agreement values (if any) from the quotes and pass it to invoice_work_order and invoice_agreement.
For Quotes two additional fields:
quote_work_order
quote_agreement
Welcome to any suggestions!
Screenshots (if appropriate):
Pull Request Checklist
Issue Type (Please check one or more)
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.