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

Open
#1,372 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.