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

CLI option --max-filesize not always enforced #3245

Closed
lambrospetrou opened this issue Sep 19, 2024 · 3 comments
Closed

CLI option --max-filesize not always enforced #3245

lambrospetrou opened this issue Sep 19, 2024 · 3 comments
Labels
bug Something isn't working
Milestone

Comments

@lambrospetrou
Copy link

lambrospetrou commented Sep 19, 2024

What is the current bug behavior?

A response of 147KB Javascript file is not rejected with --max-filesize 100.

Steps to reproduce

# filename: test-filesize.hurl

GET https://unpkg.com/vue@3.4.27/dist/vue.global.prod.js
[Options]
output:test-vue.prod.js
HTTP 200

Run the above with:

hurl --test --max-filesize 100 test-filesize.hurl

The download of the file succeeds just fine.

What is the expected correct behavior?

The download should be rejected.

For other file types like images or JSON, the max filesize option applies successfully, for example:

GET https://unpkg.com/vue@3.4.27/dist/vue.global.prod.js
[Options]
output:test-vue.prod.js
HTTP 200

GET https://api6.ipify.org?format=json
[Options]
output:test-https-ipv6.json
ipv6: true
HTTP 200

GET https://cf-assets.www.cloudflare.com/slt3lc6tev37/76x52jIsr93tqZq0h3HCFW/33a7575a9dc880e0a45c0f69fcbcfc8f/cc-diagram-orange-2024.png
[Options]
output:cloudflare.png
HTTP 200

Running the above will give out:

$ hurl --report-json test-report-json --max-filesize 100 test-report-json.hurl
error: HTTP connection
  --> test-report-json.hurl:13:5
   |
13 | GET https://cf-assets.www.cloudflare.com/slt3lc6tev37/76x52jIsr93tqZq0h3HCFW/33a7575a9dc880e0a45c0f69fcbcfc8f/cc-diagram-orange-2024.png
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ (63) Maximum file size exceeded
   |

With a low enough max filesize (e.g. 10) it will also fail the second JSON request.
Instead of failing at the first entry for the JS file.

Execution context

  • Hurl Version (hurl --version):
$ hurl --version
hurl 5.0.1 (x86_64-pc-linux-gnu) libcurl/7.81.0 OpenSSL/3.0.2 zlib/1.2.11 brotli/1.0.9 zstd/1.4.8 libidn2/2.3.2 libssh/0.9.6/openssl/zlib nghttp2/1.43.0
Features (libcurl):  alt-svc AsynchDNS brotli HSTS HTTP2 IDN IPv6 Largefile libz NTLM NTLM_WB SPNEGO SSL TLS-SRP UnixSockets zstd
Features (built-in): brotli

Possible fixes

@lambrospetrou lambrospetrou added the bug Something isn't working label Sep 19, 2024
@lambrospetrou
Copy link
Author

lambrospetrou commented Sep 19, 2024

I realized now that curl (curl 7.81.0) also does not fail for that file...

curl --max-filesize 10 https://unpkg.com/vue@3.4.27/dist/vue.global.prod.js

According to https://everything.curl.dev/usingcurl/downloads/max-filesize.html though, it should stop after it exceeds the max filesize even if it doesn't know it upfront. But it doesn't.

edit: According to https://curl.se/docs/manpage.html#--max-filesize after curl 8.4.0 it should be rejected:

NOTE: before curl 8.4.0, when the file size is not known prior to download, for such files this option has no effect even if the file transfer ends up being larger than this given limit.

Starting with curl 8.4.0, this option aborts the transfer if it reaches the threshold during transfer.

It seems though that Hurl uses libcurl/7.81.0 (based on hurl --version).

So, is there a way to update the version used by Hurl, or does that depend on my system's version of curl?

@jcamiel
Copy link
Collaborator

jcamiel commented Sep 20, 2024

Hi @lambrospetrou

Given the description, my first move should have been to run the test with --verbose, and replay the curl logs outputted for every entry! I suspected something in curl because we haven't hard-coded something specific to downloaded files kind.

That's said:

  • Hurl is dynamically linked with libcurl. So, usually it uses the system libcurl. To upgrade the curl used by Hurl, one can update the system curl, of course. But, I think you can install an updated libcurl in a custom dir and play with LD_LIBRARY_PATH so that libcurl is resolved to this new file. I've not tested it myself, and it has the caveats that this new libcurl will be used by other libcurl "clients". On macOS, we have a FAQ for this, it should be good to update it with Linux.
  • we could easily add a "redundant" check for --max-filesize. Usually, we defer all the HTTP stuff to libcurl, but in this case, it can be useful to add this check, if user can't upgrade curl and is stuck with a <8.4.0 version. The caveat is that we'll not prevent the download but, at least, we can raise an error if the number of bytes constraint is not respected and don't dump the response.

Two things that are somewhat related to this:

  • a recent discussion about providing a "static curl" of Hurl here Possibility of Bundling `static-curl` #3225. For the moment, we're not doing it (and it comes also with its own caveats, mainly more maintainers work!)
  • a open issue about providing a list of curl commands to rerun a file (--curl) instead of grepping --verbose.

@jcamiel jcamiel added this to the 6.0.0 milestone Sep 20, 2024
@jcamiel jcamiel linked a pull request Sep 20, 2024 that will close this issue
@jcamiel jcamiel closed this as completed Sep 20, 2024
@lambrospetrou
Copy link
Author

lambrospetrou commented Sep 21, 2024

Thanks for the quick change inside Hurl @jcamiel. I fixed it on my side too by updating the server version to curl 8.5.0+ so that check works natively by libcurl.

The statically built Hurl would be awesome, I do love that if it ever comes, but I do see the extra overhead on you might not worth it, which is totally fair too. I did try yesterday to build curl locally and with so many options and things to have on the system it just doesn't make sense :) I upgraded the OS faster than that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants