Opened 11 years ago

Closed 11 years ago

#9822 closed defect (fixed)

Sage only examines the last cookie for a username

Reported by: timdumol Owned by: jason, was
Priority: critical Milestone: sage-4.6
Component: notebook Keywords:
Cc: jason, kcrisman, mpatel, boothby Merged in: sagenb-0.8.3
Authors: Jason Grout Reviewers: Tim Dumol
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Attachments (6)

trac_9822-cookie-path-fix.patch (4.3 KB) - added by timdumol 11 years ago.
SageNB. Attempted fix for Safari (setting cookie path to '/')
trac_9822-cookie-fix.patch (5.4 KB) - added by timdumol 11 years ago.
Replaces all others. Includes Jason Grout's fix ( http://groups.google.com/group/sage-notebook/browse_thread/thread/dbe2e0a64e9ccc38).
trac_9822-cookie-fix.2.patch (6.1 KB) - added by timdumol 11 years ago.
Gets the port number from the HTTP header instead (this fixes the issue given by Jason)
trac_9822-cookie-fix.3.patch (6.1 KB) - added by timdumol 11 years ago.
Makes the default port 443 if the connection is HTTPS.
trac_9822-cookie-fix.4.patch (6.1 KB) - added by timdumol 11 years ago.
Fixes a bug.
9822-multiple-cookies.patch (1.3 KB) - added by jason 11 years ago.
apply instead of previous patches

Download all attachments as: .zip

Change History (34)

Changed 11 years ago by timdumol

SageNB. Attempted fix for Safari (setting cookie path to '/')

comment:1 Changed 11 years ago by timdumol

  • Cc kcrisman added
  • Status changed from new to needs_review

I have no Safari (on Linux), so it would be helpful if someone with Safari could test if this fixes the issue.

comment:2 Changed 11 years ago by kcrisman

I'd like to test it, but have no way to apply it to a server where this is a problem for me :(

comment:3 Changed 11 years ago by jason

I'll test it. I had 3-4 students today who complained about the issue.

comment:4 follow-up: Changed 11 years ago by jason

