Passing cookies via middleware, ensure cookies are escaped #70
Conversation
syntax changes Add to Unreleased CHANGELOG
spec/grover/middleware_spec.rb
Outdated
| cookies: [{ domain: 'www.example.org', name: 'key', value: 'value' }] | ||
| cookies: [ | ||
| { domain: 'www.example.org', name: 'key', value: 'value' }, | ||
| { domain: 'www.example.org', name: 'escaped', value: '%26%3D%3D'} |
There was a problem hiding this comment.
seems like a reasonable test, however i'd suggest it would also be good to have a test that actually calls to Grover/Puppeteer with some encoded cookies, have the page render the contents then check the result contains the expected encoded values. Hopefully that way there will be less risk of future regressions. This would probably live in the processor_spec
There was a problem hiding this comment.
Agreed. I shall make it so.
There was a problem hiding this comment.
Added that spec, but all it really does it prove that you can pass a cookies, escaped or not to Puppeteer, the real failure on this was not realising that the cookie parser had escaped it. My bad.
abrom
left a comment
abrom
left a comment
There was a problem hiding this comment.
Change looks good @braindeaf 👍
Note per my comments I've fixed up the cookie renderer app to decode the cookie properly which now breaks your spec. Good news though is that the spec (once fixed) will make sense!
| # Changelog | ||
|
|
||
| ## Unreleased | ||
| - None |
There was a problem hiding this comment.
Can you please add an entry here for your change. Likely
### Fixed
- [#70](https://github.com/Studiosity/grover/pull/70) Ensure cookies are escaped when passing cookies via middleware ([@braindeaf][])
Co-authored-by: Andrew Bromwich <a.bromwich@gmail.com>
abrom
left a comment
abrom
left a comment
There was a problem hiding this comment.
Change looks great, thanks @braindeaf 👍
|
FYI this has been released in v0.12.3 |
2 participants
It appears I missed a vital part of passing cookies from middleware, Rack::Utils unescapes cookies and they need to be re-escaped.