Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prevent the Logger from writing sensitive values #143

Merged
merged 3 commits into from
Aug 23, 2022

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Jul 28, 2022

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Comment on lines +33 to +34
use Office365\Runtime\Auth\AuthenticationContext;
use Office365\Runtime\Auth\SamlTokenProvider;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing both paths being in the same namespace, does it make sense to allow giving class names only or even namespaces only?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both might be too broad in other cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that's why I meant "Allow", but yeah then let's keep it like this.
I just fear the moment they change an internal method name or something and it logs again

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that's why I meant "Allow", but yeah then let's keep it like this. I just fear the moment they change an internal method name or something and it logs again

Absolutely. But that can happen to namespaces as well as class names (also happened in this lib in the past).

The only sustainable solution will arrive with PHP 8.2's sensitive parameter support.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickvergessen added an integrattion test that should act as safeguard nevertheless

@blizzz blizzz force-pushed the fix/142/dont-log-sensitive-args branch 3 times, most recently from 52a3857 to a99907b Compare July 29, 2022 12:34
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the fix/142/dont-log-sensitive-args branch from c9241f1 to aaa0b21 Compare July 29, 2022 13:28
@blizzz
Copy link
Member Author

blizzz commented Jul 29, 2022

For the record: integration tests succeeds locally against the enh/noid/sensitive-methods-apps branch of the server.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@PVince81
Copy link
Member

PVince81 commented Aug 5, 2022

server PR merged, please rebase/adjust

@blizzz
Copy link
Member Author

blizzz commented Aug 23, 2022

server PR merged, please rebase/adjust

rerun failing tests against current master. All green now → merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants