Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Grape allowing invalid headers to be set #2511

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

SlakrHakr
Copy link

@SlakrHakr SlakrHakr commented Nov 4, 2024

Fixes #2334

Ensure all header values are strings according to the Rack spec.

  • Convert header values to strings using to_s in the header method in lib/grape/dsl/headers.rb.
  • Emit a warning if the header value is not a string in the header method in lib/grape/dsl/headers.rb.
  • Add tests in spec/grape/dsl/headers_spec.rb to verify that non-string header values are converted to strings and warnings are emitted.

For more details, open the Copilot Workspace session.

@grape-bot
Copy link

grape-bot commented Nov 5, 2024

1 Error
🚫 The TOC found in README.md doesn't match the sections of the file.

Here's the expected TOC for README.md:

# Table of Contents

- [What is Grape?](#what-is-grape)
- [Stable Release](#stable-release)
- [Project Resources](#project-resources)
- [Grape for Enterprise](#grape-for-enterprise)
- [Installation](#installation)
- [Basic Usage](#basic-usage)
- [Rails 7.1](#rails-71)
- [Mounting](#mounting)
  - [All](#all)
  - [Rack](#rack)
  - [Alongside Sinatra (or other frameworks)](#alongside-sinatra-or-other-frameworks)
  - [Rails](#rails)
    - [Zeitwerk](#zeitwerk)
  - [Modules](#modules)
- [Remounting](#remounting)
  - [Mount Configuration](#mount-configuration)
- [Versioning](#versioning)
  - [Strategies](#strategies)
    - [Path](#path)
    - [Header](#header)
    - [Accept-Version Header](#accept-version-header)
    - [Param](#param)
- [Describing Methods](#describing-methods)
- [Configuration](#configuration)
- [Parameters](#parameters)
  - [Params Class](#params-class)
  - [Declared](#declared)
  - [Include Parent Namespaces](#include-parent-namespaces)
  - [Include Missing](#include-missing)
  - [Evaluate Given](#evaluate-given)
  - [Parameter Precedence](#parameter-precedence)
- [Parameter Validation and Coercion](#parameter-validation-and-coercion)
  - [Supported Parameter Types](#supported-parameter-types)
  - [Integer/Fixnum and Coercions](#integerfixnum-and-coercions)
  - [Custom Types and Coercions](#custom-types-and-coercions)
  - [Multipart File Parameters](#multipart-file-parameters)
  - [First-Class JSON Types](#first-class-json-types)
  - [Multiple Allowed Types](#multiple-allowed-types)
  - [Validation of Nested Parameters](#validation-of-nested-parameters)
  - [Dependent Parameters](#dependent-parameters)
  - [Group Options](#group-options)
  - [Renaming](#renaming)
  - [Built-in Validators](#built-in-validators)
    - [allow_blank](#allow_blank)
    - [values](#values)
    - [except_values](#except_values)
    - [same_as](#same_as)
    - [length](#length)
    - [regexp](#regexp)
    - [mutually_exclusive](#mutually_exclusive)
    - [exactly_one_of](#exactly_one_of)
    - [at_least_one_of](#at_least_one_of)
    - [all_or_none_of](#all_or_none_of)
    - [Nested mutually_exclusive, exactly_one_of, at_least_one_of, all_or_none_of](#nested-mutually_exclusive-exactly_one_of-at_least_one_of-all_or_none_of)
  - [Namespace Validation and Coercion](#namespace-validation-and-coercion)
  - [Custom Validators](#custom-validators)
  - [Validation Errors](#validation-errors)
  - [I18n](#i18n)
  - [Custom Validation messages](#custom-validation-messages)
    - [presence, allow_blank, values, regexp](#presence-allow_blank-values-regexp)
    - [same_as](#same_as-1)
    - [length](#length-1)
    - [all_or_none_of](#all_or_none_of-1)
    - [mutually_exclusive](#mutually_exclusive-1)
    - [exactly_one_of](#exactly_one_of-1)
    - [at_least_one_of](#at_least_one_of-1)
    - [Coerce](#coerce)
    - [With Lambdas](#with-lambdas)
    - [Pass symbols for i18n translations](#pass-symbols-for-i18n-translations)
    - [Overriding Attribute Names](#overriding-attribute-names)
    - [With Default](#with-default)
  - [Using dry-validation or dry-schema](#using-dry-validation-or-dry-schema)
- [Headers](#headers)
  - [Request](#request)
    - [Header Value Types](#header-value-types)
    - [Header Case Handling](#header-case-handling)
  - [Response](#response)
- [Routes](#routes)
- [Helpers](#helpers)
- [Path Helpers](#path-helpers)
- [Parameter Documentation](#parameter-documentation)
- [Cookies](#cookies)
- [HTTP Status Code](#http-status-code)
- [Redirecting](#redirecting)
- [Recognizing Path](#recognizing-path)
- [Allowed Methods](#allowed-methods)
- [Raising Exceptions](#raising-exceptions)
  - [Default Error HTTP Status Code](#default-error-http-status-code)
  - [Handling 404](#handling-404)
- [Exception Handling](#exception-handling)
    - [Rescuing exceptions inside namespaces](#rescuing-exceptions-inside-namespaces)
    - [Unrescuable Exceptions](#unrescuable-exceptions)
    - [Exceptions that should be rescued explicitly](#exceptions-that-should-be-rescued-explicitly)
- [Logging](#logging)
- [API Formats](#api-formats)
  - [JSONP](#jsonp)
  - [CORS](#cors)
- [Content-type](#content-type)
- [API Data Formats](#api-data-formats)
- [JSON and XML Processors](#json-and-xml-processors)
- [RESTful Model Representations](#restful-model-representations)
  - [Grape Entities](#grape-entities)
  - [Hypermedia and Roar](#hypermedia-and-roar)
  - [Rabl](#rabl)
  - [Active Model Serializers](#active-model-serializers)
- [Sending Raw or No Data](#sending-raw-or-no-data)
- [Authentication](#authentication)
  - [Basic Auth](#basic-auth)
  - [Register custom middleware for authentication](#register-custom-middleware-for-authentication)
- [Describing and Inspecting an API](#describing-and-inspecting-an-api)
- [Current Route and Endpoint](#current-route-and-endpoint)
- [Before, After and Finally](#before-after-and-finally)
- [Anchoring](#anchoring)
- [Instance Variables](#instance-variables)
- [Using Custom Middleware](#using-custom-middleware)
  - [Grape Middleware](#grape-middleware)
  - [Rails Middleware](#rails-middleware)
  - [Remote IP](#remote-ip)
- [Writing Tests](#writing-tests)
  - [Writing Tests with Rack](#writing-tests-with-rack)
    - [RSpec](#rspec)
    - [Airborne](#airborne)
    - [MiniTest](#minitest)
  - [Writing Tests with Rails](#writing-tests-with-rails)
    - [RSpec](#rspec-1)
    - [MiniTest](#minitest-1)
  - [Stubbing Helpers](#stubbing-helpers)
- [Reloading API Changes in Development](#reloading-api-changes-in-development)
  - [Reloading in Rack Applications](#reloading-in-rack-applications)
  - [Reloading in Rails Applications](#reloading-in-rails-applications)
- [Performance Monitoring](#performance-monitoring)
  - [Active Support Instrumentation](#active-support-instrumentation)
    - [endpoint_run.grape](#endpoint_rungrape)
    - [endpoint_render.grape](#endpoint_rendergrape)
    - [endpoint_run_filters.grape](#endpoint_run_filtersgrape)
    - [endpoint_run_validators.grape](#endpoint_run_validatorsgrape)
    - [format_response.grape](#format_responsegrape)
  - [Monitoring Products](#monitoring-products)
- [Contributing to Grape](#contributing-to-grape)
- [Security](#security)
- [License](#license)
- [Copyright](#copyright)

Generated by 🚫 Danger

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Thanks.

This needs some docs (README, CHANGELOG, possibly UPGRADING depending on the answers to below).

  1. What is the current behavior of header 'integer key', 123? I am afraid we may be breaking some existing behavior.
  2. I don't think we need a warning if we're going to automatically coerce every header value .to_s, we should probably either loudly fail or document it as such.

SlakrHakr and others added 3 commits November 14, 2024 19:29
Fixes ruby-grape#2334

Ensure all header values are strings according to the Rack spec.

* Convert header values to strings using `to_s` in the `header` method in `lib/grape/dsl/headers.rb`.
* Emit a warning if the header value is not a string in the `header` method in `lib/grape/dsl/headers.rb`.
* Add tests in `spec/grape/dsl/headers_spec.rb` to verify that non-string header values are converted to strings and warnings are emitted.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/ruby-grape/grape/issues/2334?shareId=XXXX-XXXX-XXXX-XXXX).
@SlakrHakr SlakrHakr force-pushed the convert-headers-to-strings branch from 80cacb7 to 6ffceb5 Compare November 15, 2024 01:31
@SlakrHakr
Copy link
Author

SlakrHakr commented Nov 15, 2024

What is the current behavior of header 'integer key', 123? I am afraid we may be breaking some existing behavior.

fixed an existing test that shows the difference with non-string headers before and after this change
spec/grape/dsl/inside_route_spec.rb

@SlakrHakr
Copy link
Author

added documentation to README, CHANGELOG

based on my answer above, is documentation in UPGRADING needed?

@dblock
Copy link
Member

dblock commented Nov 15, 2024

added documentation to README, CHANGELOG

based on my answer above, is documentation in UPGRADING needed?

The fact that a warning is emitted now will surprise users, and we are trying them to change the code to do an explicit .to_s, so I think an entry in UPGRADING that explains this is in order, please. It doesn't hurt anyway.

@SlakrHakr the TOC needs a fix, see danger complaining.

@ericproulx WDYT about this change?

@SlakrHakr
Copy link
Author

fixed TOC and added documentation to UPGRADING doc

@ericproulx
Copy link
Contributor

added documentation to README, CHANGELOG
based on my answer above, is documentation in UPGRADING needed?

The fact that a warning is emitted now will surprise users, and we are trying them to change the code to do an explicit .to_s, so I think an entry in UPGRADING that explains this is in order, please. It doesn't hurt anyway.

@SlakrHakr the TOC needs a fix, see danger complaining.

@ericproulx WDYT about this change?

It makes sense but maybe we should emit a Deprecation instead of a warn. RSpec can be configured with raise_errors_for_deprecations! and that would probably help while upgrading instead of relying on a warn that emits on stderr.

@ericproulx
Copy link
Contributor

This suggestion could be a nice feature since its hard to comply 100% to the rack-spec. I think we could add something like lint_api! similar to do_not_route_options!. People would have the choice to add or not a condition to enforce it since it would be disabled by default.

@dblock
Copy link
Member

dblock commented Nov 16, 2024

I think I like a deprecation more as well. WDYT @SlakrHakr ?

@ericproulx
Copy link
Contributor

@SlakrHakr do you need some help ? I might make a release this weekend and your PR could be a part of it. Thanks :)

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.

Grape allows invalid headers to be set.
4 participants