Don't log bot and voice tokens in Gateway implementations (trace level) #671
Conversation
|
Should we do this for voice gateway too @lost-illusi0n? |
|
I'm not against it. Might as well be consistent across the entirety of Kord. |
|
Alright, just asking cause I don't know how sensitive voice tokens are. |
|
Probably sensitive enough, it is a token after all. I believe someone with access to it would be able to hijack the voice connection for a server (within a certain timeframe), though I haven't tried it :p |
| defaultGatewayLogger.trace { | ||
| val credentialFreeJson = when (event) { | ||
|
|
||
| is VoiceServerUpdate -> { | ||
| when (val payload = GatewayJson.parseToJsonElement(json)) { | ||
| is JsonObject -> { | ||
| val payloadCopy = buildJsonObject { | ||
| for ((k, v) in payload) put(k, v) | ||
|
|
||
| val data = payload["d"] | ||
| if (data is JsonObject) putJsonObject("d") { | ||
| for ((k, v) in data) put(k, v) | ||
| put("token", "hunter2") | ||
| } | ||
| } | ||
| payloadCopy.toString() | ||
| } | ||
| else -> json | ||
| } | ||
| } | ||
|
|
||
| else -> json | ||
| } | ||
|
|
||
| "Gateway <<< $credentialFreeJson" | ||
| } |
There was a problem hiding this comment.
This is quite heavy for just not logging the token, voice does it like this to hide secret_key, should we do it like that too instead?
lukellmann
changed the title
|
Other token types this PR didn't touch yet are interaction and webhook tokens, what about them? |
I think that, for consistency, it would be nice to hide them too, because they are sensitive too, because with interaction/webhook tokens you can send whatever message you want. By the way, wouldn't it be better to filter the sensitive data on the logger end, instead of replacing all entities with a custom |
|
Hm, how would we filter it on the logger end without a logger implementation (kord doesn't provide one)? |
Yeah, that's one of the issues with this approach: The user would need to configure their logger implementation (logback, log4j, etc) to filter tokens |
|
Not quite, Kord depends on kotlin-logging for logging, which simply wraps around the SLF4J api. I'm aware of the log4j api having an AbstractLogger class that can be implemented, which would then strip out the token(s) from any outgoing log message, and delegate it to the actual logger. I assume this can similarly be done with the slf4j abstract logger. Might be worth taking a look at. |
lukellmann
force-pushed
the
0.8.x
branch
2 times, most recently
from
a1bb076
to
1e6718a
Compare
lukellmann
changed the title
|
Mentioned trace level in title to not scare people. |
lukellmann
force-pushed
the
fix/resume-token-leak
branch
2 times, most recently
from
abe99f9
to
1214b95
Compare
lukellmann
force-pushed
the
fix/resume-token-leak
branch
from
1214b95
to
064ec37
Compare
DefaultGatewaylogs the JSON payloads it sends when trace logging is enabled. It also hides the bot token in case of anIdentifycommand. But it didn't do this forResumecommands, which also includes the bot token.Same goes for
DefaultVoiceGateway, which didn't hide the voice token forResumecommands.This PR fixes this and also updates some
toStringimplementations of data classes that contain a bot or voice token.DefaultVoiceGatewayalso hidessecret_keyforSessionDescriptionevents, this is also added for voice tokens inVoiceServerUpdateevents thatDefaultGatewayreceives.The PR also makes the
Jsoninstance used byDefaultGatewayandDefaultVoiceGatewaya property of the companion object since it can be shared safely.