Skip to content

Conversation

@tardyp
Copy link
Contributor

@tardyp tardyp commented Sep 16, 2021

previous version with svn uses blocking synchronous api and do not work well with proxies

the tag api do not contain the commit infos and thus we don't have the release date.
I think what we really want in vulnerable code is the list of actual releases, and not really the tags.

Fix #554

@tardyp tardyp force-pushed the githubtags branch 3 times, most recently from 064ad7b to 89a4b36 Compare September 17, 2021 10:49
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I would rather not remove the SVN approach. Rather we can keep both?
Using svn on GH feels klunky BUT it has benefits: you can get tags with their dates without auth and rate limiting.
So I would rather not drop yet I get your use case too of using an authenticated.

@tardyp tardyp force-pushed the githubtags branch 2 times, most recently from 0430d6b to 3bf14fc Compare September 21, 2021 15:47
@tardyp
Copy link
Contributor Author

tardyp commented Sep 21, 2021

@pombredanne thanks for the feedback.

I restored the svn approach as a fallback.

note that the svn also triggers some error 500 for big repos

svn ls --xml https://github.com/torvalds/linux/tags
<?xml version="1.0" encoding="UTF-8"?>
<lists>
<list
   path="https://github.com/torvalds/linux/tags">
svn: E170013: Unable to connect to a repository at URL 'https://github.com/torvalds/linux/tags'
svn: E175002: Unexpected server error 500 'Internal Server Error' on '/torvalds/linux/tags'

and we have no garantee github will not rate limit it once vulnerablecode is deployed everywhere. ;-)

@tardyp
Copy link
Contributor Author

tardyp commented Sep 21, 2021

even a moderately tagged project (50 tags) creates a 504 error after 2 minutes of github hard working:

svn ls --xml https://github.com/nexB/scancode-toolkit/tags
<?xml version="1.0" encoding="UTF-8"?>
<lists>
<list
   path="https://github.com/nexB/scancode-toolkit/tags">
svn: E175002: Unexpected HTTP status 504 'Gateway Timeout' on '/nexB/scancode-toolkit/tags'

@tardyp
Copy link
Contributor Author

tardyp commented Sep 21, 2021

I did some more research to see if we can use the graphql api to get the tag's commit date. it is possible:

It takes less than a second to get the first 100 tags in linux repo

https://docs.github.com/en/graphql/overview/explorer

{
  repository(name: "linux", owner: "torvalds") {
    refs(refPrefix: "refs/tags/", first: 100) {
      totalCount
      pageInfo {
        endCursor
      }
      nodes {
        name
        target {
          oid
          """ in case it is a standard tag"""
          ... on Commit {
            committedDate
          }
          """ in case it is a signed tag"""
          ... on Tag {
            target {
              ... on Commit {
                committedDate
              }
            }
          }
        }
      }
    }
  }
}

@pombredanne
Copy link
Member

Thanks for the research!
It makes sense to have the GH api first, attempting to have something authed first and second a fallback to unauthed and then fallback to SVN?

And yes, $ svn ls --xml https://github.com/torvalds/linux/tags is getting me repeated 500 errors so that's not a great solution, but on the other hand, not every repo is like Linux ;)

@tardyp
Copy link
Contributor Author

tardyp commented Sep 22, 2021

like I said in second comment scancode repo with its 50 tags is also failing. Thats why I think it is quite a dead end.

@tardyp
Copy link
Contributor Author

tardyp commented Sep 22, 2021

updated with Graphql version.

There is a 0 rate limit when not authenticated with graphql, means I directly hit a rate limit error without GH_TOKEN.
This is why I didn't implement that fallback.

svn version is kept if no GH_TOKEN is given in environ, but this
method has a lot of issues (proxy support, scalability)

Signed-off-by: Pierre Tardy <pierre.tardy@renault.com>
Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you ++
LGTM... merging shortly

@pombredanne pombredanne merged commit 6de9ed2 into aboutcode-org:main Oct 8, 2021
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.

vulnerable code uses svn to check git tags

2 participants