Skip to content

Proper header production#357

Merged
alexindigo merged 1 commit intoform-data:masterfrom
ekkis:master
Jun 16, 2017
Merged

Proper header production#357
alexindigo merged 1 commit intoform-data:masterfrom
ekkis:master

Conversation

@ekkis
Copy link
Copy Markdown

@ekkis ekkis commented Jun 16, 2017

the existing code uses a for loop to iterate through headers but does
not check that the values are actually properties, therefore it picks
up methods. these methods can appear as they are defined within the
Object.prototype and including them in the headers breaks requests as
the stringified methods likely contain carriage returns, which breaks
the request due to malformation of the header

in general, it is better to use Object.keys(headers).forEach() but I
didn’t want to change the style of the code so a call to
.hasOwnProperty() solves the problem

the existing code uses a for loop to iterate through headers but does
not check that the values are actually properties, therefore it picks
up methods.  these methods can appear as they are defined within the
Object.prototype and including them in the headers breaks requests as
the stringified methods likely contain carriage returns, which breaks
the request due to malformation of the header

in general, it is better to use Object.keys(headers).forEach() but I
didn’t want to change the style of the code so a call to
.hasOwnProperty() solves the problem
@coveralls
Copy link
Copy Markdown

coveralls commented Jun 16, 2017

Coverage Status

Coverage increased (+0.01%) to 97.99% when pulling 8ff849c on ekkis:master into 3eabb17 on form-data:master.

@ekkis
Copy link
Copy Markdown
Author

ekkis commented Jun 16, 2017

as added documentation, submitting a malformed header breaks the server like this:

/usr/src/app/node_modules/dicer/lib/HeaderParser.js:101
            this.header[h].push(m[2]);                                          
                           ^
                                                                                
TypeError: this.header[h].push is not a function
    at HeaderParser._parseHeader (/usr/src/app/node_modules/dicer/lib/HeaderParser.js:101:28)
    at HeaderParser._finish (/usr/src/app/node_modules/dicer/lib/HeaderParser.js:61:10)     
    at SBMH.<anonymous> (/usr/src/app/node_modules/dicer/lib/HeaderParser.js:41:12)         
    at emitMany (events.js:127:13)                                              
    at SBMH.emit (events.js:204:7)
    at SBMH._sbmh_feed (/usr/src/app/node_modules/streamsearch/lib/sbmh.js:159:14)          
    at SBMH.push (/usr/src/app/node_modules/streamsearch/lib/sbmh.js:56:14)     
    at HeaderParser.push (/usr/src/app/node_modules/dicer/lib/HeaderParser.js:47:19)                                                                            
    at Dicer._oninfo (/usr/src/app/node_modules/dicer/lib/Dicer.js:197:25)
    at SBMH.<anonymous> (/usr/src/app/node_modules/dicer/lib/Dicer.js:127:10)   
    at emitMany (events.js:127:13)
    at SBMH.emit (events.js:204:7)                                              
    at SBMH._sbmh_feed (/usr/src/app/node_modules/streamsearch/lib/sbmh.js:159:14)                                                                              
    at SBMH.push (/usr/src/app/node_modules/streamsearch/lib/sbmh.js:56:14)
    at Dicer._write (/usr/src/app/node_modules/dicer/lib/Dicer.js:109:17)       
    at doWrite (_stream_writable.js:329:12)

@alexindigo
Copy link
Copy Markdown
Member

Looks good. Thanks for the fix.

@alexindigo alexindigo merged commit 2cd7226 into form-data:master Jun 16, 2017
@alexindigo
Copy link
Copy Markdown
Member

Will publish updated version later tonight.

@ekkis
Copy link
Copy Markdown
Author

ekkis commented Jun 16, 2017

I didn't review the entire codebase but you might want to look for similar uses of the for loop

@alexindigo
Copy link
Copy Markdown
Member

Ok, I will check for more. Thanks.

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.

3 participants