Skip to content

fix(openapi): document optional sign request UUID headers - #9466

Open
vitormattos wants to merge 3 commits into
mainfrom
fix/document-sign-request-headers
Open

vitormattos wants to merge 3 commits into
mainfrom
fix/document-sign-request-headers

Conversation

@vitormattos

Copy link
Copy Markdown
Member

Summary

  • Add #[RequestHeader(indirect: true)] to the six public OCS operations that already support the optional libresign-sign-request-uuid via InjectionMiddleware but omitted it from OpenAPI: SignatureElementsController::{previewSignatureElement,patchSignatureElement,deleteSignatureElement} and IdDocsController::{addFiles,delete,listUnauthenticatedSignerDocuments}.
  • Preserve the three existing declarations, route-based UUID signing/progress operations, authorization rules, OCS infrastructure headers, and OpenAPI::SCOPE_IGNORE controllers.
  • Regenerate openapi.json, openapi-full.json, and corresponding TypeScript types. Administration scope is unchanged. No multipart changes (fix(api): detect multipart file uploads and reject truncated batches (#9435) #9452 / Document multipart uploads in the OpenAPI contract #9435).
  • Add an OpenAPI contract regression guarding exactly nine authorized operations and middleware unit cases asserting route/header precedence, missing sources, and unvalidated UUID propagation.

Evidence

  • Baseline OpenAPI: 3 of 9 operations included the custom header; regenerated contracts: 9 of 9, none elsewhere. Each addition is an optional in: header string parameter.
  • composer openapi: passed using locked nextcloud/openapi-extractor v1.9.1 and an isolated PHP/Node toolchain.
  • npm run typescript:generate: passed.
  • vendor/bin/phpunit -c tests/php/openapi/phpunit.xml: 23 tests, 406 assertions, passed.
  • npm run ts:check: passed.
  • npm run lint: passed with 23 pre-existing warnings and no errors.
  • PHP-CS-Fixer dry-run for changed PHP sources and tests: passed (0 changes).
  • PHP syntax checks: passed.

Limitations / follow-up review

  • Middleware-focused PHPUnit tests were added but could not execute in the standalone worktree because the Nextcloud core bootstrap (lib/base.php) is not present. Execute in a disposable Nextcloud 36 runtime; include negative authorization and valid guest/authenticated paths before merging.
  • Psalm was attempted but the standalone worktree's psalm.xml references missing Nextcloud core paths. CI must verify Psalm; no suppression was introduced.
  • The extractor emits a warning recommending an x- prefix for this custom header; renaming it would break existing clients, so the existing wire name is retained.
  • No live Behat/integration execution was claimed. No routing, authentication, service, or multipart behavior was modified.

Two incremental commits are DCO-signed and cryptographically verified. Do not merge until CI and runtime authorization checks are reviewed.

Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
@github-actions

github-actions Bot commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

Code Metrics Report

Note: Metrics describe the revision analyzed by the successful PHPUnit run. Newer commits with pending, canceled or failed checks may not yet be represented in this report.

main (642daf6) #9466 (a8fa04b) +/-
Coverage 76.4% 76.4% 0.0%
Test Execution Time 3m42s 3m13s -29s
Details
  |                     | main (642daf6) | #9466 (a8fa04b) | +/-  |
  |---------------------|----------------|-----------------|------|
  | Coverage            |          76.4% |           76.4% | 0.0% |
  |   Files             |            512 |             512 |    0 |
  |   Lines             |          28845 |           28845 |    0 |
  |   Covered           |          22063 |           22063 |    0 |
+ | Test Execution Time |          3m42s |           3m13s | -29s |

Code coverage of files in pull request scope (55.4% → 55.4%)

Files Coverage +/- Status
lib/Controller/IdDocsController.php 40.0% 0.0% modified
lib/Controller/SignFileController.php 81.5% 0.0% modified
lib/Controller/SignatureElementsController.php 31.9% 0.0% modified

Reported by octocov

@vitormattos

Copy link
Copy Markdown
Member Author

/backport to stable35

@backportbot-libresign backportbot-libresign Bot added the backport-request Request to backport a change to supported stable branches label Nov 13, 2026
@vitormattos

Copy link
Copy Markdown
Member Author

/backport to stable34

@vitormattos

Copy link
Copy Markdown
Member Author

/backport to stable33

@vitormattos
vitormattos marked this pull request as ready for review November 13, 2026 17:27
@vitormattos
vitormattos requested a review from a team as a code owner November 13, 2026 17:27

@lfals lfals left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The six public operations that already accept libresign-sign-request-uuid and skip it for a logged-in caller now declare that header as optional. Generated OpenAPI and TypeScript types match those attributes.

One regression: OpenApiRequestHeadersTest requires sign_file-request-code-by-file-id to stay undocumented, so a client that follows the spec is rejected with Invalid UUID before requestCodeByFileId runs.

Comment on lines +18 to +29
/** The optional header applies only to methods that can identify a request without a route UUID. */
private const OPERATIONS = [
'signature_elements-create-signature-element',
'signature_elements-get-signature-elements',
'signature_elements-use-saved-signature-elements',
'signature_elements-preview-signature-element',
'signature_elements-patch-signature-element',
'signature_elements-delete-signature-element',
'id_docs-add-files',
'id_docs-delete',
'id_docs-list-unauthenticated-signer-documents',
];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sign_file-request-code-by-file-id consumes libresign-sign-request-uuid, and this allow-list requires that header to stay undocumented.

  • SignFileController::requestCodeByFileId is #[RequireSigner] on POST /api/{apiVersion}/sign/file_id/{fileId}/code, which has no {uuid} (SignFileController.php:491).
  • InjectionMiddleware::requireSigner() reads getParam('uuid', header), and SignerValidator::validateUuidFormat('') throws Invalid UUID before the action runs. A logged-in caller is not exempt (InjectionMiddleware.php:317).
  • ModalVerificationCode.requestCode posts this route when fileId is set and sends neither the header nor uuid.

Please confirm this operation should stay off the contract, or add the same optional header and include it in OPERATIONS:

'sign_file-request-code-by-file-id',

$present[] = $id;
self::assertSame(self::SIGN_REQUEST_HEADER, $headers[0]['name']);
self::assertSame(self::DESCRIPTION, $headers[0]['description']);
self::assertFalse($headers[0]['required'] ?? false, "The header is optional for {$id}");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

signature_elements-use-saved-signature-elements is not optional for the caller who can succeed.

  • useSavedSignatureElements is #[RequireSignRequestUuid] with no skipIfAuthenticated, and POST /signature/elements/saved has no {uuid} and no documented query or body UUID (SignatureElementsController.php:185).
  • An anonymous email signer with no header and no uuid parameter gets Invalid UUID from validateUuidFormat before the action runs.
  • A logged-in caller still fails in the action with Saved signatures are only available to signers identified by email, even when the UUID is valid.
  • The six headers added here use skipIfAuthenticated: true, so optional matches those.

Please confirm this is the intended product behaviour, or stop asserting required === false for signature_elements-use-saved-signature-elements and mark that header required.

Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>

This branch has not been deployed

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

Labels

backport-request Request to backport a change to supported stable branches

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants