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

Validate cookie name prefixes #10648

Merged
merged 10 commits into from
Dec 23, 2022

Conversation

Blacksmoke16
Copy link
Member

Validates cookie names with prefixes as per https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-05#section-4.1.3.

I'm also not really sure why the other name/value validation raises an IO::Error? Seems more appropriate as an ArgumentError.

Copy link
Contributor

@Sija Sija left a comment

Choose a reason for hiding this comment

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

Pedantic: whitespaces

src/http/cookie.cr Outdated Show resolved Hide resolved
src/http/cookie.cr Outdated Show resolved Hide resolved
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

I'm doubting this is a good change in terms of API design. Now validate_name not just validates the name itself but in the context of other values. This creates some implicit restrictions when using the API in a builder flow, enforcing a specific order of property assignments.

I would suggest to add a dedicated method to validate the entire cookie. This validation could be applied automatically in parse_set_cookie.

spec/std/http/cookie_spec.cr Outdated Show resolved Hide resolved
src/http/cookie.cr Outdated Show resolved Hide resolved
@Blacksmoke16
Copy link
Member Author

I would suggest to add a dedicated method to validate the entire cookie. This validation could be applied automatically in parse_set_cookie.

@straight-shoota parse_set_cookie wouldn't handle all the ways you could create a cookie, e.g. when creating it via .new or using the name setter.

This creates some implicit restrictions when using the API in a builder flow, enforcing a specific order of property assignments.

I don't see how this could be avoided. Otherwise you could "build" an invalid cookie and wouldn't even know it.

@straight-shoota
Copy link
Member

The answer is that in order to validate the cookie value, you need to validate explicitly when building an instance. I don't think a setter should apply implicit validation on an incomplete cookie.

@Blacksmoke16
Copy link
Member Author

@straight-shoota Oh I see. You were proposing this method be public, then just also call it in the constructor. What are your thoughts on how it should work then? Like .valid? : Bool, or .validate! : Nil | NoReturn, or?

@straight-shoota
Copy link
Member

Prob. both?

Better supports Cookie builder pattern
src/http/cookie.cr Outdated Show resolved Hide resolved
src/http/cookie.cr Outdated Show resolved Hide resolved
spec/std/http/cookie_spec.cr Outdated Show resolved Hide resolved
spec/std/http/cookie_spec.cr Outdated Show resolved Hide resolved
src/http/cookie.cr Show resolved Hide resolved
src/http/cookie.cr Outdated Show resolved Hide resolved
src/http/cookie.cr Outdated Show resolved Hide resolved
src/http/cookie.cr Outdated Show resolved Hide resolved
src/http/cookie.cr Outdated Show resolved Hide resolved
Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @Blacksmoke16 🙏

@straight-shoota straight-shoota added this to the 1.7.0 milestone Dec 22, 2022
@straight-shoota straight-shoota merged commit a0ca346 into crystal-lang:master Dec 23, 2022
@Blacksmoke16 Blacksmoke16 deleted the cookie-prefixes branch July 21, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants