Skip to content

fix: exclude information about the repo and owner - #136

Merged
woodruffw merged 1 commit into
zizmorcore:mainfrom
funnelfiasco:exclude_github_repo_info
Nov 8, 2024
Merged

fix: exclude information about the repo and owner#136
woodruffw merged 1 commit into
zizmorcore:mainfrom
funnelfiasco:exclude_github_repo_info

Conversation

@funnelfiasco

Copy link
Copy Markdown
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 <ben@kusari.dev>
@woodruffw

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member

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 zizmorcore:main Nov 8, 2024
@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