Skip to content

Use GitHub API for pushes by default and require explicit github-token when using custom tokens - #692

Merged
Andarist merged 26 commits into
mainfrom
fix/github-token-push
Aug 3, 2026
Merged

Use GitHub API for pushes by default and require explicit github-token when using custom tokens#692
Andarist merged 26 commits into
mainfrom
fix/github-token-push

Conversation

@Andarist

@Andarist Andarist commented Jul 6, 2026

Copy link
Copy Markdown
Member

This should fix auth headers when the action gets used with actions/checkout's default persist-credentials: true

related to sveltejs/kit#16248

@changeset-bot

changeset-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: cb7d7f0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@changesets/action Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Andarist
Andarist marked this pull request as ready for review July 29, 2026 23:14
@Andarist
Andarist requested review from bluwy and emmatown as code owners July 29, 2026 23:14
@Andarist
Andarist requested a review from beeequeue July 29, 2026 23:14
Comment thread src/github.test.ts
).toEqual(remote.requests.map(() => [getAuthorization(actionToken)]));
}, 15_000);

it("uses the push URL when it differs from the fetch URL", async () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we need to test for this? is it a case we handle explicitly?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

we handle it here:

action/src/github.ts

Lines 109 to 121 in 99664fb

// `git push origin` may use remote.origin.pushurl instead of the fetch URL,
// and Git supports multiple push URLs. Ask Git for the effective targets so
// the URL-specific auth below applies to every HTTP destination.
const { stdout } = await getExecOutput(
"git",
["remote", "get-url", "--push", "--all", "origin"],
{
cwd: this.cwd,
ignoreReturnCode: true,
// A user-configured remote can contain credentials.
silent: true,
},
);

Comment thread src/github.ts Outdated
Comment on lines +139 to +147
// GIT_CONFIG_COUNT/KEY_n/VALUE_n add command-scoped config. Preserve any
// existing entries and append ours. `http.extraHeader` is multi-valued, so
// merely adding our Authorization header would make Git send both tokens.
// An empty value resets the list; the following value adds only our token.
//
// In v1, `github-token` lived in ~/.netrc. When checkout had already
// supplied Authorization through an extraheader, that header took
// precedence and ~/.netrc was effectively a fallback. These entries
// intentionally make `github-token` win for pushes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i think a full description of how we override the token and why we do it that way should be written somewhere else, the current doc comments are both duplicated and split up so they are hard to read.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added a short explainer on why we are doing this stuff at the top of this function but kept comments relevant to specific code lines close to them (while improving them a little bit). I feel it's easier to follow the steps performed here this way

Comment thread src/test-utils.ts Outdated
Comment thread src/test-utils.ts
Comment on lines +118 to +248
async function runGitHttpBackend(
cwd: string,
request: IncomingMessage,
response: ServerResponse,
) {
const requestUrl = new URL(
request.url ?? "/",
`http://${request.headers.host ?? "localhost"}`,
);
const env: NodeJS.ProcessEnv = {
...process.env,
CONTENT_LENGTH: request.headers["content-length"] ?? "0",
GATEWAY_INTERFACE: "CGI/1.1",
GIT_HTTP_EXPORT_ALL: "1",
GIT_PROJECT_ROOT: cwd,
PATH_INFO: decodeURIComponent(requestUrl.pathname),
QUERY_STRING: requestUrl.search.slice(1),
REMOTE_ADDR: request.socket.remoteAddress ?? "",
REQUEST_METHOD: request.method ?? "GET",
SERVER_PROTOCOL: `HTTP/${request.httpVersion}`,
};
if (request.headers["content-type"] !== undefined) {
env.CONTENT_TYPE = request.headers["content-type"];
}

const backend = spawn("git", ["http-backend"], {
env,
stdio: ["pipe", "pipe", "pipe"],
});
request.pipe(backend.stdin);

const stdout: Buffer[] = [];
const stderr: Buffer[] = [];
backend.stdout.on("data", (chunk: Buffer) => stdout.push(chunk));
backend.stderr.on("data", (chunk: Buffer) => stderr.push(chunk));

const exitCode = await new Promise<number | null>((resolve, reject) => {
backend.on("error", reject);
backend.on("close", resolve);
});
if (exitCode !== 0) {
throw new Error(
`git http-backend exited with ${exitCode}: ${Buffer.concat(stderr).toString("utf8")}`,
);
}

const output = Buffer.concat(stdout);
let separator = Buffer.from("\r\n\r\n");
let headerEnd = output.indexOf(separator);
if (headerEnd === -1) {
separator = Buffer.from("\n\n");
headerEnd = output.indexOf(separator);
}
if (headerEnd === -1) {
throw new Error("git http-backend returned an invalid CGI response");
}

let status = 200;
const headers = output.subarray(0, headerEnd).toString("utf8").split(/\r?\n/);
for (const header of headers) {
const separatorIndex = header.indexOf(":");
if (separatorIndex === -1) continue;

const name = header.slice(0, separatorIndex);
const value = header.slice(separatorIndex + 1).trim();
if (name.toLowerCase() === "status") {
status = Number.parseInt(value, 10);
} else {
response.setHeader(name, value);
}
}

response.writeHead(status);
response.end(output.subarray(headerEnd + separator.length));
}

async function listen(server: http.Server) {
const waiter = Promise.withResolvers();

server.on("listening", waiter.resolve);
server.on("error", waiter.reject);

server.listen(0);

try {
await waiter.promise;
return server;
} finally {
server.off("listening", waiter.resolve);
server.off("error", waiter.reject);
}
}

async function createGitHttpServer(cwd: string) {
const requests: RecordedRequest[] = [];
const server = http.createServer((request, response) => {
const recordedRequest = recordRequest(request);
requests.push(recordedRequest);

void runGitHttpBackend(cwd, request, response).catch((error: unknown) => {
response.destroy(
Error.isError(error)
? error
: new Error("Server error", { cause: error }),
);
});
});

await listen(server);
const address = server.address();
assert(
!!address && typeof address !== "string",
"Failed to get server address",
);

return {
origin: `http://127.0.0.1:${address.port}`,
requests,
async [Symbol.asyncDispose]() {
await new Promise<void>((resolve, reject) => {
server.close((error) => {
if (error) {
reject(error);
return;
}
resolve();
});
});
},
};
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should probably be in its own file and split up a bit.
i would also prefer if we use srvx so we can use Request/Response instead of node's http server

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO node's createServer isn't bad and we shouldn't try to avoid it. Its API is not the most modern, but we don't need to risk running a server framework that might have its own issues, plus Request-Response-based servers also have their own limitations which don't exist in createServer.

@beeequeue beeequeue Jul 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i just prefer Requst/Response over node's api 😜
srvx is the one used by h3/nuxt (soon?), so i think it'd be fine for our use case in tests.

like i wrote before, i'm getting overwhelmed by the test setups we're creating so anything to simplify them would be a win in my eyes

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hm, I get this is a little bit hard to follow - but at the same time it's not something we need to care about deeply as this is test-only glue code. What matters is in the test bodies (which admittedly are kinda long right now). I added some code comments here to make it easier to follow.

I think it would be good to split up this file in the future - but given the test files are now in the same directory as the source files and this is a "single-package" repo, I don't want to restructure this too much right now.

Comment thread src/github.test.ts

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i think these test files are getting a bit overwhelming... my brain struggles massively to read them

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I moved some shared stuff into in-file utils. I hope this will help with the readability of those tests

@Andarist Andarist changed the title Ensure github-token wins for git-cli pushes Use GitHub API for pushes by default and require explicit github-token when using custom tokens Aug 3, 2026
@Andarist
Andarist added this pull request to the merge queue Aug 3, 2026
Merged via the queue into main with commit cb3f011 Aug 3, 2026
7 checks passed
@Andarist
Andarist deleted the fix/github-token-push branch August 3, 2026 11:03
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