-
Notifications
You must be signed in to change notification settings - Fork 862
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
Verify NFT content media type before pinning #17392
Conversation
@@ -222,7 +222,8 @@ enum WalletPinServiceErrorCode { | |||
ERR_WRONG_METADATA_FORMAT = 4, | |||
ERR_ALREADY_PINNED = 5, | |||
ERR_NOT_PINNED = 6, | |||
ERR_PINNING_FAILED = 7 | |||
ERR_PINNING_FAILED = 7, | |||
ERR_MEDIA_TYPE_UNSUPORTED = 8 |
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.
UNSUPORTED ->
UNSUPPORTED
@@ -149,11 +152,94 @@ std::string BraveWalletPinService::ErrorCodeToString( | |||
return "ERR_NOT_PINNED"; | |||
case mojom::WalletPinServiceErrorCode::ERR_PINNING_FAILED: | |||
return "ERR_PINNING_FAILED"; | |||
case mojom::WalletPinServiceErrorCode::ERR_MEDIA_TYPE_UNSUPORTED: | |||
return "ERR_MEDIA_TYPE_UNSUPORTED"; |
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.
UNSUPPORTED
content_type_checker_->CheckContentTypeSupported( | ||
*image_url, | ||
base::BindOnce(&BraveWalletPinService::OnContentTypeChecked, | ||
weak_ptr_factory_.GetWeakPtr(), std::move(service), | ||
std::move(token), std::move(cids), | ||
std::move(callback))); |
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.
This needs S&P review as we are getting arbitrary url resource from blockchain and forcing browser to make a request to it.
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.
It is a bit different : actually we make a call to the IPFS gateway which we use to fetch metadata on the previous step. Final url looks like https://ipfs.io/CID/...
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.
I think it's fine. It's not arbitrary, like @cypt4 said and we only call IPFS gateway. This code also doesn't fetch a file, only response headers.
|
||
raw_ptr<PrefService> pref_service_; | ||
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_; | ||
// Reports that are actively being sent. |
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.
What reports?
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.
👍
Verification passed on
|
Uplift of #17392 (squashed) to beta
Resolves brave/brave-browser#28775
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
After an NFT added to the wallet, media type of the "image" field is checked. So if it differs from "image/.." NFT won't be pinned. Probably we can test this by sniffing NFT gateway for the https://nftstorage.link(example)/ipfs/IMAGE_CID request by changing content_type here from image/png to blablabla