Fix auth vulnerability

[gmlewis]

No description provided.

[codecov]

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%)	⬆️

[molnarg]

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]

@molnarg - PRs are welcome. Thank you.

[k0ral]

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]

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

[ran-arigur]

@mrbobbytables : Would you happen to know (1) whether this vulnerability still exists and (2) whether the vulnerability affects any paths that literally contain .. as a substring (as this change implies), rather than just paths that contain .. as a path component (which is more understandable IMHO)? I ask because there's no good way to work around this check in cases where a file– or directory-name happens to actually include .., so if the check can safely be narrowed, that would be very helpful.