Skip to content

fix(oauth): strip whitespace from HTTP header values on Linux#1401

Merged
RealKai42 merged 2 commits intomainfrom
kaiyi/fix-ascii-header-strip
Mar 11, 2026
Merged

fix(oauth): strip whitespace from HTTP header values on Linux#1401
RealKai42 merged 2 commits intomainfrom
kaiyi/fix-ascii-header-strip

Conversation

@RealKai42
Copy link
Copy Markdown
Collaborator

@RealKai42 RealKai42 commented Mar 11, 2026

Summary

  • Strip trailing whitespace/newlines from ASCII header values in _ascii_header_value() to fix connection errors on certain Linux systems (e.g. kernel 6.8.0-101) where platform.version() returns strings with trailing newlines
  • Add regression tests for header value sanitization

Test plan

  • pytest tests/auth/test_ascii_header.py — 5 tests pass

Checklist

  • I have read the CONTRIBUTING document.
  • I have linked the related issue, if any.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have run make gen-changelog to update the changelog.
  • I have run make gen-docs to update the user documentation.

Open with Devin

Copilot AI review requested due to automatic review settings March 11, 2026 09:43
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

Open in Devin Review

Copy link
Copy Markdown
Contributor

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 hardens OAuth HTTP header generation by sanitizing ASCII header values to avoid connection errors when Linux platform metadata (notably platform.version()) contains trailing whitespace/newlines.

Changes:

  • Strip leading/trailing whitespace from ASCII header values in _ascii_header_value().
  • Add regression tests covering newline/whitespace sanitization in _ascii_header_value() and _common_headers().
  • Document the fix in the English/Chinese release notes and root changelog.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/kimi_cli/auth/oauth.py Trims ASCII header values before building common OAuth headers.
tests/auth/test_ascii_header.py Adds regression coverage for header sanitization and whitespace-free common headers.
docs/zh/release-notes/changelog.md Adds unreleased note describing the Linux header whitespace fix (ZH).
docs/en/release-notes/changelog.md Adds unreleased note describing the Linux header whitespace fix (EN).
CHANGELOG.md Adds unreleased entry for the header sanitization fix.

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

Comment on lines 195 to 201
def _ascii_header_value(value: str, *, fallback: str = "unknown") -> str:
try:
value.encode("ascii")
return value
return value.strip()
except UnicodeEncodeError:
sanitized = value.encode("ascii", errors="ignore").decode("ascii").strip()
return sanitized or fallback
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

_ascii_header_value() now strips whitespace for ASCII-only values, but unlike the Unicode-sanitization path it does not fall back when the stripped result becomes empty (e.g., value is only whitespace). Consider returning fallback when value.strip() is empty as well, to keep behavior consistent and avoid emitting empty header values.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +12
"""Tests for _ascii_header_value and _common_headers in oauth module.

Regression tests for the issue where Linux kernel version strings containing
trailing whitespace/newlines (e.g. platform.version() returning
"#101-Ubuntu SMP ...\n") would be included in HTTP headers, causing
connection errors.
"""

from unittest.mock import patch

from kimi_cli.auth.oauth import _ascii_header_value, _common_headers

Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

This new test module is missing from __future__ import annotations, which appears to be used consistently across the existing test suite. Adding it keeps test files stylistically consistent and avoids surprises with forward references in type hints.

Copilot uses AI. Check for mistakes.
def test_no_whitespace_in_header_values(self, _mock_device_id, mock_platform) -> None:
"""All header values must be free of leading/trailing whitespace."""
mock_platform.node.return_value = "myhost"
mock_platform.version.return_value = "#101-Ubuntu SMP\n"
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

The test patches the entire kimi_cli.auth.oauth.platform module, but only configures node() and version(). Because _common_headers() also calls _device_model() (which calls platform.system(), platform.machine(), etc.), those calls become MagicMocks and can take unintended branches (e.g., system == "Darwin" evaluating truthy), making the test less robust. Prefer patching only platform.node/platform.version, or explicitly set system()/machine()/release() return values to deterministic strings.

Suggested change
mock_platform.version.return_value = "#101-Ubuntu SMP\n"
mock_platform.version.return_value = "#101-Ubuntu SMP\n"
mock_platform.system.return_value = "Linux"
mock_platform.machine.return_value = "x86_64"
mock_platform.release.return_value = "6.8.0-101"

Copilot uses AI. Check for mistakes.
@RealKai42 RealKai42 merged commit c0b4893 into main Mar 11, 2026
28 checks passed
@RealKai42 RealKai42 deleted the kaiyi/fix-ascii-header-strip branch March 11, 2026 09:59
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