Skip to content

feat(gis-integration): implement spec (#462)#483

Closed
rubenvdlinde wants to merge 9 commits into
developmentfrom
feature/462/gis-integration
Closed

feat(gis-integration): implement spec (#462)#483
rubenvdlinde wants to merge 9 commits into
developmentfrom
feature/462/gis-integration

Conversation

@rubenvdlinde
Copy link
Copy Markdown
Contributor

Closes #462

Summary

Auto-generated draft PR for OpenSpec change gis-integration.
The Hydra builder ran the spec but could not run gh pr create itself
(Phase D+E credential strip — Claude has no GH_TOKEN by design).
The entrypoint detected commits on the feature branch with no PR and
created this draft so the reviewer + security + applier can proceed.

Spec Reference

Commits on this branch

Files changed

  • appinfo/routes.php
  • lib/Controller/WfsExportController.php
  • lib/Service/WfsExportService.php
  • openspec/changes/gis-integration/design.md
  • openspec/changes/gis-integration/tasks.md
  • task-audit.json
  • tests/Unit/Controller/WfsExportControllerTest.php
  • tests/Unit/Controller/WmsWfsControllerTest.php
  • tests/Unit/Service/LocationServiceTest.php
  • tests/Unit/Service/WfsExportServiceTest.php

PR auto-created by Hydra builder entrypoint (hydra_ensure_pr_exists)
because Claude's session closed without running gh pr create.
Reviewer + applier follow as normal.

@github-actions
Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/procest @ 278453e

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 100/100
npm ✅ 420/420
PHPUnit
Newman ⏭️
Playwright

Coverage: 0% (0/81 statements)

Spec coverage: 6% (30 tests / 512 specs)


Quality workflow — 2026-05-19 02:55 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
Copy link
Copy Markdown
Contributor Author

Security Review — Clyde Barcode

Result: PASS (0 fixed, 0 unfixed, 0 blocking)

  • Total findings in PR diff: 0
  • Fixed: 0
  • Unfixed: 0
  • Verdict: pass

All 14 hydra gates green. Semgrep (p/security-audit, p/owasp-top-ten, p/secrets) — 0 findings. composer audit — no PHP CVEs. gitleaks — 303 total findings, 0 in PR diff files (inherited debt). npm audit — 16 CVEs all pre-existing in Vue 2 / @nextcloud/* ecosystem (not introduced by this PR). PHPUnit 13/13 pass.

Manual OWASP review: auth annotations correct and consistent with sibling GisProxyController; input validation thorough (allowlisting, hard caps, floatval+bounds for bbox); no raw SQL; JSON-only responses (no XSS surface); GET endpoints (no CSRF concern); Host header validated by Nextcloud TrustedDomainMiddleware before controller runs.

Out-of-scope inherited debt (informational, non-blocking)

  • npm audit: 16 low/moderate/high CVEs in Vue 2 ecosystem (vue ReDoS, vue-template-compiler XSS, vue-routertinyglobbypicomatch). Not introduced by this PR — pre-existed on development. Recommend a dedicated dep-bump PR targeting @nextcloud/vue major version upgrade.
  • gitleaks: 303 findings in unchanged files; 0 in PR diff.

🤖 Changes Clyde Barcode applied

  • 93fd1db — fix(security-review bounded): Clyde post-run mechanical commit

View full diff · 1 file changed, 1 insertion(+), 1 deletion(-)

@github-actions
Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/procest @ b988db7

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 100/100
npm
PHPUnit
Newman
Playwright

Quality workflow — 2026-05-19 04:08 UTC

Download the full PDF report from the workflow artifacts.

@github-actions
Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/procest @ 2817617

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 100/100
npm ✅ 420/420
PHPUnit
Newman ⏭️
Playwright ⏭️

Coverage: 0% (0/81 statements)


Quality workflow — 2026-05-19 04:13 UTC

Download the full PDF report from the workflow artifacts.

* - status: Filter by case status (optional)
* - caseType: Filter by case type name (optional)
*
* @NoAdminRequired
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The WfsExport endpoint is explicitly designed for external GIS applications (QGIS, ArcGIS, MapInfo) that authenticate via HTTP Basic Auth or OIDC — as stated in both the controller doc-block and design.md. However, getFeatures() is missing @NoCSRFRequired.

Nextcloud's SecurityMiddleware enforces CSRF for every request not annotated with @NoCSRFRequired, regardless of HTTP verb. passesCSRFCheck() requires either an OCS-APIRequest header or a valid requesttoken parameter — neither of which an external GIS client sends. Since WfsExportController extends Controller (not OCSController), the middleware throws CrossSiteRequestForgeryException → 403, and the WFS layer is completely inaccessible to external tools.

For comparison, MetricsController (Prometheus scraper), HealthController (container health checks), and all DrcController ZGW-API endpoints all carry @NoCSRFRequired for exactly this reason.

Fix: add * @NoCSRFRequired directly below * @NoAdminRequired here.

/**
* Return WFS GetCapabilities descriptor for this endpoint.
*
* @NoAdminRequired
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same root cause as getFeatures(): getCapabilities() is also missing @NoCSRFRequired.

The standard WFS client discovery flow calls /capabilities first. If this returns a CSRF 403, external GIS tools (QGIS, ArcGIS, etc.) cannot even discover the feature types, let alone add the layer — the entire integration is dead on arrival.

Fix: add * @NoCSRFRequired directly below * @NoAdminRequired here.

return [];
}

$params = ['_limit' => $limit];
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

fetchLocations() always fetches up to $limit (max 2000) location records with only ['_limit' => $limit], then applies status and caseType filters in PHP memory via applyFilters().

This means a request for status=closed&maxFeatures=10 still loads 2000 rows from the database before discarding most of them. Not a blocker given the 2000 hard cap, but as case location datasets grow for active municipalities this will become noticeably expensive.

Suggested follow-up: pass caseStatus and caseType directly into $params so OpenRegister can filter at query time:

if ($status !== null) {
    $params['caseStatus'] = $status;
}
if ($caseType !== null) {
    $params['caseType'] = $caseType;
}

Copy link
Copy Markdown

@WilcoLouwerse WilcoLouwerse left a comment

Choose a reason for hiding this comment

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

REQUEST_CHANGES — 2 blocking issues, 1 informational concern.

Blockers

Both WFS endpoints (getFeatures and getCapabilities) are missing @NoCSRFRequired. Nextcloud's SecurityMiddleware enforces CSRF on every route not annotated with that tag, regardless of HTTP verb. External GIS applications (QGIS, ArcGIS, MapInfo) connecting via HTTP Basic Auth or OIDC — the explicitly stated use case in the doc-block and design.md — will receive a 403 CrossSiteRequestForgeryException on every request, making the WFS layer completely inaccessible.

Fix: add * @NoCSRFRequired below * @NoAdminRequired on both methods. Compare MetricsController, HealthController, and DrcController which all carry this annotation for the same reason (external consumers without Nextcloud session cookies).

Informational

Positives

  • Architecture is correct: WfsExportService reads exclusively through OpenRegister via SettingsService.getObjectService() — no direct outbound HTTP calls, no PDOK bypass.
  • All new PHP files carry correct SPDX headers, EUPL-1.2 license, and copyright blocks.
  • Tasks 18–24 are all implemented and verified; AC 6 (WFS GeoJSON export) is complete.
  • All CI checks pass (PHPUnit PHP 8.3/8.4 × NC stable31/32, CodeQL, ESLint, Psalm, PHPStan).

rubenvdlinde added a commit that referenced this pull request May 25, 2026
Ports the unique outbound WFS-export capability from PR #483
(feature/462/gis-integration) onto the current development baseline.
That branch had diverged from an unrelated history root and could not
be merged directly without reverting large amounts of merged dev work,
so only the genuinely-unique, self-contained additions are carried over:

- lib/Service/WfsExportService.php — builds a GeoJSON FeatureCollection
  of case locations from OpenRegister (uses SettingsService.getObjectService
  + findObjects, already on dev) plus a WFS GetCapabilities descriptor.
- lib/Controller/WfsExportController.php — GET /api/gis/wfs (GetFeature)
  and /api/gis/wfs/capabilities, NoAdminRequired with an authenticated-
  session guard, typeName/outputFormat/maxFeatures/bbox/status/caseType params.
- appinfo/routes.php — two additive wfsExport routes alongside the existing
  gis_proxy / wms_wfs proxy routes.
- Unit tests for both classes.

This complements dev's existing proxy-based GIS work (GisProxy/WmsWfs);
the WFS export (procest cases AS a layer) was absent on dev.
@rubenvdlinde
Copy link
Copy Markdown
Contributor Author

Superseded by #604 (merged), which ports the unique work from this PR onto the current development baseline.

This branch had diverged from an unrelated history root (~117 commits behind dev) and could not be merged directly — its shared-file changes (routes.php, openspec change dirs) would have reverted merged dev work, and dev had independently grown a proxy-based GIS implementation (GisProxy/WmsWfs + LocationService + the gis-integration change dir with builds).

The genuinely-unique, self-contained additions — WfsExportService, WfsExportController, the two /api/gis/wfs routes, and both unit tests — were carried over verbatim in #604 (API-compatible with dev's SettingsService/ObjectService, @SPEC retargeted to dev's TASK-GIS-04/06, php -l clean).

No work lost. Closing in favour of #604.

@rubenvdlinde rubenvdlinde deleted the feature/462/gis-integration branch May 25, 2026 15:38
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.

3 participants