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

Add disable_cookies macro. #1180

Merged
merged 2 commits into from Jun 10, 2020
Merged

Add disable_cookies macro. #1180

merged 2 commits into from Jun 10, 2020

Conversation

wout
Copy link
Contributor

@wout wout commented Jun 10, 2020

Purpose

Implements #1178.

Description

When used, the Set-Cookie header will not be written to the response headers.

class Events::Show < ApiAction
  disable_cookies

  get "/events/:id" do
    ...
  end
end

Checklist

  • - An issue already exists detailing the issue/or feature request that this PR fixes
  • - All specs are formatted with crystal tool format spec src
  • - Inline documentation has been added and/or updated
  • - Lucky builds on docker with ./script/setup
  • - All builds and specs pass on docker with ./script/test

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

This looks good. Does this let us disable cookies being set for asset calls? My guess is no since those skip the RouteHandler and run through the StaticFilesHandler..... So I guess the better question is, do we need to be able to disable cookies at that level?

@paulcsmith
Copy link
Member

@jwoertink I think they are no longer set for assets at all since there is no longer a middleware. I think that is ok/expected since I can't think of a reason assets would need to have cookies sent right? I really don't know how this is typically handled in frameworks, but I'd imagine there is no reason to set them on assets and would just add extra bloat so I think we're good

@wout
Copy link
Contributor Author

wout commented Jun 10, 2020

@jwoertink I had to look into it but I think cookies are not sent on requests for static assets. Here is an example:

$ curl -I 127.0.0.1:3001/assets/images/features/pulse.png
HTTP/1.1 200 OK
etag: W/"1591695350"
last-modified: Tue, 09 Jun 2020 09:35:50 GMT
content-type: image/png
content-length: 430020
connection: keep-alive
Date: Wed, 10 Jun 2020 15:37:47 GMT

Maybe that used to be the case before with the old SessionHandler, but now cookies are written right before the response body is printed. So I think you'll never get cookie headers for static files.

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

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

Looking good! Just one minor suggestion that should help with potential inheritance issues

src/lucky/renderable.cr Outdated Show resolved Hide resolved
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Sweet. Cool with me! 👍

Co-authored-by: Paul Smith <paulcsmith@users.noreply.github.com>
@confact
Copy link
Contributor

confact commented Jun 10, 2020

This looks like just want I need to build it. Something we should consider later is to maybe disable the cookie as default for the API actions in lucky_cli. But that is for later.

Good job @wout - Love you! :D

@paulcsmith paulcsmith merged commit c9b929b into luckyframework:master Jun 10, 2020
@paulcsmith
Copy link
Member

Looks great! And thanks for verifying no cookies are sent for static files 👍

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 this pull request may close these issues.

None yet

4 participants