Policy generation ignores expiry time when receiving a hash with string keys

[taylorthurlow]

Hi, I submitted an issue in the filestack-rails repo (filestack/filestack-rails#175), which after some poking around I have solved and found that it's a small issue with this gem instead. I'll be submitting a pull request shortly, but I'd like to outline my original issue and proposed changes.

---

Using filestack-rails, I set up a security policy this way:

config.filestack_rails.security = { 'expiry' => 4663446066,  'call' => %w[pick read stat write writeUrl store convert remove exif] }

This hash is handled by the filestack-rails gem and passed directly into a new FilestackSecurity object. The issue lies in how FilestackSecurity handles it's input. Here's a short debugger session to detail the issue.

[62, 71] in /home/taylor/code/ceremony/vendor/ruby/2.4.0/gems/filestack-2.1.0/lib/filestack/models/filestack_security.rb
   62:   #
   63:   # Manage options and convert hash to json string
   64:   #
   65:   def create_policy_string(options)
   66:     debugger
=> 67:     options[:expiry] = expiry_timestamp(options)
   68:     options.to_json
   69:   end
   70: 
   71:   #
(byebug) options
{"expiry"=>1514764799, "call"=>["pick", "read", "stat", "write", "writeUrl", "store", "convert", "remove", "exif"]}
(byebug) n

[63, 72] in /home/taylor/code/ceremony/vendor/ruby/2.4.0/gems/filestack-2.1.0/lib/filestack/models/filestack_security.rb
   63:   # Manage options and convert hash to json string
   64:   #
   65:   def create_policy_string(options)
   66:     debugger
   67:     options[:expiry] = expiry_timestamp(options)
=> 68:     options.to_json
   69:   end
   70: 
   71:   #
   72:   # Get expiration timestamp by adding seconds in option or using default
(byebug) options
{"expiry"=>1514764799, "call"=>["pick", "read", "stat", "write", "writeUrl", "store", "convert", "remove", "exif"], :expiry=>1509490768}
(byebug) n

[42, 51] in /home/taylor/code/ceremony/vendor/ruby/2.4.0/gems/filestack-2.1.0/lib/filestack/models/filestack_security.rb
   42:   #
   43:   # @param [String]   secret    Your filestack security secret
   44:   # @param [Hash]     options   Hash of options - see constructor
   45:   def generate(secret, options)
   46:     policy_json = create_policy_string(options)
=> 47:     @policy = Base64.urlsafe_encode64(policy_json)
   48:     @signature = OpenSSL::HMAC.hexdigest('sha256', secret, policy)
   49:   end
   50: 
   51:   # Sign the URL by appending policy and signature URL parameters
(byebug) policy_json
"{\"expiry\":1509490768,\"call\":[\"pick\",\"read\",\"stat\",\"write\",\"writeUrl\",\"store\",\"convert\",\"remove\",\"exif\"]}"
(byebug) 

The expiry_timestamp function adds a duplicate key to the options hash. It does this because it incorrectly uses a symbolized key :expiry instead of the string key 'expiry'. expiry_timestamp checks the wrong key, and thinks that there was no expiry key passed in to begin with, and offers the default expiry length of 1 hour. Using string keys instead of symbol keys in create_policy_string and expiry_timestamp fixes the problem. My solution will probably be to call .symbolize_keys on the options hash, ensuring that it doesn't matter whether the end user configures the gem using string keys or symbol keys.

All that said, I don't normally do pull requests or dig through gem source, so I'm not really sure what the process is for getting this change made in the filestack gem and then getting filestack-rails to accept the new version as a dependency - currently it requires 2.2.0. I'll submit a pull request. If i've done anything incorrectly, sorry!

[taylorthurlow]

Closed my pull request. I had incorrectly assumed that symbolize_keys was a ruby thing, not an ActiveSupport thing.

I'm curious about others' opinions here. Is it a better idea to only allow symbol keys? Using symbol keys definitely feels more correct, but I can still define a hash using string keys.

[staturecrane]

Hey @taylorthurlow. Sorry I've been MIA. I've got a lot on my plate right now, and maintenance on the Ruby and Rails SDK has had to wait. I think it's okay to change the README and define that only symbol keys are supported. But I would also welcome the input of others as to whether or not that specification will be too restrictive in production.

[taylorthurlow]

@staturecrane Don't worry about it. I don't have particularly strong feelings either way. I'm sure changing any available documentation, and perhaps throwing a small disclaimer in would be enough to prevent this sort of problem in the future.

If you'd like to close this that's fine, otherwise I'll leave it open in case anyone else wants to voice an opinion.

[jfelchner]

@taylorthurlow the symbolize_keys method is very small and can be copied from ActiveSupport as something like lib/filestack/core_ext/hash.rb with:

module Filestack
class  Hash
  def self.symbolize_keys
    # Contents here
  end
end
end

and then called when the config is initialized as: Filestack::Hash.symbolize_keys(config_hash) or whatever.

[taylorthurlow]

@jfelchner I've opened a pull request with some changes following your suggestion. I went for a far simpler implementation of symbolize_keys, mostly because transform_keys isn't a thing until Ruby 2.5, and the Rails 4.x implementation is all tied up with the Key object.

[gabifija]

@taylorthurlow - The issue is solved in current released version of Ruby SDK.

[jfelchner]

@gabifiolek solved how exactly? 😂 A link to a commit or CHANGELOG entry would be nice.

[taylorthurlow]

@jfelchner So... it looks like my pull request was closed and after an essentially identical commit was made to fix the issue. Maybe there's something I'm not seeing here but it looks like my changes were just copied.

@gabifiolek ?

Pull request in question is here: #31

[taylorthurlow]

Hey don't mean to bother you @staturecrane as I believe you handed off this repo to someone else, but not sure who else to ping about this. See my previous comment about my commits seemingly being copied without credit.

[staturecrane]

@taylorthurlow I understand your frustration, but I no longer work for this company nor contribute to this project and so I have no more information than you about this.