Skip to content

Fix Slack auth filter not halting on invalid signature#67

Closed
JuanVqz wants to merge 2 commits intomainfrom
fix-slack-auth-filter
Closed

Fix Slack auth filter not halting on invalid signature#67
JuanVqz wants to merge 2 commits intomainfrom
fix-slack-auth-filter

Conversation

@JuanVqz
Copy link
Member

@JuanVqz JuanVqz commented Mar 6, 2026

Problem

valid_slack_request? in Slack::ApplicationController returned a boolean but never called render or head, so the before_action never halted the filter chain. Every Slack endpoint accepted any request regardless of signature validity.

# Before — always proceeds to action
def valid_slack_request?
  @verified ||= verify_slack_signature  # just a boolean, no halt
end

Fix

Added head :unauthorized and return to halt the chain when verification fails, consistent with the pattern used in #66:

def valid_slack_request?
  head :unauthorized and return unless verify_slack_signature
end

Tests

Added Slack::PuzzlesControllerTest covering:

  • Requests with an invalid signature → 401
  • Requests with an expired timestamp → 401
  • Requests with a valid signature → allowed through (200)

@JuanVqz JuanVqz requested review from mateusdeap and torresga March 6, 2026 00:16
@JuanVqz JuanVqz self-assigned this Mar 6, 2026
JuanVqz added 2 commits March 5, 2026 18:21
`valid_slack_request?` returned a boolean but never called `head` or
`render`, so unauthenticated Slack requests were silently allowed through.
Added `head :unauthorized and return` to halt the filter chain, consistent
with the fix applied in #66.
@JuanVqz JuanVqz force-pushed the fix-slack-auth-filter branch from b84c0cf to b535ade Compare March 6, 2026 00:25
JuanVqz added a commit that referenced this pull request Mar 6, 2026
Replaces #66, #67, #68, #70 with a single cohesive fix:

- Merge check_session_expiry + check_user_token into one authenticate!
  callback, eliminating the double-render risk by design
- SessionsController and Slack::ApplicationController now each skip a
  single authenticate! instead of managing multiple callbacks
- Rename valid_slack_request? to verify_slack_request! and make it halt
  with head :unauthorized when signature verification fails
- Replace head :unprocessable_entity in send_message with logging and
  Sentry.capture_exception so notification failures never affect the
  HTTP response back to Slack
@JuanVqz
Copy link
Member Author

JuanVqz commented Mar 6, 2026

Superseded by #71 which addresses this and three other related bugs with a more robust design.

@JuanVqz JuanVqz closed this Mar 6, 2026
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.

1 participant