Credential broker: allow-rules match a query-truncated path, while the outbound call uses the raw path #4653

Summary

CredentialBrokerService::request() checks the allow-rules against one form of the path and sends another.
normalisePath() (lib/Service/Credential/CredentialBrokerService.php:1475) URL-decodes the caller's path once and cuts it at the first ?. Both the traversal check and assertRuleAllowed() (:1278) see only that truncated, decoded form. resolveAndLockUrl() (:1315) then builds the outbound URL from the raw caller path ($baseUrl . $path, call at :273).

A path such as /search/code%3F/../../user/keys decodes to /search/code?/../../user/keys. The rule check sees /search/code, which has no .. segment, so the GitHub rule GET /search/code matches. The outbound URL keeps %3F/../../user/keys. If the HTTP stack removes dot segments before sending, the request reaches GET /user/keys on the same host. curl removes them by default unless CURLOPT_PATH_AS_IS is set. The host lock still holds, so the reach is any path on the provider host, for any method that has at least one rule.

Status of the evidence

• Read in the code on development (0c61e0952): the check uses the truncated path and the call uses the raw path.
• Reproduced only with plain curl against a local server. Not yet verified through Nextcloud's IClientService (Guzzle on the curl handler) with a real brokered request.

Impact (if confirmed)

Every provider's allow-list (lib/Settings/credential-providers.json) is reduced to "same host, any path" for a caller that is already admitted to use the credential. For GitHub that means reads such as /user/keys with whatever scopes the stored token carries. The rules exist to prevent exactly that.

Proposed fix

• Derive one canonical path and use it for both the check and the call. Resolve dot segments (RFC 3986 §5.2.4) after the single decode, and reject a path whose canonical form differs from the form that will be sent.
• Or: refuse an encoded ? / # (%3F, %23) in the path part, take the query only from the raw path's first literal ?, and send the canonical path.
• Tests: /search/code%3F/../../user/keys, /search/code%3f/..%2f..%2fuser/keys and /search/code/%2e%2e/%2e%2e/user/keys are all denied. The existing /repos/{o}/{r}/compare/a...b case still passes.

Done when

• A brokered GET with each of the paths above returns the static 403 and makes no outbound call.
• A test that sends the canonical path, not the raw one, goes red when resolveAndLockUrl() is given $path again.

Found during the persistence audit in the review of openregister#4642. Pre-existing; related public issue on broker auditing: #4652.