Skip to content

fix(security): add tls warnings, standard cookie parsing and explicit ti#198

Open
omercnet wants to merge 2 commits intomainfrom
shuni/task-20260312062955
Open

fix(security): add tls warnings, standard cookie parsing and explicit ti#198
omercnet wants to merge 2 commits intomainfrom
shuni/task-20260312062955

Conversation

@omercnet
Copy link
Copy Markdown
Member

Fixes descope/etc#14534

View workflow run

Summary

I've successfully implemented the security improvements requested for the Ruby SDK. Here's what was done:

1. TLS Skip Verification Warning

  • Added a runtime warning in lib/descope/mixins/initializer.rb when skip_verify is enabled
  • The warning clearly states this is INSECURE and should only be used in local development
  • Also ensured the verify_ssl option is properly passed to RestClient

2. Standard Library Cookie Parsing

  • Replaced custom regex-based cookie parsing with Ruby standard library (CGI.unescape)
  • Added proper error handling with logging for cookie parsing failures
  • The new implementation in lib/descope/mixins/http.rb follows RFC 6265 more closely

3. HTTP Timeout Configuration

  • Verified that @timeout_seconds is configured (default: 60 seconds from Common::DEFAULT_TIMEOUT_SECONDS)
  • Added explicit @timeout = @timeout_seconds assignment to ensure the HTTP module uses the correct timeout value
  • RestClient properly receives the timeout parameter for all HTTP requests

4. JWT Validation

  • Confirmed the SDK already uses the standard jwt gem (v2.7) for JWT validation
  • JWT validation includes proper signature verification, expiration checking, and algorithm validation
  • No changes needed - already using best practices

Changes Made:

lib/descope/mixins/initializer.rb:

  • Added warning log when skip_verify is true
  • Set @timeout from @timeout_seconds for HTTP requests

lib/descope/mixins/http.rb:

  • Added require 'cgi' for standard library cookie parsing
  • Replaced regex-based parse_cookie_value with standard library approach using CGI.unescape
  • Added verify_ssl: OpenSSL::SSL::VERIFY_NONE to RestClient when skip_verify is enabled
  • Added error handling and logging for cookie parsing failures

All changes maintain backward compatibility while improving security. The commit has been created and is ready for review.


Created by Shuni 🐕

- Add runtime warning when skip_verify is enabled (TLS bypass for dev only)
- Replace custom cookie parsing with standard library (CGI.unescape)
- Ensure HTTP timeout is properly configured from @timeout_seconds
- Apply verify_ssl setting to RestClient when skip_verify is enabled

These changes address security review findings while maintaining backward
compatibility. The skip_verify option remains available for local development
but now emits a clear warning about its insecure nature.

Co-authored-by: Shuni <251468265+shuni-bot[bot]@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 12, 2026 06:34
Copy link
Copy Markdown

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 implements several Ruby SDK security-related improvements: warning when TLS verification is disabled, switching cookie parsing away from a regex extractor, and ensuring the configured request timeout is actually used by the HTTP layer.

Changes:

  • Log a warning when skip_verify disables TLS certificate verification, and explicitly propagate timeout to the HTTP mixin.
  • Replace regex-based Set-Cookie parsing with a split-based approach and decoding via the Ruby standard library.
  • Add verify_ssl: OpenSSL::SSL::VERIFY_NONE to RestClient requests when skip_verify is enabled.

Reviewed changes

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

File Description
lib/descope/mixins/initializer.rb Adds TLS-skip warning and sets @timeout from configured timeout seconds.
lib/descope/mixins/http.rb Updates cookie parsing logic and applies verify_ssl option when TLS verification is disabled.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@omercnet omercnet requested review from ami-descope and dorsha March 12, 2026 07:39
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