diff --git a/lib/wreq_ruby/cookie.rb b/lib/wreq_ruby/cookie.rb index 96fdbb9..1c70588 100644 --- a/lib/wreq_ruby/cookie.rb +++ b/lib/wreq_ruby/cookie.rb @@ -28,18 +28,22 @@ class Cookie # @param options [Hash] Optional keyword arguments # @option options [String] :domain Domain attribute # @option options [String] :path Path attribute - # @option options [Integer] :max_age Max-Age in seconds - # @option options [Float] :expires Unix timestamp (seconds, float) + # @option options [Integer] :max_age Signed Max-Age in seconds; zero or negative expires immediately + # @option options [Time, Numeric] :expires Expiration time or finite Unix timestamp # @option options [Boolean] :http_only HttpOnly flag # @option options [Boolean] :secure Secure flag # @option options [Wreq::SameSite] :same_site SameSite attribute # @return [Wreq::Cookie] + # @raise [ArgumentError] if an option is unknown, duplicated, or otherwise invalid + # @raise [RangeError] if Max-Age or the expiration is outside the supported range + # @raise [TypeError] if an option has an incompatible value type # @example # c = Wreq::Cookie.new( # "sid", "abc", # domain: "example.com", # path: "/", # max_age: 3600, + # expires: Time.utc(2030, 1, 1), # http_only: true, # secure: true, # same_site: Wreq::SameSite::Lax @@ -93,11 +97,21 @@ def path def domain end - # @return [Integer, nil] Max-Age in seconds + # Returns the signed Max-Age in seconds. + # + # Zero and negative values indicate immediate expiration. + # @return [Integer, nil] def max_age end - # @return [Float, nil] Expires as Unix timestamp (seconds) + # Returns the expiration as a UTC Ruby Time. + # @return [Time, nil] + def expires_at + end + + # Returns the expiration as fractional Unix seconds. + # @deprecated Use {#expires_at} for a Ruby-native time value. + # @return [Float, nil] def expires end end diff --git a/src/cookie.rs b/src/cookie.rs index 69f3cc7..3823297 100644 --- a/src/cookie.rs +++ b/src/cookie.rs @@ -1,18 +1,30 @@ -use std::{fmt, sync::Arc, time::SystemTime}; +//! Ruby bindings for HTTP cookies and the shared cookie jar. +//! +//! Cookie expiration follows [RFC 6265]: Ruby `Time` values and finite Unix +//! timestamps are converted into signed native date-times, while non-positive +//! Max-Age values represent immediate expiration. +//! +//! [RFC 6265]: https://www.rfc-editor.org/rfc/rfc6265.html +use std::{fmt, sync::Arc}; + +use ::serde::Deserialize; use bytes::Bytes; -use cookie::{Cookie as RawCookie, Expiration, ParseError, time::Duration}; +use cookie::{Cookie as RawCookie, ParseError, time::Duration}; use magnus::{ - Error, Module, Object, RHash, RModule, RString, Ruby, TryConvert, Value, function, method, - r_hash::ForEach, typed_data::Obj, value::ReprValue, + Error, Module, Object, RHash, RModule, RString, Ruby, Time, TryConvert, Value, function, + method, r_hash::ForEach, typed_data::Obj, value::ReprValue, }; use wreq::header::{self, HeaderMap, HeaderValue}; use crate::{ error::{header_value_error, type_error}, gvl, + options::{NativeOption, Options}, }; +use self::helper::{CookieExpiration, to_ruby_time, to_unix_timestamp}; + define_ruby_enum!( /// The Cookie SameSite attribute. const, @@ -33,6 +45,34 @@ pub struct Cookie(RawCookie<'static>); #[derive(Default)] pub struct Cookies(pub Vec); +/// Keyword attributes used to build a [`Cookie`]. +#[derive(Deserialize)] +struct Builder { + /// The Domain attribute. + domain: Option, + + /// The Path attribute. + path: Option, + + /// The signed Max-Age in seconds. + #[serde(default)] + max_age: NativeOption, + + /// The absolute expiration accepted through Magnus conversion. + #[serde(default)] + expires: NativeOption, + + /// Whether the cookie is inaccessible to client-side scripts. + http_only: Option, + + /// Whether the cookie is restricted to secure connections. + secure: Option, + + /// The SameSite policy. + #[serde(default)] + same_site: NativeOption>, +} + /// A good default `CookieStore` implementation. /// /// This is the implementation used when simply calling `cookie_store(true)`. @@ -42,71 +82,68 @@ pub struct Cookies(pub Vec); #[magnus::wrap(class = "Wreq::Jar", free_immediately, size)] pub struct Jar(pub Arc); -// ===== impl Cookie ===== - -impl Cookie { - /// Create a new [`Cookie`]. - pub fn new(args: &[Value]) -> Result { - let args = - magnus::scan_args::scan_args::<(String, String), (), (), (), magnus::RHash, ()>(args)?; - #[allow(clippy::type_complexity)] - let keywords: magnus::scan_args::KwArgs< - (), - ( - Option, - Option, - Option, - Option, - Option, - Option, - Option>, - ), - (), - > = magnus::scan_args::get_kwargs( - args.keywords, - &[], - &[ - "domain", - "path", - "max_age", - "expires", - "http_only", - "secure", - "same_site", - ], - )?; - - let (name, value) = args.required; +// ===== impl Builder ===== + +impl Builder { + /// Deserialize and convert one validated Cookie attribute Hash. + /// + /// # Errors + /// + /// Returns `ArgumentError` for unknown or duplicate attributes and retains + /// the Ruby conversion error for invalid values. + fn from_options(options: Options<'_>) -> Result { + let mut builder = options.validate_keys::()?.deserialize::()?; + extract_native_option!(options, builder, max_age); + extract_native_option!(options, builder, expires); + extract_native_option!(options, builder, same_site); + Ok(builder) + } + /// Build a native Cookie from its required identity and optional attributes. + fn build(mut self, name: String, value: String) -> Cookie { let mut cookie = RawCookie::new(name, value); - if let Some(domain) = keywords.optional.0 { + if let Some(domain) = self.domain { cookie.set_domain(domain); } - if let Some(path) = keywords.optional.1 { + if let Some(path) = self.path { cookie.set_path(path); } - if let Some(max_age) = keywords.optional.2 { - cookie.set_max_age(Duration::seconds(max_age as i64)); + if let Some(max_age) = self.max_age.take() { + cookie.set_max_age(Duration::seconds(max_age)); } - if let Some(expires) = keywords.optional.3 { - let duration = std::time::Duration::from_secs_f64(expires); - if let Some(system_time) = SystemTime::UNIX_EPOCH.checked_add(duration) { - cookie.set_expires(Expiration::DateTime(system_time.into())); - } + if let Some(expires) = self.expires.take() { + cookie.set_expires(expires.into_inner()); } - cookie.set_http_only(keywords.optional.4); - cookie.set_secure(keywords.optional.5); + cookie.set_http_only(self.http_only); + cookie.set_secure(self.secure); - if let Some(same_site) = keywords.optional.6 { + if let Some(same_site) = self.same_site.take() { cookie.set_same_site(same_site.into_ffi()); } - Ok(Self(cookie)) + Cookie(cookie) + } +} + +// ===== Ruby Cookie API ===== + +impl Cookie { + /// Create a new [`Cookie`] from a name, value, and keyword attributes. + /// + /// # Errors + /// + /// Returns `ArgumentError` for unknown or duplicate attributes and retains + /// the Ruby conversion error for invalid values. + pub fn new(ruby: &Ruby, args: &[Value]) -> Result { + let args = magnus::scan_args::scan_args::<(String, String), (), (), (), RHash, ()>(args)?; + let (name, value) = args.required; + Builder::from_options(Options::new(ruby, args.keywords)) + .map(|builder| builder.build(name, value)) } /// The name of the cookie. @@ -133,13 +170,13 @@ impl Cookie { self.0.secure().unwrap_or(false) } - /// Returns true if 'SameSite' directive is 'Lax'. + /// Return whether the SameSite directive is Lax. #[inline] pub fn same_site_lax(&self) -> bool { self.0.same_site() == Some(cookie::SameSite::Lax) } - /// Returns true if 'SameSite' directive is 'Strict'. + /// Return whether the SameSite directive is Strict. #[inline] pub fn same_site_strict(&self) -> bool { self.0.same_site() == Some(cookie::SameSite::Strict) @@ -157,41 +194,60 @@ impl Cookie { self.0.domain() } - /// Get the Max-Age information. + /// Return the signed Max-Age in seconds. #[inline] pub fn max_age(&self) -> Option { self.0.max_age().map(|d| d.whole_seconds()) } - /// The cookie expiration time. + /// Return the cookie expiration as a UTC Ruby `Time`. + pub fn expires_at(ruby: &Ruby, rb_self: &Self) -> Result, Error> { + rb_self + .0 + .expires_datetime() + .map(|value| to_ruby_time(ruby, value)) + .transpose() + } + + /// Return the cookie expiration as legacy fractional Unix seconds. #[inline] pub fn expires(&self) -> Option { - match self.0.expires() { - Some(Expiration::DateTime(offset)) => { - let system_time = SystemTime::from(offset); - system_time - .duration_since(SystemTime::UNIX_EPOCH) - .ok() - .map(|d| d.as_secs_f64()) - } - None | Some(Expiration::Session) => None, - } + self.0.expires_datetime().map(to_unix_timestamp) } } +// ===== Native Cookie helpers ===== + impl Cookie { + /// Clone this cookie for insertion into the native jar. + /// + /// [RFC 6265 section 5.2.2] treats a non-positive Max-Age as immediate + /// expiration. The native jar currently recognizes zero, so a negative + /// value is normalized only in this insertion clone; the Ruby cookie keeps + /// its original signed value. + /// + /// [RFC 6265 section 5.2.2]: https://www.rfc-editor.org/rfc/rfc6265.html#section-5.2.2 + fn clone_for_jar(&self) -> RawCookie<'static> { + let mut cookie = self.0.clone(); + if cookie.max_age().is_some_and(Duration::is_negative) { + cookie.set_max_age(Duration::ZERO); + } + + cookie + } + /// Parse cookies from a `HeaderMap`. - pub fn extract_headers_cookies(headers: &HeaderMap) -> Vec { + pub(crate) fn extract_headers_cookies(headers: &HeaderMap) -> Vec { headers .get_all(header::SET_COOKIE) .iter() - .map(Cookie::parse) - .flat_map(Result::ok) + .filter_map(|value| Self::parse(value).ok()) .map(RawCookie::into_owned) .map(Cookie) .collect() } + /// Parse one Set-Cookie header value. fn parse<'a>(value: &'a HeaderValue) -> Result, ParseError> { std::str::from_utf8(value.as_bytes()) .map_err(cookie::ParseError::from) @@ -241,7 +297,7 @@ impl TryConvert for Cookies { impl Jar { /// Create a new [`Jar`] with an empty cookie store. pub fn new() -> Self { - Jar(Arc::new(wreq::cookie::Jar::default())) + Self(Arc::new(wreq::cookie::Jar::default())) } /// Get all cookies. @@ -262,7 +318,7 @@ impl Jar { /// Add a cookie to this jar. pub fn add(&self, cookie: Value, url: String) { if let Ok(cookie) = Obj::::try_convert(cookie) { - gvl::nogvl(|| self.0.add(cookie.0.clone(), &url)) + return gvl::nogvl(|| self.0.add(cookie.clone_for_jar(), &url)); } if let Ok(cookie_str) = String::try_convert(cookie) { @@ -281,6 +337,112 @@ impl Jar { } } +mod helper { + //! Ruby time conversion helpers for `Wreq::Cookie`. + + use cookie::time::{Duration, OffsetDateTime, error::ComponentRange}; + use magnus::{ + Error, Integer, Ruby, Time, TryConvert, Value, + time::{Offset, Timespec}, + }; + + use crate::error::{argument_error, range_error}; + + /// A validated cookie expiration accepted from Ruby. + /// + /// Ruby `Time` values retain nanosecond resolution. Integer timestamps avoid a + /// floating-point conversion, while other Numeric values are accepted as + /// finite fractional Unix timestamps. + pub(super) struct CookieExpiration(OffsetDateTime); + + impl CookieExpiration { + /// Return the validated UTC expiration used by the native cookie. + pub(super) fn into_inner(self) -> OffsetDateTime { + self.0 + } + } + + impl TryConvert for CookieExpiration { + fn try_convert(value: Value) -> Result { + let ruby = Ruby::get_with(value); + expiration_from_value(&ruby, value).map(Self) + } + } + + /// Convert a native cookie expiration into a UTC Ruby `Time`. + pub(super) fn to_ruby_time(ruby: &Ruby, value: OffsetDateTime) -> Result { + ruby.time_timespec_new( + Timespec { + tv_sec: value.unix_timestamp(), + tv_nsec: i64::from(value.nanosecond()), + }, + Offset::utc(), + ) + } + + /// Convert a native cookie expiration into legacy fractional Unix seconds. + pub(super) fn to_unix_timestamp(value: OffsetDateTime) -> f64 { + value.unix_timestamp() as f64 + f64::from(value.nanosecond()) / 1_000_000_000.0 + } + + /// Convert a supported Ruby expiration value into a native UTC date-time. + fn expiration_from_value(ruby: &Ruby, value: Value) -> Result { + if let Some(time) = Time::from_value(value) { + return expiration_from_time(ruby, time); + } + + if let Some(integer) = Integer::from_value(value) { + return integer + .to_i64() + .and_then(|seconds| expiration_from_seconds(ruby, seconds)); + } + + expiration_from_float(ruby, f64::try_convert(value)?) + } + + /// Convert a Ruby `Time` without passing through unsigned `SystemTime` math. + fn expiration_from_time(ruby: &Ruby, value: Time) -> Result { + let timespec = value.timespec()?; + let nanosecond = u32::try_from(timespec.tv_nsec) + .ok() + .filter(|value| *value < 1_000_000_000) + .ok_or_else(|| { + argument_error(ruby, "time nanoseconds are outside the supported range") + })?; + + expiration_from_seconds(ruby, timespec.tv_sec)? + .replace_nanosecond(nanosecond) + .map_err(|error| expiration_range_error(ruby, error)) + } + + /// Convert exact signed Unix seconds into the native cookie time range. + fn expiration_from_seconds(ruby: &Ruby, seconds: i64) -> Result { + OffsetDateTime::from_unix_timestamp(seconds) + .map_err(|error| expiration_range_error(ruby, error)) + } + + /// Convert a finite fractional Unix timestamp without using a panicking API. + fn expiration_from_float(ruby: &Ruby, seconds: f64) -> Result { + if !seconds.is_finite() { + return Err(argument_error(ruby, "timestamp must be finite")); + } + + let duration = Duration::checked_seconds_f64(seconds) + .ok_or_else(|| range_error(ruby, "timestamp is outside the supported range"))?; + OffsetDateTime::UNIX_EPOCH + .checked_add(duration) + .ok_or_else(|| range_error(ruby, "timestamp is outside the supported range")) + } + + /// Map the native date-time range error to Ruby's `RangeError`. + fn expiration_range_error(ruby: &Ruby, error: ComponentRange) -> Error { + range_error( + ruby, + format!("timestamp is outside the supported range: {error}"), + ) + } +} + pub fn include(ruby: &Ruby, gem_module: &RModule) -> Result<(), Error> { // SameSite enum let same_site_class = gem_module.define_class("SameSite", ruby.class_object())?; @@ -300,6 +462,7 @@ pub fn include(ruby: &Ruby, gem_module: &RModule) -> Result<(), Error> { cookie_class.define_method("path", method!(Cookie::path, 0))?; cookie_class.define_method("domain", method!(Cookie::domain, 0))?; cookie_class.define_method("max_age", method!(Cookie::max_age, 0))?; + cookie_class.define_method("expires_at", method!(Cookie::expires_at, 0))?; cookie_class.define_method("expires", method!(Cookie::expires, 0))?; // Jar class diff --git a/src/error.rs b/src/error.rs index e3a36a2..3549407 100644 --- a/src/error.rs +++ b/src/error.rs @@ -192,6 +192,11 @@ pub fn argument_error(ruby: &Ruby, message: impl Into) -> MagnusError { MagnusError::new(ruby.exception_arg_error(), message.into()) } +/// Build a `RangeError` from a validation message. +pub fn range_error(ruby: &Ruby, message: impl Into) -> MagnusError { + MagnusError::new(ruby.exception_range_error(), message.into()) +} + /// Build a `TypeError` from a conversion message. pub fn type_error(ruby: &Ruby, message: impl Into) -> MagnusError { MagnusError::new(ruby.exception_type_error(), message.into()) diff --git a/test/cookie_test.rb b/test/cookie_test.rb index ec2bdb4..82b80e3 100644 --- a/test/cookie_test.rb +++ b/test/cookie_test.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true require "test_helper" +require "open3" +require "rbconfig" class CookieTest < Minitest::Test def setup @@ -84,7 +86,9 @@ def test_max_age_and_expires_optional @jar.add("exp=1; Expires=#{t.gmtime.strftime("%a, %d %b %Y %H:%M:%S GMT")}; Path=/", @base_url) c2 = @jar.get_all.find { |c| c.name == "exp" } assert c2 - # expires returns Float (unix seconds) or nil + # expires_at returns Time and expires retains the numeric compatibility API + assert_kind_of Time, c2.expires_at + assert_predicate c2.expires_at, :utc? if (e = c2.expires) assert_kind_of Float, e assert_operator e, :>, Time.now.to_f - 1_000_000 # sanity bound @@ -104,6 +108,7 @@ def test_cookie_new_minimal assert_nil c.domain assert_nil c.max_age assert_nil c.expires + assert_nil c.expires_at assert_equal false, (c.http_only || c.http_only?) assert_equal false, (c.secure || c.secure?) @@ -112,7 +117,7 @@ def test_cookie_new_minimal end def test_cookie_new_full_attributes - exp = Time.now.to_f + 7200.0 + exp = Time.utc(2030, 1, 1, 0, 0, Rational(123_456_789, 1_000_000_000)) c = Wreq::Cookie.new("sess", "v", domain: "example.com", path: "/", @@ -130,18 +135,171 @@ def test_cookie_new_full_attributes # Max-Age returns seconds as Integer assert_equal 3600, c.max_age - # Expires returns Float seconds-since-epoch (with small tolerance) - assert c.expires - assert_kind_of Float, c.expires - assert_in_delta exp, c.expires, 2.0 + assert_equal exp, c.expires_at + assert_predicate c.expires_at, :utc? + assert_in_delta exp.to_f, c.expires, 1e-6 assert_equal true, (c.http_only || c.http_only?) assert_equal true, (c.secure || c.secure?) - # constructor currently sets SameSite to none assert_equal true, c.same_site_lax? assert_equal false, c.same_site_strict? end + def test_cookie_new_uses_shared_keyword_validation + cookie = Wreq::Cookie.new("sid", "abc", **{"path" => "/"}) + assert_equal "/", cookie.path + + secret = "must-not-appear" + unknown_error = assert_raises(ArgumentError) do + Wreq::Cookie.new("sid", "abc", domian: secret) + end + assert_includes unknown_error.message, ":domian" + refute_includes unknown_error.message, secret + + duplicate_options = {path: "/one"} + duplicate_options["path"] = "/two" + duplicate_error = assert_raises(ArgumentError) do + Wreq::Cookie.new("sid", "abc", **duplicate_options) + end + assert_includes duplicate_error.message, "duplicate option: :path" + end + + def test_expires_accepts_past_time + expiration = Time.at(Rational(-5, 4)).utc + cookie = Wreq::Cookie.new("past", "value", expires: expiration) + + assert_equal expiration, cookie.expires_at + assert_in_delta(-1.25, cookie.expires, 1e-9) + end + + def test_expires_accepts_integer_and_fractional_timestamps + [1_893_456_000, 1_893_456_000.125, -1.25].each do |timestamp| + cookie = Wreq::Cookie.new("timestamp", "value", expires: timestamp) + + assert_kind_of Time, cookie.expires_at + assert_predicate cookie.expires_at, :utc? + assert_in_delta timestamp, cookie.expires_at.to_f, 1e-6 + assert_in_delta timestamp, cookie.expires, 1e-6 + end + end + + def test_expires_rejects_non_finite_timestamps + [Float::NAN, Float::INFINITY, -Float::INFINITY].each do |timestamp| + error = assert_raises(ArgumentError) do + Wreq::Cookie.new("invalid", "value", expires: timestamp) + end + + assert_match(/expires.*finite/, error.message) + end + end + + def test_expires_rejects_unrepresentable_times + expirations = [ + -(2**63), + 2**63 - 1, + -Float::MAX, + Float::MAX, + Time.utc(10_000, 1, 1) + ] + + expirations.each do |expiration| + error = assert_raises(RangeError) do + Wreq::Cookie.new("invalid", "value", expires: expiration) + end + + assert_match(/expires.*supported range/, error.message) + end + end + + def test_max_age_accepts_signed_boundaries_without_wrapping + [-(2**63), -1, 0, 2**63 - 1].each do |max_age| + cookie = Wreq::Cookie.new("max-age", "value", max_age: max_age) + + assert_equal max_age, cookie.max_age + end + + [-(2**63) - 1, 2**63].each do |max_age| + assert_raises(RangeError) do + Wreq::Cookie.new("max-age", "value", max_age: max_age) + end + end + end + + def test_non_positive_max_age_removes_cookie_from_jar + [-1, 0].each do |max_age| + jar = Wreq::Jar.new + jar.add("session=old; Path=/", @base_url) + deletion = Wreq::Cookie.new("session", "gone", path: "/", max_age: max_age) + + jar.add(deletion, @base_url) + + assert_equal max_age, deletion.max_age + assert_empty jar.get_all + end + end + + def test_past_expiration_removes_cookie_from_jar + @jar.add("session=old; Path=/", @base_url) + deletion = Wreq::Cookie.new( + "session", + "gone", + path: "/", + expires: Time.at(-1).utc + ) + + @jar.add(deletion, @base_url) + + assert_empty @jar.get_all + end + + def test_expiration_regressions_exit_subprocess_normally + lib_dir = File.expand_path("../lib", __dir__) + script = <<~RUBY + require "wreq" + + past = Wreq::Cookie.new("past", "value", expires: -1.0) + abort "past timestamp was not retained" unless past.expires_at == Time.at(-1).utc + + [Float::NAN, Float::INFINITY, -Float::INFINITY].each do |timestamp| + begin + Wreq::Cookie.new("invalid", "value", expires: timestamp) + rescue ArgumentError + next + end + + abort "non-finite timestamp did not raise ArgumentError" + end + + [Float::MAX, -Float::MAX].each do |timestamp| + begin + Wreq::Cookie.new("invalid", "value", expires: timestamp) + rescue RangeError + next + end + + abort "unrepresentable finite timestamp did not raise RangeError" + end + + [-(2**63) - 1, 2**63].each do |max_age| + begin + Wreq::Cookie.new("invalid", "value", max_age: max_age) + rescue RangeError + next + end + + abort "out-of-range Max-Age did not raise RangeError" + end + + puts "ok" + RUBY + + stdout, stderr, status = Open3.capture3(RbConfig.ruby, "-I", lib_dir, "-e", script) + + assert status.success?, "subprocess failed with #{status.inspect}: #{stderr}" + assert_equal "ok\n", stdout + refute_match(/panicked|fatal|access violation|cannot convert float seconds to Duration/i, stderr) + end + def test_same_site_flags_from_parsed_header @jar.clear @jar.add("s1=1; Path=/; SameSite=Strict", @base_url)