This repository was archived by the owner on May 29, 2026. It is now read-only.
chore(quality): apply Hydra full-branch review findings — SPDX, gate-9, SSRF, path-traversal#33
Merged
Conversation
… SourceBinding Two findings from Clyde Barcode's full-branch security audit on PR #29 (see #32 for the tracking issue, the verdict comments on PR #29, and the dogfood context). 1. SSRF defence-in-depth — ExternalReleaseInstallerService.php authenticatedDownload() and verifyChecksum() fetched URLs from the GitHub API browser_download_url field without 'nextcloud' => ['allow_local_address' => false]. A malicious release in a trusted-source-listed repo could redirect the download to an internal address, forwarding the PAT Bearer token. PatValidator already sets this option correctly — mirrored the pattern in both methods. (OWASP A10:2021) 2. Path-traversal guard — SourceBinding.php The constructor accepted any string for owner/repo. fnmatch in TrustedSourceList lets `*` match `/`, so `ConductionNL/../../../x` passes the allowlist. Adding preg_match validation to the constructor rejects invalid characters at the boundary; charset matches GitHub's own owner/repo rules (alphanumerics, dot, dash, underscore). (CWE-22 / OWASP A01:2021) Both fixes mirror Conduction's existing patterns in the same app and do not introduce new dependencies. Bounded mechanical changes, no behaviour change for legitimate inputs. Bundles 31 SPDX header additions + the gate-9 #[NoAdminRequired] batch removal from Juan Claude van Damme's earlier review run (commit 1cc7df9 on the pr-branch ref) in the same PR.
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Applies the actionable findings from the 2026-05-02 Hydra full-branch security + code review dogfood (tracking issue) directly to
development. PR #29 was already merged when the dogfood ran, so Hydra's commits landed on a Hydra-internalpr-branchref with no path todevelopment. This PR brings the validated work onto the canonical branch.What's in the diff
From Juan Claude van Damme's code-review run (commit
1cc7df9on Hydra'spr-branch):@license AGPL-3.0-or-later+@copyright Conduction B.V.PHPDoc headers added to every file underlib/(gate-1 / ADR-014).lib/Controller/ApiController.php—#[NoAdminRequired]removed from 14 admin-only OCS methods; the body'sif (!isAdmin()) return 403guard remains as defence-in-depth (gate-9 / ADR-005 / ADR-016 semantic-auth fix). NC middleware now enforces admin at the framework level.Re-applied manually from Clyde Barcode's security-review run (his commit
2355f7awas lost — never pushed to remote due to a Hydra entrypoint bug, see ConductionNL/hydra#216):lib/Service/ExternalReleaseInstallerService.php—'nextcloud' => ['allow_local_address' => false]added toauthenticatedDownload()andverifyChecksum()HTTP options. SSRF defence-in-depth — without this, a malicious GitHub release could redirect the download to an internal address and leak the PAT Bearer token. MirrorsPatValidator's existing pattern. (OWASP A10:2021)lib/Service/Source/SourceBinding.php— constructor nowpreg_match-validatesowner/repocharset. TheTrustedSourceListfnmatchallowlist's*matches/, soConductionNL/../../../xwould otherwise pass; this rejects path-traversal at the boundary. Charset matches GitHub's own owner/repo rules. (CWE-22 / OWASP A01:2021)Quality
phpunit91/91 PASS,php-cs-fixer checkPASS,composer auditno CVEs, all hydra-gates PASSSourceBindingTesttests use'ConductionNL'/'openregister'— both pass the new regex (alphanumerics)Provenance
Test plan
SourceBinding::github('ConductionNL', 'openregister')constructs cleanlySourceBinding::github('ConductionNL', '../../../etc')throwsInvalidArgumentException