Session cookies are using an unsafe expiry max value

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: whimboo, Assigned: nicole_byer, Mentored)

Tracking

({good-first-bug})

57 Branch
Firefox 58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Based on the discussion on bug 1408962 comment 8:

(In reply to Tim Taubert [:ttaubert] from comment #8)
> (In reply to Henrik Skupin (:whimboo) from comment #4)
> > Interesting fact Peter! Here is the exact description:
> > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> > Global_Objects/Number/MAX_SAFE_INTEGER
> > 
> > Which makes me wonder why we make use of the following line of code:
> > https://dxr.mozilla.org/mozilla-central/source/browser/components/
> > sessionstore/SessionCookies.jsm#20-21
> > 
> > Tim, you added that code via bug 903398. Can you please have a look if we
> > currently have faulty behavior here?
> 
> Looks like we should probably switch to Number.MAX_SAFE_INTEGER for that
> const. I copied that line from the cookie code in sessionstore that has been
> around since Firefox 2. I'm guessing that the author meant to actually use
> 2^53.

Mike, can someone please get this fixed? Not sure how critical that is, but maybe it might be a good mentored bug?
Flags: needinfo?(mdeboer)
OK, sure. I also read "[...]the integer 2^53 + 1 can't be directly represented in IEEE-754 but instead rounds to 2^53 under round-to-nearest and round-to-zero rounding."[1]; so I think when this value is normalized, it'll also be rounded to 2^53 using that same principle. Is there still an issue, then?

Regardless, I think it'd be best to use `Number.MAX_SAFE_INTEGER` of course.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isSafeInteger
Mentor: mdeboer
Flags: needinfo?(mdeboer)
Keywords: good-first-bug

Comment 2

2 years ago
Hi, I’m a recent CS graduate and would like to start contributing. Would I be able to take care of this?
Thanks!
Nicole Byer
Hi Nicole, thanks for reaching out and wanting to help out!

You can follow the steps outlined in this document: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction and get a working build of Firefox. This time you'll be working in frontend Javascript code, so you can use the faster artifact mode to build Firefox.

Then you'll want to open the file browser/components/sessionstore/SessionCookies.jsm in your favorite code editor and change the lines at https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionCookies.jsm#20-21.

Specifically, you'll need to change the Max.pow() statement to use Number.MAX_SAFE_INTEGER instead.

If you've got any further questions, please feel free to reach out to me (or others) here or on IRC (channels #introduction and #developers).
Priority: -- → P3
(Assignee)

Comment 4

2 years ago
Hi Mike,
I got a working build of Firefox and changed the line of code (I'll attach the file). What is the next step? Do I need to test this?
Thanks for your help!
Nicole
(Assignee)

Comment 5

2 years ago
Posted patch SessionCookies.jsm (obsolete) — Splinter Review
Lines 20-21 changed to use Number.MAX_SAFE_INTEGER
Hi Nicole, great that you've been able to get a build running! Next step is to submit a patch for review. You can read all about that here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

This is step 4 in the Contributing tutorial at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction.
(Assignee)

Comment 7

2 years ago
Ok, great! Are you the reviewer? (I also see ttaubert when I run hg annotate). Thanks!
Yeah, I'm the module owner, so I can review your patch.
(Assignee)

Comment 9

2 years ago
Posted patch SessionCookies.jsm (obsolete) — Splinter Review
Changed lines 20-21 to use Number.MAX_SAFE_INTEGER
Attachment #8921457 - Attachment is obsolete: true
Attachment #8921478 - Flags: review?(mdeboer)
Comment on attachment 8921478 [details] [diff] [review]
SessionCookies.jsm

Nicole, the file you attached is not a patch, but the whole SessionCookies.jsm file, which is something we can't really review.
We always review sets of changes (i.e. a patch), which is quite a bit smaller and doesn't require you to point out what you've changed and where you changed something. Instead you can focus on explaining why you made the change in a commit message.

I really recommend you to give https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch a thorough read-through to see how you can generate a patch.
Attachment #8921478 - Flags: review?(mdeboer)
(Assignee)

Comment 11

2 years ago
Hi,
Sorry about that; I hope this is more along the right lines!
Attachment #8921478 - Attachment is obsolete: true
Attachment #8921544 - Flags: review?(mdeboer)
Comment on attachment 8921544 [details] [diff] [review]
Proposed patch

Review of attachment 8921544 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect! Thanks for this :)
Attachment #8921544 - Flags: review?(mdeboer) → review+
Assignee: nobody → nicole_byer
Status: NEW → ASSIGNED
(Assignee)

Comment 13

2 years ago
Great, thanks for your help!

Comment 14

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44a2659e778a
Changed use of 2^62 to  Number.MAX_SAFE_INTEGER (represents 2^53 - 1) to avoid faulty behavior. r=mikedeboer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/44a2659e778a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.