matrix-org / matrix-org/matrix-hookshot
GrantChecker's slash-containing account-data type breaks behind reverse proxies that decode %2F, and crashes the whole command
- Dominant language
- TypeScript
- Stars
- 450
- Forks
- 95
- Avg merge
- 1d 2h
- Merged PRs (30d)
- 23
Description
`GrantChecker` (`src/grants/GrantCheck.ts`) caches a successful per-user access check by writing Matrix room account data with an event **type** containing literal `/` characters:
```ts
const GRANT_ACCOUNT_DATA_KEY = "uk.half-shot.matrix-hookshot.grant";
private getKey(connectionIdStr: string): string {
return `${GRANT_ACCOUNT_DATA_KEY}/${this.grantType}/${connectionIdStr}`.toLowerCase();
}
```
e.g. `uk.half-shot.matrix-hookshot.grant/github/`. `matrix-bot-sdk`'s `setRoomAccountData` correctly `encodeURIComponent`s this before sending, so the outbound request is spec-compliant, but any intermediary between hookshot and the homeserver that decodes `%2F` back into a literal `/` before the homeserver's own router sees it will turn one path segment into several, and the homeserver 404s (`M_UNRECOGNIZED` on Synapse) because no route matches the mangled path.
This is the default, non-configurable behavior of at least one major hosting platform (Azure Container Apps: [microsoft/azure-container-apps#1427](https://github.com/microsoft/azure-container-apps/issues/1427), confirmed by Microsoft as deliberate and with no way to disable it), and similar `%2F`-normalizing behavior is common across other gateways/reverse proxies (Envoy, various API gateways) for the same "URL confusion attack" defense-in-depth reasoning. Any hookshot deployment sitting behind such a proxy, even for purely internal, same-network traffic to its own homeserver, will hit this.
Confirmed live in our own deployment: **every** `!hookshot github repo add` (and any connector using `GrantChecker`) fails with a generic `Failed to handle command.`, even though the actual authorization check (does this user have access to this repo?) succeeds first. The failure is entirely in the follow-up "cache this for next time" write:
```ts
public async grantConnection(roomId: string, connectionId: cId) {
const cidStr = GrantChecker.stringifyConnectionId(connectionId);
log.info(`Granting ${roomId}/${cidStr}`);
await this.intent.underlyingClient.setRoomAccountData(
this.getKey(cidStr), roomId, { granted: true } as GrantContent,
);
}
```
This call isn't wrapped in a try/catch anywhere between here and the top-level command handler, so any failure here aborts the entire provisioning operation, even though the authorization decision already succeeded.
## Reproduction
Confirmed live against a real deployment (Synapse + hookshot 7.4.4, Azure Container Apps): the identical `setRoomAccountData` call succeeds (200) with a slash-free type and fails (404 `M_UNRECOGNIZED`) only when the type contains `%2F`, isolated by testing directly against Synapse's own API from inside the hookshot container, bypassing hookshot's code entirely, so this is squarely a URL-encoding-survives-the-network-path issue, not anything specific to hookshot's HTTP client.
Contributor guide
Research direction
Start in src/grants/GrantCheck.ts, reading getKey and grantConnection, then reproduce setRoomAccountData with slash-containing and slash-free event types through the affected proxy path. Done means the authorization check and subsequent provisioning command complete successfully when the account-data type contains a connection identifier.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- authorization, backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 68/100