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 auth vulnerability #2805

Merged
merged 2 commits into from Jun 12, 2023
Merged

Fix auth vulnerability #2805

merged 2 commits into from Jun 12, 2023

Conversation

gmlewis
Copy link
Collaborator

@gmlewis gmlewis commented Jun 12, 2023

No description provided.

Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Merging #2805 (49af546) into master (3efdd2c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2805   +/-   ##
=======================================
  Coverage   98.06%   98.06%           
=======================================
  Files         132      132           
  Lines       11650    11655    +5     
=======================================
+ Hits        11424    11429    +5     
  Misses        154      154           
  Partials       72       72           
Impacted Files Coverage Δ
github/messages.go 100.00% <ø> (ø)
github/event.go 100.00% <100.00%> (ø)
github/repos_contents.go 87.82% <100.00%> (+0.23%) ⬆️

@gmlewis gmlewis merged commit a23606b into google:master Jun 12, 2023
9 checks passed
@gmlewis gmlewis deleted the fix-auth-bypass branch June 12, 2023 23:39
@molnarg
Copy link

molnarg commented Jul 13, 2023

It's a bit fishy, not sure if it can be bypassed by encoding the dots. Using https://pkg.go.dev/net/url#URL.ResolveReference and seeing if the result is different than the input would be a more bulletproof way to do it. Also, it could be applied to the whole u := fmt.Sprintf("repos/%s/%s/contents/%s", owner, repo, escapedPath) path, to ensure there is no ../ in any of the other parameters either.

@gmlewis
Copy link
Collaborator Author

gmlewis commented Jul 13, 2023

@molnarg - PRs are welcome. Thank you.

@k0ral
Copy link
Contributor

k0ral commented Jul 17, 2023

Could we get more information on the actual vulnerability this is protecting from, and the timeline to its resolution, if any ? Right now, I am considering multiple options:

  • submitting a PR to use a catchable sentinel error
  • submitting a PR to make that protection an opt-out
  • waiting for the vulnerability to be fixed, then submitting a PR to remove that protection

As I cannot find any reference to the vulnerability in the GitHub API documentation, I have difficulty making an informed decision 🙂 .

@gmlewis
Copy link
Collaborator Author

gmlewis commented Jul 17, 2023

@mrbobbytables - are you able to share any more information on the vulnerability that was reported to you?

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.

None yet

4 participants