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

Fix: Invalid regular expression: missing / #1110 #1111

Closed
wants to merge 1 commit into from
Closed

Fix: Invalid regular expression: missing / #1110 #1111

wants to merge 1 commit into from

Conversation

dskaiser82
Copy link

@dskaiser82 dskaiser82 commented Aug 18, 2022

I didn't write this code fix and give the credit to @GreepTheSheep

Temporary Fix for YouTube's new signature which is linked to this issue
#1110

Please review for approval

closes #1110
closes #1113

@GreepTheSheep
Copy link

ty for making this PR

@dskaiser82
Copy link
Author

Greep while we're waiting for this PR to be merged in I will be using the fork you made!

@richardabear
Copy link

Thanks for this guys. I'll be moving to this as well for now. hopefully this gets merged in soon.

@dskaiser82
Copy link
Author

Looks like I have one approval but need 2 more approvals

Copy link

@dsnsgithub dsnsgithub left a comment

Choose a reason for hiding this comment

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

This pull request solves both #1110 and #1113

@dskaiser82
Copy link
Author

We have some approvals but this is the blocker:
"First-time contributors need a maintainer to approve running workflows. Learn more.
1 successful check"

Who is a maintainer?

@dskaiser82
Copy link
Author

Maybe
TimeForANinja
VoltrexMaster
fent

Can help as maintainers?

garoxas added a commit to garoxas/extractor-project that referenced this pull request Aug 20, 2022
@arcovoltaico
Copy link

It seems the project needs more maintainers or at least in different time zones as this critical issue has made the library unusable for a day. Anyway impressive effort solving it, thanks so much!

@RyloRiz
Copy link

RyloRiz commented Aug 21, 2022

It's been nearly an additional twelve hours. This is a breaking change and needs to be fixed ASAP.

@Trott
Copy link

Trott commented Aug 21, 2022

It's been nearly an additional twelve hours. This is a breaking change and needs to be fixed ASAP.

This is an open source project that is being provided to you for free. No one asked you to use it, no one sold it to you, and most importantly, no one owes you anything. The license is clear on this: 'THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED...'

If you really need this "fixed ASAP", you can do it yourself. The code is right there in this pull request. Here's one of many ways to hot-patch an existing installation.

cd node_modules/ytdl-core
curl -L https://github.com/fent/node-ytdl-core/pull/1111.patch | patch lib/sig.js

You could also follow the suggestion at #1110 (comment) to use the patch author's fork.

Or you could downgrade to 4.10.0.

@Trott
Copy link

Trott commented Aug 21, 2022

@dskaiser82 @GreepTheSheep This repository uses semantic-release so you'll probably need to edit the first line of the commit message to trigger a release. Maybe use git commit --amend to change it to fix: adjust for YouTube's new signature and then force push to the PR branch?

@dskaiser82
Copy link
Author

@Trott Agree with you btw on the open source comment, and the multiple temp solutions out there.
I'll update the commit, but I dont think thats the blocker now.

It's "2 workflows awaiting approval
First-time contributors need a maintainer to approve running workflows. Learn more.
1 successful check"

I dont think we have any maintainers paying attention to the PR.....even so yes I can ammend the commit

@Trott
Copy link

Trott commented Aug 21, 2022

I'll update the commit, but I dont think thats the blocker now.

Yeah, I don't think it's a blocker at the moment. I'm just saying that you might want to have the commit message formatted the way the tool will expect it so that it doesn't end up delaying things later on.

I dont think we have any maintainers paying attention to the PR

@fent @TimeForANinja

@dskaiser82
Copy link
Author

"A repository owned by a personal account has two permission levels: the repository owner and collaborators. For more information, see "Permission levels for a personal account repository."

Need that "write access".

@dskaiser82
Copy link
Author

@Trott When the Merge button shows, we will do a squashed commit the GitHub GUI let's the user do. We can squash then.
We have the same flow at work.

This is an example of a squashed commit:

