From d86c0d65519b1435e159ef5cbde2036ffb91e093 Mon Sep 17 00:00:00 2001 From: Suleiman Latrsh Date: Wed, 12 Aug 2026 14:56:05 -0400 Subject: [PATCH] Create new Release. Assisted-By: devx/75813201-8afc-470d-b349-7c262774f128 --- .github/workflows/gardener-notify-event.yml | 35 ++++++ .github/workflows/gardener-notify-slack.yml | 116 ++++++++++++++++++++ CHANGELOG.md | 6 + src/Internal/Exchange/RefreshToken.php | 20 ++++ src/Internal/Exchange/TokenExchange.php | 50 +++++---- src/Internal/Helpers/AppHomeRedirect.php | 6 + src/Internal/Verify/AppProxy.php | 47 ++++++-- src/Version.php | 2 +- 8 files changed, 251 insertions(+), 31 deletions(-) create mode 100644 .github/workflows/gardener-notify-event.yml create mode 100644 .github/workflows/gardener-notify-slack.yml diff --git a/.github/workflows/gardener-notify-event.yml b/.github/workflows/gardener-notify-event.yml new file mode 100644 index 0000000..f03b719 --- /dev/null +++ b/.github/workflows/gardener-notify-event.yml @@ -0,0 +1,35 @@ +name: Gardener - Notify Event +# Tiny event capturer: stashes the triggering issue/PR payload as an artifact +# for `gardener-notify-slack.yml` to pick up via workflow_run. +# +# Why two workflows? When Dependabot triggers a workflow, GitHub forces +# GITHUB_TOKEN to read-only and hides Actions secrets — so labeling and +# Slack posting from this workflow would fail on every Dependabot PR. A +# workflow_run-triggered follow-up runs in the default-branch context with +# full permissions and secret access, regardless of the upstream actor. +# +# Uses pull_request_target so fork-opened PRs still produce an artifact. +# No code is checked out here; this workflow only reads the pre-parsed +# event payload, so there is no pwn-request surface. +on: + issues: + types: [opened, labeled] + pull_request_target: + types: [opened, labeled] + +permissions: + contents: read + +jobs: + capture: + if: github.event.action == 'opened' || github.event.label.name == 'devtools-gardener' + runs-on: ubuntu-latest + steps: + - name: Stash event payload + run: cp "$GITHUB_EVENT_PATH" event.json + + - uses: actions/upload-artifact@v4 + with: + name: gardener-event + path: event.json + retention-days: 1 diff --git a/.github/workflows/gardener-notify-slack.yml b/.github/workflows/gardener-notify-slack.yml new file mode 100644 index 0000000..8bcb5a8 --- /dev/null +++ b/.github/workflows/gardener-notify-slack.yml @@ -0,0 +1,116 @@ +name: Gardener - Notify Slack +# Runs after `Gardener - Notify Event` completes and does the real work: +# applies the devtools-gardener label and posts a summary to Slack. +# +# The workflow_run trigger runs this job in the default-branch context with +# full GITHUB_TOKEN permissions and Actions secret access — this is what +# lets it succeed for Dependabot-opened PRs, where the upstream event +# workflow can't label or reach secrets directly. +on: + workflow_run: + workflows: ['Gardener - Notify Event'] + types: [completed] + +permissions: + contents: read + issues: write + pull-requests: write + actions: read + +jobs: + notify: + # `conclusion == success` also covers runs where the capture job was + # skipped by its `if` gate (no matching label, etc.) — in that case + # no artifact was uploaded, so the download step below no-ops. + if: github.event.workflow_run.conclusion == 'success' + runs-on: ubuntu-latest + steps: + - name: Download event payload + id: download + continue-on-error: true + uses: actions/download-artifact@v4 + with: + name: gardener-event + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Add devtools-gardener label + if: steps.download.outcome == 'success' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_REPO: ${{ github.repository }} + run: | + ACTION=$(jq -r '.action' event.json) + # On `labeled` events the label is already there — skip. + if [ "$ACTION" != "opened" ]; then + exit 0 + fi + NUMBER=$(jq -r '(.issue // .pull_request).number' event.json) + if jq -e 'has("pull_request")' event.json > /dev/null; then + gh pr edit "$NUMBER" --add-label devtools-gardener + else + gh issue edit "$NUMBER" --add-label devtools-gardener + fi + + - name: Post to Slack + if: steps.download.outcome == 'success' + continue-on-error: true + env: + SLACK_BOT_TOKEN: ${{ secrets.SLACK_GARDENER_BOT_TOKEN }} + SLACK_CHANNEL_ID: ${{ vars.GARDENER_SLACK_CHANNEL_ID }} + run: | + KIND=$(jq -r 'if has("pull_request") then "PR" else "Issue" end' event.json) + # Pull the body out, truncate, then convert GitHub Markdown to + # Slack mrkdwn. Links and fenced code blocks are stashed before + # the HTML-escape pass so their contents survive verbatim (a `&` + # inside a URL must stay raw, and code content shouldn't be + # mangled). Blockquote `> ` markers are also stashed so the + # `>` → `>` escape doesn't break them. Everything else is + # HTML-escaped so user-supplied `<`, `>`, `&` can't collide + # with Slack link syntax or injected mentions like . + BODY=$(jq -r '(.issue // .pull_request).body // ""' event.json) + if [ ${#BODY} -gt 1000 ]; then + BODY="${BODY:0:1000}…" + fi + BODY=$(printf '%s' "$BODY" | perl -0777 -pe ' + my @u; + s{\[([^\]]+)\]\(([^)]+)\)}{push @u, $2; "\x01$#u\x02$1\x03"}ge; + my @c; + s{^```[^\n]*\n(.*?)\n```$}{push @c, $1; "\x04$#c\x05"}gems; + s/^> /\x06/gm; + s/^#{1,6}\s+(.+)$/*$1*/gm; + s/\*\*(.+?)\*\*/*$1*/g; + s/^(\s*)- \[x\]\s+/$1✓ /gm; + s/^(\s*)[-*]\s+/$1• /gm; + s/&/&/g; + s//>/g; + s/\x06/> /g; + s{\x01(\d+)\x02(.*?)\x03}{"<$u[$1]|$2>"}ge; + s{\x04(\d+)\x05}{"```\n$c[$1]\n```"}ge; + ') + jq \ + --arg channel "$SLACK_CHANNEL_ID" \ + --arg kind "$KIND" \ + --arg body "$BODY" \ + ' + def escape: gsub("&";"&") | gsub("<";"<") | gsub(">";">"); + + (.issue // .pull_request) as $i + | ([$i.labels[]?.name | select(. != "devtools-gardener")] + | map("`\(.)`") | join(" ")) as $labels + | (if $kind == "PR" + then " · \($i.changed_files) files, +\($i.additions)/-\($i.deletions)" + + (if $i.draft then " · draft" else "" end) + else "" end) as $meta + | [ "*<\($i.html_url)|\($kind) #\($i.number)>* — \(($i.title | escape))", + "_opened by \($i.user.login)\($meta)_" ] + + (if $body != "" then [$body] else [] end) + + (if $labels != "" then [$labels] else [] end) + | join("\n") as $msg + | { channel: $channel, text: "\($kind) #\($i.number): \($i.title)", + blocks: [{ type: "section", text: { type: "mrkdwn", text: $msg } }] } + ' event.json | curl -sf -X POST \ + -H "Authorization: Bearer $SLACK_BOT_TOKEN" \ + -H 'Content-type: application/json; charset=utf-8' \ + -d @- https://slack.com/api/chat.postMessage diff --git a/CHANGELOG.md b/CHANGELOG.md index a5b11f5..b02d2d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.1.5] + +- Verify the dest property is not a malicious URL before making a token exchange request +- Reject App Proxy requests with multiple `shop` query parameters with a 401 response. +- Refreshing a non-expiring token now returns a no-refresh-needed result instead of an error + ## [0.1.4] - Add optional `expiring` parameter to `exchangeUsingTokenExchange`. Defaults to `true`. Pass `false` to request a non-expiring token. If `false`, `refreshToken` and `refreshTokenExpires` will be null in result. diff --git a/src/Internal/Exchange/RefreshToken.php b/src/Internal/Exchange/RefreshToken.php index d18cc87..60df7b1 100644 --- a/src/Internal/Exchange/RefreshToken.php +++ b/src/Internal/Exchange/RefreshToken.php @@ -53,6 +53,26 @@ public static function refresh(TokenExchangeAccessToken|array $accessToken, arra } } + // A non-expiring token has no expiry and no refresh token; it never needs refreshing. + // (An expiring token that still has a refresh token must fall through and refresh.) + if (empty($expires) && empty($refreshToken)) { + return new TokenExchangeResult( + ok: true, + shop: $shop, + accessToken: null, + log: new Log( + code: 'non_expiring_no_refresh_needed', + detail: 'Access token does not expire, so no refresh is needed. Proceed with business logic.' + ), + httpLogs: [], + response: new ResponseInfo( + status: 200, + body: '', + headers: (object)[] + ) + ); + } + // Check if access token is still valid (with 60-second buffer) if (!empty($expires)) { $expiryTime = strtotime($expires); diff --git a/src/Internal/Exchange/TokenExchange.php b/src/Internal/Exchange/TokenExchange.php index 4656f76..bb1e577 100644 --- a/src/Internal/Exchange/TokenExchange.php +++ b/src/Internal/Exchange/TokenExchange.php @@ -209,30 +209,38 @@ public static function exchange( ); } - // Validate shop URL format (basic validation) - if (strpos($shop, '.myshopify.com') === false) { - return new TokenExchangeResult( - ok: false, - shop: null, - accessToken: null, - log: new Log( - code: 'configuration_error', - detail: "Expected idToken.claims.dest to be a valid shop URL (e.g., 'https://shop.myshopify.com' or 'shop.myshopify.com')" - ), - httpLogs: [], - response: new ResponseInfo( - status: 500, - body: '', - headers: (object)[] - ) - ); + // Validate shop URL format - dest must end with .myshopify.com + $shopWithoutProtocol = preg_replace('#^https?://#', '', $shop); + $invalidShopError = new TokenExchangeResult( + ok: false, + shop: null, + accessToken: null, + log: new Log( + code: 'configuration_error', + detail: "Expected idToken.claims.dest to be a valid shop URL (e.g., 'https://shop.myshopify.com' or 'shop.myshopify.com')" + ), + httpLogs: [], + response: new ResponseInfo( + status: 500, + body: '', + headers: (object)[] + ) + ); + + if (!str_ends_with($shopWithoutProtocol, '.myshopify.com')) { + return $invalidShopError; } - // Normalize shop URL for API request - $shopUrl = (strpos($shop, 'https://') === 0) ? $shop : 'https://' . $shop; + // Extract shop name by removing .myshopify.com suffix + $shopName = substr($shopWithoutProtocol, 0, -strlen('.myshopify.com')); - // Extract shop name (remove https:// and .myshopify.com) - $shopName = str_replace(['https://', 'http://', '.myshopify.com'], '', $shop); + // Validate the extracted shop name against allowed pattern + if (!preg_match('/^[a-zA-Z0-9][a-zA-Z0-9\-]*$/', $shopName)) { + return $invalidShopError; + } + + // Normalize shop URL for API request + $shopUrl = 'https://' . $shopWithoutProtocol; // Step 2: Make the Token Exchange Request $requestedTokenType = "urn:shopify:params:oauth:token-type:{$accessMode}-access-token"; diff --git a/src/Internal/Helpers/AppHomeRedirect.php b/src/Internal/Helpers/AppHomeRedirect.php index b609e36..80e5c3e 100644 --- a/src/Internal/Helpers/AppHomeRedirect.php +++ b/src/Internal/Helpers/AppHomeRedirect.php @@ -189,6 +189,12 @@ private static function isValidRelativeUrl(string $redirectUrl): bool return false; } + // Must not be backslash-prefixed (/\evil.com) — browsers normalize \ to / + // per the WHATWG URL Standard, turning it into a protocol-relative URL + if (strlen($redirectUrl) > 1 && $redirectUrl[1] === '\\') { + return false; + } + return true; } diff --git a/src/Internal/Verify/AppProxy.php b/src/Internal/Verify/AppProxy.php index 7ec5d17..b95da16 100644 --- a/src/Internal/Verify/AppProxy.php +++ b/src/Internal/Verify/AppProxy.php @@ -49,25 +49,54 @@ public static function verify(array $req, array $config): ResultWithLoggedInCust if ($queryString !== '') { $pairs = explode('&', $queryString); foreach ($pairs as $pair) { + if ($pair === '') { + continue; + } + + // Bare params without "=" (e.g. a second `&shop`) are kept as + // empty-valued params so that duplicate keys are still detected, + // matching parse_qs(keep_blank_values=True) in the Python package. if (strpos($pair, '=') !== false) { [$key, $value] = explode('=', $pair, 2); $key = urldecode($key); $value = urldecode($value); + } else { + $key = urldecode($pair); + $value = ''; + } - // Check if this key already exists (for multiple values with same key) - if (isset($params[$key])) { - // Convert to array if not already - if (!is_array($params[$key])) { - $params[$key] = [$params[$key]]; - } - $params[$key][] = $value; - } else { - $params[$key] = $value; + // Check if this key already exists (for multiple values with same key) + if (isset($params[$key])) { + // Convert to array if not already + if (!is_array($params[$key])) { + $params[$key] = [$params[$key]]; } + $params[$key][] = $value; + } else { + $params[$key] = $value; } } } + // Reject requests with multiple shop URL params + if (isset($params['shop']) && is_array($params['shop'])) { + return new ResultWithLoggedInCustomerId( + ok: false, + shop: null, + loggedInCustomerId: null, + log: new LogWithReq( + code: 'multiple_shop_parameters', + detail: 'Request has multiple `shop` query parameters. Respond 401 Unauthorized using the provided response.', + req: Request::redactForLog($req) + ), + response: new ResponseInfo( + status: 401, + body: 'Unauthorized', + headers: (object)[] + ) + ); + } + // Check for missing timestamp if (!isset($params['timestamp'])) { return new ResultWithLoggedInCustomerId( diff --git a/src/Version.php b/src/Version.php index 7b74712..9746818 100644 --- a/src/Version.php +++ b/src/Version.php @@ -4,4 +4,4 @@ namespace Shopify\App; -const VERSION = '0.1.4'; +const VERSION = '0.1.5';