feat: require environmentSubdomain or an explicit useLegacyDomain opt-out - #649
Open
armando-rodriguez-cko wants to merge 5 commits into
Open
feat: require environmentSubdomain or an explicit useLegacyDomain opt-out#649armando-rodriguez-cko wants to merge 5 commits into
armando-rodriguez-cko wants to merge 5 commits into
Conversation
…-out The merchant-specific subdomain is how merchants should reach the API, but it was optional and an unset value silently fell back to api.checkout.com, so a forgotten subdomain looked exactly like a deliberate opt-out and the SDK could not warn about either. Callers must now choose: set environmentSubdomain, or call the already-deprecated useLegacyDomain(). Both, or neither, throws. An invalid subdomain now throws instead of being quietly ignored, which is a second breaking change: callers passing a malformed value are currently served by the shared host and never find out. environmentSubdomain no longer needs environment() to be set first, since the EnvironmentSubdomain is now built when the configuration is assembled. The Previous (ABC) platform predates merchant-specific subdomains and stays exempt via requiresEnvironmentSubdomain(). Mirrors checkout-sdk-net#590. Refs INT-1688.
…tion
Seven integration fixtures build their own clients outside SandboxTestFixture
(OAuth, Issuing, Accounts, Accounts payout schedules, APM previews, card
metadata), so the mandatory subdomain would have failed them at construction.
They now share TestDomainConfiguration.configureDomain, which uses the shared
hosts. Applying the merchant-specific subdomain instead 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_client
when the token request went to {subdomain}.access.sandbox.checkout.com. The
reason is recorded on the class so nobody repeats the experiment.
🔴 Risk Classification: MAJORApproval route: AI Review + Human Approval Required Classification reasons
Operational gates
Files analysed: 19 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. This PR makes 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+105
to
+109
| final CheckoutApi checkoutApi = new CheckoutSdkBuilder().staticKeys() | ||
| .publicKey(VALID_DEFAULT_PK) | ||
| .secretKey(VALID_DEFAULT_SK) | ||
| .environment(Environment.SANDBOX) | ||
| .useLegacyDomain() |
Comment on lines
+135
to
+140
| () -> new CheckoutSdkBuilder().staticKeys() | ||
| .publicKey(VALID_DEFAULT_PK) | ||
| .secretKey(VALID_DEFAULT_SK) | ||
| .environment(Environment.SANDBOX) | ||
| .environmentSubdomain("1234doma") | ||
| .useLegacyDomain() |
| @SuppressWarnings("deprecation") | ||
| public static <T extends CheckoutApiClient> AbstractCheckoutSdkBuilder<T> configureDomain( | ||
| final AbstractCheckoutSdkBuilder<T> builder) { | ||
| return builder.useLegacyDomain(); |
…onfiguration CI runs the full suite, so three OAuthTestIT cases that build their own client were still failing at construction. Local runs excluded integration tests, which is why they were missed.
🔴 Risk Classification: MAJORApproval route: AI Review + Human Approval Required Classification reasons
Operational gates
Files analysed: 19 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 |
Flagged in review, and fair: a bulk edit added the subdomain to the Previous (ABC) builder, the one platform that is exempt from needing one. That made the test misleading and, worse, removed the only coverage of the exemption actually working. It builds without a subdomain again.
🔴 Risk Classification: MAJORApproval route: AI Review + Human Approval Required Classification reasons
Operational gates
Files analysed: 19 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 |
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.
🔴 Risk Classification: MAJORApproval route: AI Review + Human Approval Required Classification reasons
Operational gates
Files analysed: 22 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 |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes the merchant-specific subdomain (MSSD) mandatory. Callers must now call
environmentSubdomain(...), or explicitly opt out with the new, already-@DeprecateduseLegacyDomain(). Setting both, or neither, throwsCheckoutArgumentExceptionat client construction. MSSD is no longer beta and non-MSSD usage will be deprecated, so the previous silent fallback toapi.checkout.comhad to go.Changes
AbstractCheckoutSdkBuilder— the subdomain is held as aStringand theEnvironmentSubdomainis built when the configuration is assembled, soenvironmentSubdomain()no longer has to be called afterenvironment(); newuseLegacyDomain()marked@Deprecated; newvalidateEnvironmentSettings(); newrequiresEnvironmentSubdomain()EnvironmentSubdomain—createUrlWithSubdomainthrows on an invalid subdomain instead of returning the URL unchangedCheckoutPreviousSdkBuilder— Previous/ABC exempted viarequiresEnvironmentSubdomain()gradle.properties— 7.15.0 to 8.0.0CheckoutSdkBuilderTest— covers all four combinations (subdomain only, legacy only, both, neither), an invalid subdomain, and the Previous exemptionDefaultCheckoutConfigurationTest— the parameterised bad-subdomain case now asserts the throw instead of the silent fallbackTestDomainConfiguration(new) +SandboxTestFixtureand seven integration fixtures — every client the suite builds now chooses a domainFixed along the way
environmentSubdomain()used to build the URLs from whateverenvironmentwas set at call time, so calling it beforeenvironment()silently produced the wrong host.API Reference
Breaking changes
Yes, two.
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.