Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ def _gitlab_commit_statuses(ctx: FeatureContext):
target_url = target_url or None,
description = description or None,
api_base = api_base,
auth_kind = "bearer",
auth_kind = _state["_auth_kind"],
)
if not result["success"]:
_LOG.warn(ctx.std, "commit_status POST failed (state=%s): %s" % (
Expand All @@ -249,7 +249,7 @@ def _gitlab_commit_statuses(ctx: FeatureContext):
if _state.get("_gitlab_token"):
return True
auth_reason = {}
token = gitlab_token_cache.get_or_mint(
token, auth_kind = gitlab_token_cache.resolve_token(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Don't pass GitLab PATs into GitHub job lookup

When ASPECT_GITLAB_TOKEN is used from a GitHub Actions runner (the explicit non-GitLab-CI path this change enables), this call can return the user's GitLab PAT. A few lines later the feature calls resolve_build_url(ctx, existing_app_token = token), and on GitHub Actions that resolver treats existing_app_token as a GitHub App token and sends it to the GitHub jobs API as Authorization: Bearer ... before falling back. That leaks the GitLab token to GitHub in this supported cross-CI configuration; avoid passing explicit GitLab tokens into the GitHub URL resolver, or skip that resolver when auth_kind == "private_token".

Useful? React with 👍 / 👎.

ctx,
project_path = project_path,
project_id = mr.get("project_id"),
Expand All @@ -263,6 +263,7 @@ def _gitlab_commit_statuses(ctx: FeatureContext):
))
return False
_state["_gitlab_token"] = token
_state["_auth_kind"] = auth_kind

# Re-resolve CI URL with the token in scope (mirrors GH path —
# safe no-op when the CI URL is env-derived rather than
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ def _post_comments(ctx, gl, comments, existing_markers, task_key):
body = c["body"],
position = position,
api_base = gl["api_base"],
auth_kind = "bearer",
auth_kind = gl["auth_kind"],
)
if res["success"]:
if marker:
Expand All @@ -251,7 +251,7 @@ def _post_comments(ctx, gl, comments, existing_markers, task_key):
def _get_existing_markers(ctx, gl, task_key):
"""Return `{marker: discussion_id}` for THIS task's existing lint
discussions on the MR (dedup against prior runs)."""
listed = gitlab.discussions.list(ctx, token = gl["token"], project = gl["project_path"], mr_iid = gl["mr_iid"], api_base = gl["api_base"], auth_kind = "bearer")
listed = gitlab.discussions.list(ctx, token = gl["token"], project = gl["project_path"], mr_iid = gl["mr_iid"], api_base = gl["api_base"], auth_kind = gl["auth_kind"])
if not listed["success"]:
return {}
markers = {}
Expand Down Expand Up @@ -460,7 +460,7 @@ def _cleanup_and_resolve(ctx, gl, relevant_diagnostics, run_id, task_key, sugges
for m in suggestion_markers:
desired[m] = True

listed = gitlab.discussions.list(ctx, token = gl["token"], project = gl["project_path"], mr_iid = gl["mr_iid"], api_base = gl["api_base"], auth_kind = "bearer")
listed = gitlab.discussions.list(ctx, token = gl["token"], project = gl["project_path"], mr_iid = gl["mr_iid"], api_base = gl["api_base"], auth_kind = gl["auth_kind"])
if not listed["success"]:
return

Expand All @@ -474,9 +474,9 @@ def _cleanup_and_resolve(ctx, gl, relevant_diagnostics, run_id, task_key, sugges
genuinely_clean = not relevant_diagnostics and not suggestion_markers
plan = compute_cleanup(listed["discussions"], desired, run_id, task_key, genuinely_clean = genuinely_clean)
for e in plan["delete"]:
gitlab.discussions.delete(ctx, token = gl["token"], project = gl["project_path"], mr_iid = gl["mr_iid"], discussion_id = e["discussion_id"], note_id = e["note_id"], api_base = gl["api_base"], auth_kind = "bearer")
gitlab.discussions.delete(ctx, token = gl["token"], project = gl["project_path"], mr_iid = gl["mr_iid"], discussion_id = e["discussion_id"], note_id = e["note_id"], api_base = gl["api_base"], auth_kind = gl["auth_kind"])
for did in plan["resolve"]:
gitlab.discussions.resolve(ctx, token = gl["token"], project = gl["project_path"], mr_iid = gl["mr_iid"], discussion_id = did, resolved = True, api_base = gl["api_base"], auth_kind = "bearer")
gitlab.discussions.resolve(ctx, token = gl["token"], project = gl["project_path"], mr_iid = gl["mr_iid"], discussion_id = did, resolved = True, api_base = gl["api_base"], auth_kind = gl["auth_kind"])

def _gitlab_lint_impl(ctx: FeatureContext):
lint_trait = ctx.traits[LintTrait]
Expand All @@ -491,12 +491,13 @@ def _gitlab_lint_impl(ctx: FeatureContext):
max_pr_comments = ctx.args.max_pr_comments

auth_reason = {}
token = gitlab_token_cache.get_or_mint(
token, auth_kind = gitlab_token_cache.resolve_token(
ctx,
project_path = mr["project_path"],
project_id = mr.get("project_id"),
# See gitlab_status_comments.axl: request `api` directly until the
# roles.ts mr_notes.write scope mapping is fixed (ENG-1652 follow-up).
# (Ignored on the explicit-token path, which doesn't mint.)
roles = ["api"],
_reason = auth_reason,
)
Expand All @@ -508,7 +509,7 @@ def _gitlab_lint_impl(ctx: FeatureContext):

# One call gets both the `diff_refs` SHA triple (for positions) and the
# per-file diffs (for the new_line→old_line classification below).
mr_res = gitlab.merge_requests.list_changes(ctx, token = token, project = mr["project_path"], mr_iid = mr["mr_iid"], api_base = api_base, auth_kind = "bearer")
mr_res = gitlab.merge_requests.list_changes(ctx, token = token, project = mr["project_path"], mr_iid = mr["mr_iid"], api_base = api_base, auth_kind = auth_kind)
if not mr_res["success"]:
_LOG.warn(ctx.std, "Could not fetch MR %d changes: %s" % (mr["mr_iid"], mr_res.get("error", "unknown")))
return
Expand All @@ -534,6 +535,7 @@ def _gitlab_lint_impl(ctx: FeatureContext):
"project_path": mr["project_path"],
"mr_iid": mr["mr_iid"],
"api_base": api_base,
"auth_kind": auth_kind,
"diff_refs": diff_refs,
"changed_lines": {},
"prefix": "",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ def _gitlab_status_comments(ctx: FeatureContext):
if _state.get("_gitlab_token"):
return True
auth_reason = {}
token = gitlab_token_cache.get_or_mint(
token, auth_kind = gitlab_token_cache.resolve_token(
ctx,
project_path = project_path,
project_id = project_id,
Expand All @@ -174,7 +174,8 @@ def _gitlab_status_comments(ctx: FeatureContext):
# `gitlabScope: null` and `read_api`, which together resolve
# to scope `read_api` only — and GitLab's notes API requires
# `api` scope. Until roles.ts marks `mr_notes.write` as
# `api`-scoped, request `api` here directly.
# `api`-scoped, request `api` here directly. (Ignored on the
# explicit-token path, which doesn't mint.)
roles = ["api"],
_reason = auth_reason,
)
Expand All @@ -185,6 +186,7 @@ def _gitlab_status_comments(ctx: FeatureContext):
))
return False
_state["_gitlab_token"] = token
_state["_auth_kind"] = auth_kind
return True

def _fetch_existing_note_body(ctx, token):
Expand All @@ -201,7 +203,7 @@ def _gitlab_status_comments(ctx: FeatureContext):
# The notes API doesn't expose a single-note GET that filters by id
# uniquely without `list`; we approximate by listing and matching id.
# Cheap on a small page; expensive only when the MR has many notes.
listed = gitlab.notes.list(ctx, token = token, project = project_path, mr_iid = mr_iid, api_base = api_base, auth_kind = "bearer")
listed = gitlab.notes.list(ctx, token = token, project = project_path, mr_iid = mr_iid, api_base = api_base, auth_kind = _state["_auth_kind"])
if not listed["success"]:
_LOG.warn(ctx.std, "List notes failed: %s" % listed.get("error", "unknown"))
return None
Expand All @@ -212,7 +214,7 @@ def _gitlab_status_comments(ctx: FeatureContext):
# Note vanished — fall through to re-discovery.
_state.pop("_note_id", None)

listed = gitlab.notes.list(ctx, token = token, project = project_path, mr_iid = mr_iid, api_base = api_base, auth_kind = "bearer")
listed = gitlab.notes.list(ctx, token = token, project = project_path, mr_iid = mr_iid, api_base = api_base, auth_kind = _state["_auth_kind"])
if not listed["success"]:
_LOG.warn(ctx.std, "List notes failed: %s" % listed.get("error", "unknown"))
return None
Expand All @@ -236,7 +238,7 @@ def _gitlab_status_comments(ctx: FeatureContext):
note_id = _state["_note_id"],
body = body,
api_base = api_base,
auth_kind = "bearer",
auth_kind = _state["_auth_kind"],
)
if not res["success"]:
_LOG.warn(ctx.std, "Update note failed: %s" % res.get("error", "unknown"))
Expand All @@ -250,7 +252,7 @@ def _gitlab_status_comments(ctx: FeatureContext):
mr_iid = mr_iid,
body = body,
api_base = api_base,
auth_kind = "bearer",
auth_kind = _state["_auth_kind"],
)
if not res["success"]:
_LOG.warn(ctx.std, "Create note failed: %s" % res.get("error", "unknown"))
Expand All @@ -260,7 +262,7 @@ def _gitlab_status_comments(ctx: FeatureContext):
# Race mitigation: if two tasks both POSTed within the same
# window, keep the lowest note id and delete the rest. Stable
# choice across writers so they converge on the same survivor.
listed2 = gitlab.notes.list(ctx, token = token, project = project_path, mr_iid = mr_iid, api_base = api_base, auth_kind = "bearer")
listed2 = gitlab.notes.list(ctx, token = token, project = project_path, mr_iid = mr_iid, api_base = api_base, auth_kind = _state["_auth_kind"])
if not listed2["success"]:
return True
dupes = [n for n in listed2["notes"] if marker in (n.get("body") or "")]
Expand All @@ -277,7 +279,7 @@ def _gitlab_status_comments(ctx: FeatureContext):
mr_iid = mr_iid,
note_id = d["id"],
api_base = api_base,
auth_kind = "bearer",
auth_kind = _state["_auth_kind"],
)
if not del_res["success"]:
_LOG.trace("Cleanup of duplicate note %s failed; will retry next cycle" % d["id"])
Expand Down
35 changes: 35 additions & 0 deletions crates/aspect-cli/src/builtins/aspect/lib/gitlab_test.axl
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Run with:
"""

load("./gitlab.axl", "gitlab")
load("./gitlab_token_cache.axl", "gitlab_token_cache")

def _eq(label, got, want):
if got != want:
Expand Down Expand Up @@ -153,6 +154,40 @@ def _test_impl(ctx):
"https://self-hosted.gitlab.com/api/v4",
)

# resolve_token — the explicit-token escape hatch (self-hosted path).
# ASPECT_GITLAB_TOKEN short-circuits before any mint and selects the
# PRIVATE-TOKEN header convention; absent it, the mint path is used
# and auth_kind is "bearer" (and None on mint failure).
def _ctx_with_env(env_vars):
return struct(
std = struct(
env = struct(
var = lambda name: env_vars.get(name, ""),
temp_dir = lambda: "/tmp",
),
fs = struct(exists = lambda path: False),
process = struct(id = lambda: 1234),
),
aspect = struct(auth = struct(credentials = lambda profile = None: None)),
)

tok, kind = gitlab_token_cache.resolve_token(
_ctx_with_env({"ASPECT_GITLAB_TOKEN": " glpat-xxx "}),
project_path = "group/project",
)
_eq("resolve_token / explicit token value (trimmed)", tok, "glpat-xxx")
_eq("resolve_token / explicit token uses private_token", kind, "private_token")

# No explicit token + no creds → mint path fails → (None, "bearer").
reason = {}
tok, kind = gitlab_token_cache.resolve_token(
_ctx_with_env({"CI": "true"}),
project_path = "group/project",
_reason = reason,
)
_eq("resolve_token / fallback no-creds returns None", tok, None)
_eq("resolve_token / fallback auth_kind is bearer", kind, "bearer")

# commit_status.STATES exposed
if "success" not in gitlab.commit_status.STATES:
fail("commit_status.STATES missing 'success'")
Expand Down
39 changes: 39 additions & 0 deletions crates/aspect-cli/src/builtins/aspect/lib/gitlab_token_cache.axl
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,45 @@ def get_or_mint(ctx, project_path, project_id = None, profile = None, roles = No
ctx.std.fs.write(path, tok)
return tok

# Explicit-token escape hatch. Set to a GitLab personal / group / project
# access token with `api` scope to bypass the Aspect GitLab App entirely.
_EXPLICIT_TOKEN_ENV = "ASPECT_GITLAB_TOKEN"

def resolve_token(ctx, project_path, project_id = None, profile = None, roles = None, _reason = None):
"""Resolve a GitLab token plus the auth header convention to use,
returning `(token, auth_kind)`.

