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(fetch): missing body-mixin brand checks #1498

Merged
merged 2 commits into from Jun 18, 2022

Conversation

KhafraDev
Copy link
Member

@KhafraDev KhafraDev commented Jun 16, 2022

This adds brand checks to body mixin methods & properties. These are webidl checks done throughout the spec. This disallows the following:

// accessing these methods without a proper "this" value
Response.prototype.bodyUsed
Response.prototype.body
Request.prototype.bodyUsed
Request.prototype.body

Request.prototype.text.call({ some: 'this value' })
Response.prototype.text.call({ some: 'this value' })
// same for all other body mixin methods
// "this" must be a Request/Response object now.

These checks are already done everywhere else.

Refs: #1436 #1430

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2022

Codecov Report

Merging #1498 (89d97f4) into main (a1600f9) will decrease coverage by 0.09%.
The diff coverage is 97.95%.

@@            Coverage Diff             @@
##             main    #1498      +/-   ##
==========================================
- Coverage   94.74%   94.64%   -0.10%     
==========================================
  Files          49       49              
  Lines        4297     4312      +15     
==========================================
+ Hits         4071     4081      +10     
- Misses        226      231       +5     
Impacted Files Coverage Δ
lib/fetch/body.js 98.51% <97.87%> (+0.18%) ⬆️
lib/fetch/request.js 85.89% <100.00%> (ø)
lib/fetch/response.js 93.33% <100.00%> (ø)
lib/client.js 97.63% <0.00%> (-0.66%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1600f9...89d97f4. Read the comment docs.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Could you clarify in the PR description what this does? It's a bit unclear:

@KhafraDev
Copy link
Member Author

Done, sorry 😄

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit e83b849 into nodejs:main Jun 18, 2022
@KhafraDev KhafraDev deleted the body-mixin-brand-checks branch June 18, 2022 16:46
KhafraDev added a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
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.

None yet

3 participants