-
Notifications
You must be signed in to change notification settings - Fork 15k
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
Do not capture cookies and credentials in net log #13065
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. How did this regress?!
@nornagon Currently, all the net logs we get displays explicit cookie information which requires additional stripping, which wasn't the case with Electron < Ref on capture mode: https://cs.chromium.org/chromium/src/net/log/net_log_capture_mode.h?type=cs&g=0 |
Ah yeah, I get what's going on, but I'm wondering why it changed with v2. It looks like the code was previously set to log cookies & credentials. |
@sethlu I'm also not sure how this was a regression, can you explain how it changed in 2.0.0 it looks like we were explicitly asking for cookies and creds before 🤔 |
Agreed that the behavior changed with CH61 upgrade in 0ad967c#diff-6c198fc87cc0c1f184c47aacd927fabd , before that we were using the default capture mode where private data was stripped out. Thanks for the change! |
Ah I see, thanks @deepak1556 |
/trop run backport |
The backport process for this PR has been manually initiated, here we go! :D |
We have automatically backported this PR to "2-0-x", please check out #13537 |
An error occurred while attempting to backport this PR to "3-0-x", you will need to perform this backport manually |
@sethlu Can you please manually backport the change to the |
@alexeykuzmin Thanks for letting me know! I think this was merged into master before |
@alexeykuzmin @sethlu is right. It was included in the 3.0.0-beta.1 release: https://github.com/electron/electron/releases/tag/v3.0.0-beta.1 |
@sethlu Oh, you're right. I should've checked the merge commit date. |
Should fix a regression issue since v2.0.0 that began logging cookies and credentials in the net log dump.
Example before:
Example after (with
[64 bytes were stripped]
):