hack/releaser: preserve query string on CloudFront redirects#25393
hack/releaser: preserve query string on CloudFront redirects#25393dvdksn wants to merge 1 commit into
Conversation
The CloudFront redirect Lambda dropped the request query string when issuing 301 responses, so UTM tags and other params were lost on redirected URLs. Append the query string to both the exact and prefix redirect Location headers, and add unit tests wired into the releaser CI job. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for docsdocker ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
area/release
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR modifies only infrastructure and build tooling files:
.github/workflows/build.ymldocker-bake.hclhack/releaser/Dockerfilehack/releaser/cloudfront-lambda-redirects.jshack/releaser/cloudfront-lambda-redirects.test.js
No user-facing documentation pages, Markdown files, front matter, or style guide concerns were found. No vendored/generated files were edited. No redirects are needed (no pages were removed or moved).
The change correctly preserves query strings (UTM parameters, etc.) on CloudFront 301 redirects and adds CI-wired unit tests. Nothing to flag from a documentation quality perspective.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
No documentation quality issues found. This PR is entirely infrastructure/tooling changes (JavaScript, Dockerfile, HCL, GitHub Actions YAML) with no documentation content modified. The query-string preservation logic and unit tests look correct.
| const withQuery = (location) => { | ||
| if (!request.querystring) { | ||
| return location; | ||
| } | ||
| const separator = location.includes('?') ? '&' : '?'; | ||
| return location + separator + request.querystring; | ||
| }; |
There was a problem hiding this comment.
thought: I'd personally default to allow-listing this (e.g. just support UTM parameters)
(I can't see any exploitative angles at the moment, would be just in fear of unknown-unknowns)
suggestion: Manipulate this with URL instead of doing raw string concatenation
If location were something like https://foo.com?x=123#some-header, this would yield things like https://foo.com?x=123#some-header&utm_source=....
I'd personally parse location into a URL and request.querystring into a URLSearchParams, then merge the latter into the former's search params, then read its .href to get the final URL.
There was a problem hiding this comment.
Pull request overview
Fixes the CloudFront redirect Lambda@Edge handler so 301 redirects preserve the incoming request query string (e.g., UTM parameters), preventing query params from being dropped during exact-match and prefix redirects.
Changes:
- Append
request.querystringto redirectLocationheaders for exact-match and prefix redirects. - Add
node --testunit tests that render the Go-templated Lambda source similarly tohack/releaser/aws.goand exercise the handler behavior. - Run the new unit tests in CI by adding a
releaser-testDockerfile stage and corresponding Bake target, invoked in the existingreleaserworkflow job.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
hack/releaser/cloudfront-lambda-redirects.js |
Preserves query strings when constructing redirect Location headers. |
hack/releaser/cloudfront-lambda-redirects.test.js |
Adds Node unit tests validating query-string preservation and directory rewrite pass-through behavior. |
hack/releaser/Dockerfile |
Introduces a releaser-test stage to run the Node unit tests. |
docker-bake.hcl |
Adds a releaser-test Bake target for CI execution. |
.github/workflows/build.yml |
Runs releaser-test alongside releaser-build in the releaser job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR correctly fixes the query-string preservation bug in the CloudFront redirect Lambda and adds unit tests for the new behavior. No documentation content files are modified. No vendored or generated files are touched. No redirects are missing.
The core logic is sound: withQuery() correctly appends the query string to both exact-match and prefix redirects, using & when the target already contains a ?. The test suite covers the main scenarios (exact redirect with/without query string, appending to an existing query string, prefix strip/no-strip, and directory rewrite pass-through).
Summary
The CloudFront redirect Lambda (
cloudfront-lambda-redirects.js) dropped the request query string when issuing 301 responses, so UTM tags and other params were lost on redirected URLs. This appends the query string to theLocationheader of both the exact-match and prefix redirects (the directory/markdown rewrite branch already forwards the request, so params survive there).Adds unit tests (
node --test, zero dependencies) that render the Go template the same wayaws.godoes and exercise the handler. The tests run in CI via a newreleaser-testDockerfile stage / bake target, baked alongsidereleaser-buildin the existingreleaserjob.Learnings
text/template({{.RedirectsJSON}}/{{.RedirectsPrefixesJSON}}), rendered and zipped byhack/releaser/aws.go. To unit-test it, substitute the placeholders the same wayaws.godoes, then evaluate the result as a CommonJS module.test-go-redirectsvalidates redirect data against the builtpublic/site, so it's the wrong host for a fast pure-JS test — thereleaserjob, which already builds fromhack/releaser, is the natural home and has no Hugo dependency.Generated by Claude Code