Skip to content

Verify git checkout path is in site directory #5

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 2 commits into from
Mar 10, 2022

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Mar 9, 2022

This is unlikely, as we verify inputs come from GitHub using signatures, but it's best to be safe about this.

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2022

This pull request introduces 1 alert when merging 080a29c into 61de8b0 - view on LGTM.com

new alerts:

  • 1 for Uncontrolled data used in path expression

@QuLogic QuLogic force-pushed the safer-repo-update branch from 080a29c to c3b7a7a Compare March 9, 2022 22:02
@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2022

This pull request introduces 1 alert when merging c3b7a7a into 61de8b0 - view on LGTM.com

new alerts:

  • 1 for Uncontrolled data used in path expression

@QuLogic QuLogic mentioned this pull request Mar 9, 2022
@QuLogic QuLogic force-pushed the safer-repo-update branch from c3b7a7a to 518ac57 Compare March 10, 2022 00:39
@lgtm-com
Copy link

lgtm-com bot commented Mar 10, 2022

This pull request introduces 1 alert when merging 518ac57 into 79993d7 - view on LGTM.com

new alerts:

  • 1 for Uncontrolled data used in path expression

This is unlikely, as we verify inputs come from GitHub using signatures,
but it's best to be safe about this.
@tacaswell
Copy link
Member

In the right fix here to hard-code the repo we expect and launder it through a dictionary lookup?

@QuLogic QuLogic force-pushed the safer-repo-update branch from 518ac57 to a52631a Compare March 10, 2022 01:13
@QuLogic
Copy link
Member Author

QuLogic commented Mar 10, 2022

So looking at it closer, I think it's impossible to break out. We check that repo == repository, where one is the path given in the webhook, and the other is the request URL fragment. The latter cannot go up because aiohttp normalizes the URL, and it would not trigger the request handler.

I also think that LGTM and CodeQL are not correctly understanding pathlib, but I can open an issue there for that separately.

@lgtm-com
Copy link

lgtm-com bot commented Mar 10, 2022

This pull request fixes 1 alert when merging a52631a into 79993d7 - view on LGTM.com

fixed alerts:

  • 1 for Uncontrolled data used in path expression

@QuLogic QuLogic merged commit 02b318b into matplotlib:main Mar 10, 2022
@QuLogic QuLogic deleted the safer-repo-update branch March 10, 2022 04:01
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.

2 participants