search

pigskin / kodi-gamepass

NFL Game Pass add-on for Kodi
Other
123 stars 83 forks source link

Debug Setting #402

Closed OnceUponALoop closed 2 years ago

OnceUponALoop commented 5 years ago

The addon doesn't set a debug setting but queries for it. This was previously mitigated by passing debug=true to pigskin but that was removed in 4981804cd53e3d645d15e3c4968c003e1dc42613

New logging was imported from generator-kodi-addon in commit 2237c868b1d27b090e5ac1042d4cfece306d5868 and 4981804cd53e3d645d15e3c4968c003e1dc42613 but didn't include the additional setting in settings.xml.

The resultant error is

DEBUG: CSettingsManager: requested setting (debug) was not found.

I believe this could be fixed by just adding the setting and strings removed in 52897b9bad526b8fe6655b99483b545ab4f1beb2

took a minute to wrap my head around it so i'm not sure about the conclusion, can someone verify that my assumptions are correct? I'll submit a pull request if it's indeed the case.

aqw commented 5 years ago

@OnceUponALoop Ahh; thanks for catching this. I did not realize that generator-kodi-addon assumed the presence of a debug setting. Thanks for catching that.

A PR would be most welcome.

But, with one caveat. make_request() is now used by only a handful of functions. And the password filtering for logging was done in make_request(). _log_request() is where the logging is done now, so that should gain scrubbing abilities (preferably optional). However... as I've been working on pigskin, I have found /a lot/ of personal data returned by the Game Pass servers. It would be good if we scrubbed for username, password, and profile information (see https://github.com/aqw/pigskin/blob/master/tests/conftest.py).

There's a lot in flux right now, so thanks for helping to clean up some of the fallout. :-)

---Alex

OnceUponALoop commented 5 years ago

We can probably take this a step further and update the logging to utilize the levels. Set the log levels throughout to error, warning, info where applicable and leave the detailed request/response dumps in loglevel debug.

This should limit the exposure of sensitive info and the setting could have a disclaimer indicating possible data leakage if loglevel is set to debug.

aqw commented 5 years ago

We can probably take this a step further and update the logging to utilize the levels. Set the log levels throughout to error, warning, info where applicable and leave the detailed request/response dumps in loglevel debug.

That sounds perfect.

the setting could have a disclaimer indicating possible data leakage if loglevel is set to debug.

Hmm... the thing is, the Game Pass service doesn't always behave the same way everywhere, so user supplied debug logs are the only effective way for users to share what the heck is going on with their environment. And with debugging enabled, we will log (through the GP responses) the user's full name, date of birth, country, username, and password.

The users are going to need an easy way to give us a complete picture of their environment without setting themselves up for identity theft or doxxing.

jm-duke commented 2 years ago

Doing some clean-up and this issue report is really old, so I'm closing it.