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

1.0.0 #48

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

1.0.0 #48

wants to merge 2 commits into from

Conversation

jonathanong
Copy link
Member

basically hits all the 1.0.0 milestones

  • API has changed to var cookies = require('Cookies')([options], [keys]) then req.cookies = res.cookies = cookies(req, res).
  • automatically works as middleware, so you can do app.use(require('cookies')([keys]))
  • .get() and .set() now only set raw cookies
  • .sign() and .unsign() for signed cookies, with .encoded to optionally base64 encode as well. maybe a better name for .unsign()
  • .encode() and .decode() for base64 encoded cookies
  • .encrypt() and .decrypt() for encrypted cookies
  • .clear() to clear a cookie quickly

i found juggling all the options a little weird since you could have combos.
thus i opted to have each of them be a pair of methods.

Option changes:

  • overwrite defaults to true

To do:

  • secure proxy stuff, waiting on @dougwilson on that :D
  • node 0.8 support as there seems to be an issue with buffers or something

Maybe:

  • I took out the framework tests (since i rewrote everything basically). might need to add those
  • Pretty sure this will break in connect2/express3 as it assumes you don't use the setHeader patch
  • A way to do iv's with encrypted cookies would be nice
  • More tests in general would be nice

To test, you need to install keygrip master and npm link it since i haven't published v2 yet

please test and give me feedback before i push v1

/cc @dougwilson @Fishrock123

@Fishrock123 Fishrock123 added this to the 1.0.0 milestone Jun 19, 2014
name.replace(/[-[\]{}()*+?.,\\^$|#\s]/g, "\\$&") +
"=([^;]*)"
)
}
Copy link
Member

Choose a reason for hiding this comment

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

What's this actually do?

See: #9

Breif comments on some of these thing would be useful before 1.0.0

@Fishrock123
Copy link
Member

@jonathanong is this dependant on crypto-utils/keygrip#14 (keygrip 2.0.0) ?

@dougwilson
Copy link
Contributor

is this dependant on crypto-utils/keygrip#14 (keygrip 2.0.0) ?

yes

@Fishrock123
Copy link
Member

node 0.8 support as there seems to be an issue with buffers or something

Keygrip is 0.10+ anyways, so.. (Well, the current master is.)

@dougwilson
Copy link
Contributor

Keygrip is 0.10+ anyways, so.. (Well, the current master is.)

See crypto-utils/keygrip@71df3df#commitcomment-6552647

It can likely be made node 0.8, I just have to look into it. I just want them to release node 0.12 already!

@Fishrock123
Copy link
Member

I just want them to release node 0.12 already!

Yeah, but we are honestly looking at months still.
It isn't really that near ready. :/

@dougwilson
Copy link
Contributor

Yeah, but we are honestly looking at months still.

Yea, that's what sucks :( If they did rel 0.12, I would be more willing to drop node 0.8 support for things is all :)

@Fishrock123
Copy link
Member

I'm surprised people still use 0.8 - at this point it really isn't (that) more stable than 0.10, and npm is broken in it, etc.

Aside from a couple obscure minor performance regressions. (/off-topic rant)

@dougwilson
Copy link
Contributor

There are certain private PaaS systems (not to name names) where the 0.10 on them is 0.10.1, which is extremely buggy, so there is no other choice.

@Fishrock123
Copy link
Member

where the 0.10 on them is 0.10.1

oh god

@dougwilson
Copy link
Contributor

oh god

it was also the release before Joyent turned off heartbeats, so it is vulnerable to heartbleed.

@dougwilson
Copy link
Contributor

Travis is just failing on 0.8 as of this time

cool :) just let it keep failing until i check out what it would take to make 0.8 work :)

@jonathanong
Copy link
Member Author

so aside from the keygrip stuff which can just easily disabled by checking whether the methods exist,
the other issue seems to be something about missing this function in 0.8: http://nodejs.org/api/buffer.html#buffer_buf_copy_targetbuffer_targetstart_sourcestart_sourceend

screen shot 2014-06-21 at 9 01 14 pm

@dougwilson
Copy link
Contributor

buf.copy exists in node.js 0.8: https://github.com/joyent/node/blob/v0.8.26/lib/buffer.js#L515

@dougwilson
Copy link
Contributor

Looks like the error you posted is caused because at least one of the array elements you passed into Buffer.concat was not a Buffer: https://github.com/joyent/node/blob/v0.8.26/lib/buffer.js#L505

@jonathanong
Copy link
Member Author

Hmmm maybe crypto api changed then

@dougwilson
Copy link
Contributor

Maybe, haha. I plan to knock out the first todo tomorrow, perhaps I'll look into node.js 0.8 as well to determine if whatever the issue is can be made to work there or not.

@dougwilson
Copy link
Contributor

The current master keygrip has Node.js 0.8 support again, BTW.

@jokesterfr
Copy link

I'm sorry for asking, when do you plan to release v1.0.0?

@dougwilson
Copy link
Contributor

When the kinks are worked out.

@fengmk2
Copy link
Member

fengmk2 commented Aug 5, 2014

Waiting for 1.0.0~~~
💯

@fengmk2
Copy link
Member

fengmk2 commented Sep 5, 2014

Any progress?

@fengmk2
Copy link
Member

fengmk2 commented Apr 9, 2015

stop? @jonathanong

@jonathanong
Copy link
Member Author

@dougwilson what's the status on this? should i rebase? or did you have some ideas?

@jonathanong
Copy link
Member Author

@dougwilson any status updateS?

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

Successfully merging this pull request may close these issues.

5 participants