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

RequestError: Parse Error: Missing expected CR after header value #3772

Closed
fvillena opened this issue Jul 15, 2022 · 32 comments · Fixed by #3776
Closed

RequestError: Parse Error: Missing expected CR after header value #3772

fvillena opened this issue Jul 15, 2022 · 32 comments · Fixed by #3776

Comments

@fvillena
Copy link

fvillena commented Jul 15, 2022

Current Behavior

When doing a simple GET request with no payload to an endpoint that should return a binary buffer (an image) I get the following error:

RequestError: Parse Error: Missing expected CR after header value

The node was working before version 3. Maybe it is related to nodejs/node#43798

Expected Behavior

Return a binary buffer.

Steps To Reproduce

This is the node:

[
    {
        "id": "912ff074.7418",
        "type": "http request",
        "z": "61690af24820e126",
        "name": "",
        "method": "GET",
        "ret": "bin",
        "paytoqs": "ignore",
        "url": "http://192.168.7.14:8023/snapshot.cgi",
        "tls": "",
        "persist": false,
        "proxy": "",
        "authType": "",
        "senderr": false,
        "credentials": {},
        "x": 250,
        "y": 40,
        "wires": [
            [
                "bad44d5a.892d3"
            ]
        ]
    }
]

Example flow

[
    {
        "id": "469f2259.87ca3c",
        "type": "inject",
        "z": "61690af24820e126",
        "name": "",
        "props": [
            {
                "p": "payload"
            },
            {
                "p": "topic",
                "vt": "str"
            }
        ],
        "repeat": "",
        "crontab": "",
        "once": false,
        "onceDelay": 0.1,
        "topic": "",
        "payload": "",
        "payloadType": "date",
        "x": 100,
        "y": 120,
        "wires": [
            [
                "912ff074.7418"
            ]
        ]
    },
    {
        "id": "912ff074.7418",
        "type": "http request",
        "z": "61690af24820e126",
        "name": "",
        "method": "GET",
        "ret": "bin",
        "paytoqs": "ignore",
        "url": "http://192.168.7.14:8023/snapshot.cgi",
        "tls": "",
        "persist": false,
        "proxy": "",
        "authType": "",
        "senderr": false,
        "credentials": {},
        "x": 250,
        "y": 40,
        "wires": [
            [
                "bad44d5a.892d3"
            ]
        ]
    }
]

Environment

  • Node-RED version: v3.0.0
  • Node.js version: 16.16.0
  • npm version: -
  • Platform/OS: Docker
  • Browser: Chrome
@hardillb
Copy link
Member

Nothing should have changed in the http-request node between 2.2.2 and 3.0.0. It's still using the same got library to make HTTP requests.

What has changed is the image tagged with latest will now be running NodeJS 16 rather than 14, you should be able to test with 14 by pulling the latest-14 tagged container.

What is the device you are making the request to? I assume it's a network camera? Are you sure the device is following the spec and inserting the required crlf after the headers and before the payload?

@Steve-Mcl
Copy link
Contributor

