feat: require the subdomain option or an explicit useLegacyDomain opt-out - #448
Conversation
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 11 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 36 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
🟠 Advisory review: Concerns worth a lookThis PR needs a human approval. Before you give it, these are the things I'd want resolved. The PR correctly enforces the mandatory subdomain requirement by replacing deprecation warnings with thrown ValueErrors, adding the Concerns
This is not an approval. wall-e cannot auto-approve this PR — it is an opinion to help whoever does. Advisory review · us.anthropic.claude-sonnet-4-6 · wall-e 2026.06.19-02 |
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 36 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
🔴 Risk Classification: MAJORApproval route: AI Review + Human Approval Required Classification reasons
Operational gates
Files analysed: 39 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
🔴 Risk Classification: MAJORApproval route: AI Review + Human Approval Required Classification reasons
Operational gates
Files analysed: 38 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 34 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
…-out Initializing without a subdomain already emitted a deprecation warning, so this finishes the job: it now throws a ValueError. Callers must either set `subdomain`, or opt out explicitly with `useLegacyDomain: true`, which ships deprecated in the type definitions. Both, or neither, throws. An invalid subdomain also throws now instead of being silently dropped back to the shared host, which is a second breaking change: a malformed value currently works and the caller never finds out. Two exemptions. A custom `host` replaces the base URL outright, so the merchant has already said where requests go. Previous (ABC) keys predate merchant-specific subdomains, matched by the new PREVIOUS_SECRET_KEY_REGEX, which covers both live and sandbox key shapes (MBC_LIVE_SECRET_KEY_REGEX only matched live). Mirrors checkout-sdk-net#590. Refs INT-1688.
…is unset CI failed at module load with "subdomain is required". Twenty-four test files read the subdomain from CHECKOUT_MERCHANT_SUBDOMAIN, but no workflow exports it, so the option arrived undefined. That used to be harmless, it just meant no subdomain; now it throws. They all spread domainOptions() instead, which uses the subdomain when the variable is set and the legacy hosts otherwise. Exporting the variable in CI would not be enough on its own: the sandbox OAuth clients are not provisioned for the subdomain, so the token request would come back invalid_client. This only reproduced in CI because the variable happens to be set locally.
Flagged in review. A custom host is exempt from *requiring* a subdomain, since it replaces the base URL outright, but a subdomain passed alongside one was neither validated nor used: it was quietly dropped. Now the format check applies whenever a subdomain is present, wherever the base URL comes from.
The suite could only run against the shared hosts, so the subdomain path this PR makes mandatory had no integration coverage. Reviewers flagged that on every SDK, and it is the right thing to flag. The domain helper now has two modes. Default is unchanged, the shared hosts, because the sandbox OAuth clients are not provisioned for the subdomain and the token request returns invalid_client. Set CHECKOUT_TEST_USE_SUBDOMAIN=true and the suite runs against CHECKOUT_MERCHANT_SUBDOMAIN instead, so once sandbox is provisioned like production it is a one-line change in the workflows, already wired and documented, rather than a rewrite of every fixture. The switch is deliberately separate from CHECKOUT_MERCHANT_SUBDOMAIN, which CI already exports: provisioning should drive the behaviour, not the presence of a secret.
Versions are bumped on master during the release, not in a feature branch, per the release workflow. This branch should carry only the change itself; the major bump is classified and applied when the release is cut.
Two problems with the previous approach. It needed a new variable in 21 workflow files, which is not viable without access to create secrets. And it wrapped the builder chain in a configureDomain helper that is not part of the public API, so the tests stopped looking like the code a merchant would actually write. Every fixture now calls the real opt-out inline, in the chain, with a comment saying why: the sandbox OAuth clients are not provisioned for the merchant-specific subdomain, so the token request comes back invalid_client. When sandbox is provisioned, those calls become the subdomain setter. The unit tests covering all four combinations are untouched: they already used the public API directly.
e412755 to
122c10e
Compare
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 34 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
…th client The dedicated sandbox clients are not provisioned for the merchant subdomain; the default client now carries every scope the suites need.
🔴 Risk Classification: MAJORApproval route: AI Review + Human Approval Required Classification reasons
Operational gates
Files analysed: 40 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
…ault OAuth client" This reverts commit 746d499.
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 34 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
… review findings - EnvironmentSubdomain.createUrlWithSubdomain now throws a ValueError on an invalid subdomain instead of silently returning the un-prefixed URL, and no longer swallows URL parsing errors; JSDoc, the .d.ts and the unit tests are updated to match the other SDKs. - Restore the original CRLF line endings in src/config.js and types/dist/Checkout.d.ts so the diff only shows the real changes. - Document the custom host route in the README legacy-domain section and the Private Link pl- prefixed subdomain in the Subdomain value section. - Add a unit test asserting a NAS-shaped secret key is not exempt from the subdomain requirement, and a comment documenting that the Previous (ABC) exemption is inferred from the secret-key shape.
🟡 Risk Classification: MINORApproval route: AI Review + Human Approval Classification reasons
Operational gates
Files analysed: 36 wall-e 2026.06.19-02 · policy |
🔬 Debug — why this classification?Each reason code emitted by the classifier, its source clause in the AI in SDLC Control Framework, and what it means.
Kinds:
See issue #3 for the proposal to formalise this map as Appendix A of the standards doc. wall-e 2026.06.19-02 · debug |
|
2 participants
Summary
Makes the merchant-specific subdomain (MSSD) mandatory. Initializing without a
subdomainalready emitted a deprecation warning, so this finishes the job: it now throws aValueError. Callers must either setsubdomain, or explicitly opt out withuseLegacyDomain: true, which ships@deprecatedin the type definitions. Both, or neither, throws.Changes
src/auth-builder.js— newvalidateDomainOptions()replaces the two deprecation warnings; theEnvironmentSubdomainis only built once the options are known valid; newisPreviousPlatform()src/config.js— newPREVIOUS_SECRET_KEY_REGEXtypes/dist/Checkout.d.ts— documentssubdomainas required and addsuseLegacyDomain, marked@deprecatedso editors andtscflag ittest/config/config.js,test/environment-subdomain/environment-subdomain-integration.js— the cases that asserted invalid subdomains being silently dropped now assert the throw; added coverage for the legacy opt-out and for both-settest/transfers,test/hosted-payments,test/payments-links— the clients there now pass the subdomain their ownnockhost already expectedTwo exemptions
A custom
hostreplaces the base URL outright, so the merchant has already said where requests go. Previous (ABC) keys predate merchant-specific subdomains.Fixed along the way
The Previous exemption needed a new regex:
MBC_LIVE_SECRET_KEY_REGEXonly matches live keys, so sandbox Previous keys (sk_test_<uuid>) were not recognised as that platform.Verification
884 tests passing, 0 failing, 79 pending. The one
eslinterror (prefer-constinsrc/api/balances/balances.js) is pre-existing on master and untouched here.API Reference
Breaking changes
Yes, two. This needs a major release, classified and versioned when the release is cut.
api.checkout.com/access.checkout.comnow fails at client construction. Migration: set the subdomain, or use the legacy-domain opt-out as a temporary measure. The Previous (ABC) platform is unaffected.README
Updated in this PR: a "Subdomain value" section above the Default example, the subdomain added to the configuration samples, and a "Legacy domain (emergency use only)" section at the bottom.
Notes
The suite routes every client it builds through a single helper that uses the shared hosts. Applying the merchant-specific subdomain there looked better, since it is the path merchants are being moved to, but the sandbox OAuth clients are not provisioned for it: .NET CI failed 224 integration tests with
invalid_clientwhen the token request went to{subdomain}.access.sandbox.checkout.com. Binding those OAuth clients to the subdomain is a platform task and should land before merchants are told the subdomain is mandatory.Reference implementation: checkout-sdk-net#590. Tracked as INT-1688.
No version bump here: that happens on master when the release is cut, per the release workflow.
Review follow-ups (2026-08-31)
Breaking changes, complete list:
Behaviour note: host resolution is now deferred to build time, which also fixes a latent order-dependence bug where setting the subdomain before the environment produced the wrong host.
Deprecation signal: documentation (JSDoc); the custom host option remains a documented third opt-out route.