Merge pull request #33 from github/hmac-improvements

Enhance HMAC timestamp validation: improve parsing logic and add test…

Author: GrantBirki

lib/hooks/plugins/auth/hmac.rb

@@ -202,80 +202,84 @@ def self.valid_timestamp?(headers, config)
           return false if timestamp_value.strip.empty?
 
           parsed_timestamp = parse_timestamp(timestamp_value.strip)
-          return false unless parsed_timestamp
+          return false unless parsed_timestamp.is_a?(Integer)
 
-          # parsed_timestamp is a Time object
-          now = Time.now.utc
+          now = Time.now.utc.to_i
           (now - parsed_timestamp).abs <= tolerance
         end
 
         # Parse timestamp value supporting both ISO 8601 UTC and Unix formats
         #
-        # Attempts to parse the timestamp in the following order:
-        # 1. ISO 8601 UTC format (e.g., "2025-06-12T10:30:00Z")
-        # 2. Unix timestamp (e.g., "1609459200")
-        #
         # @param timestamp_value [String] The timestamp string to parse
-        # @return [Time, nil] Time object if parsing succeeds, nil otherwise
+        # @return [Integer, nil] Epoch seconds if parsing succeeds, nil otherwise
         # @note Security: Strict validation prevents various injection attacks
         # @api private
         def self.parse_timestamp(timestamp_value)
-          # Try ISO 8601 first, then Unix
+          # Reject if contains any control characters, whitespace, or null bytes
+          if timestamp_value =~ /[\u0000-\u001F\u007F-\u009F]/
+            log.warn("Auth::HMAC validation failed: Timestamp contains invalid characters")
+            return nil
+          end
           ts = parse_iso8601_timestamp(timestamp_value)
           return ts if ts
           ts = parse_unix_timestamp(timestamp_value)
           return ts if ts
-          nil
+
+          # If neither format matches, return nil
+          log.warn("Auth::HMAC validation failed: Timestamp (#{timestamp_value}) is not valid ISO 8601 UTC or Unix format")
+          return nil
         end
 
-        # Check if timestamp string looks like ISO 8601 UTC format
+        # Check if timestamp string looks like ISO 8601 UTC format (must have UTC indicator)
         #
         # @param timestamp_value [String] The timestamp string to check
-        # @return [Boolean] true if it appears to be ISO 8601 format
+        # @return [Boolean] true if it appears to be ISO 8601 format (with or without UTC indicator)
         # @api private
         def self.iso8601_timestamp?(timestamp_value)
-          # Accepts Z, +00:00, or +0000, and T or space as separator
-          !!(timestamp_value =~ /\A\d{4}-\d{2}-\d{2}[T ]\d{2}:\d{2}:\d{2}(?:\.\d+)?(Z|\+00:00|\+0000)\z/)
+          !!(timestamp_value =~ /\A\d{4}-\d{2}-\d{2}[T ]\d{2}:\d{2}:\d{2}(?:\.\d+)?(Z|\+00:00|\+0000)?\z/)
         end
 
-        # Parse ISO 8601 UTC timestamp string
+        # Parse ISO 8601 UTC timestamp string (must have UTC indicator)
         #
         # @param timestamp_value [String] ISO 8601 timestamp string
-        # @return [Time, nil] Time object if parsing succeeds, nil otherwise
-        # @note Only accepts UTC timestamps (ending with 'Z' or explicit UTC)
+        # @return [Integer, nil] Epoch seconds if parsing succeeds, nil otherwise
+        # @note Only accepts UTC timestamps (ending with 'Z', '+00:00', '+0000')
         # @api private
         def self.parse_iso8601_timestamp(timestamp_value)
-          # Normalize 'YYYY-MM-DD HH:MM:SS(.fraction)? +0000' to 'YYYY-MM-DDTHH:MM:SS(.fraction)?+00:00'
           if timestamp_value =~ /\A(\d{4}-\d{2}-\d{2}) (\d{2}:\d{2}:\d{2}(?:\.\d+)?)(?: )\+0000\z/
             timestamp_value = "#{$1}T#{$2}+00:00"
           end
+          # Ensure the timestamp explicitly includes a UTC indicator
+          return nil unless timestamp_value =~ /(Z|\+00:00|\+0000)\z/
           return nil unless iso8601_timestamp?(timestamp_value)
-          t = Time.iso8601(timestamp_value) rescue nil
+          t = Time.parse(timestamp_value) rescue nil
           return nil unless t
-          # Only accept UTC (Z, +00:00, or +0000)
-          return t if t.utc? || t.utc_offset == 0
-          nil
+          # The check for UTC indicator in regex makes this t.utc? or t.utc_offset == 0 redundant
+          # but kept for safety, though it should always be true now if Time.parse succeeds.
+          (t.utc? || t.utc_offset == 0) ? t.to_i : nil
         end
 
-        # Parse Unix timestamp string
+        # Parse Unix timestamp string (must be positive integer, no leading zeros except for "0")
         #
         # @param timestamp_value [String] Unix timestamp string
