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

ipfs pin add does mis-inteprets raw base64 CIDs #7739

Open
ribasushi opened this issue Oct 27, 2020 · 3 comments
Open

ipfs pin add does mis-inteprets raw base64 CIDs #7739

ribasushi opened this issue Oct 27, 2020 · 3 comments
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization

Comments

@ribasushi
Copy link
Contributor

Description:

The cid of the welcome-directory expressed in base64 is mAXASIB57vcD6ZYQ/tYlAHE70HNp1AguaAHUkeFDiIrnsbzRj

This CID, while valid, is interpreted as a path on the cli:

ipfs pin add mAXASIB57vcD6ZYQ/tYlAHE70HNp1AguaAHUkeFDiIrnsbzRj
Error: invalid path "mAXASIB57vcD6ZYQ/tYlAHE70HNp1AguaAHUkeFDiIrnsbzRj": length greater than remaining number of bytes in buffer

For contrast:

ipfs pin add QmQPeNsJPyVWPFDVHb77w8G42Fvo15z4bG2X8D2GhfbSXc
pinned QmQPeNsJPyVWPFDVHb77w8G42Fvo15z4bG2X8D2GhfbSXc recursively
ipfs pin add bafybeia6po64b6tfqq73lckadrhpihg2oubaxgqaoushquhcek46y3zumm
pinned bafybeia6po64b6tfqq73lckadrhpihg2oubaxgqaoushquhcek46y3zumm recursively

Why is this important / not purely academic?

The default payload-cid (the root CID of the stored DAG) encoding in filecoin is base64, which contains / in its character set: https://github.com/filecoin-project/go-fil-markets/blob/v1.0.0/storagemarket/impl/clientutils/clientutils.go#L70

cc @lidel

@ribasushi ribasushi added kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization labels Oct 27, 2020
@aschmahmann
Copy link
Contributor

This doesn't really sound like a bug to me. Anywhere in go-ipfs that expects a path is going to have a problem with encodings that have / in them. Different domains are going to support different bases, that's why multibase exists (e.g. DNS doesn't support mixed cases or spaces, and paths don't support /).

While we could add a --is-not-a-path flag and that would work for this one case it would actually fail because now I cannot do ipfs pin add /ipns/mAXASIB57vcD6ZYQ/tYlAHE70HNp1AguaAHUkeFDiIrnsbzRj if I wanted to.

What's the downside of filecoin switching to use base64url instead of base64? (e.g. ipfs cid format -b=u mAXASIB57vcD6ZYQ/tYlAHE70HNp1AguaAHUkeFDiIrnsbzRj -> uAXASIB57vcD6ZYQ_tYlAHE70HNp1AguaAHUkeFDiIrnsbzRj

@ribasushi
Copy link
Contributor Author

Hmmmm I thought a path is expected only when you do /ipfs/... or /ipns/.... Everything else is taken as a verbatim CID.

Am I misunderstanding the contract?

@aschmahmann
Copy link
Contributor

aschmahmann commented Oct 27, 2020

A path is definitely expected whenever you do /ipfs/... or /ipns/.... However, we, for convenience and legacy reasons, generally allow an "ipfs path" to just be a raw CID also.

e.g. most use cases are like ipfs get bafyabc, so requiring ipfs get /ipfs/bafyabc seems excessive.

https://github.com/ipfs/go-path/blob/1533d95b57ea49396dbbbafa39bbefcdae8f3198/path.go#L87

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization
Projects
None yet
Development

No branches or pull requests

2 participants