Skip to content

Conversation

@yash-rajpal
Copy link
Member

@yash-rajpal yash-rajpal commented Oct 14, 2025

Proposed changes (including videos or screenshots)

Enabling trace method call setting crashes the server due to how httpHeaders is consumed.

  • Meteor DDP - When using DDP, it exposes httpHeaders as plain javascript object
  • Hono - When using HTTP calls or API through hono, it exposes httpHeaders as instance of Header class.

We were not handling both cases properly which was causing the crash.

Issue(s)

fixes #36011

Steps to test or reproduce

  • Enable trace method calls setting.
  • Login using DDP client
  • Server crashes

Further comments

SUP-861

Summary by CodeRabbit

  • Bug Fixes
    • Fixed a server crash when enabling the “Trace method calls” setting.
    • Improved handling of incoming request headers to be more robust across different sources.
    • Ensured sensitive values (such as auth tokens and specific cookies) remain consistently redacted in logs and diagnostics.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Oct 14, 2025

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is targeting the wrong base branch. It should target 7.12.0, but it targets 7.11.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Oct 14, 2025

🦋 Changeset detected

Latest commit: 7fbf328

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 42 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/http-router Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/federation-service Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/license Patch
@rocket.chat/media-calls Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/ui-voip Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@yash-rajpal yash-rajpal added this to the 7.12.0 milestone Oct 14, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 14, 2025

Walkthrough

Updates getModifiedHttpHeaders to accept both Headers and plain objects, adding a runtime branch to normalize input. Maintains existing redaction for x-auth-token and rc_token in cookies. Adds unit tests parameterized over both input types. Adds a changeset entry for a patch release documenting the fix.

Changes

Cohort / File(s) Summary
Changeset metadata
.changeset/new-eels-deliver.md
Adds patch changeset for @rocket.chat/meteor noting fix for crash when “Trace method calls” is enabled.
Header processing implementation
apps/meteor/app/lib/server/functions/getModifiedHttpHeaders.ts
Extends function to accept Headers or Record<string, string>. Normalizes via Object.fromEntries when Headers. Preserves redaction of x-auth-token and rc_token in cookies.
Unit tests for header processing
apps/meteor/tests/unit/app/lib/server/functions/getModifiedHttpHeaders.tests.ts
Refactors tests to run against both input variants (Headers, plain object) via parameterized helper. Keeps existing expectations for redaction behavior.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant Func as getModifiedHttpHeaders
  Note over Caller,Func: Normalize input and redact sensitive header data

  Caller->>Func: getModifiedHttpHeaders(httpHeaders)
  alt httpHeaders is Headers
    Func->>Func: Convert via Object.fromEntries(headers)
  else httpHeaders is Record<string,string>
    Func->>Func: Use object directly
  end

  Func->>Func: Redact x-auth-token -> "[redacted]"
  Func->>Func: Redact rc_token inside Cookie header
  Func-->>Caller: Return modified headers object
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • ggazzo

Poem

I twitch my ears at headers’ trails,
Two paths converge, yet neither fails—
I sniff the tokens, hide their scent,
rc_cookie crumbs, to void are sent.
With patched-up hops, the server sings,
Trace away—no broken things! 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title Check ❓ Inconclusive The provided title is concise but does not clearly describe the specific fix for handling httpHeaders in getModifiedHttpHeaders to prevent the server crash when trace method calls are enabled, making it too generic to convey the change’s intent precisely. Consider updating the title to explicitly mention preventing the crash caused by handling httpHeaders in getModifiedHttpHeaders when trace method calls are enabled.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues Check ✅ Passed The pull request updates getModifiedHttpHeaders to correctly handle both Headers instances and plain objects, preventing the TypeError described in issue #36011 and ensuring trace method calls no longer crash the server, and adds parameterized tests to verify both input forms which addresses the linked issues’ coding requirements.
Out of Scope Changes Check ✅ Passed All modifications in this pull request directly support handling the httpHeaders crash when trace method calls are enabled and include related test updates and a changeset entry for the patch, with no unrelated code alterations introduced.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/trace-method-calls

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between fef8715 and 9cec222.