-        # @return [Time, nil] Time object if parsing succeeds, nil otherwise
+        # @return [Integer, nil] Epoch seconds if parsing succeeds, nil otherwise
+        # @note Only accepts positive integer values, no leading zeros except for "0"
         # @api private
         def self.parse_unix_timestamp(timestamp_value)
           return nil unless unix_timestamp?(timestamp_value)
           ts = timestamp_value.to_i
           return nil if ts <= 0
-          Time.at(ts).utc
+          ts
         end
 
-        # Check if timestamp string looks like Unix timestamp format
+        # Check if timestamp string looks like Unix timestamp format (no leading zeros except "0")
         #
         # @param timestamp_value [String] The timestamp string to check
         # @return [Boolean] true if it appears to be Unix timestamp format
         # @api private
         def self.unix_timestamp?(timestamp_value)
-          !!(timestamp_value =~ /\A\d+\z/) || timestamp_value == "0"
+          return true if timestamp_value == "0"
+          !!(timestamp_value =~ /\A[1-9]\d*\z/)
         end
 
         # Compute HMAC signature based on configuration requirements
@@ -325,7 +329,7 @@ def self.compute_signature(payload:, headers:, secret:, config:)
         #   - {body}: Replaced with the raw payload
         # @example Template usage
         #   template: "{version}:{timestamp}:{body}"
-        #   result: "v0:1609459200:{"event":"push"}"
+        #   result: "v0:1609459200:{\"event\":\"push\"}"
         # @api private
         def self.build_signing_payload(payload:, headers:, config:)
           template = config[:payload_template]
@@ -355,7 +359,7 @@ def self.build_signing_payload(payload:, headers:, config:)
         #   - :algorithm_prefixed: "sha256=abc123..." (GitHub style)
         #   - :hash_only: "abc123..." (Shopify style)
         #   - :version_prefixed: "v0=abc123..." (Slack style)
-        # @note Defaults to algorithm_prefixed format for unknown format styles
+        # @note Defaults to algorithm-prefixed format for unknown format styles
         # @api private
         def self.format_signature(hash, config)
           format_style = FORMATS[config[:format]]

spec/acceptance/acceptance_tests.rb

@@ -116,6 +116,162 @@
       end
     end
 
