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

http: Cookie extend #359

Merged
merged 14 commits into from Apr 27, 2019

Conversation

3 participants
@zekth
Copy link
Contributor

commented Apr 24, 2019

Add delCookie and setCookie.

I have a concern about the multiple Set-Cookie header. It's common to use BUT Headers does not give us the possibility to do this. Do you think any workaround is possible? I think we can keep it like this for the moment and work on multiple Set-Cookie headers handling in a future PR.

Also Days / Months format are hardcoded in the module because ATM there is some discussions about embedding or not the ICU in Deno which may break this module. Ref: denoland/deno#1968

zekth and others added some commits Apr 24, 2019

Show resolved Hide resolved http/cookie.ts Outdated

zekth added some commits Apr 25, 2019

Show resolved Hide resolved http/cookie.ts
@@ -19,3 +106,33 @@ export function getCookie(rq: ServerRequest): Cookie {
}
return {};
}

/* Set the cookie header properly in the Response */

This comment has been minimized.

Copy link
@ry

ry Apr 27, 2019

Contributor

Use jsdoc style comments.

Show resolved Hide resolved http/cookie.ts Outdated
@ry

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

Regarding the naming of the functions getCookie, setCookie, and delCookie... I wonder if you could research a similar functionality in Go (and Node?) and see what they call these functions? I have the feeling there are better names we could use here.

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

About the name consistency for the delCookie there is no standard way. Some languages uses SetCookie with expiration date way earlier (like it's done here) or clearCookie / rmCookie / delCookie . Depends on the framework. Personnaly after this research i like clearCookie

For setCookie, example of golang : https://golang.org/pkg/net/http/#SetCookie . But setCookie fits our styleguide.

Finally for getCookie example in golang : readCookies ref: https://golang.org/src/net/http/cookie.go . And concerning node there is no helper for this. For example express add this to its ServerResponse class.

zekth added some commits Apr 27, 2019


export type SameSite = "Strict" | "Lax";

export function cookieDateFormat(date: Date): string {

This comment has been minimized.

Copy link
@ry

ry Apr 27, 2019

Contributor

Is this a specific date format for cookies? Does it have a name? If it's not just for cookies, consider moving this to //datetime

This comment has been minimized.

Copy link
@zekth

zekth Apr 27, 2019

Author Contributor

I was thinking it was specific but no. It's called IMF-fixdate. I'll move it to datetime. Also is date.toIMF() good naming for you?

ref: https://tools.ietf.org/html/rfc7231#section-7.1.1.1

This comment has been minimized.

Copy link
@ry

ry Apr 27, 2019

Contributor

Is there a name for this in Go? Perhaps you can borrow that.

This comment has been minimized.

Copy link
@zekth

zekth Apr 27, 2019

Author Contributor

No, the only mention of it i can find is IMF-fixdate

This comment has been minimized.

Copy link
@ry

ry Apr 27, 2019

Contributor

I think you can call it "RFC1123_GMT"

We don't have the same way of serializing datetime formats as Go. I'd like to slowly move in that direction tho. So I think you can just keep the toIMF() where it is now. But please familiarize yourself with how I think this would be ideally structured:
https://github.com/golang/go/blob/f67f5511ee0f225bcc8943994ba6139eed375e85/src/net/http/server.go#L903-L909
https://github.com/golang/go/blob/f67f5511ee0f225bcc8943994ba6139eed375e85/src/net/http/cookie.go#L200-L204
Consider adding some of the commentary from Go to the jsdoc of "toIMF()"

Show resolved Hide resolved http/cookie.ts
Show resolved Hide resolved http/cookie.ts Outdated
Show resolved Hide resolved http/cookie_test.ts
@ry

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

Go doesn't have a corresponding function to delCookie, it just has this:
https://github.com/golang/go/blob/f67f5511ee0f225bcc8943994ba6139eed375e85/src/net/http/cookie.go#L29
Do you think we can also get away with not having delCookie?

Show resolved Hide resolved http/cookie.ts
@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

@ry about the go Implementation, it's not really common about the manipulation of the MaxAge. We can do both i think, because a lot of frameworks use to have deleteCookie functions too. I'm ok with both implementation but i think it's more easy to use to have a delCookie function.

Just about the toIMF() review. You want me to port the code from golang to the toIMF() function in a future PR?

Show resolved Hide resolved http/cookie.ts Outdated
Show resolved Hide resolved http/cookie.ts Outdated
Show resolved Hide resolved http/cookie.ts Outdated
Show resolved Hide resolved http/cookie.ts Outdated

zekth added some commits Apr 27, 2019

Show resolved Hide resolved http/cookie.ts Outdated
@ry

ry approved these changes Apr 27, 2019

Copy link
Contributor

left a comment

LGTM - nice work

@ry ry merged commit f111469 into denoland:master Apr 27, 2019

5 checks passed

denoland.deno_std Build #20190427.10 succeeded
Details
denoland.deno_std (Linux) Linux succeeded
Details
denoland.deno_std (Mac) Mac succeeded
Details
denoland.deno_std (Windows) Windows succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@zekth zekth deleted the zekth:cookie_extend branch Apr 27, 2019

@motss

This comment has been minimized.

Copy link

commented Apr 29, 2019

@zekth @ry

Correct me if I'm wrong for the following codes:

deno_std/http/cookie.ts

Lines 78 to 83 in f111469

const c = req.headers.get("Cookie").split(";");
for (const kv of c) {
const cookieVal = kv.split("=");
const key = cookieVal.shift().trim();
out[key] = cookieVal.join("");
}

In the case of splitting the cookie value with = and joining them back with "", it will form an incorrect value as compared to the original one. For example, 'PREF=al=en-GB&f1=123; wide=1; SID=123'.

The expected output should be

{
  PREF: 'al=en-GB&f1=123',
  wide: '1',
  SID: '123'
}

instead of

{
  PREF: alen-GB&f1123,
  wide: '1',
  SID: '123'
}
@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@zekth @ry

Correct me if I'm wrong for the following codes:

deno_std/http/cookie.ts

Lines 78 to 83 in f111469

const c = req.headers.get("Cookie").split(";");
for (const kv of c) {
const cookieVal = kv.split("=");
const key = cookieVal.shift().trim();
out[key] = cookieVal.join("");
}
In the case of splitting the cookie value with = and joining them back with "", it will form an incorrect value as compared to the original one. For example, 'PREF=al=en-GB&f1=123; wide=1; SID=123'.

The expected output should be

{
  PREF: 'al=en-GB&f1=123',
  wide: '1',
  SID: '123'
}

instead of

{
  PREF: alen-GB&f1123,
  wide: '1',
  SID: '123'
}

Right, fair analysis. Should be out[key] = cookieVal.join("=");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.