Skip to content

fix(runner-server): attach static GitHub PAT by configured origin, not scheme - #351

Merged
Bnjoroge1 merged 5 commits into
Bnjoroge/lock-refactorfrom
gh-simulate/pat-http
Oct 6, 2026
Merged

Bnjoroge1 merged 5 commits into
Bnjoroge/lock-refactorfrom
gh-simulate/pat-http

Conversation

@Bnjoroge1

@Bnjoroge1 Bnjoroge1 commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

crates/preloop-runner-server/src/actions.rs attached the static GitHub PAT with bearer_auth only when the outgoing URL started with https:// (two sites: action-ref commit lookup and action-archive tarball download). Pointing the engine at a plain-http GitHub emulator (gh-simulate local mode, e.g. http://127.0.0.1:8888) silently dropped the credential, so the anonymous API budget (60/hr) was exhausted almost immediately.

Change

Replace the scheme check with a same-origin check against the configured GitHub endpoints: a new fn url_targets_configured_github(url, urls) -> bool parses both URLs and compares host + effective port, ignoring scheme. The PAT is attached when the request target matches any of github_urls (api_url/server_url/graphql_url), and never to an unrelated host.

  • Real github.com still attaches over https (unchanged behavior).
  • The gh-simulate local emulator now receives the PAT regardless of scheme.
  • A lookalike/different host never receives the credential.

Added a unit test covering: plain-http same-origin match, different port/host no-match, real github.com https match, and lookalike-host no-match.

Verification

  • cargo check -p preloop-runner-server — clean.
  • cargo test -p preloop-runner-server --lib pat_targets_configured_github — passes.

Refs #349 (does not close it — other PRs cover the remaining hardcoded-host work).


Summary by cubic

Fixes the static GitHub PAT being dropped when the engine targets a plain-http GitHub emulator, which previously exhausted the anonymous API budget.

  • Replaces the https:// scheme check with a host and effective port comparison against the configured github_urls endpoints.
  • Attaches the PAT when the request URL matches any configured GitHub endpoint regardless of scheme, and never to an unrelated or lookalike host.
  • Adds unit tests covering plain-http same-origin matches, different host/port no-matches, and real github.com over https.
  • Re-pins dtolnay/rust-toolchain in the control-plane workflow to a valid commit that zizmor accepts.

Refs #349 (does not close it).

Written for commit 95dd234. Summary will update on new commits.

Review in cubic

…t scheme

The static GitHub PAT was attached to action-resolution and tarball
requests only when the outgoing URL started with `https://`. Pointing the
engine at a plain-http GitHub emulator (gh-simulate local mode) therefore
silently dropped the credential, and the anonymous API budget (60/hr) was
exhausted almost immediately.

Replace the scheme check with `url_targets_configured_github`, which
attaches the PAT when the request URL's host (and effective port) matches
one of the configured `github_urls` endpoints, regardless of scheme. Real
github.com still attaches over https; an unrelated host never receives
the credential.

Refs #349
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9ed04d76-68b2-4136-9b3a-74b20b0512d5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 1 file

Confidence score: 3/5

  • In crates/preloop-runner-server/src/actions.rs, sending the PAT makes the unchanged resolve_ref_to_sha_omits_pat_over_http test fail because its mock asserts that Authorization is absent; update the test reproduction to match the new configuration.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/preloop-runner-server/src/actions.rs">

<violation number="1" location="crates/preloop-runner-server/src/actions.rs:225">
P2: blocker: This now sends the PAT to the HTTP mock used by the unchanged `resolve_ref_to_sha_omits_pat_over_http` test, whose handler asserts that `Authorization` is absent. Update that reproduction for the new configured-origin contract and assert the header is present; the helper-only test would still pass if `bearer_auth` were removed.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

let mut request = crate::shared_http::CLIENT.get(&url);
if let Some(pat) = state.static_github_pat()
&& url.starts_with("https://")
&& url_targets_configured_github(&url, &state.github_urls)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: blocker: This now sends the PAT to the HTTP mock used by the unchanged resolve_ref_to_sha_omits_pat_over_http test, whose handler asserts that Authorization is absent. Update that reproduction for the new configured-origin contract and assert the header is present; the helper-only test would still pass if bearer_auth were removed.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At crates/preloop-runner-server/src/actions.rs, line 225:

<comment>blocker: This now sends the PAT to the HTTP mock used by the unchanged `resolve_ref_to_sha_omits_pat_over_http` test, whose handler asserts that `Authorization` is absent. Update that reproduction for the new configured-origin contract and assert the header is present; the helper-only test would still pass if `bearer_auth` were removed.</comment>

