Skip to content
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

fix(node-fetch): inspect null header values correctly #2051

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

ardatan
Copy link
Owner

@ardatan ardatan commented Feb 13, 2025

Fixes #1781

Summary by CodeRabbit

  • Bug Fixes

    • Improved stability by ensuring that header inspections now gracefully handle null values without causing crashes.
  • Tests

    • Added a test case to validate that headers with null values are correctly processed.

Copy link

coderabbitai bot commented Feb 13, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

This pull request patches the @whatwg-node/node-fetch package to prevent crashes when null header values are encountered during header inspection. The change updates the PonyfillHeaders class in the header inspection method by adding optional chaining to safely evaluate header values. Additionally, a new test case is added to verify that the inspection method correctly handles null values.

Changes

Files Summary
.changeset/nine-moose-shake.md Added patch note for the node-fetch package addressing crash issues with null header values.
packages/node-fetch/src/Headers.ts
packages/node-fetch/tests/Headers.spec.ts
Updated the PonyfillHeaders inspection method with optional chaining to safely handle null header values. Added a test case to validate this behavior.

Sequence Diagram(s)

sequenceDiagram
    participant Tester
    participant PonyfillHeaders
    Tester->>PonyfillHeaders: Set header with null value
    PonyfillHeaders->>PonyfillHeaders: Call inspect() method
    PonyfillHeaders->>PonyfillHeaders: Evaluate value?.includes(',')
    PonyfillHeaders-->>Tester: Return formatted header inspection result (handles null safely)
Loading

Poem

Hoppity hop, I code with delight,
Null headers no longer give a fright.
Optional checks keep crashes at bay,
In each line of code, I joyfully play.
With carrots and code, I cheer and sway!
🥕🐇 Happy bounces all the way!


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa9a8c9 and 6a18972.

📒 Files selected for processing (3)
  • .changeset/nine-moose-shake.md (1 hunks)
  • packages/node-fetch/src/Headers.ts (1 hunks)
  • packages/node-fetch/tests/Headers.spec.ts (1 hunks)
✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@ardatan ardatan merged commit 252f68b into master Feb 13, 2025
22 of 24 checks passed
Copy link
Contributor

🚀 Snapshot Release (alpha)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package Version Info
@whatwg-node/node-fetch 0.7.9-alpha-20250213122700-6a189724db9ab6211349779c44665b0b4f1006af npm ↗︎ unpkg ↗︎

Copy link
Contributor

@benchmarks/node-fetch results (consumeBody)

   ✓ active_handles.................: avg=140.72334 min=12      med=140     max=184     p(90)=160     p(95)=163    
     data_received..................: 19 MB  645 kB/s
     data_sent......................: 12 MB  413 kB/s
     http_req_blocked...............: avg=2.09µs    min=701ns   med=1.36µs  max=3.14ms  p(90)=2.09µs  p(95)=2.4µs  
     http_req_connecting............: avg=256ns     min=0s      med=0s      max=1.93ms  p(90)=0s      p(95)=0s     
     http_req_duration..............: avg=23.65ms   min=3.44ms  med=22.88ms max=1.22s   p(90)=29.51ms p(95)=31.07ms
       { expected_response:true }...: avg=23.65ms   min=3.44ms  med=22.88ms max=1.22s   p(90)=29.51ms p(95)=31.07ms
     http_req_failed................: 0.00%  ✓ 0           ✗ 126449
     http_req_receiving.............: avg=35.04µs   min=10.84µs med=26.5µs  max=17.48ms p(90)=40.51µs p(95)=46.74µs
     http_req_sending...............: avg=10.61µs   min=3.62µs  med=6.77µs  max=8.34ms  p(90)=10.46µs p(95)=14.04µs
     http_req_tls_handshaking.......: avg=0s        min=0s      med=0s      max=0s      p(90)=0s      p(95)=0s     
     http_req_waiting...............: avg=23.61ms   min=3.39ms  med=22.84ms max=1.22s   p(90)=29.47ms p(95)=31.01ms
     http_reqs......................: 126449 4214.529935/s
     iteration_duration.............: avg=47.41ms   min=10.55ms med=45.67ms max=1.24s   p(90)=51.92ms p(95)=57ms   
     iterations.....................: 63204  2106.581705/s
     vus............................: 100    min=100       max=100 
     vus_max........................: 100    min=100       max=100 

