-
Notifications
You must be signed in to change notification settings - Fork 100
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
feat(gateway): visualize dag-cbor and dag-json #315
Conversation
Codecov Report
@@ Coverage Diff @@
## main #315 +/- ##
==========================================
+ Coverage 49.36% 49.66% +0.29%
==========================================
Files 281 282 +1
Lines 33708 33878 +170
==========================================
+ Hits 16640 16824 +184
+ Misses 15329 15300 -29
- Partials 1739 1754 +15
|
d215a1b
to
00428cd
Compare
7f1a2ce
to
86278bd
Compare
68f4d1f
to
557b1ba
Compare
I'm opening this for review to get feedback and for others to take it for a spin. I was quite conservative on how we handle the preview. If some error happens, no preview is shown (this usually means that the DAG isn't valid). If it all works, we show a preview. |
d49baa8
to
f26dc1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @hacdias I think this is good enough to ship, but needs:
- Kubo PR to be green, fine to reuse one from feat(gateway): improved templates, user friendly errors #298 (review) after that PR lands: feat(gateway): human error pages, dag-cbor/dag-json preview kubo#9904
- Add a regression test that confirms CBOR bytes that get turned into strings in HTML get sanitized (just to be sure)
188b07b
to
37da207
Compare
f659215
to
6b2ef99
Compare
Kubo is green: ipfs/kubo#9904 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hacdias while testing in Kubo, I've found some things we should address before merging:
- subdomain gateways should never return dag.html from a path, only return redirect: http://localhost:8080/ipfs/bafyreihnpl7ami7esahkfdnemm6idx4r2n6u3apmtcrxlqwuapgjsciihy MUST redirect to http://bafyreihnpl7ami7esahkfdnemm6idx4r2n6u3apmtcrxlqwuapgjsciihy.ipfs.localhost:8080
- while at this, for HTML responses, we should also ensure the generated HTML document is returned with a scope limited to the CID, the same way dir listings are. On path gateway http://127.0.0.1:8080/ipfs/bafyreihnpl7ami7esahkfdnemm6idx4r2n6u3apmtcrxlqwuapgjsciihy MUST return HTTP 301 redirect to http://127.0.0.1:8080/ipfs/bafyreihnpl7ami7esahkfdnemm6idx4r2n6u3apmtcrxlqwuapgjsciihy/ (normalize with
/
at the end, just like unixfs dirs)- this is an important precaution in case there is ever a bug in sanitization and/or our service worker protection is ever removed
@lidel I just pushed a fix for both, including tests. In addition, I have a few notes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, lgtm, merging. I will bubble up to ipfs/kubo#9904 and review there after CI is green.
Closes #307. Builds on top of #298.
gateway/assets/README.md
go mod edit -replace=github.com/ipfs/boxo=path/to/your/local/boxo