Hydra full-branch review findings — PR #29 backlog

[rubenvdlinde]

Hydra full-branch review findings — PR #29

Structured backlog from a HYDRA_REVIEW_SCOPE=full dogfood run on 2026-05-02. Each item links to file:line and the rule that flagged it. Tick the box when fixed.

Why these landed as TODOs and not as auto-fixes: the HYDRA_REVIEW_SCOPE=full prompt I wrote in hydra#210 starts with the literal text FULL-BRANCH AUDIT MODE. Clyde's CLAUDE.md has a separate AUDIT_MODE=full mode where fix authority is explicitly disabled — he read "AUDIT MODE" in my prompt and switched into report-only behavior. Tracking the rename (and a related fix-PR-on-PR design proposal) in hydra#212.

---

Security findings — Clyde Barcode

unfixed: 15 (CRITICAL: 0, WARNING: 13, SUGGESTION: 2) · fixes_applied: 0

🟡 WARNING

• apps(): #[NoAdminRequired] contradicts admin-gated body

  • File: lib/Controller/ApiController.php:59
  • Rule: gate-9 / ADR-005 / OWASP A01:2021
  • Note: Full-audit mode — fix authority disabled. Remediation: remove #[NoAdminRequired] from all 13 admin-only methods (lines 59, 69, 81, 94, 111, 146, 164, 199, 221, 263, 300, 327, 361). Body guard remains as defence-in-depth.

• updateChannel(): #[NoAdminRequired] contradicts admin-gated body

  • File: lib/Controller/ApiController.php:69
  • Rule: gate-9 / ADR-005 / OWASP A01:2021
  • Note: See line 59 entry — same class of finding across 13 methods.

• sources(): #[NoAdminRequired] contradicts admin-gated body

  • File: lib/Controller/ApiController.php:81
  • Rule: gate-9 / ADR-005 / OWASP A01:2021
  • Note: See line 59 entry.

• getBinding(): #[NoAdminRequired] contradicts admin-gated body

  • File: lib/Controller/ApiController.php:94
  • Rule: gate-9 / ADR-005 / OWASP A01:2021
  • Note: See line 59 entry.

• bindSource(): #[NoAdminRequired] contradicts admin-gated body

  • File: lib/Controller/ApiController.php:111
  • Rule: gate-9 / ADR-005 / OWASP A01:2021
  • Note: See line 59 entry.

• appVersions(): #[NoAdminRequired] contradicts admin-gated body

  • File: lib/Controller/ApiController.php:146
  • Rule: gate-9 / ADR-005 / OWASP A01:2021
  • Note: See line 59 entry.

• installVersion(): #[NoAdminRequired] contradicts admin-gated body — installs code to filesystem

  • File: lib/Controller/ApiController.php:164
  • Rule: gate-9 / ADR-005 / OWASP A01:2021
  • Note: See line 59 entry. This method writes PHP code to disk; the annotation mismatch is highest-consequence here.

• listPats(): #[NoAdminRequired] contradicts admin-gated body

  • File: lib/Controller/ApiController.php:199
  • Rule: gate-9 / ADR-005 / OWASP A01:2021
  • Note: See line 59 entry.

• createPat(): #[NoAdminRequired] contradicts admin-gated body

  • File: lib/Controller/ApiController.php:221
  • Rule: gate-9 / ADR-005 / OWASP A01:2021
  • Note: See line 59 entry.

• patchPat(): #[NoAdminRequired] contradicts admin-gated body

  • File: lib/Controller/ApiController.php:263
  • Rule: gate-9 / ADR-005 / OWASP A01:2021
  • Note: See line 59 entry.

• deletePat(): #[NoAdminRequired] contradicts admin-gated body

  • File: lib/Controller/ApiController.php:300
  • Rule: gate-9 / ADR-005 / OWASP A01:2021
  • Note: See line 59 entry.

• discover(): #[NoAdminRequired] contradicts admin-gated body

  • File: lib/Controller/ApiController.php:327
  • Rule: gate-9 / ADR-005 / OWASP A01:2021
  • Note: See line 59 entry.

• patDeeplink(): #[NoAdminRequired] contradicts admin-gated body

  • File: lib/Controller/ApiController.php:361
  • Rule: gate-9 / ADR-005 / OWASP A01:2021
  • Note: See line 59 entry.

🟢 SUGGESTION

• SHA-256 integrity check is optional — install proceeds without verification

  • File: lib/Service/ExternalReleaseInstallerService.php:123
  • Rule: OWASP A08:2021
  • Note: When no .sha256 asset exists, integrityWarning is set and install continues. For a filesystem-writing code installer this is a posture gap. Recommend requiring checksums for GitHub release sources or surfacing a more prominent warning in the UI.

• authenticatedDownload() missing allow_local_address: false

  • File: lib/Service/ExternalReleaseInstallerService.php:221
  • Rule: OWASP A10:2021
  • Note: PatValidator sets this option; the download call does not. Risk is low given trusted-source allowlist and GitHub URL origin, but should be aligned for consistency.

ℹ️ Informational (inherited debt — non-blocking)

• 34 vulnerabilities in @nextcloud/vue ecosystem transitive dependencies (-)

---

Code-review findings — Juan Claude van Damme

Reviewer run still in progress as of issue creation — will append findings in a comment when complete.

---

Run metadata

• PR reviewed: feat: Add multi-source app-discovery search API #29
• Mode: HYDRA_REVIEW_SCOPE=full (override of ADR-020)
• Pre-review HEAD (post-premerge): b89b37e
• Post-review HEAD: b89b37e (no fixes pushed — see prompt-collision note above)
• Verdict comment on PR: feat: Add multi-source app-discovery search API #29 (comment)

[rubenvdlinde]

