Skip to content

feat(anonymisation-append-basis-summary-flag): implement spec (#154)#187

Draft
rubenvdlinde wants to merge 9 commits into
developmentfrom
feature/154/anonymisation-append-basis-summary-flag
Draft

feat(anonymisation-append-basis-summary-flag): implement spec (#154)#187
rubenvdlinde wants to merge 9 commits into
developmentfrom
feature/154/anonymisation-append-basis-summary-flag

Conversation

@rubenvdlinde
Copy link
Copy Markdown
Contributor

@rubenvdlinde rubenvdlinde commented May 18, 2026

Closes #154

Summary

Auto-generated draft PR for OpenSpec change anonymisation-append-basis-summary-flag.
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

  • CHANGELOG.md
  • docs/features/anonymization.md
  • lib/Controller/AnonymizationController.php
  • lib/Controller/BatchAnonymizationController.php
  • lib/Service/AnonymizationService.php
  • lib/Service/BatchAnonymizeService.php
  • openspec/changes/anonymisation-append-basis-summary-flag/design.md
  • openspec/changes/anonymisation-append-basis-summary-flag/tasks.md
  • task-audit.json
  • tests/bootstrap-unit.php
  • tests/stubs/NextcloudStubs.php
  • tests/unit/Controller/AnonymizationControllerTest.php
  • tests/unit/Service/AnonymizationServiceTest.php
  • tests/unit/Service/BatchAnonymizeServiceTest.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.

rubenvdlinde and others added 5 commits March 5, 2026 11:23
@rubenvdlinde
Merge pull request #38 from ConductionNL/development
a48088a 
chore: Merge development into main
@actions-user
Bump version to 0.0.33 [skip ci]
c293e40
@claude
feat: add appendBasisSummary flag to anonymise endpoints (#154)
34f4787 
Add optional appendBasisSummary boolean (default false) to per-document
and batch anonymise endpoints. When true, calls GrondslagenSummaryService
after the anonymised file is written. PDF mode appends a summary page;
preserve mode writes a separate _grondslagen.pdf alongside and returns
summaryFileId/summaryFilePath in the response. Summary failures surface
as a structured warning field (HTTP 200) — the anonymised file is always
preserved. Pre-change callers see no behaviour change.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
chore: set design.md status to pr-created (#154)
25b036e 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
chore: add task-audit.json per Rule 2a (#154)
48af505 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 18, 2026

Quality Report — ConductionNL/docudesk @ c44d894

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

Quality workflow — 2026-05-18 19:20 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
Copy link
Copy Markdown
Contributor Author

rubenvdlinde commented May 19, 2026

Code Review — Juan Claude van Damme

Result: FAIL (1 fix applied, 3 unfixed findings, 2 checks skipped)

Fix Applied

  • appinfo/info.xml: Resolved merge conflict — kept PR branch version 0.0.34-unstable.8 over development's 0.0.33.

Gate Results

Gate Result Notes
gate-1 spdx-headers PASS
gate-2 forbidden-patterns PASS
gate-3 stub-scan PASS
gate-5 route-auth PASS
gate-6 orphan-auth PASS
gate-7 no-admin-idor FAIL 13 methods — ALL pre-existing on origin/development
gate-8 unsafe-auth-resolver PASS
gate-9 semantic-auth PASS
gate-14 route-reachability FAIL Gate-script false positive — snake_case vs camelCase routing

Unfixed Findings

[CRITICAL] Gate-7 — No-admin-IDOR: AnonymizationController (4 methods)
File: lib/Controller/AnonymizationController.php:80,116,179,215
Methods files, upload, extract, anonymize all carry @NoAdminRequired but have no per-object ownership guard. Pre-existing on origin/development — not introduced by this PR. Security finding → escalated to Clyde.

[CRITICAL] Gate-7 — No-admin-IDOR: BatchAnonymizationController (9 methods)
File: lib/Controller/BatchAnonymizationController.php:101,136,178,199,250,303,348,368,382
Methods batchUpload, folderBatch, batchExtract, batchStatus, batchEntities, batchAnonymize, batchReport, getProfiles, updateProfiles all carry @NoAdminRequired but have no per-object ownership guard. Pre-existing on origin/development — not introduced by this PR. Security finding → escalated to Clyde.

[WARNING] Gate-14 — Route-reachability: gate-script false positive (9 methods)
File: lib/Controller/BatchAnonymizationController.php
The gate derives expected slug batchAnonymization#... (lowercase-first-char of BatchAnonymization) but Nextcloud uses snake_case batch_anonymization for multi-word controller names. All 9 routes are correctly defined in appinfo/routes.php and are functional. This is a gate-script limitation, not a broken route. Pre-existing on origin/development — not introduced by this PR.

Checks Skipped

  • composer check:strict (phpcs/psalm/phpstan/phpmd): not installed in reviewer container; builder's Rule 0b ran these in the authoritative environment per CLAUDE.md.
  • composer test:unit (phpunit): same reason. Builder passed unit tests before PR creation.

npm

  • npm run lint: PASS (no eslint errors)
  • npm test: unit suite PASS (src/store/modules/navigation.spec.ts — 3 tests); e2e failure on tests/e2e/docs-screenshots.spec.ts is pre-existing, not in PR diff.

Code Quality Assessment

The appendBasisSummary feature implementation is clean and correct:

  • AnonymizationController.anonymize() extracts and validates the flag via the new extractAppendBasisSummary() private helper (clean extraction pattern).
  • BatchAnonymizationController.batchAnonymize() validates the flag inline (minor duplication vs. the single-file controller, but acceptable given they are separate classes).
  • AnonymizationService.tryAppendBasisSummary() correctly soft-fails: catches \Throwable, logs a warning, adds a structured warning field to the result — never silently drops the anonymized file.
  • BatchAnonymizeService.anonymizeBatch() correctly forwards appendBasisSummary to each per-file call and surfaces per-file warning/summaryFileId fields.
  • Tests cover the new params, flag forwarding, warning field shape, and preserve-mode summary fields.
  • The NextcloudStubs.php correctly stubs OCP interfaces needed for controller unit tests.

Advisories

MCP coverage (advisory) — This app does not yet publish an IMcpToolProvider and has no opt-out recorded in openspec/project.md. Per ADR-035, consider whether this PR's surface (anonymization endpoints with @NoAdminRequired) should be MCP-callable. If yes, file a follow-up issue. If the app should opt out, add an ## MCP coverage block to project.md in a follow-up PR.

Out-of-scope inherited debt (informational)

  • Gate-7 violations affect ALL @NoAdminRequired methods across both anonymization controllers — these pre-date this PR and should be addressed in a dedicated security hardening PR reviewed by Clyde.
  • Gate-14 false positive for BatchAnonymizationController reflects a gate-script limitation: it doesn't apply the CamelCase→snake_case conversion that Nextcloud uses for multi-word controller names. The script should be updated to treat BatchAnonymization → batch_anonymization as the expected route prefix.

🤖 Changes Juan Claude van Damme applied

View full diff · 10 files changed, 125 insertions(+)

@rubenvdlinde
Copy link
Copy Markdown
Contributor Author

rubenvdlinde commented May 19, 2026

📍 appinfo/info.xml:32 (outside PR diff — line-comment API rejected, posted as issue comment)

[fixed: merge conflict resolved] Kept PR branch version 0.0.34-unstable.8 over development's 0.0.33 — feature branch version wins.

@rubenvdlinde
Copy link
Copy Markdown
Contributor Author

rubenvdlinde commented May 19, 2026

📍 lib/Controller/AnonymizationController.php:80 (outside PR diff — line-comment API rejected, posted as issue comment)

[unfixed: CRITICAL, security escalated to Clyde] gate-7-no-admin-idor: methods files(80), upload(116), extract(179), anonymize(215) carry @NoAdminRequired with no per-object ownership guard. Pre-existing on origin/development — not introduced by this PR.

@rubenvdlinde
Copy link
Copy Markdown
Contributor Author

rubenvdlinde commented May 19, 2026

📍 lib/Controller/BatchAnonymizationController.php:101 (outside PR diff — line-comment API rejected, posted as issue comment)

[unfixed: CRITICAL, security escalated to Clyde] gate-7-no-admin-idor: methods batchUpload(101), folderBatch(136), batchExtract(178), batchStatus(199), batchEntities(250), batchAnonymize(303), batchReport(348), getProfiles(368), updateProfiles(382) carry @NoAdminRequired with no per-object ownership guard. Pre-existing on origin/development — not introduced by this PR.

rubenvdlinde
rubenvdlinde commented May 19, 2026
View reviewed changes
Comment thread lib/Controller/BatchAnonymizationController.php
*
* @NoAdminRequired
* @NoCSRFRequired
*
Copy link
Copy Markdown
Contributor Author

@rubenvdlinde rubenvdlinde May 19, 2026

Choose a reason for hiding this comment

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

[unfixed: WARNING, gate false positive] gate-14-route-reachability: gate expects slug 'batchAnonymization#...' but Nextcloud uses snake_case 'batch_anonymization' for multi-word controller names. Routes ARE correctly defined in appinfo/routes.php. Gate-script limitation — not a real routing bug. Pre-existing on origin/development.

@rubenvdlinde
Copy link
Copy Markdown
Contributor Author

rubenvdlinde commented May 19, 2026

📍 lib/Service/AnonymizationService.php:183 (outside PR diff — line-comment API rejected, posted as issue comment)

[info] tryAppendBasisSummary() catch(\Throwable) is correct soft-failure behavior for a non-auth feature method — gate-8 correctly does not flag this (not an auth/permission resolver).

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 19, 2026

Quality Report — ConductionNL/docudesk @ 70fa65f

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

Coverage: 0% (0/10 statements)

Quality workflow — 2026-05-19 03:09 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
Copy link
Copy Markdown
Contributor Author

rubenvdlinde commented May 19, 2026

Security Review — Clyde Barcode

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

Check Result
Semgrep p/security-audit + p/owasp-top-ten ✅ 0 findings (4 PHP files)
Semgrep p/secrets ✅ 0 findings (9 files)
gitleaks ✅ no secrets
composer audit --locked ✅ no PHP dep CVEs
npm audit --production ⚠️ 59 CVEs (inherited — package.json unchanged)
Manual OWASP review ✅ 1 SUGGESTION only

Findings

[SUGGESTION] outputFormat lacks allowlist validation (lib/Controller/AnonymizationController.php:236)

The outputFormat string param is accepted from user input without an allowlist check. Currently safe — only compared with === 'preserve' and logged as a PSR-3 context field, never used in file paths or OS calls. Recommend: in_array($outputFormat, ['pdf', 'preserve'], true) check at controller boundary, HTTP 400 on unknown values.

Inherited debt (informational, non-blocking)

  • gate-7: 12–13 @NoAdminRequired methods across both controllers lack per-object ownership guards. All pre-existing (same counts on origin/development); this PR added zero new unguarded public methods. Recommend a dedicated ADR-005 compliance PR.
  • gate-14: BatchAnonymizationController slug mismatch (batchAnonymization vs batch_anonymization). All routes correctly registered — gate false positive. Pre-existing.
  • npm audit: 59 CVEs in Vue2 ecosystem transitive deps. package.json unchanged by this PR. Recommend a dedicated dep-bump PR.

See inline comments for per-finding detail.

🤖 Changes Clyde Barcode applied

None — review-only, no commits pushed to your branch.

rubenvdlinde
rubenvdlinde commented May 19, 2026
View reviewed changes
Comment thread lib/Controller/AnonymizationController.php
}

if (isset($params['outputFormat']) === true) {
$outputFormat = (string) $params['outputFormat'];
Copy link
Copy Markdown
Contributor Author

@rubenvdlinde rubenvdlinde May 19, 2026

Choose a reason for hiding this comment

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

[unfixed: SUGGESTION] Rule: OWASP A03:2021 — outputFormat accepted as any string without allowlist validation. Currently safe (only compared with === 'preserve', never used in file paths/OS calls), but recommend in_array(['pdf','preserve']) check at controller boundary with HTTP 400 on unknown values.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 19, 2026

Quality Report — ConductionNL/docudesk @ ad5ef2b

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

Coverage: 0% (0/10 statements)

Quality workflow — 2026-05-19 03:22 UTC

Download the full PDF report from the workflow artifacts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Rem-Dam Rem-Dam Awaiting requested review from Rem-Dam Rem-Dam will be requested when the pull request is marked ready for review Rem-Dam is a code owner

@bbrands02 bbrands02 Awaiting requested review from bbrands02 bbrands02 will be requested when the pull request is marked ready for review bbrands02 is a code owner

@rjzondervan rjzondervan Awaiting requested review from rjzondervan rjzondervan will be requested when the pull request is marked ready for review rjzondervan is a code owner

@WilcoLouwerse WilcoLouwerse Awaiting requested review from WilcoLouwerse WilcoLouwerse will be requested when the pull request is marked ready for review WilcoLouwerse is a code owner

@remko48 remko48 Awaiting requested review from remko48 remko48 will be requested when the pull request is marked ready for review remko48 is a code owner

@SudoThijn SudoThijn Awaiting requested review from SudoThijn SudoThijn will be requested when the pull request is marked ready for review SudoThijn is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rubenvdlinde @actions-user