Skip to content

StufController::inkomend() WSSE refusal is untested — its two sibling public routes are not #843

Description

@rubenvdlinde

What

StufController::inkomend() verifies a WSSE signature and returns 422 on mismatch. Nothing tests that refusal. No test in the repo references inkomend at all.

Why this is worth filing rather than shrugging at

Its two sibling public endpoints have a purpose-built suite — tests/Unit/Service/Stuf/StufSoapRequestDispatcherAuthTest.php — whose own docblock states the standard exactly right:

These four cases exist because StufController::zaken() and ::personen() are #[PublicPage] routes: nothing in Nextcloud's middleware stack will ever refuse a caller on them, so the refusal has to come from the dispatcher and has to be tested here.

The suite is written so that DELETING the guard makes it red.

inkomend() is the third #[PublicPage] route in the same controller and is held to a different standard. Delete the verifyWsse call and every test still passes.

What is covered

  • verifyWsse() itself is exercised by the dispatcher auth suite, so the comparison logic (hash_equals on username and vault-resolved password) is not untested.
  • The gap is specifically inkomend()'s wiring to it — that the controller calls the verifier, and that a mismatch produces 422 rather than falling through to detectBerichtSoort() and processing the envelope.

Why it is probably untested

public function inkomend(): DataResponse {
    $rawXml = (string)file_get_contents(filename: 'php://input');

Reading php://input directly gives a unit test no seam to drive the body through. That is very likely the actual reason this endpoint has no test while its siblings do.

Suggested fix

Introduce a seam for the request body — inject a small body reader, or read via IRequest — then add the three arms the sibling suite already models:

  1. tampered WSSE → 422 (the refusal only exists because the guard runs)
  2. unresolvable endpoint → 400
  3. positive control: a verified sender reaches detectBerichtSoort() — so a guard that refused everything would also fail

Not fixed here because changing how a live StUF webhook reads its body is a behavioural change to a statutory integration, and the sequencing belongs with whoever owns the StUF spec.

Found while auditing ADR-082 public-endpoint coverage. Throttling itself is fine — all three endpoints carry #[AnonRateLimit(limit: 300, period: 60)].

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions