Open longzheng opened 2 years ago
Refactored authorization handling since axios@1.1.3 does not behave correctly if you set both auth and headers['authorization'], manually set basic or digest auth based on flow
Are you sure about this? According to the docs and my experience AxiosBasicCredentials
will set the authorization
header with AxiosRequestConfig
. Then later, the authorization
header is overrode if digest auth is needed by manually setting the authorization
header.
@magic7s would you be interested in making sure this doesn't invoke that bug on your NVR?
Refactored authorization handling since axios@1.1.3 does not behave correctly if you set both auth and headers['authorization'], manually set basic or digest auth based on flow
Are you sure about this? According to the docs and my experience
AxiosBasicCredentials
will set theauthorization
header withAxiosRequestConfig
. Then later, theauthorization
header is overrode if digest auth is needed by manually setting theauthorization
header.
Yeah I reproed this behaviour with this test snippet
import Axios from 'axios';
Axios.request('https://enhkh5nho0zi5.x.pipedream.net', {
auth: {
username: 'user',
password: 'pass',
},
headers: {
accept: 'multipart/x-mixed-replace',
authorization: 'test',
},
});
You can run here https://stackblitz.com/edit/node-xirxpb?file=index.js with node index.js
in the terminal
Then you can see here https://requestbin.com/r/enhkh5nho0zi5/2IJ5KElLjQ7arCAHPf4I40oUCwY
Host: enhkh5nho0zi5.x.pipedream.net
X-Amzn-Trace-Id: Root=1-6388733b-61af32ca1f6066fc4b179ff9
accept: multipart/x-mixed-replace
authorization: Basic dXNlcjpwYXNz
user-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/107.0.0.0 Safari/537.36
origin: https://nodexirxpb-xcza.w-credentialless.staticblitz.com
sec-fetch-site: cross-site
sec-fetch-mode: cors
sec-fetch-dest: empty
accept-encoding: gzip, deflate, br
accept-language: en-AU,en;q=0.9
As you can see, even though I'm overriding the authorization
header with test
, it still sent the basic auth header authorization: Basic dXNlcjpwYXNz
The only two alternatives are
authorization
for both basic auth/digest (which is the approach I went with)authorization: xxx
and config auth: undefined
when doing digest authwould you be interested in making sure this doesn't invoke that bug on your NVR?
Sorry I don't have my instance of Homebridge any more.
Refactored authorization handling since axios@1.1.3 does not behave correctly if you set both auth and headers['authorization'], manually set basic or digest auth based on flow
Are you sure about this? According to the docs and my experience
AxiosBasicCredentials
will set theauthorization
header withAxiosRequestConfig
. Then later, theauthorization
header is overrode if digest auth is needed by manually setting theauthorization
header.Yeah I reproed this behaviour with this test snippet
import Axios from 'axios'; Axios.request('https://enhkh5nho0zi5.x.pipedream.net', { auth: { username: 'user', password: 'pass', }, headers: { accept: 'multipart/x-mixed-replace', authorization: 'test', }, });
You can run here https://stackblitz.com/edit/node-xirxpb?file=index.js with
node index.js
in the terminalThen you can see here https://requestbin.com/r/enhkh5nho0zi5/2IJ5KElLjQ7arCAHPf4I40oUCwY
Host: enhkh5nho0zi5.x.pipedream.net X-Amzn-Trace-Id: Root=1-6388733b-61af32ca1f6066fc4b179ff9 accept: multipart/x-mixed-replace authorization: Basic dXNlcjpwYXNz user-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/107.0.0.0 Safari/537.36 origin: https://nodexirxpb-xcza.w-credentialless.staticblitz.com sec-fetch-site: cross-site sec-fetch-mode: cors sec-fetch-dest: empty accept-encoding: gzip, deflate, br accept-language: en-AU,en;q=0.9
As you can see, even though I'm overriding the
authorization
header withtest
, it still sent the basic auth headerauthorization: Basic dXNlcjpwYXNz
The only two alternatives are
* always manually set `authorization` for both basic auth/digest (which is the approach I went with) * set header `authorization: xxx` and config `auth: undefined` when doing digest auth
You're totally right. I played with the repro you provided (thanks! TIL about requestbin.com)
@kushagharahi let me know if there's anything else you might need to merge this PR.
- Upgraded to
axios@1.1.3
which properly emits theend
event if the connection is killed because the NVR restarts, for example on restart it will now emit the following *
Is there a specific change in axios that addresses it? I checked their changelog (https://github.com/axios/axios/blob/v1.x/CHANGELOG.md) and don't see relevant issue marked resolved. When you have established TCP connection and await data sent from remote host (i.e. camera), there is no way to tell its down. That is unless host sends regular heartbeat message per its API or your code in parallel and proactively keeps healthchecking host's status.
Is there a specific change in axios that addresses it? I checked their changelog (https://github.com/axios/axios/blob/v1.x/CHANGELOG.md) and don't see relevant issue marked resolved.
I didn't see, but because the previous version was pre-1.0, I suspect there's some pretty significant changes under the hood which could explain the change.
When you have established TCP connection and await data sent from remote host (i.e. camera), there is no way to tell its down.
Sorry I'm not familiar with the specifics of the TCP connection but at the Axios response level it definitely fires the end
event. Can you reproduce this behaviour with this PR on your device?
Sorry I'm not familiar with the specifics of the TCP connection but at the Axios response level it definitely fires the
end
event. Can you reproduce this behaviour with this PR on your device?
I will try your change when I have a chance and reply back
@kushagharahi Just double checking if you still want this PR. If so I'll try to resolve the conflicts.
Fixes #42
axios@1.1.3
which properly emits theend
event if the connection is killed because the NVR restarts, for example on restart it will now emit the followingaxios@1.1.3
due to different typesaxios@1.1.3
does not behave correctly if you set bothauth
andheaders['authorization']
, manually set basic or digest auth based on flow