📒 Files selected for processing (3)
  • .changeset/new-eels-deliver.md (1 hunks)
  • apps/meteor/app/lib/server/functions/getModifiedHttpHeaders.ts (1 hunks)
  • apps/meteor/tests/unit/app/lib/server/functions/getModifiedHttpHeaders.tests.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/tests/unit/app/lib/server/functions/getModifiedHttpHeaders.tests.ts (1)
apps/meteor/app/lib/server/functions/getModifiedHttpHeaders.ts (1)
  • getModifiedHttpHeaders (1-26)
🪛 LanguageTool
.changeset/new-eels-deliver.md

[grammar] ~5-~5: There might be a mistake here.
Context: ...eteor': patch --- Fixes an issue where enabling trace method calls setting was crashing...

(QB_NEW_EN)


[grammar] ~5-~5: There might be a mistake here.
Context: ...d calls setting was crashing the server.

(QB_NEW_EN)

⏰ 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). (3)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (5)
.changeset/new-eels-deliver.md (1)

1-5: LGTM! Changeset properly documents the fix.

The patch release entry correctly describes the issue and aligns with the PR objectives. The static analysis grammar hints are false positives—the text is clear and grammatically correct.

apps/meteor/app/lib/server/functions/getModifiedHttpHeaders.ts (2)

1-8: Elegant solution for handling dual input types.

The runtime branching with instanceof Headers correctly normalizes both Meteor DDP (plain object) and Hono (Headers class) inputs. The spread operator ensures immutability while maintaining efficiency. This directly addresses the crash reported in issue #36011.


1-26: No call sites outside tests found. The only references to getModifiedHttpHeaders are in unit tests, which already handle both Headers and plain objects; the signature change is fully backward compatible.

apps/meteor/tests/unit/app/lib/server/functions/getModifiedHttpHeaders.tests.ts (2)

5-14: Excellent parameterized test design.

The inputVariants array with makeInput factory functions elegantly covers both the HTTP path (Headers) and DDP path (plain object) without duplicating test logic. This approach improves maintainability while ensuring comprehensive coverage of both input types.


17-73: Comprehensive test coverage for both input variants.

The parameterized tests thoroughly validate all redaction scenarios (x-auth-token, rc_token in cookies, both, neither) for both input types. The nested describe blocks provide clear test organization and output.


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.

@yash-rajpal yash-rajpal marked this pull request as ready for review October 14, 2025 22:11
@yash-rajpal yash-rajpal requested a review from a team as a code owner October 14, 2025 22:11
@codecov
Copy link

codecov bot commented Oct 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.63%. Comparing base (4ab2b7f) to head (7fbf328).
⚠️ Report is 4 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop   #37229   +/-   ##
========================================
  Coverage    67.63%   67.63%           
========================================
  Files         3341     3341           
  Lines       114016   114018    +2     
  Branches     20667    20668    +1     
========================================
+ Hits         77118    77120    +2     
+ Misses       34222    34219    -3     
- Partials      2676     2679    +3     
Flag Coverage Δ
e2e 57.34% <ø> (-0.01%) ⬇️
unit 71.60% <100.00%> (-0.01%) ⬇️

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

🚀 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.

Copy link
Member

@abhinavkrin abhinavkrin left a comment

Choose a reason for hiding this comment

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

LGTM!

@yash-rajpal yash-rajpal added the stat: QA assured Means it has been tested and approved by a company insider label Oct 17, 2025
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Oct 17, 2025
@kodiakhq kodiakhq bot merged commit 2f0c4af into develop Oct 17, 2025
49 checks passed
@kodiakhq kodiakhq bot deleted the fix/trace-method-calls branch October 17, 2025 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nothing works with “Trace method calls” enabled in 7.6.0

3 participants