Skip to content

Use RFC 6265 cookies from net-cookies #21

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 9, 2017
Merged

Use RFC 6265 cookies from net-cookies #21

merged 2 commits into from
Mar 9, 2017
+623 −325

Conversation

@LiberalArtist
Copy link
Contributor

@LiberalArtist LiberalArtist commented Mar 8, 2017

Modifies web-server/http/cookie-parse, web-server/http/cookie,
and web-server/http/id-cookie to use net/cookies from the net-cookies
package, rather than the deprecated net/cookie.

resolves #20

@LiberalArtist
Use RFC 6265 cookies from net-cookies
7974d4d 
Modifies web-server/http/cookie-parse, web-server/http/cookie,
and web-server/http/id-cookie to use net/cookies from the net-cookies
package, rather than the deprecated net/cookie.
jeapostrophe
jeapostrophe reviewed Mar 8, 2017
View reviewed changes
web-server-doc/web-server/scribblings/http.scrbl Outdated
Constructs a cookie with the appropriate fields.

This is a wrapper around @racket[make-cookie] from @racketmodname[net/cookies/server]
for backwords compatability. The @racket[comment] argument is ignored.
Copy link
Contributor

@jeapostrophe jeapostrophe Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

backwards

jeapostrophe
jeapostrophe reviewed Mar 8, 2017
View reviewed changes
web-server-doc/web-server/scribblings/http.scrbl
for backwords compatability. The @racket[comment] argument is ignored.
If @racket[expires] is given as a string, it should match
@link["https://tools.ietf.org/html/rfc7231#section-7.1.1.2"]{RFC 7231, Section 7.1.1.2},
in which case it will be converted to a @racket[date?] value.
Copy link
Contributor

@jeapostrophe jeapostrophe Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean it will fail if it does not? Does net/cookies enforce a contract?

Copy link
Contributor Author

@LiberalArtist LiberalArtist Mar 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

net/cookies/server specifies (or/c date? #f). The old implementation of make-cookie specifies (or/c false/c string?), which is inherited from cookie:add-expires in net/cookie. So, since net/cookies/server doesn't accept a string, the wrapper make-cookie will convert it if possible or throw an exception.

@jeapostrophe
Copy link
Contributor

jeapostrophe commented Mar 8, 2017

Thanks! Is it possible to add a few tests of the new arguments/features?

@LiberalArtist
Copy link
Contributor Author

LiberalArtist commented Mar 8, 2017

Definitely — I haven't really looked at web-server-test yet, but I can later this afternoon.

@LiberalArtist
Copy link
Contributor Author

LiberalArtist commented Mar 8, 2017 •
edited
Loading

Working on the tests, I've found an inconsistency. Given this definition:

(define (make-reqc h)
   (make-request
    #"GET" (string->url "http://test.com/foo")
    (list (make-header #"Cookie" h)) (delay empty) #f
   "host" 80 "client"))

Evaluating (cookie-header->alist (header-value (car (request-headers/raw (make-reqc #"$Version=\"1\"; name=\"value\""))))) produces '((#"$Version" . #"\"1\"") (#"name" . #"\"value\"")). I use cookie-header->alist to implement the new request-cookies.

However, the old version of request-cookies eliminates the quotes: using the old definition, (request-cookies (make-reqc #"$Version=\"1\"; name=\"value\"")) was expected to produce '(#s(client-cookie "$Version" "1" #f #f) #s(client-cookie "name" "value" #f #f)).

I'm not immediately sure what the source of the discrepancy is or which behavior is correct.

ETA: I think this is a bug in cookie-header->alist. From a quick read of the RFC, it looks like cookie-value = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE ), so the quotes should be removed. I will see if I can patch net-cookies.

ETA: It looks like net-cookies specifically expects this behavior for cookie-header->alist. For now, I'm just going to handle this in request-cookies.

@LiberalArtist
Copy link
Contributor Author

LiberalArtist commented Mar 8, 2017 •
edited
Loading

This test

(let ()
        (define in "hell\"w\"o") ;<--- this is not a cookie-value?
        (define out #"id=\"hell\\\"w\\\"o\"")
        (test-check "quotes (pr14194)" header-equal?
                    (cookie->header (make-cookie "id" in))
                    (make-header #"Set-Cookie" out))
        (test-equal? "quotes (pr14194)"
                     (reqc out)
                     (list (make-client-cookie "id" in #f #f))))

raises a contract violation because cookie-value? from net/cookies/common rejects values that contain double-quotes (or control characters, whitespace, commas, semicolons, or backslashes).

Should the test be eliminated? Or is cookie-value? wrong?

ETA: It looks like cookie-value? is correct per the RFC: cookie-octet excludes DQUOTE.

@LiberalArtist
Copy link
Contributor Author

LiberalArtist commented Mar 8, 2017

Likewise, strings of the form ".foo" do not satisfy domain-value?: they need to be of the form "host.foo" or just "host".

@LiberalArtist
Copy link
Contributor Author

LiberalArtist commented Mar 9, 2017

Ok, I have pushed some tests.

@jeapostrophe
Copy link
Contributor

jeapostrophe commented Mar 9, 2017

The tests look good. Do you have anything else planned or should we commit now?

@LiberalArtist
Copy link
Contributor Author

LiberalArtist commented Mar 9, 2017

I don't have anything else in mind, so I think it's ready to go.

@jeapostrophe jeapostrophe merged commit 4135e33 into racket:master Mar 9, 2017
@jeapostrophe
Copy link
Contributor

jeapostrophe commented Mar 9, 2017

Thank you very much!

@samth
Copy link
Member

samth commented Mar 12, 2017

Unfortunately, this change causes significant problems for our continuous integration, because net-cookies depends on all of the Racket documentation. We therefore need to make some sort of change here. A few options:

  1. Revert this change (bad).
  2. Split the net-cookies package into -lib, -test, -doc variants.
  3. Something else?

I prefer option 2, but I'm open to other suggestions.

@LiberalArtist
Copy link
Contributor Author

LiberalArtist commented Mar 14, 2017

Splitting net-cookies seems reasonable to me, but I'm not 100% clear on how to do it. Is it just a matter of submitting a pull request to reorganize the git repository, or is there something the maintainer needs to do to update the package catalog?

@samth
Copy link
Member

samth commented Mar 14, 2017 via email

LiberalArtist added a commit to LiberalArtist/racket-cookies that referenced this pull request Mar 14, 2017
@LiberalArtist
Split package into -lib, -test, and -doc variants
22ab823 
(Needed so that web-server can depend on this:
racket/web-server#21 )

The package catalog will also need to be updated
to reflect this change.
@LiberalArtist LiberalArtist mentioned this pull request Mar 14, 2017
Merged
@LiberalArtist
Copy link
Contributor Author

LiberalArtist commented Mar 14, 2017

I've submitted a pull request to split net/cookies: RenaissanceBug/racket-cookies#6

@jeapostrophe
Copy link
Contributor

jeapostrophe commented Mar 15, 2017

Can you prepare the pull-request for web-server to change its dep?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeapostrophe jeapostrophe jeapostrophe left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Cookies and RFC 6265

3 participants

@LiberalArtist @jeapostrophe @samth