Skip to content

[rb] Permit case insensitive sameSite attribute on cookie creation #16096

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

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

Conversation

luke-hill
Copy link
Contributor

@luke-hill luke-hill commented Jul 28, 2025

User description

This handles situations where you get a direct injection from a valid set-header response in API requests

🔗 Related Issues

N/A

💥 What does this PR do?

Change the behaviour of create cookie to permit lax as a valid input.

It seems that something on the browser level is not permitting this as a valid attribute, but this is regularly returned by API requests. It becomes hard to triage

🔧 Implementation Notes

💡 Additional Considerations

Should this be implemented polyglot @diemol ?

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Bug fix


Description

  • Capitalize sameSite cookie attribute values for browser compatibility

  • Handle case-insensitive input from API responses

  • Fix cookie creation when receiving lowercase values


Diagram Walkthrough

flowchart LR
  A["API Response"] -- "lowercase sameSite" --> B["Cookie Manager"]
  B -- "capitalize()" --> C["Browser Compatible Cookie"]
Loading

File Walkthrough

Relevant files
Bug fix
manager.rb
Capitalize sameSite cookie attribute values                           

rb/lib/selenium/webdriver/common/manager.rb

  • Modified add_cookie method to capitalize sameSite attribute values
  • Changed from direct assignment to using .capitalize method
  • Ensures browser compatibility with proper case formatting
+1/-1     

This handles situations where you get a direct injection from a valid set-header response in API requests
@selenium-ci selenium-ci added the C-rb Ruby Bindings label Jul 28, 2025
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The capitalize method only capitalizes the first letter and lowercases the rest, which may not work correctly for all valid sameSite values. For example, 'LAX' would become 'Lax' instead of the expected 'Lax', but 'SAMESITE' would become 'Samesite' instead of potentially expected values.

opts[:sameSite] = same_site.capitalize if same_site
Input Validation

The code assumes same_site is a string that responds to capitalize, but there's no validation to ensure it's actually a string or that the capitalized result is a valid sameSite value like 'Strict', 'Lax', or 'None'.

opts[:sameSite] = same_site.capitalize if same_site

Copy link
Contributor

qodo-merge-pro bot commented Jul 28, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add type safety for capitalize

The capitalize method will fail if same_site is not a string (e.g., symbol or
nil). Add type checking or use to_s before calling capitalize to prevent runtime
errors.

rb/lib/selenium/webdriver/common/manager.rb [53]

-opts[:sameSite] = same_site.capitalize if same_site
+opts[:sameSite] = same_site.to_s.capitalize if same_site
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that if same_site is a symbol (a common pattern in Ruby) instead of a string, the call to .capitalize will raise a NoMethodError, and the proposed fix using .to_s makes the method more robust.

Medium
General
Ensure proper sameSite value formatting

The capitalize method only capitalizes the first letter, but valid sameSite
values are case-sensitive and should be "Strict", "Lax", or "None". Use
downcase.capitalize to ensure proper formatting regardless of input case.

rb/lib/selenium/webdriver/common/manager.rb [53]

-opts[:sameSite] = same_site.capitalize if same_site
+opts[:sameSite] = same_site.downcase.capitalize if same_site
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that capitalize alone is insufficient for normalizing the same_site value, and the proposed downcase.capitalize makes the code more robust against varied user input casing.

Medium
  • Update

@diemol
Copy link
Member

diemol commented Aug 5, 2025

What issue is this solving?

@luke-hill
Copy link
Contributor Author

The issue being resolved is when you get a cookie response that is invalidly cased. However speaking on slack it maybe should be bubbled up to driver vendors to permit people submitting it case insensitively

When selenium sets this attribute with the wrong case it crashes the driver. i.e. setting "lax" is an invalid value but "Lax" is a valid value

@diemol
Copy link
Member

diemol commented Aug 6, 2025

It does sound like something that needs to be raised with the browser driver. Is this happening for all browser drivers?

@luke-hill
Copy link
Contributor Author

I'm not 100%. I have just patched this here because I've done an equivalent patch on my end for chrome. In my current job I do not use much other than chrome (I could use firefox I figure, but I don't yet).

@diemol
Copy link
Member

diemol commented Aug 6, 2025

OK, so I don't think this makes sense as a workaround in Selenium. It should be raised to the driver. Could you please help us with that? Thanks!

@diemol diemol added the G-chromedriver Requires fixes in ChromeDriver label Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-rb Ruby Bindings G-chromedriver Requires fixes in ChromeDriver Review effort 2/5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants