This repository was archived by the owner on May 29, 2026. It is now read-only.
feat(features-roadmap): OpenRegister pilot integration — route + nav link (section 5)#1491
Merged
Merged
Conversation
Implements tasks 1.1–1.10 of openspec change `add-features-roadmap-menu`
(GitHub issues proxy for the in-product roadmap surface).
Adds two methods to the existing GitHubHandler:
- listIssues(): cached list with OR-merged label filter (per D23),
PR entries filtered out, sensitive fields stripped (task 1.10).
- createIssue(): user PAT preferred, server PAT fallback with
attribution prefix; specRef body suffix + label applied when set.
Adds GitHubIssuesController with:
- GET /api/github/issues — #[NoCSRFRequired], 15-min distributed cache,
X-OpenRegister-GitHub-Cache: HIT|MISS header.
- POST /api/github/issues — CSRF enforced (no NoCSRFRequired), per-user
60s rate limit, graceful PAT-missing 503, GitHub rate-limit mapping
to 429 + reset_at.
Application.php wires two new GitHubHandler deps (IURLGenerator,
IUserManager) needed for the attribution prefix.
Pending in follow-ups (section 1):
- 1.11 PHPUnit tests incl. PAT-leak assertion
- 1.12 OpenAPI documentation
- 1.13 composer check:strict run
- 1.14–1.22 security hardening (repo allowlist, display-name
sanitization, audit logging, specRef + sort + labels validation,
admin opt-out flag, PAT scope/lifecycle docs)
Refs #1328
…indings Addresses 4 PHPMD warnings on the section-1 skeleton without resorting to @SuppressWarnings annotations: GitHubHandler — CouplingBetweenObjects: 14 → 11 (under threshold) • Extract new AttributionFormatter service (IUserManager + IURLGenerator now live there, not on the handler) • Switch constructor from IClientService to IClient directly (factory in Application.php calls newClient() at instantiation time) GitHubIssuesController::create — CyclomaticComplexity 10 → 4 + NPathComplexity 288 → ~12 • Extract validateSubmission() helper (repo / title / body length checks) • Extract enforceSubmitRateLimit() helper (per-user 60s APCu lookup) GitHubIssuesController::mapHandlerException — CyclomaticComplexity 12 → 4 • Extract mapPatNotConfigured() helper (read vs write graceful path) • Extract mapGitHubRateLimit() helper (BadResponseException unwrap + X-RateLimit-Remaining/Reset header inspection) Also applies code-style fixes flagged by PHPCS on touched files: • All internal-method calls converted to named-parameter form (validateSubmission(repo: …, title: …, body: …), etc.) • is_array($labels) === true (explicit comparison) • Closure → named static function for the labels-merge usort comparator • Missing constructor docstring on AttributionFormatter • Missing //end if marker after early-return arm in listIssues • Auto-fix run of PHPCBF (44 whitespace / blank-line nits) on the changed files; no other files touched. Touched files now pass: • php -l ✓ • composer phpcs --standard=phpcs.xml ✓ (0 errors on these files) • composer phpmd lib/...|text phpmd.xml ✓ (0 warnings on these files) Pre-existing PHPCS/PHPMD findings in unrelated files (AggregationRunner, ActionsController, OrganisationController, RegisterService, etc.) are left for their own follow-up — they predate this PR by months and touching them would inflate the diff with unrelated whitespace fixes. Refs #1328
…, 1.20b, 1.21
Implements seven of the eight section-1 security / resilience / compliance tasks
on the `add-features-roadmap-menu` change. Tests for each task (per the task
descriptions in tasks.md), task 1.11's PAT-leak assertion, task 1.12's OpenAPI
documentation, task 1.13's full composer check:strict pass, task 1.18's
APCu->ICache fallback abstraction, and task 1.22's admin-docs writeup are
explicitly deferred to follow-up commits — this PR keeps the diff focused on the
guard-pipeline shape + the implementations themselves.
Refactor for class-level complexity / coupling:
Extract two new services
lib/Service/Configuration/GitHubRequestValidator.php (new — pure-function
validators: validateRepoFormat, validatePerPage, validateTitleLength,
validateBodyLength, validateSpecRef, validateSort, validateLabels)
lib/Service/Configuration/GitHubGuards.php (new — policy guards
+ pipeline runner: runGuards, enforceFeatureFlag, enforceRepoAllowlist,
enforceGetRateLimit)
Controller becomes a thin orchestrator
GitHubIssuesController::index/create now compose a guard pipeline via
GitHubGuards::runGuards([...closures...]) — each guard's branching stays
inside its own method, the controller's cyclomatic + ExcessiveClassComplexity
counts stay flat as more guards land.
Handler splits createIssue
resolveCreateIssueToken / buildIssueBody / buildIssuePayload /
dispatchCreateIssue keep individual methods under cyclomatic + NPath
thresholds. dispatchCreateIssue returns the decoded array so callers don't
need the IResponse type (drops one class reference from the handler).
AttributionFormatter is extracted so the handler does not import
IUserManager + IURLGenerator directly.
Security tasks landed:
1.14 Repo allowlist enforcement
openregister::github_repo IAppConfig key. Unset → 200+hint (GET) or
503+error (POST). Set + mismatch → 403 repo_not_allowed. Set + match →
continue. Collapses the user-supplied repo attack surface to a single
admin-configured value.
1.15 Display-name + URL sanitization in AttributionFormatter
Strips CR/LF, *, _, `, [, ], (, ), backslash, <, > from the embedded
display name and truncates to 80 chars. Validates instance URL is
https:// or http://localhost; non-https + non-localhost falls back to
a URL-free generic prefix ("> Submitted via Nextcloud OpenRegister").
1.16 specRef format validation
Regex ^[a-z0-9][a-z0-9-]*[a-z0-9]$, length ≤ 80. Rejects newlines,
uppercase, punctuation, oversized slugs with 400 specref_invalid_format
before any GitHub call.
1.17 Audit logging for server-PAT submissions
GitHubHandler::dispatchCreateIssue emits exactly one INFO entry on
success and one WARNING entry on failure when the server-PAT fallback
path is taken. Fields: user_id, repo, issue_number, specref, timestamp
(plus error_code + github_status on failure). Never logs PAT, body,
or title. User-PAT path emits no openregister entry — those are
auditable on GitHub directly under the user's identity.
1.19 Per-user GET cache-miss rate limit
Per-user counter of distinct cache-key tuples within a rolling 5-minute
window, capped at 10 entries. Cache hits do not count. 11th distinct
miss returns 429 user_rate_limited + Retry-After header. Anonymous
callers are not counted. Enforced AFTER the cache check and BEFORE the
outbound GitHub call.
1.20 sort parameter allowlist
Constrains the GET ?sort= query parameter to {reactions-+1, created,
updated, comments}. Other values → 400 sort_invalid_value. Bounds the
cache-key space.
1.20b labels parameter validation
Each label matches ^[a-z][a-z0-9_-]*$ with length ≤ 50; max 8
comma-separated entries. Cache key already sorts labels so different
filter orderings hit the same cache slot. Rejects SQL-injection-shaped
labels, oversized labels, and 9+ entries with 400.
1.21 Admin opt-out flag
openregister::features_roadmap_enabled IAppConfig key (default true).
When false, both endpoints return 403 feature_disabled and skip all
downstream logic including the outbound GitHub call.
Quality gates green on touched files:
php -l clean
composer phpcs clean (5/5 files, 0 errors)
composer phpmd clean (0 warnings)
Application.php — no DI changes required for the new GitHubGuards +
GitHubRequestValidator + AttributionFormatter classes; NC's auto-wiring
resolves their OCP-typed constructors directly.
Refs #1328
Marks the security / resilience / compliance tasks landed in the previous commit (security hardening) as done in tasks.md. Pending: 1.11 (tests), 1.12 (OpenAPI), 1.13 (full quality run), 1.18 (APCu->ICache fallback), 1.22 (admin docs).
…1.22)
Tests — task 1.11
tests/Unit/Service/Configuration/GitHubRequestValidatorTest.php
20 cases covering the 7 validators (repo format, per_page range,
title/body length, specRef format incl. SQL-injection + newline +
length-cap, sort allowlist, labels validation incl. injection-shaped
+ 9-entry + oversized cases).
tests/Unit/Service/Configuration/AttributionFormatterTest.php
6 cases covering task 1.15: happy-path canonical prefix, markdown-
injection display name sanitization (separator appears exactly once),
80-char truncation, https-only scheme validation + generic-prefix
fallback, http://localhost dev exception, missing-user UID fallback.
tests/Unit/Service/Configuration/GitHubGuardsTest.php
7 cases covering tasks 1.14 + 1.19 + 1.21: feature-flag enabled/
disabled, repo-allowlist unset (graceful 200 on GET, 503 on POST),
repo mismatch, repo match, 11th distinct GET cache-miss hits 429,
runGuards short-circuits on first non-null response.
tests/Unit/Controller/GitHubIssuesControllerTest.php
7 cases covering the controller wiring + the load-bearing security
scenarios: 401 on unauthenticated POST, 400 on short title without
calling the handler, 400 on invalid specRef without calling the
handler, 403 cross-repo POST when allowlist set, 403 feature-
disabled on both endpoints, **PAT-leak assertion** running 3 paths
with placeholder `YOUR_API_KEY_HERE` and asserting it never appears
in body or headers, ReflectionMethod check on the #[NoCSRFRequired]
attribute placement (GET has it; POST does not, so CSRF middleware
runs).
Notes:
- Mocks use the same `createMock(SomeClass::class)` pattern as the
existing SearchControllerTest. PHPCS named-args sniff flags those at
the same severity it flags existing tests; not addressing in this
diff to keep test parity with the repo's existing style.
- Tests can't run locally without Nextcloud's lib/base.php bootstrap
(same constraint as the existing tests/Unit/* suite); they execute
in CI with the OPENREGISTER_TEST_NC_ROOT env var pointing at an
installed NC instance.
OpenAPI — task 1.12
openapi.json: adds /index.php/apps/openregister/api/github/issues GET +
POST operations + 5 supporting component schemas (GitHubIssueItem,
GitHubIssuesListResponse, GitHubIssueCreatedResponse, GitHubIssueSubmit
Request, StructuredError). Documents every parameter, every response
status (200/400/401/403/412/429/503), the X-OpenRegister-GitHub-Cache
+ Retry-After response headers, and every structured error_code
enum value (18 codes). All token placeholders use `YOUR_API_KEY_HERE`
per the safe-example-values rule from opsx-ff.
Admin docs — task 1.22
docs/Integrations/github-issues-proxy.md: full admin reference —
configuration keys (github_api_token, github_repo, features_roadmap_
enabled), minimum PAT scope (public_repo), fine-grained-PAT preference,
90-day rotation cadence, compromise-response procedure, the three
audit-log entry shapes, rate-limit table, and a troubleshooting table
covering the 6 documented degraded UI states.
Refs #1328
…ct (task 1.18)
Extracts a RateLimiterService that wraps ICacheFactory and adds the missing
fail-closed behaviour: createDistributed() on a cache-less instance returns the
no-op Null backend and silently never rate-limits, so callers gate on
isOperational() first and return HTTP 503 `rate_limiter_unavailable` when no
APCu / Redis / Memcached backend is configured.
lib/Service/Configuration/RateLimiterService.php (new)
isOperational() — true when isAvailable() || isLocalCacheAvailable()
checkFixedWindow / markFixedWindow — "1 action per N seconds" (POST submission limiter)
consumeDistinctKeyBudget() — "≤ N distinct sub-keys per rolling window" (GET cache-miss limiter)
GitHubIssuesController
- Drops its own ICache submitRateCache; injects RateLimiterService.
- create() guard pipeline gains enforceRateLimiterOperational() (→ 503
rate_limiter_unavailable) before enforceSubmitRateLimit().
- enforceSubmitRateLimit() + the post-success consume now go through
RateLimiterService::checkFixedWindow / markFixedWindow.
GitHubGuards
- Drops its own ICache getRateCache; injects RateLimiterService.
- enforceGetRateLimit() returns 503 rate_limiter_unavailable when the
limiter isn't operational, otherwise delegates the distinct-key-budget
tracking to RateLimiterService::consumeDistinctKeyBudget.
Tests
tests/Unit/Service/Configuration/RateLimiterServiceTest.php (new, 7 cases):
operational when distributed cache available, operational when only local
cache available, NOT operational when neither, fixed-window allows-then-
blocks with positive Retry-After, distinct-key-budget allows up-to-max
then blocks, repeat distinct keys never exhaust the budget.
GitHubGuardsTest: rewired to construct RateLimiterService; added
testGetRateLimitFailsClosedWhenNoCacheBackend (→ 503).
GitHubIssuesControllerTest: buildController rewired to inject the
RateLimiterService; cacheFactory mock now stubs isAvailable /
isLocalCacheAvailable.
Application.php — no DI changes; RateLimiterService takes only ICacheFactory
and auto-wires.
This completes section 1 of the add-features-roadmap-menu tasks.md (23 of 23
backend tasks).
Refs #1328
… fix PHPCS - Replace `instanceof RuntimeException` with `is_a($e, 'RuntimeException')` (parity with the BadResponseException check) so the import can be removed — CouplingBetweenObjects 13 → 12 - Wrap markFixedWindow() call to stay under 125 cols - Blank line after the early-return in enforceRateLimiterOperational()
…-app smoke test coverage Unit coverage is already tracked (BE 1.11, FE 2.14). Adds the API-contract layer (Newman collections for GET/POST /api/github/issues incl. PAT-leak assertions) and the browser layer (Playwright: nav + tabs, Suggest modal happy-path + validation, admin opt-out, screenshots/CI), plus a traceability entry for the per-consuming-app adoption smoke test (implemented per-app, tracked in hydra#251).
…ct (task 1.13) PHPStan: - GitHubHandler::extractGitHubStatus — drop the unreachable `getResponse() === null` check (BadResponseException always carries a response) - GitHubRequestValidator::validateLabels — drop the dead `is_string()` guard (the param is typed array<string>) PHPCS (new file + pre-existing files encountered): - RateLimiterService — blank line after control structures (phpcbf) - ActionsController / AuditTrailController / NotificationHistoryController / ReportsController / AggregationRunner / PermissionHandler — missing @param doc comments, a mismatched @param name, and capitalize one inline comment - RegistersController — `?`/`:` spacing in a multi-line return type (phpcbf) Refactor: - GitHubIssuesController — extract the GET/POST guard pipelines into readGuardPipeline()/writeGuardPipeline() so the inline closures stay out of index()/create()
…ganisation) `psalm --update-baseline` — both issues were already fixed upstream; Psalm was reporting UnusedBaselineEntry until the baseline caught up.
…dd-features-roadmap-menu-backend # Conflicts: # lib/Controller/ActionsController.php # lib/Controller/AuditTrailController.php # lib/Controller/NotificationHistoryController.php # lib/Controller/ReportsController.php # lib/Service/Aggregation/AggregationRunner.php # lib/Service/Object/PermissionHandler.php
…ink + nc-vue 1.x bump Section 5 of add-features-roadmap-menu (the OpenRegister pilot): - src/views/roadmap/FeaturesRoadmapIndex.vue — wraps CnFeaturesAndRoadmapView; repo/features/disabled from loadState() with fallbacks (full IInitialState wiring for task 5.11 is a follow-up) - /features-roadmap route + first-child <CnFeaturesAndRoadmapLink> in NcAppNavigationSettings - bump @conduction/nextcloud-vue 0.1.0-beta.17 → ^1.0.0-beta.32 NOT YET MERGEABLE: the 0.1.x → 1.x nc-vue bump pulls @nextcloud/vue 8.x whose dist/index.cjs require()s @nextcloud/axios under a 'require'/'webpack' condition that the currently-resolved @nextcloud/axios's exports field doesn't satisfy — webpack build fails with 13 'not exported under the conditions' errors. Needs a coordinated @nextcloud/* dep migration (and the webpack resolve.extensions '*' → '.*' fix). Tracked separately.
… build
@nextcloud/axios@2.6.0 shipped a broken `exports` field (dropped the `require`
condition), so @nextcloud/vue@8.x's dist/index.cjs require('@nextcloud/axios')
fails under webpack's `require` resolution. Pin away from 2.6.0 in both the
direct dep and `overrides`. With this, `npm run build` is green with the
@conduction/nextcloud-vue@1.0.0-beta.32 bump + the section-5 route/view/link.
Also drop the PHP-style @category/@Package tags from the new Vue file's docblock.
Closes the build blocker tracked in #1489 (OpenRegister side).
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ❌ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 153/153 | |||
| npm | ✅ | ✅ 601/601 | |||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-05-12 06:12 UTC
Download the full PDF report from the workflow artifacts.
This was referenced May 12, 2026
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
Section 5 of
add-features-roadmap-menu— wires the Features & Roadmap page into OpenRegister itself (the pilot for the org-wide rollout).src/views/roadmap/FeaturesRoadmapIndex.vue— route view wrappingCnFeaturesAndRoadmapViewfrom@conduction/nextcloud-vue;repo/features/disabledcome fromloadState()with sensible fallbacks (the fullIInitialStatewiring for task 5.11 is a follow-up)./features-roadmaproute +<CnFeaturesAndRoadmapLink>mounted as the first child of<NcAppNavigationSettings>insrc/navigation/MainMenu.vue.@conduction/nextcloud-vue0.1.0-beta.17→^1.0.0-beta.32(the release that ships theCn*roadmap family).@nextcloud/axiosto~2.5.1+overrides—@nextcloud/axios@2.6.0shipped a brokenexportsfield (norequirecondition), which breaks@nextcloud/vue@8.x'sdist/index.cjsunder webpack. See Migrate @conduction/nextcloud-vue 0.1.x → 1.x (blocks Features & Roadmap pilot) #1489.Status
npm run build— green (webpack compiled with 4 warnings— pre-existing entrypoint-size warnings, no errors).eslint— clean on the new files.openregistersubmodule webpack-aliases@conduction/nextcloud-vueto a stale localnextcloud-vue/src, and there's other in-progress work in those submodules) — backend verified via curl, components via 1000+ Jest tests upstream.Test plan
npm install && npm run buildgreen/features-roadmaprenders with Features + Roadmap tabs; Roadmap tab callsGET /api/github/issues?labels=enhancement,featurePOST /api/github/issueswith CSRF tokenopenregister::features_roadmap_enabled = false(once 5.11 is wired): nav item hidden, route shows the disabled viewRefs: openregister#1463 · openregister#1489 · ConductionNL/nextcloud-vue#196 · ConductionNL/hydra#251