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

Brave: regression in embedded /api/v0/add #757

Closed
lidel opened this issue Sep 12, 2019 · 0 comments · Fixed by #794
Closed

Brave: regression in embedded /api/v0/add #757

lidel opened this issue Sep 12, 2019 · 0 comments · Fixed by #794
Assignees
Labels
area/brave Issues related to Brave Browser kind/bug A bug in existing code (including security flaws)

Comments

@lidel
Copy link
Member

lidel commented Sep 12, 2019

Part of #716, writing down so the details are not forgotten :)

Context

Embedded js-ipfs in Brave is using chrome-net, http-node, iso-url and some other tricks to convince @hapi/hapi HTTP server that it is running in Node.js 😬

ipfs/js-ipfs#2379 switched ipfs.add to ipfs._addAsyncIterator and it introduced regression in /api/v0/add HTTP API endpoint exposed by embedded js-ipfs in Brave.

Provisional fix

It seems old polyfills are no longer enough. Real fix requires more time to investigate, so for now we switched (2728436) to version of js-ipfs before ipfs/js-ipfs#2379.

How to reproduce

The problem is present only in HTTP API exposed by js-ipfs embedded in ipfs-companion running in Brave.

/api/v0/add before ipfs/js-ipfs#2379 works fine:

> curl -v 'http://127.0.0.1:5003/api/v0/add?pin=false&wrapWithDirectory=false&progress=true&wrap-with-directory=false&stream-channels=true' -H 'content-type: multipart/form-data; boundary=--------------------------386573610243845979939562' -H 'Connection: keep-alive' --data-binary $'----------------------------386573610243845979939562\r\nContent-Disposition: file; filename="test-ipfs-add.txt"\r\nContent-Type: application/octet-stream\r\n\r\ntest-ipfs-add\n\r\n----------------------------386573610243845979939562--\r\n'
< HTTP/1.1 200 OK
< x-chunked-output: 1
< content-type: application/json; charset=utf-8
< trailer: X-Stream-Error
< access-control-allow-headers: X-Stream-Output, X-Chunked-Output, X-Content-Length
< access-control-expose-headers: X-Stream-Output, X-Chunked-Output, X-Content-Length,WWW-Authenticate,Server-Authorization
< vary: origin
< cache-control: no-cache
< Date: Wed, 09 Oct 2019 14:48:46 GMT
< Connection: keep-alive
< Transfer-Encoding: chunked
< 
{"Bytes":14}
{"Name":"test-ipfs-add.txt","Hash":"QmZdTTS3UcAPVMYpbvzmKakuchgntxgsfjNKDv2fWkkPxB","Size":22}

/api/v0/add after ipfs/js-ipfs#2379 is missing response body in Brave:

> curl -v 'http://127.0.0.1:5003/api/v0/add?pin=false&wrapWithDirectory=false&progress=true&wrap-with-directory=false&stream-channels=true' -H 'content-type: multipart/form-data; boundary=--------------------------386573610243845979939562' -H 'Connection: keep-alive' --data-binary $'----------------------------386573610243845979939562\r\nContent-Disposition: file; filename="test-ipfs-add.txt"\r\nContent-Type: application/octet-stream\r\n\r\ntest-ipfs-add\n\r\n----------------------------386573610243845979939562--\r\n'
< HTTP/1.1 200 OK
< x-chunked-output: 1
< content-type: application/json; charset=utf-8
< trailer: X-Stream-Error
< access-control-allow-headers: X-Stream-Output, X-Chunked-Output, X-Content-Length
< access-control-expose-headers: X-Stream-Output, X-Chunked-Output, X-Content-Length,WWW-Authenticate,Server-Authorization
< vary: origin
< cache-control: no-cache
< Date: Wed, 11 Sep 2019 14:02:03 GMT
< Connection: keep-alive
< Transfer-Encoding: chunked
< 
* Connection #0 to host 127.0.0.1 left intact

However, the same code (/api/v0/add after ipfs/js-ipfs#2379) running as jsipfs daemon works fine!:

$ curl -v 'http://127.0.0.1:5002/api/v0/add?pin=false&wrapWithDirectory=false&progress=true&wrap-with-directory=false&stream-channels=true' -H 'content-type: multipart/form-data; boundary=--------------------------386573610243845979939562' -H 'Connection: keep-alive' --data-binary $'----------------------------386573610243845979939562\r\nContent-Disposition: file; filename="test-ipfs-add.txt"\r\nContent-Type: application/octet-stream\r\n\r\ntest-ipfs-add\n\r\n----------------------------386573610243845979939562--\r\n'
< HTTP/1.1 200 OK
< x-chunked-output: 1
< content-type: application/json; charset=utf-8
< trailer: X-Stream-Error
< access-control-allow-headers: X-Stream-Output, X-Chunked-Output, X-Content-Length
< access-control-expose-headers: X-Stream-Output, X-Chunked-Output, X-Content-Length,WWW-Authenticate,Server-Authorization
< vary: origin
< cache-control: no-cache
< connection: close
< Date: Wed, 09 Oct 2019 14:41:40 GMT
< Transfer-Encoding: chunked
< 
{"Name":"test-ipfs-add.txt","Bytes":14}
{"Name":"test-ipfs-add.txt","Hash":"QmZdTTS3UcAPVMYpbvzmKakuchgntxgsfjNKDv2fWkkPxB","Size":22}
@lidel lidel added kind/bug A bug in existing code (including security flaws) area/brave Issues related to Brave Browser labels Sep 12, 2019
@lidel lidel self-assigned this Sep 12, 2019
lidel added a commit that referenced this issue Sep 12, 2019
This switch to js-ipfs before PR-2379

ipfs/js-ipfs#2379 switched ipfs.add
to ipfs._addAsyncIterator and it broke /api/v0/add exposed by embedded
js-ipfs in Brave.

It seems old polyfills are no longer enough. Real fix requires more time
to investigate, so for now we switch to version before js-ipfs/PR-2379.

Used js-ipfs commit is from ipfs/js-ipfs#2304 before it
was rebased on top of master after PR-2379, making it the last safe
version.

Real fix will be tracked in
#757
@lidel lidel mentioned this issue Sep 12, 2019
17 tasks
@lidel lidel mentioned this issue Oct 1, 2019
4 tasks
lidel added a commit that referenced this issue Oct 14, 2019
This switches to async iterator version of ipfs.add
(introduced to js-ipfs in ipfs/js-ipfs#2517)
and ensures Node streams are replaced by deterministic version of readable-stream

Closes #757
lidel added a commit that referenced this issue Oct 15, 2019
* fix: /api/v0/add in Brave

This switches to async iterator version of ipfs.add
(introduced to js-ipfs in ipfs/js-ipfs#2517)
and ensures Node streams are replaced by deterministic version of readable-stream

Closes #757

* fix(cid): fast finish + allow osx to fail
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/brave Issues related to Brave Browser kind/bug A bug in existing code (including security flaws)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant