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

requests onResponse params body not support raw binary data like notepack data #362

Open
ganshiqingyuan opened this issue Apr 23, 2021 · 12 comments

Comments

@ganshiqingyuan
Copy link

addBody (data) {
    const req = this.peek()
    if (req) {
      req.body += data
    }
  }

addBody in pipelinedRequestsQueue.js changes it to utf8 string but not notepack decode

@mcollina
Copy link
Owner

Can you please add a way to reproduce your problem?

@ganshiqingyuan
Copy link
Author

Can you please add a way to reproduce your problem?

requests: [{
  path: "xxx",
  setupRequest: function (client) {
    client.body = notepack.encode(xxx)
    return client
  },
  onResponse: function (status,body, context) {
    const decodeStr = notepack.decode(body);
    context.xxx = decodeStr.xxx
  }
}]

like this.
"notepack.io": "^2.3.0"

@mcollina
Copy link
Owner

mcollina commented Apr 23, 2021

Please provide a reproducible example: https://stackoverflow.com/help/minimal-reproducible-example

@ganshiqingyuan
Copy link
Author

ganshiqingyuan commented Apr 23, 2021

this test resp reappeared
if change addBody to

addBody (data) {
    const req = this.peek()
    if (req) {
      if(Buffer.isBuffer(data)){
        if(!Buffer.isBuffer(req.body)){
          req.body = Buffer.from('')
        }
        req.body = Buffer.concat([req.body, data])
      }
      else
      req.body += data
    }
  }

works ,,but this make all body become binary..do we need a option to control this

@mcollina
Copy link
Owner

I'm not sure your test would work because you are sending a body with a GET request - it's not supported in Fastify.

@ganshiqingyuan
Copy link
Author

I'm not sure your test would work because you are sending a body with a GET request - it's not supported in Fastify.

this test not care about the data of sending ,,the bug happen on response decode,,,you can clone it and have a test,,it realy work,,if you change addBody function in pipelinedRequestsQueue.js and it works good

@ganshiqingyuan
Copy link
Author

i can delete the requestData of a:123

@ganshiqingyuan
Copy link
Author

I'm not sure your test would work because you are sending a body with a GET request - it's not supported in Fastify.

have you test this problem?

@mcollina
Copy link
Owner

mcollina commented May 7, 2021

We do not parse bodies for GETs in fastify.

@ganshiqingyuan
Copy link
Author

We do not parse bodies for GETs in fastify.

i has delete the body,

@twylite
Copy link

twylite commented Dec 21, 2022

I am using autocannon to load test an HTTPS endpoint that accepts (via POST) and returns Content-Type 'application/x-thrift', i.e. an payload encoded using Apache Thrift TBinaryProtocol.

I am experiencing the problem described in this issue, which is that PipelinedRequestsQueue.addBody() appends data (a Buffer) to req.body (a string), which implicitly invokes data.toString() with the default encoding 'utf8'. The first bytes of any TBinaryProtocol message header are <0x80, 0x01, ...> which is an invalid UTF8 sequence, so the resulting req.body starts <U+FFFD, U+0001, ...>, which is an irreversible corruption of the binary data.

To reproduce save this code as server.js and run it:

const http = require('http');
http.createServer((req,res)=> {
  res.writeHead(200, { 'Content-Type': 'application/octet-stream' });
  res.end(Buffer.from('80010002000000106973737565437265646974546f6b656e000000010f00', 'hex'));
}).listen(8888);

You can capture the server response using curl -i http://127.0.0.1:8888/ --output - > tmp.out and check it in a hex/binary editor

Then save this code as client.js and run it:

const autocannon = require('autocannon')
opts = {
  url: 'http://127.0.0.1:8888/',
  connections: 1,
  amount: 1,
  verifyBody: (body) => {
    console.log('Body:', Buffer.from(body, 'binary').toString('hex'))
  }
}
const cannonInst = autocannon(opts).on('done', (res) => { console.log(res.statusCodeStats) })

The client outputs Body: fd010002000000106973737565437265646974546f6b656e000000010f00, instead of the expected 8001.... The leading fd is a truncation of U+FFFD produced by Buffer.from(body, 'binary').

My understanding is that the root cause is that httpClient.js and pipelinedRequestsQueue.js do not handle encoding/charset; or at least I cannot locate any code that does, and the req.body += data in PipelinedRequestsQueue.addBody() always treats data as having UTF8 encoding.

A backwards-compatible solution may be to introduce a this.opts.responseCharset = this.opts.responseCharset || 'utf8' in function Client in httpClient.js, and then in the call to this.pipelinedRequests.addBody(body.slice(start, start + len)) change the body to body.slice(start, start + len).toString(this.opts.responseCharset). The responseCharset value can then be modified via setupClient.

@twylite
Copy link

twylite commented Dec 21, 2022

The order of initialisation in function Client in httpClient.js allows me to implement a work-around by subclassing PipelinedRequestsQueue and overriding addBody(data) to specify the encoding of data. This works, but it is brittle and clumsy, and I would not consider it a solution.

// My autocannon script
import autocannon from 'autocannon'
import PipelinedRequestsQueue from 'autocannon/lib/pipelinedRequestsQueue.js'
  
class MyPipelinedRequestsQueue extends PipelinedRequestsQueue {
  addBody (data) {
    const req = this.peek()
    if (req) {
      req.body += data.toString('binary') // <----- specify encoding
    }
  }
} 

var opts = {
  // ...
  method: 'POST',
  headers: { 'Content-Type': 'application/x-thrift' },
  setupClient: (client) => {
    client.pipelinedRequests = new MyPipelinedRequestsQueue() // <----- use my subclass
  }
}

var instance = autocannon(opts)

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

No branches or pull requests

3 participants