Precedence:

* `ASPECT_GITLAB_TOKEN` (a personal / group / project access token)
takes precedence when set and is sent as `PRIVATE-TOKEN`
(`auth_kind = "private_token"`). This is the path for self-hosted
GitLab: the Aspect GitLab App is registered on `gitlab.com`, so a
minted token can't authenticate against another instance — the
caller must supply a token valid for their own host. No minting,
no per-process caching (a PAT has no refresh-rotation hazard).
* Otherwise mint via the Aspect GitLab App through `get_or_mint`
and send as a bearer token (`auth_kind = "bearer"`), with the
per-process caching described above.

Returns `(None, "bearer")` on mint failure, with `_reason` populated
exactly as `gitlab.authenticate` documents. The explicit-token path
never fails here — an invalid token surfaces as a 401 at the first
API call, handled by the feature's existing warn-and-skip path."""
explicit = ctx.std.env.var(_EXPLICIT_TOKEN_ENV) or ""
if explicit:
return explicit.strip(), "private_token"
tok = get_or_mint(
ctx,
project_path = project_path,
project_id = project_id,
profile = profile,
roles = roles,
_reason = _reason,
)
return tok, "bearer"

gitlab_token_cache = struct(
get_or_mint = get_or_mint,
resolve_token = resolve_token,
)
Loading