Let me say again that it would be *really* nice if the sagenb repository was located somewhere in the sage repository, so that it was easy to apply patches and make fixes (without having to hunt down the spkg, read the instructions again, untar it, etc). Maybe the sage spkg could by default do the equivalent of extracting in sage/devel/sagenb/ and doing the developer install (so the source and repository would be in sage/devel/sagenb/

comment:5 in reply to: ↑ 4 Changed 11 years ago by kcrisman

Thanks for testing! I hope to start using it a little on the side next week.

Let me say again that it would be *really* nice if the sagenb repository was located somewhere in the sage repository, so that it was easy to apply patches and make fixes (without having to hunt down the spkg, read the instructions again, untar it, etc). Maybe the sage spkg could by default do the equivalent of extracting in sage/devel/sagenb/ and doing the developer install (so the source and repository would be in sage/devel/sagenb/

I have asked about this a lot too, though of course I also haven't stepped up and tried to figure out how to use python setup or whatever to create hg_sagenb or something. Same with Pynac - it's just dumb that these things which (currently) are only used in Sage can't be easily modified.

comment:6 Changed 11 years ago by mpatel

Changed 11 years ago by timdumol

comment:7 Changed 11 years ago by timdumol

  • Authors set to Jason Grout, Tim Dumol

comment:8 follow-up: Changed 11 years ago by jason

Setting the port for the cookie worries me. How does that play with using a proxy server (where Sage thinks it is running on a certain port, like 8000, but the web browser thinks it is running on port 80)?

Changed 11 years ago by timdumol

Gets the port number from the HTTP header instead (this fixes the issue given by Jason)

comment:9 in reply to: ↑ 8 Changed 11 years ago by timdumol

Replying to jason:

Setting the port for the cookie worries me. How does that play with using a proxy server (where Sage thinks it is running on a certain port, like 8000, but the web browser thinks it is running on port 80)?

Good point. I've added a new patch that fixes that issue.

comment:10 Changed 11 years ago by jason

What if the connection is over https? Can you assume that if you don't see a port number, then the port number is 80? Why can't we just not set the port number (like before)? (Disclaimer: I don't know much about this, so if someone that does thinks everything is all right, well, okay).

Also, I notice you use hasHeader. Should my part of the patch be changed to have:

if request.headers.hasHeader('cookie'):

Changed 11 years ago by timdumol

Makes the default port 443 if the connection is HTTPS.

comment:11 Changed 11 years ago by timdumol

It is insecure to let any site under the domain to access the cookie (cross-site scripting). I've made the port 443 if the notebook is secure. It also poses a problem if (in the admittedly rare case) the user decides to forward several ports to one notebook server.

getHeader() returns None if the header is not found, so it works either way.

Changed 11 years ago by timdumol

Fixes a bug.

comment:12 Changed 11 years ago by timdumol

Bug fixed in latest patch.

comment:13 Changed 11 years ago by jason

  • Cc mpatel boothby added
  • Priority changed from major to critical

The cookie issue preventing logins makes this critical, I think. I'm confident about that part of this patch. I'm not confident about the other stuff done on cookies in this patch (just because I'm not familiar with them very much anymore).

Mitesh or Tom: you are some of the resident web experts. Can you look at this patch?

comment:14 follow-up: Changed 11 years ago by jason

  • Status changed from needs_review to needs_work

I installed this on my server (4.5.2) where I have apache forwarding port 80 (outside) to port 8000 (the local sage server). On logging in, I get a browser message: "Please enable cookies or delete all Sage cookies and localhost cookies in your browser and try again." In Firebug, I see I have two cookies: cookie_test_80, and nb_session_8000. That looks wrong, doesn't it?

When I delete all of my cookies from that server, I still can't log in (same error). After the error page comes up, and I click "Continue", I see the cookie_test_80 cookie show up in FireCookies?.

Can we split this ticket into two tickets? A very high priority one that fixes the bug I found (my fix has been working for the past couple of months for me), and another ticket which takes care of the other issues? That way the checking-only-last-cookie error can be taken care of right away.

comment:15 Changed 11 years ago by jason

With just my part of the patch, I see a cookie_test_8000 and a nb_session_8000 cookie. So apparently the problem is that after the patch above, we have a cookie_test_80 cookie.

comment:16 in reply to: ↑ 14 Changed 11 years ago by kcrisman

Can we split this ticket into two tickets? A very high priority one that fixes the bug I found (my fix has been working for the past couple of months for me), and another ticket which takes care of the other issues? That way the checking-only-last-cookie error can be taken care of right away.

Yes, please! I've been avoiding using Sage this semester (other than for me personally) some because the cookie issue just seems to have gotten worse...

comment:17 Changed 11 years ago by jason

I've split Tim's changes to cookies out to #10029, since they don't seem to be quite ready yet.

Changed 11 years ago by jason

apply instead of previous patches

comment:18 follow-up: Changed 11 years ago by jason

  • Status changed from needs_work to needs_review

Apply only 9822-multiple-cookies.patch. The rest of the work on this ticket has been moved to #10029.

comment:19 Changed 11 years ago by jason

  • Authors changed from Jason Grout, Tim Dumol to Jason Grout

comment:20 Changed 11 years ago by jason

  • Summary changed from Cookies are still causing problems in SageNB (Safari) to Sage only examines the last cookie for a username

comment:21 follow-up: Changed 11 years ago by mpatel

I think we should get this and the currently positively reviewed SageNB tickets at {32} into a new SageNB 0.8.3.

But I can't work on this now, as I'm a bit busy with getting 4.6.alpha2 ready to release and working on #3524. That will put the 4.6 cycle in feature freeze, so it's likely that the new Cython, NumPy, Pynac, SageNB, and SciPy packages will have to wait for 4.6.1.

comment:22 follow-up: Changed 11 years ago by kcrisman

But I can't work on this now, as I'm a bit busy with getting 4.6.alpha2 ready to release and working on #3524. That will put the 4.6 cycle in feature freeze, so it's likely that the new Cython, NumPy, Pynac, SageNB, and SciPy packages will have to wait for 4.6.1.

Not to reopen the whole numbering issue, but it seems weird that major upgrades to big pieces of Sage would be 4.6.1 - and possibly wait weeks. Is it possible to keep the 4.6 cycle going, or would it make more sense to wait on all these? I realize you're doing the work - just a query. (Partly this is the fear that 4.6.1 might not happen for a while, since I expect you to take a break after this release! You certainly deserve it.)

comment:23 in reply to: ↑ 21 Changed 11 years ago by mpatel

Replying to mpatel:

I think we should get this and the currently positively reviewed SageNB tickets at {32} into a new SageNB 0.8.3.

I've opened #10036 for this.

comment:24 in reply to: ↑ 22 Changed 11 years ago by mpatel

Replying to kcrisman:

But I can't work on this now, as I'm a bit busy with getting 4.6.alpha2 ready to release and working on #3524. That will put the 4.6 cycle in feature freeze, so it's likely that the new Cython, NumPy, Pynac, SageNB, and SciPy packages will have to wait for 4.6.1.

Not to reopen the whole numbering issue, but it seems weird that major upgrades to big pieces of Sage would be 4.6.1 - and possibly wait weeks. Is it possible to keep the 4.6 cycle going, or would it make more sense to wait on all these? I realize you're doing the work - just a query. (Partly this is the fear that 4.6.1 might not happen for a while, since I expect you to take a break after this release! You certainly deserve it.)

Thanks! I may well take a break after 4.6 is out.

I'll try to reply soon on sage-devel about the possibility of waiting longer for 4.6.alpha2 or adding an alpha3.

Right now, I need to investigate a potential problem with current trial alpha2. The current merge script is here.

comment:25 in reply to: ↑ 18 Changed 11 years ago by mpatel

Replying to jason:

Apply only 9822-multiple-cookies.patch. The rest of the work on this ticket has been moved to #10029.

Tim, does this patch look good to you?

comment:26 Changed 11 years ago by timdumol

  • Status changed from needs_review to positive_review

Looks good to me.

comment:27 Changed 11 years ago by mpatel

  • Reviewers set to Tim Dumol

comment:28 Changed 11 years ago by mpatel

  • Merged in set to sagenb-0.8.3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.