From 7d9dd2720f792aa0c703a6337f835beca03ed386 Mon Sep 17 00:00:00 2001 From: Reuven Harrison Date: Fri, 22 May 2026 21:01:56 +0300 Subject: [PATCH] pr-comment: preserve https:// URLs in free /review base_file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The free review URL builder runs the base/revision inputs through sed 's/.*://' intending to strip the git-ref prefix in "origin/main:openapi.yaml" so the /review page receives just "openapi.yaml". For URL-shaped inputs like "https://raw.githubusercontent.com/o/r/main/foo.yaml", the same sed strips "https:" too and leaves a broken "//raw.githubusercontent.com/..." in the base_file= parameter. The /review page then tries to GET the broken URL, the fetch fails, and the access-denied screen renders — which misreports the cause as authorization ("@owner doesn't have access to o/r") even though the real problem is the malformed URL the page received. Surfaced via oasdiff-test/test#59, whose workflow uses a URL-shaped base. (Workflows using the git-ref form are unaffected — that path still works correctly.) Fix: wrap the strip in a case-branch helper that passes http:// and https:// inputs through unchanged, only running the sed on inputs that look like a git ref. Two regression tests cover both branches: - pr_comment_free_review_url_preserves_https_base - pr_comment_free_review_url_strips_git_ref_prefix Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/test.yaml | 111 ++++++++++++++++++++++++++++++++++++ pr-comment/entrypoint.sh | 21 ++++++- 2 files changed, 129 insertions(+), 3 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 82f3a98..b5f2ea4 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -750,3 +750,114 @@ jobs: exit 1 fi echo "PASS" + + pr_comment_free_review_url_preserves_https_base: + runs-on: ubuntu-latest + name: Test pr-comment free review URL keeps the https:// scheme on URL-style base + # Regression test for a bug surfaced by oasdiff-test/test#59: when the + # workflow passes a raw.githubusercontent.com URL as the base input + # (as the integration-test repo does), the entrypoint's + # base_path=$(echo "$base" | sed 's/.*://') + # stripped the "https:" prefix and left a broken "//raw.github..." + # URL. The free /review page then tried to GET that broken URL and + # rendered "access denied" with no useful diagnostic — because the + # generic access-denied screen masked the real cause (malformed URL). + # The fix passes URL-shaped inputs through unchanged. + steps: + - uses: actions/checkout@v6 + - name: Stub oasdiff to a no-changes spec + run: | + set -euo pipefail + mkdir -p /tmp/stub + cat > /tmp/stub/oasdiff <<'STUB' + #!/bin/sh + echo '[]' + STUB + chmod +x /tmp/stub/oasdiff + + mkdir -p /tmp/run + export GITHUB_REF=refs/pull/123/merge + export GITHUB_REPOSITORY=oasdiff-test/test + export GITHUB_SHA=deadbeef + export GITHUB_BASE_REF=main + export GITHUB_STEP_SUMMARY=/tmp/run/step-summary + cat > /tmp/run/event.json <&1) + echo "--- entrypoint output ---" + echo "$out" + + if ! echo "$out" | grep -q "::notice::.*View API changes"; then + echo "FAIL: notice line missing" >&2 + exit 1 + fi + notice=$(echo "$out" | grep "::notice::.*View API changes") + # The encoded base_file= param must contain the full URL, not the + # stripped "//raw..." form. https:// URL-encodes to https%3A%2F%2F. + if echo "$notice" | grep -q 'base_file=https%3A%2F%2Fraw.githubusercontent.com'; then + echo "PASS: full https://raw... URL preserved in base_file" + elif echo "$notice" | grep -q 'base_file=%2F%2Fraw.githubusercontent.com'; then + echo "FAIL: 'https:' was stripped from base — the strip_ref_prefix URL guard is missing" >&2 + echo "notice line: $notice" >&2 + exit 1 + else + echo "FAIL: base_file= param did not contain the raw.githubusercontent.com host as expected" >&2 + echo "notice line: $notice" >&2 + exit 1 + fi + + pr_comment_free_review_url_strips_git_ref_prefix: + runs-on: ubuntu-latest + name: "Test pr-comment free review URL strips origin/main: prefix from git-ref base" + # Companion to pr_comment_free_review_url_preserves_https_base: when + # the base input is git-ref form (origin/main:openapi.yaml), the + # prefix must still be stripped so the /review page receives just the + # path. The previous sed-based implementation handled this correctly; + # this test guards the case-branch refactor against breaking it. + steps: + - uses: actions/checkout@v6 + - name: Stub oasdiff to a no-changes spec + run: | + set -euo pipefail + mkdir -p /tmp/stub + cat > /tmp/stub/oasdiff <<'STUB' + #!/bin/sh + echo '[]' + STUB + chmod +x /tmp/stub/oasdiff + + mkdir -p /tmp/run + export GITHUB_REF=refs/pull/123/merge + export GITHUB_REPOSITORY=foo/bar + export GITHUB_SHA=deadbeef + export GITHUB_BASE_REF=main + export GITHUB_STEP_SUMMARY=/tmp/run/step-summary + cat > /tmp/run/event.json <&1) + echo "--- entrypoint output ---" + echo "$out" + + notice=$(echo "$out" | grep "::notice::.*View API changes") + # base_file should be just "multi-file/openapi.yaml" (URL-encoded slash), + # not the full "origin/main:multi-file/openapi.yaml". + if echo "$notice" | grep -q 'base_file=multi-file%2Fopenapi.yaml'; then + echo "PASS: origin/main: prefix stripped from base" + else + echo "FAIL: base_file= did not have the git-ref prefix stripped" >&2 + echo "notice line: $notice" >&2 + exit 1 + fi diff --git a/pr-comment/entrypoint.sh b/pr-comment/entrypoint.sh index 50f8893..8ab4340 100755 --- a/pr-comment/entrypoint.sh +++ b/pr-comment/entrypoint.sh @@ -77,9 +77,24 @@ repo="${GITHUB_REPOSITORY#*/}" # Emit free review annotation (public repos only — no auth required) urlencode() { printf '%s' "$1" | jq -sRr @uri; } -# Strip git ref prefix (e.g. "origin/main:path.yaml" -> "path.yaml", "HEAD:path.yaml" -> "path.yaml") -base_path=$(echo "$base" | sed 's/.*://') -rev_path=$(echo "$revision" | sed 's/.*://') +# Resolve the on-disk / on-repo path portion of `base` and `revision` for +# the free /review URL. Two input shapes are supported: +# +# 1. Git-ref form — "origin/main:openapi.yaml" or "HEAD:openapi.yaml". +# The sed strips everything up to the colon so we keep just the path. +# 2. URL form — "https://raw.githubusercontent.com/o/r/main/foo.yaml". +# The naive sed would also strip "https:" and leave a broken +# "//raw.githubusercontent.com/..." which the /review page then can't +# fetch (it renders as the access-denied screen with a misleading +# "owner doesn't have access" message). Pass URLs through unchanged. +strip_ref_prefix() { + case "$1" in + http://*|https://*) printf '%s' "$1" ;; + *) printf '%s' "$1" | sed 's/.*://' ;; + esac +} +base_path=$(strip_ref_prefix "$base") +rev_path=$(strip_ref_prefix "$revision") # Prefer the base SHA over the branch name so the link is commit-pinned. # Fall back through `git rev-parse origin/` before resorting to # the branch name itself, so push-event triggers (no pull_request payload)