<file context>
@@ -198,7 +222,7 @@ async fn resolve_ref_to_sha(
     let mut request = crate::shared_http::CLIENT.get(&url);
     if let Some(pat) = state.static_github_pat()
-        && url.starts_with("https://")
+        && url_targets_configured_github(&url, &state.github_urls)
     {
         request = request.bearer_auth(pat);
</file context>

@Bnjoroge1

Copy link
Copy Markdown
Collaborator Author

CI status note: the red on this PR is not from the diff. Base branch Bnjoroge/lock-refactor tip (a8a0b61) is already failing control 16/17/18 and Supply chain audit; several other checks (Runner light conformance, pullfrog, node-externals) failed inside infrastructure steps — actions/checkout errors and Upload … report artifact failures — while the test steps themselves passed. Local cargo build/cargo test on the touched crates is green.

Bnjoroge1 and others added 4 commits October 2, 2026 18:48
The previous check run used the broken macstudio golden/provisioning state.
No source files changed.
The pinned SHA (6bed0761) is not a commit in dtolnay/rust-toolchain;
zizmor flags it as impostor-commit. Re-pin to the real stable head
89b12181 (same pin ci.yml uses).
@Bnjoroge1
Bnjoroge1 force-pushed the Bnjoroge/lock-refactor branch from 0a37f46 to bfc9439 Compare October 3, 2026 02:04
@Bnjoroge1
Bnjoroge1 merged commit 492dc60 into Bnjoroge/lock-refactor Oct 6, 2026
9 of 28 checks passed
Bnjoroge1 added a commit that referenced this pull request Oct 6, 2026
#351 replaced the scheme check with a configured-origin check so the static
PAT reaches a plain-http GitHub emulator (gh-simulate local mode) instead of
being dropped. The old assertion here — no Authorization header over plain
http — contradicted that deliberate behavior and failed the merged tree.
Pin the real contract instead: a configured origin receives the PAT
regardless of scheme; the unconfigured-origin half stays covered by
pat_targets_configured_github_regardless_of_scheme in actions.rs.
Bnjoroge1 added a commit that referenced this pull request Oct 7, 2026
#351 attached the PAT to any request for a configured GitHub origin
regardless of scheme, so a configured remote http:// API URL received the
token in cleartext. Plain http now qualifies only for a loopback origin
(localhost, 127.0.0.0/8, ::1) — gh-simulate local mode keeps
authenticating — and every other origin requires https. Unconfigured
origins still never receive the PAT, and reqwest's redirect policy already
strips Authorization on any cross-origin redirect.
Bnjoroge1 added a commit that referenced this pull request Oct 7, 2026
…t scheme (#351)

* fix(runner-server): attach static PAT by configured GitHub origin, not scheme

The static GitHub PAT was attached to action-resolution and tarball
requests only when the outgoing URL started with `https://`. Pointing the
engine at a plain-http GitHub emulator (gh-simulate local mode) therefore
silently dropped the credential, and the anonymous API budget (60/hr) was
exhausted almost immediately.

Replace the scheme check with `url_targets_configured_github`, which
attaches the PAT when the request URL's host (and effective port) matches
one of the configured `github_urls` endpoints, regardless of scheme. Real
github.com still attaches over https; an unrelated host never receives
the credential.

Refs #349

* ci: rerun checks after shared engine recovery

The previous check run used the broken macstudio golden/provisioning state.
No source files changed.

* fix(ci): re-pin dtolnay/rust-toolchain in control-plane.yml

The pinned SHA (6bed0761) is not a commit in dtolnay/rust-toolchain;
zizmor flags it as impostor-commit. Re-pin to the real stable head
89b12181 (same pin ci.yml uses).

---------

Co-authored-by: Bnjoroge1 <Bnjoroge1@users.noreply.github.com>
Bnjoroge1 added a commit that referenced this pull request Oct 7, 2026
#351 replaced the scheme check with a configured-origin check so the static
PAT reaches a plain-http GitHub emulator (gh-simulate local mode) instead of
being dropped. The old assertion here — no Authorization header over plain
http — contradicted that deliberate behavior and failed the merged tree.
Pin the real contract instead: a configured origin receives the PAT
regardless of scheme; the unconfigured-origin half stays covered by
pat_targets_configured_github_regardless_of_scheme in actions.rs.
Bnjoroge1 added a commit that referenced this pull request Oct 7, 2026
#351 attached the PAT to any request for a configured GitHub origin
regardless of scheme, so a configured remote http:// API URL received the
token in cleartext. Plain http now qualifies only for a loopback origin
(localhost, 127.0.0.0/8, ::1) — gh-simulate local mode keeps
authenticating — and every other origin requires https. Unconfigured
origins still never receive the PAT, and reqwest's redirect policy already
strips Authorization on any cross-origin redirect.
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.

1 participant