Also, your flow IS sending a payload (or at least the msg has a payload with the value date.now epoch. Try deleting the payload from the inject.

@fvillena
Copy link
Author

fvillena commented Jul 15, 2022

Nothing should have changed in the http-request node between 2.2.2 and 3.0.0. It's still using the same got library to make HTTP requests.

What has changed is the image tagged with latest will now be running NodeJS 16 rather than 14, you should be able to test with 14 by pulling the latest-14 tagged container.

What is the device you are making the request to? I assume it's a network camera? Are you sure the device is following the spec and inserting the required crlf after the headers and before the payload?

Changed from nodered/node-red:latest to nodered/node-red:latest-14 and it does not work either. Changed to nodered/node-2.2.2 and it works as expected.

I am using https://github.com/Thom-x/rtsp-snapshot to take snapshots of a security camera, maybe the HTTP implementation is not following the spec, but it works in node-red v2.2.2.

@fvillena
Copy link
Author

Also, your flow IS sending a payload (or at least the msg has a payload with the value date.now epoch. Try deleting the payload from the inject.

It is not, because I am ignoring the payload on the HTTP request node.

@hardillb
Copy link
Member

Can you try and capture the request (with something like wireshark) please.

Having had a quick look at the rtsp-snapshot cgi it will end up being 2 requests

  • The first returns a 302 redirect to a file
  • The second is the file

It would be useful to know if it's the first or second request it's failing on

@Steve-Mcl
Copy link
Contributor

It is not, because I am ignoring the payload on the HTTP request node.

Yes, my apologies, I missed that.

I checked the opts being set in GOT (see below) - looks normal.

image

@fvillena
Copy link
Author

Can you try and capture the request (with something like wireshark) please.

Having had a quick look at the rtsp-snapshot cgi it will end up being 2 requests

  • The first returns a 302 redirect to a file
  • The second is the file

It would be useful to know if it's the first or second request it's failing on

This is the capture:

0000   9c fc e8 b8 f6 f5 b4 7a f1 de e4 02 08 00 45 00   .......z......E.
0010   00 9a d8 ac 40 00 3f 06 d3 40 c0 a8 07 0e c0 a8   ....@.?..@......
0020   07 12 1f 57 f6 a5 83 ae 19 a0 61 fb 74 cd 50 19   ...W......a.t.P.
0030   01 f5 f8 76 00 00 48 54 54 50 2f 31 2e 30 20 33   ...v..HTTP/1.0 3
0040   30 32 20 46 6f 75 6e 64 0d 0a 43 6f 6e 74 65 6e   02 Found..Conten
0050   74 2d 74 79 70 65 3a 20 74 65 78 74 2f 68 74 6d   t-type: text/htm
0060   6c 0a 53 74 61 74 75 73 3a 20 33 30 32 0a 4c 6f   l.Status: 302.Lo
0070   63 61 74 69 6f 6e 3a 20 73 6e 61 70 73 68 6f 74   cation: snapshot
0080   73 2f 73 6e 61 70 73 68 6f 74 5f 32 30 32 32 2d   s/snapshot_2022-
0090   30 37 2d 31 35 5f 31 38 2d 34 30 2d 35 37 2d 37   07-15_18-40-57-7
00a0   36 37 2e 6a 70 67 0a 0a                           67.jpg..

@hardillb
Copy link
Member

I've set this up locally and it's running fine with NodeJS 16.15.0

Will try with the docker image

@hardillb
Copy link
Member

Fails with Docker image on NodeJS 16.16

hardillb added a commit to hardillb/rtsp-snapshot that referenced this issue Jul 15, 2022
As of NodeJS 16.16.x Nodejs rejects HTTP responses that do not meet the spec

This patch makes the head compliant by ending lines with CRLF

See for details: 

node-red/node-red#3772
@hardillb
Copy link
Member

hardillb commented Jul 15, 2022

Pull request to fix the rtsp container you are using:

Thom-x/rtsp-snapshot#2

@hardillb
Copy link
Member

Still need to look at if GOT can be made to accept none compliant headers, but based on experience with that team I expect them to say fix the broken server (which we have tried to do)

@fvillena
Copy link
Author

Still need to look at if GOT can be made to accept none compliant headers, but based on experience with that team I expect them to say fix the broken server (which we have tried to do)

That is the best response, but I think that we should have a feature on the HTTP request node to add options like insecureHTTPParser to overcome this issues.

@hardillb
Copy link
Member

The problem is that the http-request node uses the GOT library under the covers, if they won't add support there is not much we can do (we changed the http-request node to use GOT from request in 2.2.0, we can't change it again, and really don't want to, until the next minor release 3.1.x)

@hardillb
Copy link
Member

hardillb commented Jul 16, 2022

As expected:

Got doesn't support insecureHTTPParser and never will. signal is a to-do.

sindresorhus/got#1794 (comment)

@hardillb
Copy link
Member

The fix to Thom-x/rtsp-snapshot#2 has been merged, hopefully they will push a new version to docker hub or you can every easily build it yourself.

@hardillb
Copy link
Member

I've asked GOT nicely to reconsider, but not hopeful
sindresorhus/got#2080

@hardillb
Copy link
Member

GOT not going to implement this, but suggest we can inject with own beforeRequest hook:

https://github.com/apify/got-scraping/blob/master/src/hooks/insecure-parser.ts

hardillb added a commit to hardillb/node-red that referenced this issue Jul 16, 2022
@alexbn71
Copy link

alexbn71 commented Jul 17, 2022

I am using https://github.com/Thom-x/rtsp-snapshot to take snapshots of a security camera, maybe the HTTP implementation is not following the spec, but it works in node-red v2.2.2.

Same issue to me capturing a camera snapshot after the upgrade to v3.0. I use the "http request" node

image

@hardillb
Copy link
Member

@alexbn71 Just to be 100% clear, if you are using the rtsp-snapshot container that has been fixed, you will probably need to pull the latest version from github as I don't think the author has pushed the update to docker hub yet (feel free to ask them nicely)

If you are using something else, then what ever it is is broken and in breach of the HTTP spec, so you are lucky anything works with it. There is a PR for Node-RED that will allow a broken headers to be accepted, but it will probably take a few iterations before it's merged, if you are happy to build Node-RED from src then it's here #3776, otherwise you will have to wait,

@fvillena
Copy link
Author

@alexbn71 Just to be 100% clear, if you are using the rtsp-snapshot container that has been fixed, you will probably need to pull the latest version from github as I don't think the author has pushed the update to docker hub yet (feel free to ask them nicely)

If you are using something else, then what ever it is is broken and in breach of the HTTP spec, so you are lucky anything works with it. There is a PR for Node-RED that will allow a broken headers to be accepted, but it will probably take a few iterations before it's merged, if you are happy to build Node-RED from src then it's here #3776, otherwise you will have to wait,

For me, your fix solved the issue.

@hardillb
Copy link
Member

@fvillena Please be clear (since I provided both fixes), do you mean the rtsp-snapshot PR or the Node-RED PR?

@fvillena
Copy link
Author

@fvillena Please be clear (since I provided both fixes), do you mean the rtsp-snapshot PR or the Node-RED PR?

Sorry, Thom-x/rtsp-snapshot#2 fixed my issue

@tve
Copy link

tve commented Jul 18, 2022

FYI: I'm also hitting the "RequestError: Parse Error: Missing expected CR after header value" issue. This is with a 2009 device controlling my veggie garden watering.

@Steve-Mcl Steve-Mcl linked a pull request Jul 21, 2022 that will close this issue
6 tasks
@Steph-74
Copy link

I am also getting the error in this Node RED flow on an Hyper-V Home Assistant installation. I am quite sure the issue has the same root cause as this one. I installed the KB5015814 yesterday (again) and since then this flow cannot read any data from my Grünbeck water sofenting system anymore and produces this error messages. I experienced the same behaviour 10 days ago, when the KB was installed autmatically the first time. After uninstalling the KB back then in order to get my shellys up and running, my node red flow pulled the data from the water softening system as well again.

Please forgive me for not adding more technical details - I do not rellay understand what is happening there and therefore can only provide my observations.

@Steve-Mcl
Copy link
Contributor

@knolleary I have reviewed the attached PR and discussed with @hardillb

While I realise this PR is marked as an enhancement, due to recent nodejs changes, (where it introduced stricter header separator parsing because of GHSA-q5vx-44v4-gch4) some flows will break when users update nodejs (for the upgrade to NR v3.x) - perhaps we might want to consider this for the next minor release - OR - pull forward on 3.1.0?

@hardillb
Copy link
Member

@Steve-Mcl / @knolleary The "enhancement" tag was mine, probably a mistake.

My take is this is a fix for the upstream NodeJS change, and GOT are not going to fix it. It should probably go into the next 2.2.x service release as well since this will hit anybody running on the latest 14.x/16.x/18.x NodeJS release to access "broken" http servers.

Steve-Mcl pushed a commit to Steve-Mcl/node-red that referenced this issue Aug 4, 2022
@FISEChris1337
Copy link

FISEChris1337 commented Aug 14, 2022

Hi i have same problem with latest verison from today (3.0.2: Maintenance Release) i see the " Disable strict HTTP parsing". i just try to shutdown my picoreplayer with easy GET -> http://192.168.23.20/cgi-bin/main.cgi?ACTION=shutdown but is not working anymore :( should it be fixed already?

"RequestError: Parse Error: Missing expected CR after header value"

@alexbn71
Copy link

Disable strict HTTP parsing

"Disable strict HTTP parsing" doesn't resolve the issue to me.

@hardillb
Copy link
Member

I will have another look

But @alexk111 & @FISEChris1337 it is important to remember that it the HTTP server you are trying to connect to that is broken here and not conforming to the HTTP spec. You need to raise issues against who ever owns them pointing out that it will be broken with ALL currently supported NodeJS versions going forward by default.

@alexbn71
Copy link

Yes @hardillb I know this. But in my case it's an IP Cam and I have no chance to resolve the issue on that side. Also it always worked with nodered 2.x and therefore I would like to have it working again... it's a small hope that it will still work in the future.
Thanks.

@hardillb
Copy link
Member

Add the following at line 89 in the http-request.js file

this.insecureHTTPParser = n.insecureHTTPParser

@deggle
Copy link

deggle commented Jan 31, 2024

Hi i have same problem with latest verison from today (3.0.2: Maintenance Release) i see the " Disable strict HTTP parsing". i just try to shutdown my picoreplayer with easy GET -> http://192.168.23.20/cgi-bin/main.cgi?ACTION=shutdown but is not working anymore :( should it be fixed already?

"RequestError: Parse Error: Missing expected CR after header value"

I've hit this error also with pCP and calling some scripts from Node-RED. Luckily I was making my own cgi scripts so could just implement the fix, but I've posted it below so hopefully someone will sort it in the main project files.

https://forums.slimdevices.com/forum/user-forums/linux-unix/111580-announce-picoreplayer-8-0-0?p=1670979#post1670979

Appreciate this is specific to one service and not the wider discussion around strict HTTP parsing, but as someone had explicitly noted pCP I thought I'd comment. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants