-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix Cookie SameSite constants to Pascal case per RFC specification #3608
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
Conversation
Co-authored-by: sixcolors <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3608 +/- ##
==========================================
+ Coverage 91.01% 91.13% +0.11%
==========================================
Files 112 112
Lines 11290 11293 +3
==========================================
+ Hits 10276 10292 +16
+ Misses 756 744 -12
+ Partials 258 257 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 653312e | Previous: 361d76c | Ratio |
|---|---|---|---|
Benchmark_GenericParseTypeString/benchmark_genericParseTypeString#02 |
13.56 ns/op 0 B/op 0 allocs/op |
9 ns/op 0 B/op 0 allocs/op |
1.51 |
Benchmark_GenericParseTypeString/benchmark_genericParseTypeString#02 - ns/op |
13.56 ns/op |
9 ns/op |
1.51 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the SameSite constants to match RFC6265-bis with Pascal case (SameSite) and title case (Strict, Lax, None). Looks great for spec compliance! However, the case-sensitive comparison in the PR might be too strict, as browsers like Chromium and WebKit use ASCII case-insensitive comparisons for SameSite values (e.g., samesite=strict or SAMESITE=lax work fine). Since HTTP headers use only ASCII, we should avoid Unicode case folding like strings.EqualFold for performance and stick to an ASCII-only case-insensitive comparison. A lightweight custom function like EqualASCIIFold could be added to gofiber/utils or even utils.ToLower could do the trick while keeping GoFiber’s speed. Thoughts on swapping the comparison to something like this?
…tibility Co-authored-by: sixcolors <[email protected]>
Great point about the case-sensitivity issue! I've implemented ASCII case-insensitive comparison using Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Okay that’s not bad. But if the strings are not already lowercase this will alloc. A function like: func EqualASCIIFold(s, t string) bool {
if len(s) != len(t) {
return false
}
for i := 0; i < len(s); i++ {
if s[i]|0x20 != t[i]|0x20 {
return false
}
}
return true
}would check for ascii case insensitive matches while guaranteeing no allocations. Can you create this in the https://github.com/gofiber/utils package and use that? |
Co-authored-by: sixcolors <[email protected]>
I've implemented the Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot |
Co-authored-by: sixcolors <[email protected]>
Replaced the custom Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot fix test coverage |
…functionality Co-authored-by: sixcolors <[email protected]>
Co-authored-by: sixcolors <[email protected]>
Implemented automatic |
Should we document that change, for example in |
We should reference the links from Mozilla and Chrome |
Co-authored-by: sixcolors <[email protected]>
Documented the automatic |
…urity enforcement Co-authored-by: gaby <[email protected]>
Added Mozilla and Chrome documentation links to both ctx.md and whats_new.md as requested. The documentation now includes references to the official Mozilla docs and Chrome developer guidelines for SameSite=None security requirements. Fixed in commit 69f4671. |
sixcolors
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…utes, and ensure secure attribute is set correctly for SameSite=None
…nto copilot/fix-3607
…cure attribute validation
This PR fixes the Cookie SameSite constants to use proper Pascal case values as required by the RFC specification.
Problem
The Cookie SameSite constants were using lowercase values:
However, according to RFC 6265bis, the SameSite values should be Pascal case:
"Strict" / "Lax" / "None".Solution
Updated the constants to use RFC-compliant Pascal case:
Also removed the
utils.ToLower()call inctx.gothat was converting these values back to lowercase, ensuring the cookie headers output the correct Pascal case values.Verification
SameSite=Lax,SameSite=Strict,SameSite=NoneFixes #3607.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
exampleretry.com/tmp/go-build776519753/b273/client.test -test.paniconexit0 -test.v=test2json -test.timeout=10m0s -test.count=1 -test.shuffle=on(dns block)/tmp/go-build2352507060/b273/client.test -test.paniconexit0 -test.v=test2json -test.timeout=10m0s -test.count=1 -test.shuffle=on(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.