+    describe "hmac_with_timestamp" do
+      it "successfully processes a valid POST request with HMAC signature and timestamp" do
+        payload = { text: "Hello, World!" }
+        timestamp = Time.now.utc.iso8601
+        body = payload.to_json
+        signing_payload = "#{timestamp}:#{body}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+        headers = {
+          "Content-Type" => "application/json",
+          "X-HMAC-Signature" => "sha256=#{digest}",
+          "X-HMAC-Timestamp" => timestamp
+        }
+        response = http.post("/webhooks/hmac_with_timestamp", body, headers)
+        expect(response).to be_a(Net::HTTPSuccess)
+        body = JSON.parse(response.body)
+        expect(body["status"]).to eq("success")
+      end
+
+      it "successfully processes a valid POST request with HMAC signature and timestamp and an empty payload" do
+        payload = {}
+        timestamp = Time.now.utc.iso8601
+        body = payload.to_json
+        signing_payload = "#{timestamp}:#{body}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+        headers = {
+          "Content-Type" => "application/json",
+          "X-HMAC-Signature" => "sha256=#{digest}",
+          "X-HMAC-Timestamp" => timestamp
+        }
+        response = http.post("/webhooks/hmac_with_timestamp", body, headers)
+        expect(response).to be_a(Net::HTTPSuccess)
+        body = JSON.parse(response.body)
+        expect(body["status"]).to eq("success")
+      end
+
+      it "successfully processes a valid POST request with HMAC signature and the POST has no body" do
+        timestamp = Time.now.utc.iso8601
+        signing_payload = "#{timestamp}:" # Empty body
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+        headers = {
+          "Content-Type" => "application/json",
+          "X-HMAC-Signature" => "sha256=#{digest}",
+          "X-HMAC-Timestamp" => timestamp
+        }
+        response = http.post("/webhooks/hmac_with_timestamp", nil, headers)
+        expect(response).to be_a(Net::HTTPSuccess)
+        body = JSON.parse(response.body)
+        expect(body["status"]).to eq("success")
+      end
+
+      it "fails due to using the wrong HMAC secret" do
+        payload = { text: "Hello, World!" }
+        timestamp = Time.now.utc.iso8601
+        body = payload.to_json
+        signing_payload = "#{timestamp}:#{body}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), "bad-hmac-secret", signing_payload)
+        headers = {
+          "Content-Type" => "application/json",
+          "X-HMAC-Signature" => "sha256=#{digest}",
+          "X-HMAC-Timestamp" => timestamp
+        }
+        response = http.post("/webhooks/hmac_with_timestamp", body, headers)
+        expect(response).to be_a(Net::HTTPUnauthorized)
+        expect(response.body).to include("authentication failed")
+      end
+
+      it "fails due to missing timestamp header" do
+        payload = { text: "Hello, World!" }
+        body = payload.to_json
+        signing_payload = "#{Time.now.utc.iso8601}:#{body}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+        headers = {
+          "Content-Type" => "application/json",
+          "X-HMAC-Signature" => "sha256=#{digest}"
+          # Missing X-HMAC-Timestamp header
+        }
+        response = http.post("/webhooks/hmac_with_timestamp", body, headers)
+        expect(response).to be_a(Net::HTTPUnauthorized)
+        expect(response.body).to include("authentication failed")
+      end
+
+      it "fails due to invalid timestamp format" do
+        payload = { text: "Hello, World!" }
+        invalid_timestamp = "not-a-timestamp"
+        body = payload.to_json
+        signing_payload = "#{invalid_timestamp}:#{body}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+        headers = {
+          "Content-Type" => "application/json",
+          "X-HMAC-Signature" => "sha256=#{digest}",
+          "X-HMAC-Timestamp" => invalid_timestamp
+        }
+        response = http.post("/webhooks/hmac_with_timestamp", body, headers)
+        expect(response).to be_a(Net::HTTPUnauthorized)
+        expect(response.body).to include("authentication failed")
+      end
+
+      it "rejects request with timestamp manipulation attack" do
+        payload = { text: "Hello, World!" }
+        original_timestamp = Time.now.utc.iso8601
+        manipulated_timestamp = (Time.now.utc + 100).iso8601  # Future timestamp
+
+        # Create signature with original timestamp
+        signing_payload = "#{original_timestamp}:#{payload.to_json}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+
+        # But send manipulated timestamp in header
+        headers = {
+          "Content-Type" => "application/json",
+          "X-HMAC-Signature" => "sha256=#{digest}",
+          "X-HMAC-Timestamp" => manipulated_timestamp
+        }
+
+        response = http.post("/webhooks/hmac_with_timestamp", payload.to_json, headers)
+        expect(response).to be_a(Net::HTTPUnauthorized)
+        expect(response.body).to include("authentication failed")
+      end
+
+      it "fails because the timestamp is too old" do
+        payload = { text: "Hello, World!" }
+        # Use timestamp that's 10 minutes old (beyond the tolerance)
+        expired_timestamp = (Time.now.utc - 600).iso8601
+
+        signing_payload = "#{expired_timestamp}:#{payload.to_json}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+
+        headers = {
+          "Content-Type" => "application/json",
+          "X-HMAC-Signature" => "sha256=#{digest}",
+          "X-HMAC-Timestamp" => expired_timestamp
+        }
+
+        response = http.post("/webhooks/hmac_with_timestamp", payload.to_json, headers)
+        expect(response).to be_a(Net::HTTPUnauthorized)
+        expect(response.body).to include("authentication failed")
+      end
+
+      it "fails because the wrong HMAC algorithm is used" do
+        payload = { text: "Hello, World!" }
+        timestamp = Time.now.utc.iso8601
+        body = payload.to_json
+        signing_payload = "#{timestamp}:#{body}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha512"), FAKE_ALT_HMAC_SECRET, signing_payload)
+
+        headers = {
+          "Content-Type" => "application/json",
+          "X-HMAC-Signature" => "sha512=#{digest}",
+          "X-HMAC-Timestamp" => timestamp
+        }
+
+        response = http.post("/webhooks/hmac_with_timestamp", body, headers)
+        expect(response).to be_a(Net::HTTPUnauthorized)
+        expect(response.body).to include("authentication failed")
+      end
+    end
+
     describe "slack" do
       it "successfully processes a valid POST request with HMAC signature and timestamp" do
         payload = { text: "Hello, Slack!" }
@@ -207,6 +363,23 @@
         expect(body["status"]).to eq("success")
       end
 
+      it "successfully processes request with ISO 8601 UTC timestamp (ruby default method)" do
+        payload = { text: "Hello, Slack!" }
+        iso_timestamp = Time.now.utc.iso8601
+        body = payload.to_json
+        signing_payload = "v0:#{iso_timestamp}:#{body}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+        headers = {
+          "Content-Type" => "application/json",
+          "Signature-256" => "v0=#{digest}",
+          "X-Timestamp" => iso_timestamp
+        }
+        response = http.post("/webhooks/slack", body, headers)
+        expect(response).to be_a(Net::HTTPSuccess)
+        body = JSON.parse(response.body)
+        expect(body["status"]).to eq("success")
+      end
+
       it "successfully processes request with ISO 8601 UTC timestamp using +00:00 format" do
         payload = { text: "Hello, Slack!" }
         iso_timestamp = Time.now.utc.strftime("%Y-%m-%dT%H:%M:%S+00:00")

spec/acceptance/config/endpoints/hmac_with_timestamp.yml

@@ -0,0 +1,12 @@
+path: /hmac_with_timestamp
+handler: Hello
+
+auth:
+  type: hmac
+  secret_env_key: ALT_WEBHOOK_SECRET
+  header: X-HMAC-Signature
+  timestamp_header: X-HMAC-Timestamp
+  timestamp_tolerance: 60 # 1 minute
+  algorithm: sha256
+  format: "algorithm=signature"  # produces "sha256=abc123..."
+  payload_template: "{timestamp}:{body}"