Copy link
Contributor

@benchmarks/node-fetch results (noConsumeBody)

   ✓ active_handles.................: avg=140.280292 min=12      med=140     max=183     p(90)=161     p(95)=164    
     data_received..................: 20 MB  672 kB/s
     data_sent......................: 13 MB  435 kB/s
     http_req_blocked...............: avg=2.43µs     min=661ns   med=1.42µs  max=6.17ms  p(90)=2.12µs  p(95)=2.51µs 
     http_req_connecting............: avg=463ns      min=0s      med=0s      max=5.76ms  p(90)=0s      p(95)=0s     
     http_req_duration..............: avg=22.69ms    min=2.88ms  med=21.93ms max=1.18s   p(90)=28.65ms p(95)=30.85ms
       { expected_response:true }...: avg=22.69ms    min=2.88ms  med=21.93ms max=1.18s   p(90)=28.65ms p(95)=30.85ms
     http_req_failed................: 0.00%  ✓ 0           ✗ 131730
     http_req_receiving.............: avg=36.75µs    min=9.61µs  med=24.71µs max=30.67ms p(90)=40.07µs p(95)=47.36µs
     http_req_sending...............: avg=12.61µs    min=3.42µs  med=6.96µs  max=11.61ms p(90)=10.91µs p(95)=15.59µs
     http_req_tls_handshaking.......: avg=0s         min=0s      med=0s      max=0s      p(90)=0s      p(95)=0s     
     http_req_waiting...............: avg=22.64ms    min=2.82ms  med=21.89ms max=1.18s   p(90)=28.6ms  p(95)=30.78ms
     http_reqs......................: 131730 4390.514077/s
     iteration_duration.............: avg=45.51ms    min=10.86ms med=43.77ms max=1.2s    p(90)=49.98ms p(95)=55.74ms
     iterations.....................: 65842  2194.490457/s
     vus............................: 13     min=13        max=100 
     vus_max........................: 100    min=100       max=100 

Copy link
Contributor

@benchmarks/server results (native)

     ✓ no-errors
     ✓ expected-result

   ✓ checks.........................: 100.00% ✓ 208524      ✗ 0     
     data_received..................: 21 MB   699 kB/s
     data_sent......................: 8.3 MB  278 kB/s
     http_req_blocked...............: avg=1.42µs   min=911ns    med=1.2µs    max=196.39µs p(90)=1.91µs   p(95)=2.07µs  
     http_req_connecting............: avg=1ns      min=0s       med=0s       max=127.99µs p(90)=0s       p(95)=0s      
     http_req_duration..............: avg=224.54µs min=169.14µs med=214.38µs max=7.38ms   p(90)=241.55µs p(95)=251.79µs
       { expected_response:true }...: avg=224.54µs min=169.14µs med=214.38µs max=7.38ms   p(90)=241.55µs p(95)=251.79µs
     http_req_failed................: 0.00%   ✓ 0           ✗ 104262
     http_req_receiving.............: avg=25.45µs  min=13.69µs  med=23.67µs  max=3.53ms   p(90)=30.88µs  p(95)=33.53µs 
     http_req_sending...............: avg=6.44µs   min=4.09µs   med=5.62µs   max=309.03µs p(90)=8.19µs   p(95)=8.91µs  
     http_req_tls_handshaking.......: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=192.65µs min=148.94µs med=182.69µs max=7.35ms   p(90)=206.59µs p(95)=216.11µs
     http_reqs......................: 104262  3475.120911/s
     iteration_duration.............: avg=283.16µs min=211.22µs med=272.21µs max=7.45ms   p(90)=302.74µs p(95)=314.91µs
     iterations.....................: 104262  3475.120911/s
     vus............................: 1       min=1         max=1   
     vus_max........................: 1       min=1         max=1   