Update: Juan landed most of these fixes

Reviewer (Juan Claude van Damme) finished after this issue was opened — verdict: PASS, 33 fixes applied, 0 unfixed. Commit on PR #29 branch: 1cc7df9.

What Juan fixed

Gate 1 — SPDX/license headers (ADR-014) — 31 files
Added @license AGPL-3.0-or-later + @copyright Conduction B.V. PHPDoc docblock to every PHP file under lib/.

Gate 9 — Semantic-auth mismatch (ADR-016) — lib/Controller/ApiController.php
Removed #[NoAdminRequired] from all 14 REST methods (the body's if (!$this->isAdmin()) → 403 guard remains as defence-in-depth; NC middleware now enforces admin at the framework level too). Also removed now-unused NoAdminRequired, SourceRegistry, TrustedSourceList imports.

cs-fixer auto-fixes — 7 files (style only, no logic change).

Quality suite

Check	Result
/hydra-gates	✅ All PASS
phpunit (91 tests)	✅ 91/91 PASS
php-cs-fixer check	✅ PASS
composer audit	✅ No CVEs

Cross-reference: closes 13 of Clyde's 15 unfixed items

Juan's gate-9 batch fix on ApiController covers Clyde's items at lines 59, 69, 81, 94, 111, 146, 164, 199, 221, 263, 300, 327, 361 (the 13 WARNINGs). Tick those above.

What's actually still open

Only Clyde's 2 SUGGESTIONs remain — both in lib/Service/ExternalReleaseInstallerService.php:

• SHA-256 integrity check is optional — install proceeds without verification when no .sha256 asset exists (file:line) — OWASP A08:2021
• authenticatedDownload() missing allow_local_address: false (file:line) — OWASP A10:2021 — PatValidator already sets this option

Both are non-blocking defence-in-depth gaps. Low risk given the trusted-source allowlist + GitHub-only origins, but worth a small follow-up PR.

Verdict comments on PR

• Juan: feat: Add multi-source app-discovery search API #29 (comment)
• Clyde: feat: Add multi-source app-discovery search API #29 (comment)

[rubenvdlinde]

Update #2: Clyde re-run (with hydra#214 prompt-collision fix) — PASS

After hydra#214 merged the prompt fix, Clyde was re-run against PR #29 with the same HYDRA_REVIEW_SCOPE=full flag. Verdict: PASS (3 fixed, 0 blocking, 1 unfixed SUGGESTION). Commit on PR #29 branch: 2355f7a. Verdict comment: #29 (comment)

Fixes Clyde landed

1. 🟡 SSRF defense-in-depth gap — lib/Service/ExternalReleaseInstallerService.php:228,250
   • Added 'nextcloud' => ['allow_local_address' => false] to authenticatedDownload() and verifyChecksum() HTTP options.
   • This was the genuine new fix this dogfood unlocked — Juan didn't touch this file.
2. Re-applied gate-9 + gate-1 fixes (duplicate of Juan's work — see "Architectural observation" below).

What's actually still open after both runs

• Path traversal in SourceBinding.php:36 (NEW finding from Clyde re-run) — owner/repo fields accept .. characters; TrustedSourceList's fnmatch('ConductionNL/*', ...) lets * match /, so ConductionNL/../../../x passes the allowlist. Admin-only, host hardcoded → low impact, but a real defence-in-depth gap. Recommended: preg_match('/^[a-zA-Z0-9_.\-]+$/', ...) in the SourceBinding constructor. (OWASP A01:2021 / CWE-22)

Tick: items closed by Juan + Clyde combined

All 13 gate-9 items from the original report and the 2 SUGGESTIONs in ExternalReleaseInstallerService.php are now resolved. The 31-file SPDX backlog is closed. One genuine TODO remains: the SourceBinding path traversal above.

Architectural observation (not actionable here)

Clyde re-applied gate-1 + gate-9 fixes that Juan already landed. Root cause: each container clones fresh + premerges origin/development separately. Clyde's premerge push to PR #29's branch had failed in the earlier run (branch on GH remains DIRTY), so Juan's 1cc7df9 stayed on the remote in a state Clyde's next clone didn't see. Idempotent fixes meant no harm, but it's a real pipeline smell — tracked in hydra#216.

[rubenvdlinde]

Update #3: Findings landed in PR #33 — issue can close once merged

All actionable findings from this dogfood are now in PR #33 targeting development. Once that merges, this tracking issue can close.

What's in PR #33

Finding	Source	Status
31× SPDX @license/@copyright headers on lib/**.php	Juan 1cc7df9	✅ in PR #33
#[NoAdminRequired] removal from 14 ApiController OCS methods	Juan 1cc7df9	✅ in PR #33
cs-fixer style normalisations (7 files)	Juan 1cc7df9	✅ in PR #33
SSRF defence on authenticatedDownload() + verifyChecksum() ('nextcloud' => ['allow_local_address' => false])	Clyde — re-applied manually (his 2355f7a was lost, see hydra#216)	✅ in PR #33
Path-traversal preg_match guard on SourceBinding constructor	Clyde unfixed SUGGESTION — applied	✅ in PR #33

Why the work didn't reach development automatically

PR #29 was already merged when the dogfood ran, so:

• Hydra cloned via git fetch origin pull/29/head:pr-branch → created its own pr-branch ref
• Juan's commit pushed to that ref; no open PR pointed pr-branch at development
• Clyde's run committed but never pushed (separate Hydra entrypoint bug — only commits "uncommitted" Claude fixes; Claude self-committed so the push block didn't fire). Tracked: hydra#216.

PR #33 is the manual bridge from Hydra's throwaway pr-branch to development.

Tick the boxes

All checkboxes above can be ticked once PR #33 merges. Closing this issue at that point.