Closed Bug 1470034 Opened 2 years ago Closed 2 years ago

Sync shouldn't log x-client-state or hawk auth headers

Categories

(Firefox :: Sync, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: markh, Assigned: pavanon9, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(2 files)

Even at trace level, these values should never be logged as it may expose our users to harm if the logs are made public.
We should also suppress:

* The "key" value in the JSON response from https://token.services.mozilla.com/1.0/sync/1.5
* The "pushPublicKey" and "pushAuthKey" values in the JSON response from https://api.accounts.firefox.com/v1/account/devices
Do we log these? I thought you had to set `services.common.hawk.log.sensitive` and `identity.fxaccounts.log.sensitive` to log them
Priority: -- → P2
Mentor: markh
Keywords: good-first-bug
Attached patch Suggested fix v1Splinter Review
Hi,
I'm interested in taking this up as my first contribution.

I've attached what could be the fix for suppressing headers part.
Can you please let me know if this is the right way to approach this.
Flags: needinfo?(markh)
(In reply to Pavan Veginati from comment #3)
> Created attachment 8988548 [details] [diff] [review]
> Suggested fix v1
> 
> Hi,
> I'm interested in taking this up as my first contribution.
> 
> I've attached what could be the fix for suppressing headers part.
> Can you please let me know if this is the right way to approach this.

Yeah, I think that looks great, thanks.

Addressing comment 1 is going to be trickier, but I think it would be more robust to avoid logging response bodies from rest.js completely - ie, I think that if you move:

https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/services/common/rest.js#445-448

to line 439 (ie, so we only log the response body on a failed response), and add a comment at line 445 along the lines of "// Note that for privacy/security reasons we don't log this response body", I think you've covered all the cases.

Thanks!
Assignee: nobody → pavanon9
Status: NEW → ASSIGNED
Flags: needinfo?(markh)
Hi, I submitted a review request based on suggested changes. 

Both x-client-state and hawk auth headers including JSON response from https://token.services.mozilla.com/1.0/sync/1.5 are not logged anymore.

However if we set 'services.common.hawk.log.sensitive', "pushPublicKey" and "pushAuthKey" will still be logged from here:

https://searchfox.org/mozilla-central/source/services/common/hawkclient.js#231
(In reply to Pavan Veginati from comment #6)
> Hi, I submitted a review request based on suggested changes. 

Awesome, thanks! For future reference, if you add "r?reviewer" (eg, "r?markh" in this case), then your chosen reviewer will get notified of the review request (and if you choose wrong, the person you chose can just pass it on to someone else). No bug deal though - I just pushed the change to the "try server" and if it passes (which I suspect it will), I'll r+ it and land it.

> However if we set 'services.common.hawk.log.sensitive', "pushPublicKey" and
> "pushAuthKey" will still be logged from here:

I think that's fine - it seems unlikely someone will accidentally set such a pref and there are some other sensitive things already logged when that pref is set - if we decide to do something about that, I think we should do it in a different bug.

Thanks!
Comment on attachment 8988852 [details]
Bug 1470034 - Remove x-client-state and hawk auth header trace logs.

https://reviewboard.mozilla.org/r/254010/#review260910

Looks great, thanks!
Attachment #8988852 - Flags: review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/67d9f01cbbec
Remove x-client-state and hawk auth header trace logs. r=markh
https://hg.mozilla.org/mozilla-central/rev/67d9f01cbbec
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.