Skip to content

Safely split incoming request headers, remove support for token presence in request body#39427

Merged
jacobshandling merged 8 commits intomainfrom
prevent-potentially-unbounded-splits
Feb 18, 2026
Merged

Safely split incoming request headers, remove support for token presence in request body#39427
jacobshandling merged 8 commits intomainfrom
prevent-potentially-unbounded-splits

Conversation

@jacobshandling
Copy link
Copy Markdown
Contributor

@jacobshandling jacobshandling commented Feb 5, 2026

Related issues:

See https://fleetdm.slack.com/archives/C019WG4GH0A/p1770322925865209

  • Added/updated automated tests
  • QA'd all new/changed functionality manually

Copy link
Copy Markdown
Member

@zwass zwass left a comment

Choose a reason for hiding this comment

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

How about an approach where we use strings.CutPrefix(headers, "BEARER ") and then checking the bool value and using the cut string if the prefix was found?

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.28%. Comparing base (fc31741) to head (a5c99c5).
⚠️ Report is 48 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #39427    +/-   ##
========================================
  Coverage   66.27%   66.28%            
========================================
  Files        2439     2440     +1     
  Lines      195401   195554   +153     
  Branches     8615     8615            
========================================
+ Hits       129511   129619   +108     
- Misses      54160    54200    +40     
- Partials    11730    11735     +5     
Flag Coverage Δ
backend 68.08% <100.00%> (+<0.01%) ⬆️

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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jacobshandling jacobshandling force-pushed the prevent-potentially-unbounded-splits branch from e980893 to 9fc54f4 Compare February 6, 2026 06:22
@jacobshandling
Copy link
Copy Markdown
Contributor Author

@zwass ah, that'd be nice and clean if we're okay only supporting the all-caps version?

I also based splitToken on this fix merged to jwt

@jacobshandling
Copy link
Copy Markdown
Contributor Author

@zwass it seems like "Bearer" should be treated case-insensitively while the token value should be case-sensitive. Are you okay with the current approach?

@jacobshandling jacobshandling changed the title Safely split incoming request headers Safely split incoming request headers, remove support for token presence in request body Feb 11, 2026
@iansltx
Copy link
Copy Markdown
Contributor

iansltx commented Feb 11, 2026

@jacobshandling Split of form parsing removal and dead code removal makes sense. The form field removal bits seem fine, though since they're only basically two LOC I'll defer to Zach for full PR approval.

@jacobshandling jacobshandling force-pushed the prevent-potentially-unbounded-splits branch from 43f3b2f to f38f71d Compare February 16, 2026 23:32
Comment thread server/contexts/token/token.go Outdated
Comment thread server/contexts/token/token.go Outdated
Comment thread server/contexts/token/token.go Outdated
Comment thread server/contexts/token/token.go Outdated
Comment thread server/contexts/token/token.go Outdated
@kilo-code-bot
Copy link
Copy Markdown
Contributor

kilo-code-bot Bot commented Feb 16, 2026

Code Review Summary

Status: 5 Suggestions Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 5
Issue Details (click to expand)

SUGGESTION

File Line Issue
server/contexts/token/token.go 52 Typo: foundDelimeterfoundDelimiter
server/contexts/token/token.go 53 Matching variable rename for foundDelimiter
server/contexts/token/token.go 59 Typo: "unecessary" → "unnecessary"
server/contexts/token/token.go 60 Typo: unexpectedDelimeterFoundunexpectedDelimiterFound
server/contexts/token/token.go 61 Matching variable rename for unexpectedDelimiterFound

Notes

Well, I think this is a really thoughtful change, neighbor. Replacing strings.Split with strings.Cut is a wonderful way to prevent unbounded splitting on the Authorization header — it's the kind of careful, security-minded work that helps keep everyone safe. The logic in parseHeaderLimited is clear and well-structured, and I appreciate that you've added thorough test cases covering the new behavior.

The removal of the FormValue("token") fallback is a meaningful behavioral change — I trust you've confirmed that no existing clients depend on that path. The only things I noticed were a few small spelling inconsistencies in variable names and comments, which I've left as suggestions.

Thank you for taking the time to make this improvement.

Files Reviewed (2 files)
  • server/contexts/token/token.go - 5 suggestions
  • server/contexts/token/token_test.go - 0 issues

Fix these issues in Kilo Cloud

@jacobshandling
Copy link
Copy Markdown
Contributor Author

@iansltx would you please review this PR? I confirmed @zwass is okay with someone else pushing this through

@jacobshandling jacobshandling merged commit 0eb8d43 into main Feb 18, 2026
64 of 66 checks passed
@jacobshandling jacobshandling deleted the prevent-potentially-unbounded-splits branch February 18, 2026 16:50
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.

Remove unused request form processing for bearer tokens and associated dead frontend code

3 participants