Copy link
Contributor

@benchmarks/server results (ponyfill)

     ✓ no-errors
     ✓ expected-result

   ✓ checks.........................: 100.00% ✓ 303540      ✗ 0     
     data_received..................: 30 MB   997 kB/s
     data_sent......................: 12 MB   405 kB/s
     http_req_blocked...............: avg=1.34µs   min=861ns    med=1.16µs   max=178.41µs p(90)=1.81µs   p(95)=1.97µs  
     http_req_connecting............: avg=0ns      min=0s       med=0s       max=119.89µs p(90)=0s       p(95)=0s      
     http_req_duration..............: avg=136.67µs min=91.97µs  med=132.32µs max=5.86ms   p(90)=154.7µs  p(95)=161.97µs
       { expected_response:true }...: avg=136.67µs min=91.97µs  med=132.32µs max=5.86ms   p(90)=154.7µs  p(95)=161.97µs
     http_req_failed................: 0.00%   ✓ 0           ✗ 151770
     http_req_receiving.............: avg=24.15µs  min=11.92µs  med=22.68µs  max=2.33ms   p(90)=29.92µs  p(95)=32.68µs 
     http_req_sending...............: avg=6.08µs   min=3.94µs   med=5.25µs   max=813.75µs p(90)=7.92µs   p(95)=8.47µs  
     http_req_tls_handshaking.......: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=106.42µs min=67.38µs  med=101.68µs max=5.8ms    p(90)=121µs    p(95)=127.07µs
     http_reqs......................: 151770  5058.310155/s
     iteration_duration.............: avg=193.3µs  min=138.49µs med=188.12µs max=6ms      p(90)=214.25µs p(95)=223.65µs
     iterations.....................: 151770  5058.310155/s
     vus............................: 1       min=1         max=1   
     vus_max........................: 1       min=1         max=1   

Copy link
Contributor

@benchmarks/server results (undici)

     ✓ no-errors
     ✓ expected-result

   ✓ checks.........................: 100.00% ✓ 173796      ✗ 0    
     data_received..................: 18 MB   582 kB/s
     data_sent......................: 7.0 MB  232 kB/s
     http_req_blocked...............: avg=1.76µs   min=911ns    med=1.7µs    max=252.6µs  p(90)=2.26µs   p(95)=2.52µs 
     http_req_connecting............: avg=1ns      min=0s       med=0s       max=166.37µs p(90)=0s       p(95)=0s     
     http_req_duration..............: avg=270.24µs min=196.11µs med=255.04µs max=44.66ms  p(90)=287.66µs p(95)=301.4µs
       { expected_response:true }...: avg=270.24µs min=196.11µs med=255.04µs max=44.66ms  p(90)=287.66µs p(95)=301.4µs
     http_req_failed................: 0.00%   ✓ 0           ✗ 86898
     http_req_receiving.............: avg=28.76µs  min=14.62µs  med=26.79µs  max=3.32ms   p(90)=34.59µs  p(95)=37.88µs
     http_req_sending...............: avg=8.23µs   min=4.24µs   med=8.14µs   max=387.36µs p(90)=10.3µs   p(95)=12.18µs
     http_req_tls_handshaking.......: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s     
     http_req_waiting...............: avg=233.24µs min=166.78µs med=218.54µs max=44.57ms  p(90)=248.1µs  p(95)=261µs  
     http_reqs......................: 86898   2896.478302/s
     iteration_duration.............: avg=339.83µs min=246.97µs med=324.29µs max=44.84ms  p(90)=361.6µs  p(95)=378.3µs
     iterations.....................: 86898   2896.478302/s
     vus............................: 1       min=1         max=1  
     vus_max........................: 1       min=1         max=1  

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.

Null header value results in page crash
1 participant