Tokenless V3 #533
Tokenless V3 #533
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #533 +/- ##
==========================================
- Coverage 91.48% 91.45% -0.03%
==========================================
Files 599 602 +3
Lines 16268 16394 +126
==========================================
+ Hits 14882 14993 +111
- Misses 1386 1401 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #533 +/- ##
==========================================
- Coverage 91.48% 91.45% -0.03%
==========================================
Files 599 602 +3
Lines 16268 16394 +126
==========================================
+ Hits 14882 14993 +111
- Misses 1386 1401 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 16 files with indirect coverage changes |
Codecov ReportAttention: Patch coverage is
Changes have been made to critical files, which contain lines commonly executed in production. Learn more ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #533 +/- ##
======================================
Coverage 95.78 95.78
======================================
Files 774 777 +3
Lines 17124 17549 +425
======================================
+ Hits 16401 16809 +408
- Misses 723 740 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
We need to be careful merging these changes to avoid re-introducing the bug of having an upload from a fork branch accidentally overwrite coverage in the upstream branch. Considering the upload endpoints used by the CLI either we change the CLI to send the branch info every time (maybe through headers) OR we use the info we have to make the validation that the branch name is in the format Currently I believe it should be possible to make this check:
Personally I feel that we should be able to accept / reject a request without having to look at the request body. So I'd opt to make the CLI send specific headers with the information we need. |
We could just validate that the x-tokenless-pr header is in the format we expect and has the correct repo name (it matches the one in the url)? |
|
X-Tokenless-PR is a number [1] that we used to get the correct PR from the provider. That does little for us considering the changes in this PR. If you just change the value the header name will be misleading. X-Tokenless should not match the repo name in the URL if it comes from a fork [1], cause it's supposed to be the fork slug, while the slug in the URL should be the upstream's. |
- there are now 2 tokenless authentication modes, one for the commits endpoint and one for the reports/uploads endpoints - the commit endpoint just requires the X-Tokenless header to be present and have the format "<user>:<branch name>" the branch of the commit that will be created will be set to the value of the X-Tokenless header. - the reports and uploads enpoints require the X-Tokenless header to specify the branch name of the commit that they are targetting and it should match the format from the commits endpoint - All endpoints require the repository that is being targetted to be public Signed-off-by: joseph-sentry <[email protected]>
joseph-sentry
force-pushed
the
joseph/new-tokenless
branch
from
960927f
to
fecbe1b
Compare
joseph-sentry
changed the title
| if ":" not in tokenless: | ||
| raise exceptions.AuthenticationFailed(tokenless_auth_failed_message) | ||
|
|
||
| # make sure it's backwards compatible with the old way that |
There was a problem hiding this comment.
It can't be compatible if validation fails (in the line above) because "the old way" doesn't include a : in the X-Tokenless header, ...right?
There was a problem hiding this comment.
this probably belongs in another PR, but in addition to getting rid of the get_pull_request_info() check like you do here, we also want to get rid of the "recent CI run" check. that happens in upload/helpers.py. with that gone, i think we can delete the whole upload/tokenless directory
also, the other day you made a great point about the report/upload endpoints being able to get the branch from the commit rather than needing it to be sent by the CLI. i think we can take advantage of that to simplify this logic across the API, CLI, and CI actions
on the CLI side, we already have to send the branch name in the request body for the create-commit step, and we already modify the branch name for forks there. the other two steps include which commit they are working with, so the API will be able to look up the commit and get the branch from there. so none of the commands need to set X-Tokenless, and the action doesn't have to set any extra env vars. (i think this also avoids an edge case where an attacker passes authentication by passing X-Tokenless: fork:hahaha to the do-upload endpoint but the commit SHA they passed is actually a commit on main)
then on the API side, i think the only difference between the commit endpoint and the others is where we get the branch name from? if so, i think we can merge back into one TokenAuthentication class, and if the commit sha is part of the URL we get the branch from the commit DB object, otherwise we look for the branch in the request body because we're creating a new commit
my brain is a little fried right now so this comment might not be very useful. i'll do another pass tomorrow, or maybe we could hop on a call and talk about it
Purpose/Motivation
We don't want to make GH API calls when doing tokenless anymore
Links to relevant tickets
codecov/engineering-team#1574
Related: codecov/codecov-action#1406
What does this PR do?