Fix Grape allowing invalid headers to be set #2511
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
dblock
requested changes
Nov 5, 2024
There was a problem hiding this comment.
Thanks.
This needs some docs (README, CHANGELOG, possibly UPGRADING depending on the answers to below).
- What is the current behavior of
header 'integer key', 123? I am afraid we may be breaking some existing behavior. - 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.
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
force-pushed
the
convert-headers-to-strings
branch
from
80cacb7
to
6ffceb5
Compare
fixed an existing test that shows the difference with non-string headers before and after this change |
|
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 @SlakrHakr the TOC needs a fix, see danger complaining. @ericproulx WDYT about this change? |
|
fixed TOC and added documentation to UPGRADING doc |
It makes sense but maybe we should emit a Deprecation instead of a warn. RSpec can be configured with |
|
This suggestion could be a nice feature since its hard to comply 100% to the rack-spec. I think we could add something like |
|
I think I like a deprecation more as well. WDYT @SlakrHakr ? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2334
Ensure all header values are strings according to the Rack spec.
to_sin theheadermethod inlib/grape/dsl/headers.rb.headermethod inlib/grape/dsl/headers.rb.spec/grape/dsl/headers_spec.rbto verify that non-string header values are converted to strings and warnings are emitted.For more details, open the Copilot Workspace session.