Skip to content

fix(wrangler): validate account IDs before API calls#13746

Open
edmundhung wants to merge 3 commits intomainfrom
edmundhung/fix-account-id-validation
Open

fix(wrangler): validate account IDs before API calls#13746
edmundhung wants to merge 3 commits intomainfrom
edmundhung/fix-account-id-validation

Conversation

@edmundhung
Copy link
Copy Markdown
Member

Fixes #8380.


  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: bugfix

A picture of a cute animal (not mandatory, but encouraged)

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 29, 2026

🦋 Changeset detected

Latest commit: 39f7d78

The changes in this PR will be included in the next version bump.

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 29, 2026

✅ All changesets look good

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 29, 2026

The analysis is complete. Let me now assess the overall quality of this PR:

Summary: The PR validates account IDs from user-controlled sources (config file and environment variable) before they're used in API URL paths, preventing a confusing ByteString error when non-ASCII characters are present.

Assessment:

  1. The validation regex ^[A-Za-z0-9_-]+$ is reasonable -- it's intentionally permissive (real Cloudflare account IDs are 32-char hex, but the goal is to prevent non-ASCII characters that cause ByteString failures, not to enforce the exact format).
  2. The validation is placed at the right chokepoints: getAccountId() for config values, getCloudflareAccountIdFromEnv() for env var values, and explicitly in containers/deploy.ts for its direct config.account_id usage.
  3. The exploration agent confirmed all code paths are covered -- no gaps.
  4. Tests cover both the config and env var paths with appropriate assertions.
  5. The containers/deploy.ts import ordering (../user/account-id before ../user) is technically fine since these are sibling-depth imports and the formatter will sort them.
  6. The changeset is correct.
  7. The empty string check in getCloudflareAccountIdFromEnv() is a good defensive measure.

I don't see any logic bugs, security issues, backward compatibility problems, or incorrect API behavior.

LGTM

github run

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Apr 29, 2026

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@13746

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@13746

miniflare

npm i https://pkg.pr.new/miniflare@13746

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@13746

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@13746

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@13746

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@13746

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@13746

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@13746

wrangler

npm i https://pkg.pr.new/wrangler@13746

commit: 39f7d78

@edmundhung edmundhung marked this pull request as ready for review April 30, 2026 10:30
@workers-devprod workers-devprod requested review from a team and ascorbic and removed request for a team April 30, 2026 10:31
@workers-devprod
Copy link
Copy Markdown
Contributor

Codeowners approval required for this PR:

  • @cloudflare/cloudchamber
  • @cloudflare/wrangler
Show detailed file reviewers
  • packages/wrangler/src/tests/pages/project-list.test.ts: [@cloudflare/wrangler]
  • packages/wrangler/src/tests/user.test.ts: [@cloudflare/wrangler]
  • packages/wrangler/src/containers/deploy.ts: [@cloudflare/cloudchamber @cloudflare/wrangler]
  • packages/wrangler/src/user/account-id.ts: [@cloudflare/wrangler]
  • packages/wrangler/src/user/auth-variables.ts: [@cloudflare/wrangler]
  • packages/wrangler/src/user/user.ts: [@cloudflare/wrangler]

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional findings.

Open in Devin Review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Untriaged

Development

Successfully merging this pull request may close these issues.

🐛 BUG: Cannot convert argument to a ByteString because the character at index 7 has a value of 1074 which is greater than 255.

2 participants