Fix for sending files with size 0 on stat

[Giotino]

Checklist

• I have read the documentation.
• I have included a pull request description of my changes.
• I have included some tests.

Fixes #1486

[Giotino]

This issue made me realize that we might have a TOCTOU problem.

This code simulate what Got does, but one step at a time.

const fs = require('fs');
const http = require('http');
const stream = require('stream');

fs.writeFileSync('test.txt', 'TEST');
const readStream = fs.createReadStream('test.txt');
const { size: sizeBefore } = fs.statSync('test.txt');
console.log('Size before', sizeBefore);
const request = http.request({
  host: '127.0.0.1',
  port: '8080',
  headers: {
    'content-length': sizeBefore,
  },
});

fs.writeFileSync('test.txt', 'FILE CHANGED'); // Something (not Got) write the file
const { size: sizeAfter } = fs.statSync('test.txt');
console.log('Size after', sizeAfter);

stream.pipeline(readStream, request, (err) => {
  console.log('Ended');
  console.error('Error', err);
});

The result is
Node.js output (nothing wrong):

Size before 4
Size after 12
Ended
Error undefined

Request made to the server:

GET / HTTP/1.1
content-length: 4
Host: 127.0.0.1:8080
Connection: close

FILE CHANGED

The content-length and the body length do not match.

I don't think we should care about this, it should be the user that must not write to the file while reading it.

The example before has a 100% success rate, with Got it become more difficult to replicate (but it's still possible).

Examples from Got (inserting code in specific places and praying the scheduler for cooperation):

POST / HTTP/1.1
user-agent: got (https://github.com/sindresorhus/got)
content-length: 21
accept-encoding: gzip, deflate, br
Host: 127.0.0.1:8080
Connection: close

TEST CHANGED

POST / HTTP/1.1
user-agent: got (https://github.com/sindresorhus/got)
content-length: 5
accept-encoding: gzip, deflate, br
Host: 127.0.0.1:8080
Connection: close

TESTACHANGED

[szmarczak]

This is a completely valid argument and I agree with you 100%.

[szmarczak]

So why we don't just remove the if (body instanceof ReadStream check? It would be the easiest fix to get rid of the problem mentioned :)

[Giotino]

So why we don't just remove the if (body instanceof ReadStream check? It would be the easiest fix to get rid of the problem mentioned :)

Because it's going to break the upload progress function.

Actually we can use chunked encoding for every stream and communicate the body size in a different way (not using content-length) to the code handling the upload progress.

This is a solution if the "problem mentioned" was the PR itself.
If you where talking about the TOCTOU, we have to disable the upload progress for streams, but actually it wouldn't make any sense, because, as you can see here

POST / HTTP/1.1
user-agent: got (https://github.com/sindresorhus/got)
content-length: 5
accept-encoding: gzip, deflate, br
Host: 127.0.0.1:8080
Connection: close

TESTACHANGED

the file can be modified even while reading, so the user has to be fully responsible for that.

[szmarczak]

Because it's going to break the upload progress function.

But that will be only for files. People can provide their own content-length header whenever they want to. Got doesn't lock files as it's not its job to do.

[Giotino]

But that will be only for files. People can provide their own content-length header whenever they want to. Got doesn't lock files as it's not its job to do.

Sure, but if we can still automate the upload progress on the file that supports it (the ones without size != 0) it would be great.

[Giotino]

Mouse slipped on "Close"...

BTW

I think we can still use content-length only for the files that have size > 0. In that case the upload progress is going to work as always. When we encounter a file with size == 0 there are 2 options:

1. The file is actually empty, the upload progress has no meaning anyways.
2. The file is not a "real file", it's going to be sent with chunked encoding and the upload progress is not going to work (because it can't).

[szmarczak]

Sure, but if we can still automate the upload progress on the file that supports it (the ones without size != 0) it would be great.

@Giotino I'd just read the content-length header (if present) and use that as the file size. As you've already mentioned people still can modify the file on the fly.

[Giotino]

@Giotino I'd just read the content-length header (if present) and use that as the file size. As you've already mentioned people still can modify the file on the fly.

Ok, but I think there's someone out there that use the upload progress feature on files (as it's documented).
So I think the best way to handle this should be apply the fix in this PR, then, in Got 12, completely remove the stat for files, as you suggested.

[szmarczak]

But we're already developing Got 12 in the master branch, don't we?

[Giotino]

Since we don't know how much time is gonna take to release Got 12 I would like to be able to publish little fix like this to Got 11. I think that until Got 12 is finally released we should still patch Got 11, at least when the fix is not huge and/or breaking.

[szmarczak]

/cc @sindresorhus

[sindresorhus]

@Giotino I generally agree, but doing it this way has a tendency to just delay the major version for a long time (there's always going to be "just one more fix"). I think it's better to commit the proper breaking change to master first and then the non-breaking change can be applied to a v11 branch afterward.

[sindresorhus]

Actually, since there are still no breaking changes on master, I guess we can just merge this in and do a minor release.

@Giotino Can you open an issue or PR about the breaking change?

[szmarczak]

Ah, I know why it worked on Got 10 alpha but not Got 10 beta:

got/source/request-as-event-emitter.ts

Line 394 in 72390c2

if ((uploadBodySize > 0 || options.method === 'PUT') && !is.undefined(uploadBodySize)) {

See uploadBodySize > 0. It seems I removed it for some reason...