Skip to content

feat: validate ExApp routes at registration and reject malformed data#885

Open
oleksandr-nc wants to merge 1 commit into
mainfrom
feat/validate-exapp-routes-on-register
Open

feat: validate ExApp routes at registration and reject malformed data#885
oleksandr-nc wants to merge 1 commit into
mainfrom
feat/validate-exapp-routes-on-register

Conversation

@oleksandr-nc
Copy link
Copy Markdown
Contributor

Follow-up to #882, which added defensive read-side parsing for nullable / malformed bruteforce_protection and headers_to_exclude columns.
That PR which added defensive read-side parsing for nullable / malformed bruteforce_protection and headers_to_exclude columns. That PR kept legacy rows working but left the write side silently coercing any unrecognised value to '[]' - so a developer with a typo in their info.xml got an apparently-installed ExApp whose routes didn't behave as written, with no error to point them at the mistake.

This PR moves to fail-fast: every route definition from --info-xml, --json-info runs through a new ExAppRouteHelper::normalizeAndValidate() step inside ExAppService::getAppInfo().

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 16, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2097f52e-654e-4321-89a8-d78ece4ca61e

📥 Commits

Reviewing files that changed from the base of the PR and between 1b83547 and 8d57f2e.

📒 Files selected for processing (4)
  • lib/Db/ExAppMapper.php
  • lib/Service/ExAppRouteHelper.php
  • lib/Service/ExAppService.php
  • tests/php/Service/ExAppRouteHelperTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
  • lib/Db/ExAppMapper.php
  • lib/Service/ExAppRouteHelper.php
  • lib/Service/ExAppService.php

📝 Walkthrough

Walkthrough

This PR refactors ExApp route validation by extracting normalization logic into a dedicated helper class and integrating it into the service layer. The new ExAppRouteHelper canonicalizes heterogeneous route input (typed JSON fields and XML-encoded strings) into a consistent structure, mapping access_level strings to integers and normalizing list fields to strict typed arrays. ExAppService::getAppInfo() invokes this helper and returns errors for malformed routes. ExAppMapper::registerExAppRoutes() is simplified to assume pre-normalized input, removing legacy tolerance logic. Comprehensive tests verify valid routes are normalized correctly and invalid inputs produce specific error messages.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 53.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main purpose of the PR—adding validation for ExApp routes and rejecting malformed data. It directly corresponds to the primary objective of implementing fail-fast validation.
Description check ✅ Passed The description provides relevant context by referencing the prior PR #882 and explaining why this change is necessary—moving from silent coercion to fail-fast validation to catch developer mistakes like typos in info.xml.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

….xml/--json-info

Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
@oleksandr-nc oleksandr-nc force-pushed the feat/validate-exapp-routes-on-register branch from 1b83547 to 8d57f2e Compare May 16, 2026 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant