Skip to content

bpo-41712: Vulnerable regex changed #23166

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

Merged
merged 12 commits into from
Nov 9, 2020
Merged

bpo-41712: Vulnerable regex changed #23166

merged 12 commits into from
Nov 9, 2020

Conversation

Pixmew
Copy link
Contributor

@Pixmew Pixmew commented Nov 5, 2020

Using suggestion ""For example, you can modify the sub-pattern \w+\d+ to ([A-Za-z_]*\d)+"" and converted to ([A-za-z_]+\d+)
which should Fix the issue of vulnerable regex. ""can be exploited with the following string "1.1.1"+"1" * 5000 + "!" ""

Test Result : Working as intended
Sorry if this not much this is my first pr to big org.

https://bugs.python.org/issue41712

bpo-41712: Removal of Unnessery regex conditions  
Using suggestion 
For example, you can modify the sub-pattern \w+\d+ to ([A-Za-z_]*\d)+
which should Fix the issue of vulnerable regex.
Test Result : Working as intended

Sorry if this not much this is my first pr to big org.
bpo-41712: Removal of Vulnerable regex conditions  
Using suggestion  ""For example, you can modify the sub-pattern \w+\d+ to ([A-Za-z_]*\d)+""  and converted to ([A-za-z_]+\d+)
which should Fix the issue of vulnerable regex.
Test Result : Working as intended

Sorry if this not much this is my first pr to big org.
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@Pixmew

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

Copy link
Contributor Author

@Pixmew Pixmew left a comment

Choose a reason for hiding this comment

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

should work

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Hello, thanks for your first PR to CPython. I'm not a core dev but I'll do my best to try address some of this.

@Pixmew
Copy link
Contributor Author

Pixmew commented Nov 7, 2020

Thank you Sir for your suggestions it helped me a lot.
and \w was replaced by [A-Za-z_] because \w also contains some extra characters as well which may not be valid for our task.

@Pixmew Pixmew removed the request for review from a team November 7, 2020 07:53
@Pixmew
Copy link
Contributor Author

Pixmew commented Nov 7, 2020

sorry i accendenttly removed reviewer what should i do
@Fidget-Spinner #23166

@Pixmew
Copy link
Contributor Author

Pixmew commented Nov 7, 2020

Sir what is core review

@Pixmew Pixmew changed the title bpo-41712: Vulnerable regex changes as suggested bpo-41712: Vulnerable regex changed Nov 7, 2020
@Pixmew Pixmew marked this pull request as draft November 7, 2020 13:12
@Pixmew Pixmew marked this pull request as ready for review November 7, 2020 13:12
@Pixmew Pixmew closed this Nov 7, 2020
@Pixmew Pixmew reopened this Nov 7, 2020
@Pixmew Pixmew closed this Nov 7, 2020
@Pixmew
Copy link
Contributor Author

Pixmew commented Nov 7, 2020

@Fidget-Spinner Sir can you please review the PR #23191
I Reapplide for PR because previous/This PR #23166 was not working as inteded due to my mistake
It was not requesting for review core review so i had to close this PR and file a new PR

Thnakyou sir

@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Nov 7, 2020

Sorry, I don't think you should open multiple PRs because it makes it harder to track the development of a solution. Maybe reopen this one and close the other? (I don't see any difference between the two anyways)

I'm not a core developer. The awaiting core review tag means it's waiting for a core developer to approve this PR. Probably one of the core developers who commented on the issue in bugs.python will review/approve it eventually. They receive mail notifications for the PR after all.

These things take time - a core developer may take up to a month, or maybe even longer to take a look at your PR. So I recommend exercising patience. The core devs are mostly doing this out of their own free time.

If nobody looks at your PR after maybe 2 - 4 weeks, you can comment on the bugs.python.org issue and ask for a PR.

Edit: BTW you don't have to @ me in your comments, I already receive notifications for your replies anyways.

@Pixmew
Copy link
Contributor Author

Pixmew commented Nov 7, 2020

Thankyou sir but it dosent say requested review
and i did this by mistake:
Pixmew removed the request for review from python/windows-team 8 hours ago

@Pixmew
Copy link
Contributor Author

Pixmew commented Nov 7, 2020

What do you think i should do
reopen this and close other one?

@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Nov 7, 2020

What do you think i should do reopen this and close other one?

Yes. Also I'd recommend this:

If nobody looks at your PR after maybe 2 - 4 weeks, you can comment on the bugs.python.org issue and ask for a PR.

The request for review isn't required. They can review it even without that.

Pinging @zooba for this, sorry for the noise generated.

@Pixmew
Copy link
Contributor Author

Pixmew commented Nov 7, 2020

Ok Sir Thankyou. You have been are very Kind and helpfull.

@Pixmew Pixmew reopened this Nov 7, 2020
@zooba zooba added the skip news label Nov 9, 2020
@zooba
Copy link
Member

zooba commented Nov 9, 2020

Thanks for the contribution! This doesn't need a NEWS entry, so I added the tag and removed the file (I'm worried it may concern people who read updates, because it mentions a vulnerability that they were never vulnerable to).

Once CI clears, I'll merge it.

@zooba
Copy link
Member

zooba commented Nov 9, 2020

For future contributions, you may also want to work from a branch in your repository, rather than master (see step 2 at https://devguide.python.org/pullrequest/#step-by-step-guide).

@Pixmew
Copy link
Contributor Author

Pixmew commented Nov 9, 2020

Thanks you Sir
And I don't mean any disrespects about the vulnerability but it was title as vulnerability in the issue tracker so I decided to use that.If you want I can change it.

@zooba zooba merged commit 1f73c32 into python:master Nov 9, 2020
@zooba
Copy link
Member

zooba commented Nov 9, 2020

No problems at all. There is no product impact from this change, so it doesn't need to be listed in the product changes :)

Copying the title of the original bug is fine, though in this case we quickly determined that the title was inaccurate (but never changed it).

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants