Skip to content

Conversation

Rhilip
Copy link
Contributor

@Rhilip Rhilip commented Jun 24, 2022

BitTorrent introduced the btmh: protocol in 2020 as part of its BitTorrent v2 changes. https://blog.libtorrent.org/2020/09/bittorrent-v2/
This schema like: magnet:?xt=urn:btmh:1220{64 hashs}
And This pull request will let isMagnetURI function support vaild this schema.

I Also remove the restrict of xt=urn: after magnet:?, since Magnet_URI_scheme Format notice:

Magnet URIs consist of a series of one or more parameters, the order of which is not significant, formatted in the same way as query strings that ordinarily terminate HTTP URLs.

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

@codecov
Copy link

codecov bot commented Jun 24, 2022

Codecov Report

Merging #1992 (6c94aae) into master (cfcf911) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1992   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          103       103           
  Lines         2097      2099    +2     
  Branches       473       474    +1     
=========================================
+ Hits          2097      2099    +2     
Impacted Files Coverage Δ
src/lib/isMagnetURI.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfcf911...6c94aae. Read the comment docs.

tux-tn
tux-tn previously approved these changes Jun 24, 2022
Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your PR @Rhilip and congrats for your first contribution 🎉

I Also remove the restrict of xt=urn: after magnet:?, since Magnet_URI_scheme Format notice:

Nice catch! LGTM

@tux-tn tux-tn added ready-to-land For PRs that are reviewed and ready to be landed 🎉 first-pr labels Jun 24, 2022
@Rhilip
Copy link
Contributor Author

Rhilip commented Jun 25, 2022

Please review again, thanks!
I make a fix since first commit will return true when validate wrong magnetURI which has any prefix exists in xt= component.

Before:

const magnetURIComponent = /xt(?:\.1)?=urn:((?:aich|bitprint|btih|ed2k|ed2khash|kzhash|md5|sha1|tree:tiger):[a-z0-9]{32}(?:[a-z0-9]{8})?|btmh:1220[a-z0-9]{64})($|&)/i;
const testWrongMagnetURI = 'magnet:?ttxt=urn:btmh:1220caf1e1c30e81cb361b9ee167c4aa64228a7fa4fa9f6105232b28ad099f3a302e';
magnetURIComponent.test(testWrongMagnetURI);  // return true, which is completed wrong

image

After:
image

Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your changes @Rhilip. I added a little suggestion concerning your regex, feel free to check it!

@tux-tn tux-tn added 🧹 needs-update For PRs that need to be updated before landing and removed ready-to-land For PRs that are reviewed and ready to be landed labels Jun 26, 2022
Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ! Thank you @Rhilip

@tux-tn tux-tn added ready-to-land For PRs that are reviewed and ready to be landed and removed 🧹 needs-update For PRs that need to be updated before landing labels Jun 29, 2022
Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for your contrib! 🎉

@profnandaa profnandaa merged commit 0175670 into validatorjs:master Jun 30, 2022
@Rhilip Rhilip deleted the feat-isMagnetURI-bittorrentv2 branch June 30, 2022 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🎉 first-pr ready-to-land For PRs that are reviewed and ready to be landed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants