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.

Sign up for GitHub

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

Jump to bottom

HTTP API needs POST for large parameters in NodeJS 8.14+ #3568

Closed
raybellis opened this issue Mar 1, 2019 · 8 comments
Closed

HTTP API needs POST for large parameters in NodeJS 8.14+ #3568

raybellis opened this issue Mar 1, 2019 · 8 comments
Labels
http api
Milestone
1.8.0

Comments

@raybellis
Copy link
Contributor

NodeJS 8.14.0 introduced a limit of 8KB to the size of HTTP headers that can be received.

This prevents HTTP API methods such as setText from working if the text content exceeds that size.

A possible solution would be to support the POST method in addition to GET, with any JSON object found in the HTTP body augmenting the URL parameters.
@raybellis
Copy link
Contributor Author

On further inspection, the API already has some support for POST, but it's not documented.

@raybellis
Copy link
Contributor Author

raybellis commented Mar 1, 2019 •
edited
Loading

Potential fix in branch http-post (commit 882b934) which allows all backend tests to pass, firstly by merging the body parameters into the request parameters in the app.post handler, and secondly by updating two of the backend tests to actually use .post instead of .get.

Please advise if the preference is to allow only body parameters in an HTTP POST and if so I'll amend the patch and tests accordingly. Also, the EP docs make no mention of HTTP POST support nor how to use it. I'll wait to submit a PR until I hear back.

@muxator muxator mentioned this issue Mar 2, 2019
Closed
@muxator muxator added this to the 1.8 milestone Mar 2, 2019
@raybellis raybellis changed the title HTTP API needs redesign for NodeJS 8.14+ HTTP API needs POST for large parameters in NodeJS 8.14+ Mar 12, 2019
@muxator muxator mentioned this issue Jun 23, 2019
Closed
@muxator
Copy link
Contributor

muxator commented Jun 26, 2019 •
edited
Loading

If this was not an eight year old project I would say that GETs should be used for idempotent requests (that do not mutate the state in the application), while mutations (such as setText) should be done via POSTs.

But them, I did not write the API spec, and it is on the wild for a long time now, so we have to take care of backwards compatibility.

My vote is towards retaining GET, and allowing POST.

@raybellis
Copy link
Contributor Author

In which case I think you should be able to cherry pick that commit, although the API documentation does need an update to mention POST and the need to use it if the data is too large.

@muxator
Copy link
Contributor

muxator commented Jun 26, 2019 •
edited
Loading

(sorry I hit CTRL+ENTER and posted an half backed comment).

Apart from the documentation (that should be updated), would it be possible to print a WARN in the logs when receiving a GET with a too big payload on node 8.14+? That would give an operator some hints about what needs to be done to maintain a working installation.

If, instead, it is the node runtime that discards the request at a low level, it would never hit the application, and no log could be generated, obviously.

@raybellis
Copy link
Contributor Author

I'm a little fuzzy on the details right now, but ISTR it was the latter.

The question on GET vs POST was not about deprecating the former, but whether the POST handler should work as my patch currently does, and combine the parameters supplied in the body with those in the query string (with the former taking precedence) or alternatively completely ignore any that are in the query string and only use those found in the POST body.

The disadvantage of the "body only" approach is that it then becomes mandatory to put things like the padID field in the body (for POST requests).

@muxator
Copy link
Contributor

muxator commented Jun 26, 2019

The approach taken in your patch (GET parameters are read, and eventually overridden by POST parameters of the same name) is the most sensible.

Ignore my comment on using GET vs POST: it was a rambling about how I would have designed the API in the first place. Totally not relevant now.

@muxator
Copy link
Contributor

muxator commented Jun 26, 2019 •
edited
Loading

Note to self for the documentation:

  1. Example of a command line invocation of the setHTML API that works with 882b934:

    curl -XPOST "http://localhost:9001/api/1/setHTML?apikey=<API_KEY>&padID=<PAD_ID>" -H "Content-Type: application/json" --data '{"html":"<html><body>Text content</body></html>"}'

    Emphasize that the header Content-Type: application/json is mandatory: without it the invocation fails with:

    {"code":1,"message":"html is not a string","data":null}

  2. Reference for the change in nodejs 8.14+:

    The total size of HTTP headers received by Node.js now must not exceed 8192 bytes.

    Source: https://nodejs.org/en/blog/vulnerability/november-2018-security-releases/#denial-of-service-with-large-http-headers-cve-2018-12121

muxator pushed a commit to JohnMcLear/etherpad-lite that referenced this issue Mar 30, 2020
@JohnMcLear
tests: in backend tests, use POST instead of GET for setText() and se…
117deb3 
…tHTML()

This is allowed starting from fc661ee ("core: allow URL parameters and POST
bodies to co-exist"), which landed in Etherpad 1.8.0. For the discussion, see
issue ether#3568.
@muxator muxator mentioned this issue Mar 30, 2020
Merged
muxator pushed a commit that referenced this issue Mar 31, 2020
@JohnMcLear @muxator
tests: in backend tests, use POST instead of GET for setText() and se…
352b432 
…tHTML()

This is allowed starting from fc661ee ("core: allow URL parameters and POST
bodies to co-exist"), which landed in Etherpad 1.8.0. For the discussion, see
issue #3568.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
http api
Projects
None yet
Milestone
1.8.0
Development

No branches or pull requests
2 participants
@raybellis @muxator