Repository navigation
[wrangler, workers-utils, containers-shared] Add custom Cloudflare API headers - #16739
Conversation
🦋 Changeset detectedLatest commit: b55be5a The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
@cloudflare/containers-shared
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/runtime-types
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
![ask-bonk[bot]](/_dp/https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
I'm Bonk, and I've done a quick review of your PR. PR #16739 adds configurable, redacted Cloudflare API headers across Wrangler and container API clients.
|
165a322 to
f028834
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers |
![devin-ai-integration[bot]](/_dp/https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![clanky-the-clanker[bot]](/_dp/https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
c216ea35.
This review, of f028834b
💬 Approved with comments — The implementation is sound and well tested. There are two non-blocking notes: an exported helper that nothing uses, and container debug logs that now print real header values.
Findings: 1 info · 1 suggestion (2 are inline comments on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 1 info · 1 suggestion
Two minor notes: an exported helper nothing uses, and container debug logs that now print header values.
Recommendation: 💬 Optional Nits
- [INFO] Container debug logs now print real header values, including
X-Auth-Key, when sanitization is off (packages/containers-shared/src/client/core/request.ts:510) - [SUGGESTION]
createNativeCustomApiFetchis exported but nothing uses or tests it (packages/workers-utils/src/custom-api-headers.ts:23)
View full review
createNativeCustomApiFetch(packages/workers-utils/src/custom-api-headers.ts:23) is not called anywhere and has no tests. It is still exported as public API, and it readsprocess.envon its own instead of going throughgetCustomApiHeaders.- Container request debug logging used to call
JSON.stringify(<Headers>), which prints{}. It now prints real header values, soX-Auth-KeyandX-Auth-Emailappear whenWRANGLER_LOG_SANITIZE=false.
✅ All other checks passed
🔒 Security: ✅ LGTM
No security issues found.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md concerns.
⚡ Performance: ✅ LGTM
When the variable is unset, getCustomApiHeaders returns early. When it is set, the JSON parse on each request costs little next to the network call.
📝 Documentation: ✅ LGTM
The changeset and the linked docs PR describe the feature.
Reviewed commit: f028834b
f028834 to
c216ea3
Compare
There was a problem hiding this comment.
Superseded by the review of
b55be5a2.
This review, of c216ea35
💬 Approved with comments — The implementation is sound and well tested. Two earlier non-blocking notes are still open: an exported helper that nothing uses, and container debug logs that can print X-Auth-Key and X-Auth-Email when sanitization is off.
Findings: 1 info · 1 suggestion (2 are inline comments on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 1 info · 1 suggestion
Two earlier non-blocking notes are still open, and nothing new was found.
Recommendation: 💬 Optional Nits
- [INFO] Container debug logs now print
X-Auth-KeyandX-Auth-Emailwhen sanitization is off (packages/containers-shared/src/client/core/request.ts:508) - [SUGGESTION]
createNativeCustomApiFetchis exported but never called or tested (packages/workers-utils/src/custom-api-headers.ts:24)
View full review
- Unused export:
createNativeCustomApiFetch(packages/workers-utils/src/custom-api-headers.ts:24) has no callers and no tests. It also checksprocess.env.CLOUDFLARE_API_HEADERSitself instead of going throughgetCustomApiHeaders. - Container header logging:
packages/containers-shared/src/client/core/request.ts:508deletes onlyAuthorizationbefore logging. WhenWRANGLER_LOG_SANITIZE=false,X-Auth-KeyandX-Auth-Emailare printed in the debug output. Before this change these logs printed{}.
✅ All other checks passed
🔒 Security: ✅ LGTM
No security issues found.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md concerns.
⚡ Performance: ✅ LGTM
getCustomApiHeaders returns early when CLOUDFLARE_API_HEADERS is unset. When it is set, the JSON parse on each request costs little next to the network call.
📝 Documentation: ✅ LGTM
The changeset and the linked docs PR describe the feature.
Reviewed commit: c216ea35
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
Can we avoid adding the public API that we cannot remove outside of a major bump, where we can keep it simpler.
There was a problem hiding this comment.
💬 Approved with comments — The implementation is sound and well tested, and the unused export has been removed. One non-blocking INFO is still open: debug header logs can print X-Auth-Key and X-Auth-Email when sanitization is turned off.
Findings: 1 info (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 1 info
Sound implementation. One earlier INFO on debug header logging is still open.
Recommendation: 💬 Optional Nits
- [INFO] Debug header logs can print X-Auth-Key and X-Auth-Email when WRANGLER_LOG_SANITIZE=false (also in wrangler/src/cfetch/internal.ts logRequest) (
packages/containers-shared/src/client/core/request.ts:510)
View full review
- Header logging:
packages/containers-shared/src/client/core/request.ts:507-510andpackages/wrangler/src/cfetch/internal.tslogRequestdelete onlyAuthorizationbefore logging headers. Before this change these sites ranJSON.stringifyon aHeadersobject, which prints{}. They now print real entries, so withWRANGLER_LOG_SANITIZE=falsethe legacyX-Auth-KeyandX-Auth-Emailcredentials show up in debug output. With the default setting the wholeHEADERS:payload is hidden. createNativeCustomApiFetchhas been removed since the last review, so that earlier note is resolved.
✅ All other checks passed
🔒 Security: ✅ LGTM
Header names and values are validated with validateHeaderName and validateHeaderValue. Protected headers and duplicate names are rejected, and keys are defined so that __proto__ cannot pollute the object. CLOUDFLARE_API_HEADERS is local input at the same trust level as the API token.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md concerns.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
The changeset and the linked docs PR describe the feature.
Reviewed commit: b55be5a2
workers-devprod
left a comment
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
Allow Wrangler to send additional API headers through
CLOUDFLARE_API_HEADERS, for exampleCLOUDFLARE_API_HEADERS='{"X-Custom-Header":"value"}' npx wrangler deploy. Configured values override existing unprotected names case-insensitively.Custom-header diagnostics use the existing
WRANGLER_LOG_SANITIZEsetting: values are hidden by default and included when it isfalse.