fix: support shrunk client payloads on v1 - #573
Conversation
client_payload arrives as plain JSON, base64(gzip), or a reference to a server-stashed payload. v1 resolved fields with fromJSON(fromJSON(github.event.inputs.client_payload)) in five places, which throws on the last two shapes before the step can run. Resolve those fields in a step instead, following the existing script.js idiom on this branch - a root-level module exported as a function of core, required with github.action_path. Only the five fields v1's action.yml actually consumes are emitted; v1 has no cm org checkout. CLIENT_PAYLOAD is still handed to the docker rules engine untouched. The engine resolves the envelope itself (gitstream/rules-engine:latest ships gitstream-core 2.1.301, which has that support), and passing the inflated payload through the runner env would risk E2BIG on large payloads. The deprecation notice, docker cache/pull/load steps and the engine invocation are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same resolver change as develop and v2-lite: parse first and switch on the value of `type`, so the double-encoded compressed-payload envelope the GitHub trigger will send is inflated instead of being mistaken for a raw payload and silently resolving to empty fields. Both compressed forms stay supported permanently - Bitbucket has no run-name to protect and keeps sending the bare base64(gzip) form. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same diagnostic as develop and v2-lite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#568's port resolved the YAML field expressions but passed CLIENT_PAYLOAD to the docker engine untouched. The trigger double-encodes every tier so the workflow's run-name can parse it, and gitstream-core does a single JSON.parse on a reference - so a double-encoded envelope resolves to a string, the reference is never followed, and the engine treats the envelope as the payload. Stripping the outer layer here lets both consumers read the same dispatch, without changing core or the trigger. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Secrets | View in Orca |
There was a problem hiding this comment.
✨ PR Review
The PR is well-structured and thoughtfully addresses the payload normalization problem for v1. Security design in stashUrl (pinning to resolver origin) is particularly solid. Two issues worth flagging: an orphaned JSDoc comment that misattributes documentation, and a missing guard on resolverToken before it's used as a secret and in an Authorization header.
2 issues detected:
🐞 Bug - `reference.resolverToken` is used as a credential without being validated for presence, risking an unmasked or malformed Authorization header on a malformed reference payload.
Details: fetchStashedPayload passes reference.resolverToken directly to core.setSecret and uses it in the Authorization header without first checking that it is a non-empty string. If the incoming reference object is missing the field (malformed trigger payload), core.setSecret(undefined) may silently no-op, the header becomes "Bearer undefined", and the secret is never masked in logs.
File: resolve-payload-fields.js (108-110)
🧾 Readability - Two consecutive JSDoc blocks appear above `normalizeForEngine`; the first belongs to `toStepOutputs` and is misplaced, making it appear to document the wrong function.
Details: Lines 158–162 contain a JSDoc comment that describes toStepOutputs ("Maps a resolved payload to the step outputs…"), but it is immediately followed by a second JSDoc block for normalizeForEngine. The toStepOutputs function itself appears later (line 189), leaving its intended documentation stranded and the first comment misleadingly attached to normalizeForEngine.
File: resolve-payload-fields.js (158-162)
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
There was a problem hiding this comment.
Pull request overview
Adds a payload-resolution layer to the v1 composite action so downstream steps can reliably consume key fields when client_payload is provided as plain JSON, gzip-compressed base64, or as a server-stashed reference.
Changes:
- Introduces
resolve-payload-fields.jsto parse/decode/validate the three supported payload shapes and emit step outputs. - Wires the resolver into
action.yml, replacing repeatedfromJSON(fromJSON(...))expressions withsteps.payload-fields.outputs.*. - Normalizes the payload shape passed to the rules engine while keeping YAML-facing fields small and consistent.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| resolve-payload-fields.js | New payload resolver with gzip inflation limits, stash URL validation, and step-output mapping |
| action.yml | Adds resolver step and refactors subsequent steps to use resolved outputs |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
✨ PR Review
The PR cleanly refactors payload resolution into a dedicated JS module and fixes the brittle double-fromJSON expressions in action.yml. The decompression-bomb guard, origin-pinning on the stash URL, and explicit secret-masking for the github token are all good additions. Two previously raised concerns (unguarded resolverToken and misplaced JSDoc) remain open. One new issue is noted below.
1 issues detected:
🐞 Bug - The cm repo checkout step omits the resolved `github_token` output, causing it to use `GITHUB_TOKEN` instead of the installation token when the payload provides one.
Details: The "Checkout base branch" step was updated to use steps.payload-fields.outputs.github_token || github.token, but the "Checkout cm repo" step directly below was not given the same token. If the cm repository is private and requires the installation token carried in the payload (the same reason the base-branch checkout was updated), the checkout will silently fall back to the default GITHUB_TOKEN, which likely lacks cross-repo access.
File: action.yml (113-119)
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
Inserting normalizeForEngine above it left the JSDoc describing the wrong function, and toStepOutputs with none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
✨ PR Review
The PR introduces a flexible payload resolution layer that handles plain, compressed, and server-stashed payloads. The SSRF mitigation in stashUrl is well-documented and structurally sound. The two previously identified issues (issue_1: unchecked resolverToken and issue_3: missing token on the cm-repo checkout) remain unresolved; issue_2 (stranded JSDoc) is now fixed by the new file layout.
2 issues detected:
🐞 Bug - `reference.resolverToken` is used without a truthy check, so a missing field results in an unmasked secret and a misleading Bearer header value.
Details: fetchStashedPayload passes reference.resolverToken directly to core.setSecret and into the Authorization header without first verifying it is a non-empty string. If the incoming reference is malformed and the field is absent, core.setSecret(undefined) silently no-ops, the token is never masked in logs, and the header becomes "Bearer undefined", which will also cause the fetch to fail with an unhelpful 401/403 rather than a clear validation error.
File: resolve-payload-fields.js (108-110)
🐞 Bug - The cm-repo checkout omits the `token` field while the base-branch checkout was explicitly patched to supply the payload's installation token for the same reason.
Details: The "Checkout cm repo" step does not specify a token, so it falls back to the default GITHUB_TOKEN. The preceding "Checkout base branch" step was explicitly updated to use steps.payload-fields.outputs.github_token || github.token precisely because the installation token in the payload may be required for cross-repo access. If the cm repository is private and requires that installation token, this checkout will silently fail or use insufficient credentials.
File: action.yml (113-119)
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
✨ PR Description
Purpose: Add payload resolution module to handle compressed and oversized client payloads on v1 action, replacing complex inline JSON parsing.
Main changes:
resolve-payload-fields.jsmodule handles plain JSON, base64(gzip), and server-stashed payload references with decompression bomb protectionfromJSON()expressions with resolved step outputs, improving maintainability and supporting shrunk payloadsGenerated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how