-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: send additional_permissions in token exchange request #859
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -16,15 +16,60 @@ async function getOidcToken(): Promise<string> { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| async function exchangeForAppToken(oidcToken: string): Promise<string> { | ||||||
| const DEFAULT_PERMISSIONS: Record<string, string> = { | ||||||
| contents: "write", | ||||||
| pull_requests: "write", | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical Bug: GitHub's API uses
When the API receives
Suggested change
The corresponding test expectations in |
||||||
| issues: "write", | ||||||
| }; | ||||||
|
|
||||||
| export function parseAdditionalPermissions(): | ||||||
| | Record<string, string> | ||||||
| | undefined { | ||||||
| const raw = process.env.ADDITIONAL_PERMISSIONS; | ||||||
| if (!raw || !raw.trim()) { | ||||||
| return undefined; | ||||||
| } | ||||||
|
|
||||||
| const additional: Record<string, string> = {}; | ||||||
| for (const line of raw.split("\n")) { | ||||||
| const trimmed = line.trim(); | ||||||
| if (!trimmed) continue; | ||||||
| const colonIndex = trimmed.indexOf(":"); | ||||||
| if (colonIndex === -1) continue; | ||||||
| const key = trimmed.slice(0, colonIndex).trim(); | ||||||
| const value = trimmed.slice(colonIndex + 1).trim(); | ||||||
| if (key && value) { | ||||||
| additional[key] = value; | ||||||
| } | ||||||
| } | ||||||
|
Comment on lines
+34
to
+44
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Data Loss Risk: The parser silently skips invalid lines without any warning. If a user makes a typo like Consider adding logging for skipped lines: for (const line of raw.split("\n")) {
const trimmed = line.trim();
if (!trimmed) continue;
const colonIndex = trimmed.indexOf(":");
if (colonIndex === -1) {
console.log(`Warning: Skipping malformed permission line (missing colon): "${trimmed}"`);
continue;
}
const key = trimmed.slice(0, colonIndex).trim();
const value = trimmed.slice(colonIndex + 1).trim();
if (!key || !value) {
console.log(`Warning: Skipping permission line with empty key or value: "${trimmed}"`);
continue;
}
additional[key] = value;
}This will help users debug configuration issues quickly. |
||||||
|
|
||||||
| if (Object.keys(additional).length === 0) { | ||||||
| return undefined; | ||||||
| } | ||||||
|
|
||||||
| return { ...DEFAULT_PERMISSIONS, ...additional }; | ||||||
| } | ||||||
|
|
||||||
| async function exchangeForAppToken( | ||||||
| oidcToken: string, | ||||||
| permissions?: Record<string, string>, | ||||||
| ): Promise<string> { | ||||||
| const headers: Record<string, string> = { | ||||||
| Authorization: `Bearer ${oidcToken}`, | ||||||
| }; | ||||||
| const fetchOptions: RequestInit = { | ||||||
| method: "POST", | ||||||
| headers, | ||||||
| }; | ||||||
|
|
||||||
| if (permissions) { | ||||||
| headers["Content-Type"] = "application/json"; | ||||||
| fetchOptions.body = JSON.stringify({ permissions }); | ||||||
| } | ||||||
|
|
||||||
| const response = await fetch( | ||||||
| "https://api.anthropic.com/api/github/github-app-token-exchange", | ||||||
| { | ||||||
| method: "POST", | ||||||
| headers: { | ||||||
| Authorization: `Bearer ${oidcToken}`, | ||||||
| }, | ||||||
| }, | ||||||
| fetchOptions, | ||||||
| ); | ||||||
|
|
||||||
| if (!response.ok) { | ||||||
|
|
@@ -89,9 +134,11 @@ export async function setupGitHubToken(): Promise<string> { | |||||
| const oidcToken = await retryWithBackoff(() => getOidcToken()); | ||||||
| console.log("OIDC token successfully obtained"); | ||||||
|
|
||||||
| const permissions = parseAdditionalPermissions(); | ||||||
|
|
||||||
| console.log("Exchanging OIDC token for app token..."); | ||||||
| const appToken = await retryWithBackoff(() => | ||||||
| exchangeForAppToken(oidcToken), | ||||||
| exchangeForAppToken(oidcToken, permissions), | ||||||
| ); | ||||||
| console.log("App token successfully obtained"); | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| import { describe, expect, test, beforeEach, afterEach } from "bun:test"; | ||
| import { parseAdditionalPermissions } from "../src/github/token"; | ||
|
|
||
| describe("parseAdditionalPermissions", () => { | ||
| let originalEnv: string | undefined; | ||
|
|
||
| beforeEach(() => { | ||
| originalEnv = process.env.ADDITIONAL_PERMISSIONS; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| if (originalEnv === undefined) { | ||
| delete process.env.ADDITIONAL_PERMISSIONS; | ||
| } else { | ||
| process.env.ADDITIONAL_PERMISSIONS = originalEnv; | ||
| } | ||
| }); | ||
|
|
||
| test("returns undefined when env var is not set", () => { | ||
| delete process.env.ADDITIONAL_PERMISSIONS; | ||
| expect(parseAdditionalPermissions()).toBeUndefined(); | ||
| }); | ||
|
|
||
| test("returns undefined when env var is empty string", () => { | ||
| process.env.ADDITIONAL_PERMISSIONS = ""; | ||
| expect(parseAdditionalPermissions()).toBeUndefined(); | ||
| }); | ||
|
|
||
| test("returns undefined when env var is only whitespace", () => { | ||
| process.env.ADDITIONAL_PERMISSIONS = " \n \n "; | ||
| expect(parseAdditionalPermissions()).toBeUndefined(); | ||
| }); | ||
|
|
||
| test("parses single permission and merges with defaults", () => { | ||
| process.env.ADDITIONAL_PERMISSIONS = "actions: read"; | ||
| expect(parseAdditionalPermissions()).toEqual({ | ||
| contents: "write", | ||
| pull_requests: "write", | ||
| issues: "write", | ||
| actions: "read", | ||
| }); | ||
| }); | ||
|
|
||
| test("parses multiple permissions", () => { | ||
| process.env.ADDITIONAL_PERMISSIONS = "actions: read\nworkflows: write"; | ||
| expect(parseAdditionalPermissions()).toEqual({ | ||
| contents: "write", | ||
| pull_requests: "write", | ||
| issues: "write", | ||
| actions: "read", | ||
| workflows: "write", | ||
| }); | ||
| }); | ||
|
|
||
| test("additional permissions can override defaults", () => { | ||
| process.env.ADDITIONAL_PERMISSIONS = "contents: read"; | ||
| expect(parseAdditionalPermissions()).toEqual({ | ||
| contents: "read", | ||
| pull_requests: "write", | ||
| issues: "write", | ||
| }); | ||
| }); | ||
|
|
||
| test("handles extra whitespace around keys and values", () => { | ||
| process.env.ADDITIONAL_PERMISSIONS = " actions : read "; | ||
| expect(parseAdditionalPermissions()).toEqual({ | ||
| contents: "write", | ||
| pull_requests: "write", | ||
| issues: "write", | ||
| actions: "read", | ||
| }); | ||
| }); | ||
|
|
||
| test("skips empty lines", () => { | ||
| process.env.ADDITIONAL_PERMISSIONS = | ||
| "actions: read\n\n\nworkflows: write\n\n"; | ||
| expect(parseAdditionalPermissions()).toEqual({ | ||
| contents: "write", | ||
| pull_requests: "write", | ||
| issues: "write", | ||
| actions: "read", | ||
| workflows: "write", | ||
| }); | ||
| }); | ||
|
|
||
| test("skips lines without colons", () => { | ||
| process.env.ADDITIONAL_PERMISSIONS = | ||
| "actions: read\ninvalid line\nworkflows: write"; | ||
| expect(parseAdditionalPermissions()).toEqual({ | ||
| contents: "write", | ||
| pull_requests: "write", | ||
| issues: "write", | ||
| actions: "read", | ||
| workflows: "write", | ||
| }); | ||
| }); | ||
ashwin-ant marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }); | ||
ashwin-ant marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Uh oh!
There was an error while loading. Please reload this page.