Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

We should redact the Application Service access tokens from DEBUG logging produced by synapse.http.proxyagent #13010

Closed
reivilibre opened this issue Jun 9, 2022 · 3 comments · Fixed by #13855
Closed

We should redact the Application Service access tokens from DEBUG logging produced by synapse.http.proxyagent #13010

reivilibre opened this issue Jun 9, 2022 · 3 comments · Fixed by #13855
Labels
A-Application-Service Related to AS support S-Minor Blocks non-critical functionality, workarounds exist. T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements.

Comments

@reivilibre
Copy link
Contributor

The Application Services spec requires an access_token URL parameter for authorisation.

Setting a DEBUG log level means that synapse.http.proxyagent will log these access tokens in the URLs that it logs (possibly only if a HTTP proxy is in use?).

It would be nice if they were redacted so that it's one less thing to trip homeserver admins up on when sharing debug logs for diagnosing issues.
I think we do something similar for incoming Client-Server API requests already.
@reivilibre reivilibre added S-Minor Blocks non-critical functionality, workarounds exist. T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. labels Jun 9, 2022
@richvdh
Copy link
Member

richvdh commented Jun 9, 2022

or we should just stop putting access tokens in query params, because that is a terrible idea.

matrix-org/matrix-spec#923

@richvdh
Copy link
Member

richvdh commented Jun 9, 2022

(if it wasn't for the forthcoming OAuth2 work, I'd already have gone postal and ripped out support for access tokens in query-params in C-S APIs. I'm not keen on adding more fudges to work around this particular bit of idiocy.)

@reivilibre
Copy link
Contributor Author

Ooh yes there's an MSC: matrix-org/matrix-spec-proposals#2832

@MadLittleMods MadLittleMods added the A-Application-Service Related to AS support label Jun 9, 2022
@clokep clokep mentioned this issue Sep 20, 2022
Merged
4 tasks
MadLittleMods added a commit that referenced this issue Sep 23, 2022
@MadLittleMods
Fix access token leak to logs from proxyagent (#13855)
db868db 
This can happen specifically with an application service `/transactions/10722?access_token=leaked` request

Fix #13010

---

Saw an example leak in #13423 (comment)

```
2022-08-04 14:47:57,925 - synapse.http.client - 401 - DEBUG - as-sender-signal-1 - Sending request PUT http://localhost:29328/transactions/10722?access_token=<redacted>
2022-08-04 14:47:57,926 - synapse.http.proxyagent - 223 - DEBUG - as-sender-signal-1 - Requesting b'http://localhost:29328/transactions/10722?access_token=leaked' via <HostnameEndpoint localhost:29328>
```
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
A-Application-Service Related to AS support S-Minor Blocks non-critical functionality, workarounds exist. T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix access token leak to logs from proxyagent matrix-org/synapse
3 participants
@MadLittleMods @richvdh @reivilibre