Guard the resolved endpoint, and serve a second environment - #25
Merged
Alex Arguello (alex-arguello) merged 7 commits intoAug 12, 2026
Merged
Conversation
normalizeBaseUrl validated PAYABLI_API_BASE_URL and payabliApi then resolved a path against it and sent the bearer token to whatever came out. Resolution can move the origin, so validating the base alone leaves the credential reachable at a host the allow-list was meant to exclude. The check is now a function rather than a body inside normalizeBaseUrl, and payabliApi applies it to the endpoint it is about to call. The Android demo server has applied it at both points for this reason; this closes the gap in the direction of the stricter one. Not reachable from the routes as they stand: the paths are literals and the one interpolated value is encodeURIComponent'd. It is the guard that was partial, rather than a live path.
The server loaded .env by name, so pointing it at another environment meant editing the credential file in place and editing it back. PAYABLI_ENV_FILE picks the file instead, and .gitignore already covers .env.* so a second one stays untracked. A named file that does not exist exits rather than falling through to the built-in sandbox defaults, which is the case that would otherwise run against the wrong upstream and report nothing. The startup banner prints the upstream, the env file and the entry point. Without them two runs on two environments are indistinguishable in the log, and a refusal from the wrong one reads as a bad entry point. Matches the Android demo server, so the two stay one procedure.
Copilot started reviewing on behalf of
Alex Arguello (alex-arguello)
August 12, 2026 01:42
View session
There was a problem hiding this comment.
Pull request overview
Strengthens the demo token server’s endpoint validation and supports environment-specific configuration files.
Changes:
- Validates resolved bearer-token endpoints.
- Adds
PAYABLI_ENV_FILEselection and startup diagnostics. - Documents multi-environment setup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
Example/PayabliDemo/LocalTokenServer/server.mjs |
Adds endpoint validation, environment-file selection, and startup details. |
Example/PayabliDemo/LocalTokenServer/.env.example |
Documents environment-specific configuration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The note added in this branch told a reader to pass -Ppayabli.demo.environment, which is the Android Gradle property. DemoConfiguration parses -PayabliEnvironment, so following it left the app on its previous environment while the server switched upstream, which is the mismatch the note exists to prevent.
…ntial exchangeCredentials resolved its endpoint and posted clientId and clientSecret to it without checking it against the allowed hosts, and neither fetch in this file set a redirect mode. The redirect half is live. A 307 or 308 replays the method and body, so an allowed host answering with a Location on another origin is handed the client id and secret, and the check cannot see it because a redirect target only exists after the request. Both fetches now use redirect: "manual" and report a 3xx with its target. The endpoint check is defence in depth rather than a reachable escape. A tokenPath of //host survives normalizeTokenPath, but the leading slashes are stripped before resolution: measured, //evil.example.com/steal resolves to api-qa.payabli.com/api/evil.example.com/steal, inside the allowed host. The Android demo server checks at this point and this one did not, which is the divergence being closed.
Both card-present routes forwarded the request body into an options object that reaches payabliApi, where apiBaseUrl, accessToken, clientId, clientSecret and tokenPath are all honoured, so a caller chose the upstream. Measured on the Android demo server, which had the same routes: posting an apiBaseUrl of api-sandbox sent a qa credential to sandbox, which answered InvalidCredentials. The routes now pass what they document: the entry for the device list, the entry and deviceId for the activation code. Found by sweeping for the shape a review found on the sibling, not reported here.
Copilot started reviewing on behalf of
Alex Arguello (alex-arguello)
August 12, 2026 01:53
View session
Copilot stopped reviewing on behalf of
Alex Arguello (alex-arguello) due to an error
August 12, 2026 01:53
Copilot started reviewing on behalf of
Alex Arguello (alex-arguello)
August 12, 2026 02:21
View session
The route promises an activation code. An upstream envelope that reports success with no responseData.code was returned as HTTP 200 with code: "", so a caller reads an unusable response as issuance and the device is never activated. An empty code is now a 502 naming the device and the entry point, which is the same shape the decline path already uses. Found by review on the sibling, payabli/sdk-android#37, not reported here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Example/PayabliDemo/LocalTokenServer/server.mjs:238
- With
redirect: "manual", an API redirect reaches the generic!upstream.okbranch, which preserves every status below 500. The local endpoint therefore emits a 3xx (and can lose the JSON body entirely for statuses such as 304) instead of reporting an upstream failure; it also omits the redirect target that the token-exchange path preserves. Reject 3xx responses as a 502 before parsing the body, asexchangeCredentialsdoes.
redirect: "manual",
Copilot started reviewing on behalf of
Alex Arguello (alex-arguello)
August 12, 2026 02:24
View session
Both found by review on the sibling, payabli/sdk-android#37, not reported here. The message telling someone to set PAYABLI_ENTRY named .env even when PAYABLI_ENV_FILE had selected another file, so following it edited the environment that was not running. It names the file that was loaded. A per-device lookup that came back declined was turned into null and the device left the list with nothing said, so a decline for provisioning or authorisation was indistinguishable from a device that is not there. The response carries an unavailable list of deviceId, code and text. Which codes mean a stale row is documented nowhere this server can read, so it reports what it skipped rather than deciding which declines are benign. The README here already documents the activation request shape, which was the third finding on the sibling and does not apply. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot started reviewing on behalf of
Alex Arguello (alex-arguello)
August 12, 2026 02:39
View session
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Example/PayabliDemo/LocalTokenServer/server.mjs:238
redirect: "manual"prevents the credential leak, but this path does not handle the returned 3xx asexchangeCredentialsdoes. Because a 3xx response is notok, the generic branch below throws aLocalTokenServerErrorwith the upstream 3xx status; the top-level handler then sends a redirect response without aLocationheader and omits the target thatmanualmode was intended to expose. Convert redirects to a 502 and report the target before parsing the body.
redirect: "manual",
Alex Arguello (alex-arguello)
deleted the
local-token-server/environment-and-endpoint-guard
branch
August 12, 2026 02:45
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
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.
What was wrong
payabliApiresolved a path againstPAYABLI_API_BASE_URLand sent the bearer token to whatever cameout, while only the base had been checked against the allowed hosts. Resolution can move the origin, so
validating the base alone leaves the credential reachable at a host the allow-list exists to exclude.
Separately, the server loaded
.envby name, so running against a second environment meant editing thecredential file in place and editing it back.
What this does
The host check became a function and now runs at both points.
normalizeBaseUrlstill checks theconfigured base;
payabliApiadditionally checks the endpoint it is about to call. This is notreachable from the routes as they stand, because their paths are literals and the one interpolated
value is
encodeURIComponent'd, so what was wrong is the guard being partial rather than a live path.PAYABLI_ENV_FILEpicks the env file, so a second environment is a second file:.gitignorealready covers.env.*, so the second file stays untracked. A named file that does notexist exits rather than falling through to the built-in sandbox defaults, which is the case that would
otherwise run against the wrong upstream and say nothing.
The startup banner now prints the upstream, the env file and the entry point. Two runs against two
environments were previously indistinguishable in the log, which is what makes a refusal from the wrong
upstream read as a bad entry point.
Sibling platform
The Android demo server already applied the host check at both points, and its comment gives this
reason. That difference was an unnamed divergence rather than a decision, so this converges on the
stricter side. The
PAYABLI_ENV_FILEbehaviour and the banner are the same on both, landing inpayabli/sdk-android#37, so the two servers stay one procedure to learn.Verification
No Swift changed, so the SPM targets and the XCTest suites are untouched and were not run. The server
was exercised directly:
/healthanswers, and/payabli/devicesreturns 30 devices forentry3715, so the added checkdoes not reject the endpoints the server actually calls.
PAYABLI_ENV_FILE=.env.sandbox:/healthanswers and the banner reportsapi-sandbox.payabli.com.PAYABLI_ENV_FILEnaming a file that does not exist exits 1 and says which path it looked at.