Aws-sdk-ruby: Aws::S3 unable to parse "Expires" header

[hosh]

When I tried doing a head_object, I get the following error:

irb(main):003:0* s3 = Aws::S3::Client.new(region: 'us-east-1')
=> #<Aws::S3::Client>
irb(main):004:0> s3.head_object(bucket: bucket, key: key)
ArgumentError: no time information in "5d"
        from /home/deployer/.rbenv/versions/2.2.4/lib/ruby/2.2.0/time.rb:252:in `make_time'
        from /home/deployer/.rbenv/versions/2.2.4/lib/ruby/2.2.0/time.rb:364:in `parse'
        from /home/deployer/.rbenv/versions/2.2.4/lib/ruby/gems/2.2.0/gems/aws-sdk-core-2.3.3/lib/aws-sdk-core/rest/response/headers.rb:42:in `cast_value'
        from /home/deployer/.rbenv/versions/2.2.4/lib/ruby/gems/2.2.0/gems/aws-sdk-core-2.3.3/lib/aws-sdk-core/rest/response/headers.rb:27:in `extract_header_value'

Just to make sure, I added a debug line:

DEBUG: #<Seahorse::Model::Shapes::ShapeRef:0x007fce41fc1b80 @metadata={}, @required=false, @deprecated=false, @shape=#<Seahorse::Model::Shapes::TimestampShape:0x007fce42212dd0 @metadata={"type"=>"timestam
p"}, @name="Expires", @documentation=nil>, @location="header", @location_name="Expires", @documentation=nil> "5d"

Looking at the code, https://github.com/aws/aws-sdk-ruby/blob/master/aws-sdk-core/lib/aws-sdk-core/rest/response/headers.rb#L41 it looks like it is expecting a well-formatted timestamp.

Just to be sure, I tried this with the aws-cli tool:

$ aws s3api head-object --bucket $BUCKET --key $KEY
{
    "AcceptRanges": "bytes",
    "ContentType": "image/png",
    "LastModified": "Fri, 04 Mar 2016 02:06:23 GMT",
    "ContentLength": 78616,
    "Expires": "5d",
    "ETag": "\"0a937f9c6deab66e91a87e0741cb42df\"",
    "StorageClass": "REDUCED_REDUNDANCY",
    "CacheControl": "public",
    "Metadata": {}
}

The aws cli handles this fine. The V1 handles this fine.

Here's the thing, according to RFC 2616:

The format is an absolute date and time as defined by HTTP-date in section 3.3.1; it MUST be in RFC 1123 date format:

      Expires = "Expires" ":" HTTP-date

And further:

HTTP/1.1 clients and caches MUST treat other invalid date formats, especially including the value "0", as in the past (i.e., "already expired").

So the expectation of using Time.parse seem reasonable. However, it is also more fragile.

Can you guys check into this? If this is something that borked in on the S3 service's side (a bug that did not convert the 5d into an absolute time) then there's probably a bunch of outages relating to this.

[trevorrowe]

The bug here appears to be the SDK's assumption that the expires header is a parseable time format. The CLI is passing the value through unparsed as a String. If we were to switch this behavior today, users that rely on it being an instance of Time would be broken. We may have to attempt to do the conversion and then fall back on returning a string. Another option is to add an additional field, such as #expires_string which does not convert and the #expires member would be nil when a valid Time value can not be parsed.

Thoughts?

[hosh]

For now, I have this monkeypatch in my code:

module Aws
  module Rest
    module Response
      class Headers

        def cast_value(ref, value)
          case ref.shape
          when StringShape then value
          when IntegerShape then value.to_i
          when FloatShape then value.to_f
          when BooleanShape then value == 'true'
          when TimestampShape
            if value =~ /\d+(\.\d*)/
              Time.at(value.to_f)
            else
              maybe_parse_time(value)
            end
          else raise "unsupported shape #{ref.shape.class}"
          end
        end

        private

        def maybe_parse_time(value)
          Time.parse(value)
        rescue ArgumentError => e
          raise e unless e.message =~ /^no time information in/
          value
        end
      end
    end
  end
