Skip to content

Conversation

@GrantBirki
Copy link
Contributor

No description provided.

Copilot AI review requested due to automatic review settings June 12, 2025 19:19
@GrantBirki
Copy link
Contributor Author

@copilot the lib/hooks/plugins/auth/hmac.rb file has gotten quite large and unwieldy. Please check it out and ensure it follows ruby best practices, is clean and concise, and fix the failing script/test tests related to it. Do not make any logic breaking changes

@GrantBirki GrantBirki merged commit 5eef086 into main Jun 12, 2025
14 of 22 checks passed
@GrantBirki GrantBirki deleted the built-in-auth-plugin-docs branch June 12, 2025 19:21
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the built-in HMAC authentication plugin with full ISO 8601 and Unix timestamp parsing/validation, updates acceptance tests and environment secrets, and adds documentation for built-in and custom auth plugins.

  • Added comprehensive unit tests for ISO 8601 and Unix timestamp handling in HMAC validation
  • Extended HMAC plugin to parse/validate ISO 8601 UTC timestamps, emit warnings on failures, and normalize headers
  • Updated acceptance tests, Docker Compose secrets, and docs (configuration.md & auth_plugins.md) to reflect new timestamp-aware HMAC behavior

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
spec/unit/lib/hooks/plugins/auth/hmac_spec.rb New tests for ISO 8601 and Unix timestamp validation edge cases
lib/hooks/plugins/auth/hmac.rb Grafted timestamp parsing/validation methods into HMAC plugin
spec/acceptance/docker-compose.yml Updated ALT_WEBHOOK_SECRET value
spec/acceptance/acceptance_tests.rb Acceptance tests updated to include timestamps and new signatures
lib/hooks/app/auth/auth.rb Added warning log on auth failure
docs/configuration.md Linked to new auth plugin documentation
docs/auth_plugins.md New comprehensive guide on built-in and custom auth plugins
Comments suppressed due to low confidence (1)

spec/acceptance/acceptance_tests.rb:120

  • The acceptance tests use FAKE_ALT_HMAC_SECRET, but in docker-compose.yml the ALT_WEBHOOK_SECRET was changed to "octoawesome-2-secret". Ensure the FAKE_ALT_HMAC_SECRET constant matches the updated environment value.
it "successfully processes a valid POST request with HMAC signature and timestamp" do

Comment on lines +225 to +227
return ts if ts
ts = parse_unix_timestamp(timestamp_value)
return ts if ts
Copy link

Copilot AI Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently parse_timestamp returns a Time object, but the specs expect an integer. Consider returning ts.to_i so that parse_timestamp yields an Integer rather than a Time.

Suggested change
return ts if ts
ts = parse_unix_timestamp(timestamp_value)
return ts if ts
return ts.to_i if ts
ts = parse_unix_timestamp(timestamp_value)
return ts.to_i if ts

Copilot uses AI. Check for mistakes.
# @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"
Copy link

Copilot AI Jun 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex for unix_timestamp? allows leading zeros (e.g., "0123"), but the test expects those to be invalid. Update the pattern to /\A(?:[1-9]\d*|0)\z/ to reject leading zeros except for exactly "0".

Suggested change
!!(timestamp_value =~ /\A\d+\z/) || timestamp_value == "0"
!!(timestamp_value =~ /\A(?:[1-9]\d*|0)\z/)

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants