Skip to content

fix: insert auth headers for api.github.com#754

Merged
XAMPPRocky merged 2 commits into
XAMPPRocky:mainfrom
pfnsec:fix-uri-authority-check
Mar 26, 2025
Merged

fix: insert auth headers for api.github.com#754
XAMPPRocky merged 2 commits into
XAMPPRocky:mainfrom
pfnsec:fix-uri-authority-check

Conversation

@pfnsec
Copy link
Copy Markdown
Contributor

@pfnsec pfnsec commented Mar 5, 2025

into_stream() for pagination is currently broken. Consider the following case:

        let stream_client = client.clone();
        let stream = client
            .pulls(&owner, &repo)
            .list_files(issue_number)
            .await?
            .into_stream(&stream_client);

        pin!(stream);

        let mut objects = Vec::new();
        while let Some(diff) = stream.try_next().await? {
            tracing::info("{}", &diff.filename);
        }

When more than 30 files are modified in a pull request, the Github API will return a response with a link header containing a link to the next page of results.

Octocrab correctly resolves the next URL from this header when it exhausts the first page of results, and so try_next() will make another API call to Github of the form https://api.github.com/repositories/{repo_id}/issues?page=2.

However, because the authority of this URL is api.github.com and not None, Octocrab will not include auth headers with that request, and the request will fail.

This pull request alters this behaviour to include auth headers if the URL authority is None or "api.github.com".

Fixes:
#576
#738

@pfnsec
Copy link
Copy Markdown
Contributor Author

pfnsec commented Mar 25, 2025

@XAMPPRocky
This is blocking our release to production - any chance I could get this looked at?
Big thanks in advance.

@XAMPPRocky
Copy link
Copy Markdown
Owner

Thank you for your PR, and congrats on your first contribution! 🎉

@XAMPPRocky XAMPPRocky merged commit e37e5a8 into XAMPPRocky:main Mar 26, 2025
@github-actions github-actions Bot mentioned this pull request Mar 26, 2025
@pvandervelde
Copy link
Copy Markdown

@pfnsec Should this fix the issue for all API calls? I'm calling

let installations = octocrab
        .apps()
        .installations()
        .send()
        .await
        .unwrap_or(Page::<octocrab::models::Installation>::default())
        .take_items();

    let id = InstallationId(installation_id);
    let Some(installation_index) = installations.iter().position(|l| l.id == id) else {
        return Err(Error::FailedToFindAppInstallation(
            repository_owner.to_string(),
            source_repository.to_string(),
            installation_id,
        ));
    };

    let installation = &installations[installation_index];

    let create_access_token = CreateInstallationAccessToken::default();

    // Create an access token for the installation
    let access: InstallationToken = octocrab
        .post(
            installation.access_tokens_url.as_ref().unwrap(),
            Some(&create_access_token),
        )
        .await
        .map_err(|_| {
            Error::FailedToCreateAccessToken(
                repository_owner.to_string(),
                source_repository.to_string(),
                installation_id,
            )
        })?;

    // Use the API token
    let api_with_token = octocrab::OctocrabBuilder::new()
        .personal_token(access.token)
        .build()
        .unwrap();

  let app = api_with_token.current().app().await;

The last call returns an error A JSON web token could not be decoded, similar (but potentially not the same) as in #576. I have updated to 0.44. Note that the call works if you use an octocrab instance that has authenticated as an app but not as an installation 🤷

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.

3 participants