end

I'm not entirely satisfied with it because:

1. I think there is a reasonable expectation that what comes out of Expires is an http-date (as noted in the RFC), that is enforceable by AWS and not on client side. I don't know if this is because that particular object was passed "5d" in the expires parameter when creating the S3 object, or some slipup the team responsible for S3 has. I don't know anyone in the AWS server team, but maybe you do?
2. I tried to isolate the exception being raised. Not sure if I like that regexp. If S3 consistently sends non-compliant Expires headers, that exception code gets run every single time. (Granted, the network round-trip is probably longer than the time it takes for Ruby to do that extra processing).
3. This only happens for Expires header, but the patch effects all time-shape. I don't know if that is a good thing or a bad thing. The AWS CLI doesn't do any enforcement. Maybe I'm being a type fanatic for no good purpose?

In the code I am working on, there is no existing reliance on expecting that to be a Time. On the other hand, if the S3 is sending back out invalid http-date fields, then this is moot.

I kinda wonder if there is a way to have something like https://medium.com/learnings-in-and-around-sharetribe/option-pattern-in-ruby-7b0f7c5abdb6#.aremtyrbl but that'd still require code change.

[ArthurN]

I have a related but slight variation of the problem. For us, the Expires values are all "0". The code attempts to use Time#at if it's numerical, which _would_ handle it appropriately (i.e. Time.at(0) #=> 1969-12-31 17:00:00 -0700); however, the regular expression in line 38 of that file is a little too specific in that it requires a period. Finding no period, it passes the value to Time#parse instead. Thus, like @hosh, I get a ArgumentError: no time information in "0"

[ArthurN]

Here is my monkeypatch. Note the ? I added in the regex.

(@hosh This monkeypatch approach (taken from this snippet) helps isolate the code changes a little bit better, in that for all other ref.shape types it defers to the original source. You could modify to run your maybe_parse_time)

module Aws
  module Rest
    module Response
      class Headers
        # HACKFIX monkeypatch: see https://github.com/aws/aws-sdk-ruby/issues/1184
        cast_value_method = instance_method(:cast_value)

        define_method(:cast_value) do |ref, value|
          if ref.shape.is_a?(TimestampShape)
            if value =~ /\d+(\.?\d*)/
              Time.at(value.to_f)
            else
              Time.parse(value)
            end
          else
            cast_value_method.bind(self).call(ref, value)
          end
        end
      end
    end
  end
end

Just be sure to run this after the AWS SDK gem is loaded.

[hosh]

@ArthurN using alias or alias_method doesn't work for you?

But yeah, I saw the 0 value pop up too for us.

[ArthurN]

@hosh Something like alias_method :old_cast_value, :cast_value, you mean? I'm sure that would work, too.

[trevorrowe]

I did some looking into this today. Amazon S3 will allow you to persist ANY string value into the Expires headers when making a PUT object request. I disable the SDK's validation on PUT and I sent the value "abc".

Subsequent HEAD Object reqeusts respond with the exact value given during PUT. This means I got back the string literal "abc". This means to me that the SDK needs to be robust enough to handle any possible invalid expiration header. As per the RFC quoted above (emphasis added is mine):

HTTP/1.1 clients and caches MUST treat other invalid date formats, especially including the value "0", as in the past (i.e., "already expired").

Given the SDK is not attempting to interpret the expires header, it should probably do a best effort. What I'm proposing is the following:

• Failures to extract time data from timestamp headers should simply result in nil values. This means the #expires member of the #head_object response would be nil in the error cases.
• Add an additional #expires_string member to the response, that extract the header as is, without attempting to parse. This allows access to the raw value in every scenario.

Thoughts?

[trevorrowe]

I've submitted a pull request with a proposed fix/work-around.