Skip to content

fix(flags): parse application/json content-type headers in a less brittle way (while still respecting compression headers) #31798

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

Merged
merged 5 commits into from
May 1, 2025

Conversation

dmarticus
Copy link
Contributor

Important

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Problem

Fixes the issue @ioannisj noticed here: PostHog/posthog-ios#345 (comment)

Was being too picky in parsing content-type headers.

Changes

  • relax content-type header parsing
  • test for this type of request

Does this work well for both Cloud and self-hosted?

Yes

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Modified content-type header parsing in feature flags API to handle requests with charset specifications, fixing iOS client compatibility issues.

  • Updated rust/feature-flags/src/api/request_handler.rs to use starts_with for content-type validation instead of exact matching
  • Added test case to verify handling of application/json; charset=utf-8 content-type headers
  • Maintains existing compression header support while being more lenient with charset specifications

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@dmarticus dmarticus requested review from Copilot and haacked April 30, 2025 23:00
Copy link
Contributor

@Copilot 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 relaxes the parsing of content-type headers to handle JSON requests more flexibly and to support different encoding and compression scenarios.

  • Adjusts base64 handling based solely on an explicit content-type header
  • Refines the matching of "application/json" with potential additional parameters
  • Adds a test to validate decoding of requests with a charset in the content-type header

Copy link
Contributor

@haacked haacked left a comment

Choose a reason for hiding this comment

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

I'm fine with the change as-is because I assume this addresses what posthog-ios sends us. But I do think we should follow-up and properly parse the headers in case custom API clients call us.

// Special case: if the Content-Type header explicitly specifies base64 encoding,
// this takes precedence over the query parameter for compression. In this case,
// the body is decoded as base64, and the query parameter is disregarded.
if content_type.starts_with("application/json; encoding=base64") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we parse the content type and then compare. For example, this is a valid content type: Content-Type: application/json; charset=utf-8; encoding=base64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

per slack: I'm just going to explicitly support the content types that we send from our SDKs.

@dmarticus dmarticus merged commit 3b46d64 into master May 1, 2025
66 checks passed
@dmarticus dmarticus deleted the fix/flags-400-error-on-ios branch May 1, 2025 17:31
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