feat: add IPv6 block rotating (#713)

* IPv6 Rotating ^-^

* linter got mad at missing semicolon nice

* Fixing format

Fixing the format of files so eslint does not throw any errors.

* adding colon

* added test for IPv6 Block

@mtripg6666tdr
Copy link
Contributor

mtripg6666tdr commented Aug 21, 2022

btw using close keyword in pr description like "close #issueid" will make a link to the original issue and automatically close the issue when the pr was merged, it might be better if so

@73nici
Copy link

73nici commented Aug 21, 2022

Ty for being so fast with this fix 🚀

@Trott
Copy link

Trott commented Aug 23, 2022

@TimeForANinja Anything else that needs to happen here before this can be merged?

@ghost
Copy link

ghost commented Aug 23, 2022

@Trott I don't think so. I used it with my app and it works very well.

@Trott
Copy link

Trott commented Aug 24, 2022

@Trott I don't think so. I used it with my app and it works very well.

Thanks, but I was addressing @TimeForANinja specifically because they are (AFAICT) the one person who has been active in this pull request who has the permission to merge the commit. It was a hopefully-polite way of saying "bump, please--can we merge this now?"

@TimeForANinja
Copy link
Collaborator

@TimeForANinja Anything else that needs to happen here before this can be merged?

just one thing @Trott and @dskaiser82
if it's possible to get this resolved by doing some minor, quick updates to the cutAfterJSON method i'd prefer it

@TimeForANinja
Copy link
Collaborator

TimeForANinja commented Aug 24, 2022

tried some stuff
you can find my changes here and the branch itself here
it worked for the hand full of requests i did - feel free to comment on it

no idea what exactly went wrong here #1110 (comment)
did test for try { xxx } catches (e) { xxx } in the text and they got recognized fine

@dskaiser82
Copy link
Author

@TimeForANinja

I did this PR from @GreepTheSheep 's Fork. I've personally been using the fork and know this code update works. With that being said, we can go with your changes instead and I can close this PR.

As you are a maintainer just advise what you'd like to do.

cc: @Trott

@gatecrasher777
Copy link
Contributor

gatecrasher777 commented Aug 25, 2022

@TimeForANinja

you can find my changes here and the branch itself here
it worked for the hand full of requests i did - feel free to comment on it

One of the array elements found in a base.js n-code extraction today is

    ... -1816574795, '",;/[;', function ...

which would break cutAfterJSON because you would have an orphan double quote. Perhaps if single quotes and back quotes are also added as string markers, that would do it.

I also prefer to keep cutAfterJSON to extract the complete js function. Using

    .join("");}

as the end marker (in the proposed solution) will break as soon as Youtube change it to something else. Whereas a robust cutAfterJSON would assimilate any future changes.

@dskaiser82
Copy link
Author

dskaiser82 commented Aug 26, 2022

I vouch for @gatecrasher777 change. At least the fork and this PR.

@fent Any chance you can add one more Maintainer to me, @Trott , @gatecrasher777 or @GreepTheSheep ?

We have a fix...we have a PR that at the most is long term and at least a good band aid to fix for all the users that use this.

Maybe it's time to give up on this main Repo? This process is sad and disheartending.....I get if current maintainets are over or it....I do...But can at leat communicate this and the need to jump to new fork or repo?

cc:
@TimeForANinja

@dskaiser82
Copy link
Author

And I get it..... @fent makes some thing awesome that devs use... Coders like @TimeForANinja help maintain it. Countless hours are put in to make it better and keep it a live....Noone is getting paid.....over time energy and momentum is lost....

But for the devs that do give a shit about and want to help....puts us in an frustrating situation.....which we have options....
Anyways I get it....

const functionBody = `var ${functionStart}${utils.cutAfterJSON(subBody)};${functionName}(ncode);`;
const end = body.indexOf('.join("")};', ndx);
const subBody = body.slice(ndx, end);
const functionBody = `${subBody}.join("")};${functionName}(ncode);`;
functions.push(functionBody);
Copy link
Author

Choose a reason for hiding this comment

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

Any changes requested for these lines

@TimeForANinja
Copy link
Collaborator

replaced by #1126
still ty for the pr and finding the problem

@wukko
Copy link

wukko commented Sep 1, 2022

Ironically enough, this pr actually fixes the current regex issue (#1131), unlike #1126, which is already throwing errors again.

Copy link

@pixelblob pixelblob left a comment

Choose a reason for hiding this comment

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

Works well for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SyntaxError: Missing catch or finally after try Invalid regular expression: missing /