Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,14 @@ jobs:

**Important Notes**:

- The GitHub token must have the `actions: read` permission in your workflow
- The GitHub token must have the corresponding permission in your workflow
- If the permission is missing, Claude will warn you and suggest adding it
- Currently, only `actions: read` is supported, but the format allows for future extensions
- The following additional permissions can be requested beyond the defaults:
- `actions: read`
- `checks: read`
- `discussions: read` or `discussions: write`
- `workflows: read` or `workflows: write`
- Standard permissions (`contents: write`, `pull_requests: write`, `issues: write`) are always included and do not need to be specified

## Custom Environment Variables

Expand Down
63 changes: 55 additions & 8 deletions src/github/token.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
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 +37 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

Security: Missing input validation on permission keys and values

The parser accepts arbitrary strings without validation. Consider adding a whitelist to prevent privilege escalation:

Suggested change
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;
const colonIndex = trimmed.indexOf(":");
if (colonIndex === -1) continue;
const key = trimmed.slice(0, colonIndex).trim();
const value = trimmed.slice(colonIndex + 1).trim();
// Validate against allowed permissions
const ALLOWED_KEYS = ['actions', 'checks', 'discussions', 'workflows'];
const ALLOWED_VALUES = ['read', 'write'];
if (!key || !value) continue;
if (!ALLOWED_KEYS.includes(key)) {
throw new Error(`Invalid permission key '${key}'. Allowed: ${ALLOWED_KEYS.join(', ')}`);
}
if (!ALLOWED_VALUES.includes(value)) {
throw new Error(`Invalid permission value '${value}'. Allowed: ${ALLOWED_VALUES.join(', ')}`);
}
additional[key] = value;

Without validation, malicious workflows could inject arbitrary permissions like admin: write or downgrade defaults by overriding contents: writecontents: read.

}
}

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) {
Expand Down Expand Up @@ -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");

Expand Down
97 changes: 97 additions & 0 deletions test/parse-permissions.test.ts
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",
});
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Test Coverage Gap: Missing tests for exchangeForAppToken()

The exchangeForAppToken() function was modified to accept and use the permissions parameter, but there are no tests covering:

  1. Calling with undefined permissions (existing behavior)
  2. Calling with valid permissions object
  3. Verifying the request includes Content-Type: application/json header when permissions are provided
  4. Verifying the request body includes { permissions: {...} }

Consider adding tests to verify the integration between parseAdditionalPermissions() and exchangeForAppToken().

Loading