Skip to content

🐛 fix: prevent content-type confusion in ParseVendorSpecificContentType#198

Merged
ReneWerner87 merged 2 commits into
masterfrom
fix-content-type-parser-vulnerability
May 15, 2026
Merged

🐛 fix: prevent content-type confusion in ParseVendorSpecificContentType#198
ReneWerner87 merged 2 commits into
masterfrom
fix-content-type-parser-vulnerability

Conversation

@gaby
Copy link
Copy Markdown
Member

@gaby gaby commented May 10, 2026

Motivation

  • Prevent misclassification of non-application media types caused by a length/position-based fast-path in ParseVendorSpecificContentType that could treat any top-level token with the same slash position as application/ as application/*.

Description

  • Replace the length/position check with an explicit prefix check using strings.HasPrefix(working, "application/"), change the constant to contentTypePrefixApplicationWithSlash, and add a regression test for the input aaaaaaaaaaa/vnd.api+json to ensure the original top-level type is preserved.

Summary by CodeRabbit

  • Refactor

    • Improved content-type detection logic for more reliable handling of application-style types.
  • Tests

    • Added coverage for vendor-specific content types, including extended-prefix edge cases to ensure correct normalization.

Review Change Stack

Copilot AI review requested due to automatic review settings May 10, 2026 18:15
@gaby gaby requested a review from a team as a code owner May 10, 2026 18:15
@gaby gaby requested review from ReneWerner87, efectn and sixcolors and removed request for a team May 10, 2026 18:15
@gaby gaby changed the title fix: prevent content-type confusion in ParseVendorSpecificContentType 🐛 fix: prevent content-type confusion in ParseVendorSpecificContentType May 10, 2026
@ReneWerner87 ReneWerner87 added the ☢️ Bug Something isn't working label May 10, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 98989b91-757e-4dfb-93ac-6cbb474e0cc4

📥 Commits

Reviewing files that changed from the base of the PR and between cabbc9d and 9b0ae6a.

📒 Files selected for processing (1)
  • http.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • http.go
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Analyze (actions)
  • GitHub Check: Analyze (go)

📝 Walkthrough

Walkthrough

This PR refactors ParseVendorSpecificContentType to use strings.HasPrefix instead of length-based prefix checking. A new contentTypePrefixApplicationWithSlash string constant replaces the old contentTypePrefixApplicationWithSlashLen integer constant. A test case validates vendor-type normalization with extended prefixes.

Changes

Content Type Prefix Refactoring

Layer / File(s) Summary
Constants Definition
http.go
Introduced contentTypePrefixApplicationWithSlash constant and removed contentTypePrefixApplicationWithSlashLen for cleaner prefix checking.
Prefix Detection Logic
http.go
Changed application/* detection from comparing slashIndex+1 against precomputed length to using strings.HasPrefix with the new constant.
Test Coverage
http_test.go
Added edge-case assertion verifying vendor-specific +json normalization with extended prefixes (e.g., aaaaaaaaaaa/vnd.api+jsonaaaaaaaaaaa/json).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • gofiber/utils#125: Modifies the same ParseVendorSpecificContentType function with an optional case-insensitive parameter, indicating related maintenance to this parser function.

Suggested reviewers

  • sixcolors
  • ReneWerner87
  • efectn

Poem

🐰 A rabbit found a string prefix bright,
Replaced a number with clearer sight,
Vendor +json now trims its way,
Long prefixes bow and say hooray,
Hopping home with tests all right! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main security/bug fix: preventing content-type confusion in ParseVendorSpecificContentType by replacing a flawed length-based check with an explicit prefix validation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-content-type-parser-vulnerability

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.1)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the content-type parsing logic in http.go by replacing a manual length check with a more readable strings.HasPrefix call. It also adds a new test case in http_test.go to verify the handling of vendor-specific content types with custom prefixes. I have no feedback to provide as there were no review comments to evaluate.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.15%. Comparing base (79a8938) to head (9b0ae6a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #198   +/-   ##
=======================================
  Coverage   84.15%   84.15%           
=======================================
  Files          14       14           
  Lines        1155     1155           
=======================================
  Hits          972      972           
  Misses        152      152           
  Partials       31       31           
Flag Coverage Δ
unittests 84.15% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gaby
Copy link
Copy Markdown
Member Author

gaby commented May 10, 2026

@copilot Error: http.go:14:1: File is not properly formatted (gofmt)

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 fixes a correctness bug in ParseVendorSpecificContentType where a position/length-based “application/*” fast-path could misclassify non-application media types whose slash happened to be at the same index as "application/". The change makes the check explicit and adds a regression test to prevent reintroducing the misclassification.

Changes:

  • Replace the slash-position equality check with strings.HasPrefix(working, "application/") to only apply the known-type mapping to true application/* top-level types.
  • Replace the length constant with a string prefix constant (contentTypePrefixApplicationWithSlash).
  • Add a regression test ensuring "aaaaaaaaaaa/vnd.api+json" is parsed as "aaaaaaaaaaa/json" (not "application/json").

Reviewed changes

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

File Description
http.go Tightens the application/* detection logic to an explicit prefix match to avoid top-level type confusion.
http_test.go Adds a regression test covering the prior misclassification scenario.

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

Copy link
Copy Markdown
Contributor

Copilot AI commented May 10, 2026

@copilot Error: http.go:14:1: File is not properly formatted (gofmt)

Fixed in 9b0ae6a by running the required formatting on http.go; local go test ./... passes and the gofmt/gofumpt issue is resolved.

Copilot finished work on behalf of gaby May 10, 2026 18:23
@ReneWerner87 ReneWerner87 merged commit 2220a60 into master May 15, 2026
20 checks passed
@ReneWerner87 ReneWerner87 deleted the fix-content-type-parser-vulnerability branch May 15, 2026 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

☢️ Bug Something isn't working codex

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants