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

[rfc] Making HTTP::Cookie fully? comply with RFC 6265. #5033

Closed
bmmcginty opened this issue Sep 24, 2017 · 4 comments · Fixed by #10564
Closed

[rfc] Making HTTP::Cookie fully? comply with RFC 6265. #5033

bmmcginty opened this issue Sep 24, 2017 · 4 comments · Fixed by #10564

Comments

@bmmcginty
Copy link
Contributor

bmmcginty commented Sep 24, 2017

Edited for correctness.
It is possible the below isn't needed, if we aren't trying to support Internet Explorer as a client to a Crystal server.

Looking at HTTP::cookie (in the latest Crystal revision), we aren't complying with RFC 6265 as regards max-age and expires.
See:
https://tools.ietf.org/html/rfc6265#section-4.1.2.2

https://tools.ietf.org/html/rfc6265#section-5.2.2

  1. According to these, we should be noting a cookie as expired when the max-age header <=0 (assuming we aren't attempting to emulate Internet Explorer). This _is _done currently, but it discards the expires header.
  2. We should be storing max-age as a separate value, and should be able to determine it's actual time based on the creation time of the cookie.
  3. The max_age and expires values should both be modifiable so that exact cookies can be created as desired.

I've got a PR I'm working on to do the following:

  1. add creation_time to each cookie, set to when #initialize is run.
  2. change #expires to return the expires header value.
  3. make max_age a Time::Span
  4. generate string cookies using max-age and expires if they are defined.

Issues:

  1. #expires now returns the expires value, not the actual expiration time (which can be dependent on max-age if provided). I don't have a suitable name to replace this method with. In other words, I need a method name for a method to give the calculated expiration time for a cookie.

Thoughts would be appreciated.

@asterite
Copy link
Member

Maybe we can use expiration_time to return the Time. Other than that, your PR will be very welcome 👍

@watzon
Copy link
Contributor

watzon commented Jun 26, 2019

I was going to make a separate issue, but seeing as this is here already I'll just add to it. It seems as though Cookie#parse_set_cookie currently tries to set max_age to an Int32 which breaks with higher numbers. I attempted to parse a cookie earlier and got the following stack trace:

Unhandled exception: Invalid Int32: 9999999999 (ArgumentError)
  from /home/watzon/.asdf/installs/crystal/0.29.0/share/crystal/src/string.cr:411:83 in 'to_i32'
  from /home/watzon/.asdf/installs/crystal/0.29.0/share/crystal/src/string.cr:322:5 in 'to_i'
  from /home/watzon/.asdf/installs/crystal/0.29.0/share/crystal/src/http/cookie.cr:104:32 in 'parse_set_cookie'
  from /home/watzon/.asdf/installs/crystal/0.29.0/share/crystal/src/http/cookie.cr:153:11 in 'fill_from_headers'
  from /home/watzon/.asdf/installs/crystal/0.29.0/share/crystal/src/http/cookie.cr:139:27 in 'from_headers'
  from lib/halite/src/halite/client.cr:268:7 in 'store_cookies_from_response'
  from lib/halite/src/halite/client.cr:262:7 in 'handle_response'
  from lib/halite/src/halite/client.cr:251:7 in 'handle_response'
  from lib/halite/src/halite/client.cr:153:7 in 'perform'
  from lib/halite/src/halite/client.cr:109:9 in '->'
  from lib/halite/src/halite/client.cr:255:3 in 'perform_chain'
  from lib/halite/src/halite/client.cr:108:7 in 'request'
  from lib/halite/src/halite/chainable.cr:447:7 in 'request:headers:params:raw:tls'
  from lib/halite/src/halite/chainable.cr:5:5 in 'get:headers'
  from src/arachnid/agent.cr:323:34 in 'visit_page'
  from src/arachnid/agent.cr:205:11 in 'run'
  from src/arachnid/agent.cr:176:14 in 'start_at'
  from src/arachnid/agent.cr:134:7 in 'start_at'
  from src/arachnid/arachnid.cr:27:5 in 'start_at'
  from src/arachnid.cr:7:1 in '__crystal_main'
  from /home/watzon/.asdf/installs/crystal/0.29.0/share/crystal/src/crystal/main.cr:97:5 in 'main_user_code'
  from /home/watzon/.asdf/installs/crystal/0.29.0/share/crystal/src/crystal/main.cr:86:7 in 'main'
  from /home/watzon/.asdf/installs/crystal/0.29.0/share/crystal/src/crystal/main.cr:106:3 in 'main'
  from __libc_start_main
  from _start
  from ???

@RX14
Copy link
Contributor

RX14 commented Jul 9, 2019

@watzon this is a separate issue, please make a new issue or PR. Numbers greater than Int32::MAX can be truncated in practice, since recording values of more than 68 years in this field is unneccesary.

@watzon
Copy link
Contributor

watzon commented Jul 9, 2019

@RX14 I already opened a PR #7925 which should fix both issues

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

Successfully merging a pull request may close this issue.

4 participants