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: exclude information about the repo and owner #136

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

funnelfiasco
Copy link
Contributor

This would seem to fall into the category of "if these values are unsafe, we have bigger problems to deal with"

This would seem to fall into the category of "if these values are
unsafe, we have bigger problems to deal with"

Signed-off-by: Ben Cotton <[email protected]>
@woodruffw
Copy link
Owner

Thanks @funnelfiasco! I think this is going to be similar to #128 -- excluding repository_id and repository_owner_id seems 100% right to me, but I need to think about the others.

(AFAIK repo/owner information is probably safe, but URL quoting/escaping isn't always super consistent, and there may be funny things that someone can do with Enterprise deployment URLs.)

@woodruffw woodruffw added the enhancement New feature or request label Nov 7, 2024
@funnelfiasco
Copy link
Contributor Author

Of course GitHub doesn't provide documentation on what's actually possible, but in the case of these values there's nothing an attacker could do to modify them. So even if it's possible and someone names their repository Robert'); DROP TABLE Students;-- then plenty of other things will probably break before they ever get to running a GitHub workflow.

@woodruffw
Copy link
Owner

Of course GitHub doesn't provide documentation on what's actually possible, but in the case of these values there's nothing an attacker could do to modify them.

I think that's right, except for in the fork context -- in that context the attacker controls all of these, but I agree that there's still no injection risk with them (I suppose someone could name their GitHub account reboot or something like that, but they still don't have the escape primitives needed).

Given that, I'm good with this. Thanks for the PR!

@woodruffw woodruffw added false-positive bugfix Fixes a known bug and removed enhancement New feature or request labels Nov 8, 2024
@woodruffw woodruffw changed the title Exclude information about the repo and owner fix: exclude information about the repo and owner Nov 8, 2024
@woodruffw woodruffw merged commit 7f30170 into woodruffw:main Nov 8, 2024
4 checks passed
@woodruffw woodruffw mentioned this pull request Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a known bug false-positive
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants