fix(providers/kno-commerce): Adjust param passing to query string#6047
fix(providers/kno-commerce): Adjust param passing to query string#6047NathanHu725 wants to merge 3 commits into
Conversation
…pe as connection config
The previous config passed grant_type (and any scope we'd add) via
token_params, which Nango places in the request body. Kno's OAuth token
endpoint silently ignores the body and only reads query-string params,
returning HTTP 400 invalid_request unless both grant_type and scope are
present in the URL itself.
Tested live against the Kno production token endpoint with the exact
shape Nango produces (Basic auth header + form-urlencoded body):
| URL has gt+scope, body empty → 200 OK
| URL empty, body has gt+scope → 400 invalid_request
| URL has gt only, body has scope → 400 invalid_request
| URL has scope only (no gt) → 400 invalid_request
Both grant_type and scope are mandatory and must live in the URL.
Fix:
* Embed grant_type=client_credentials directly in token_url (always this
value — Kno only supports the client-credentials grant).
* Interpolate ${connectionConfig.scope} into the URL so customers can
set the scope at connect time, matching the templating pattern used
by commercetools, databricks-workspace, and others. Defaulting/example
set to "SURVEYS RESPONSES QUESTIONS WEBHOOKS" — the four read scopes
Kno publishes — with a pattern guard against typos and injection.
* Drop token_params entirely; Kno ignores the body.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83c9a6ea0b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - e-commerce | ||
| auth_mode: OAUTH2_CC | ||
| token_url: https://app-api.knocommerce.com/api/oauth2/token | ||
| token_url: https://app-api.knocommerce.com/api/oauth2/token?grant_type=client_credentials&scope=${connectionConfig.scope} |
There was a problem hiding this comment.
Keep Kno scopes out of the token body
When a Kno integration has oauth_scopes configured in Nango, the OAuth2 CC controller copies it into connectionConfig.oauth_scopes and getOauthClientCredentials serializes that field into the POST body. This new URL adds a separate query-string scope, so those integrations no longer send the tested body: empty request; Kno still receives a body scope parameter, which the commit notes is rejected. Use a single scope source in the query or suppress the body scope for this provider.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not sure how the scope can be suppressed here - it seems like the body is just ignored by kno. It doesn't hurt to leave it from my local tests.
There was a problem hiding this comment.
Thanks for your contribution. The scopes are an integration setting that must be configured by the developer when creating the integration in Nango, or overridden when creating the connect session.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a7b7df1eb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/shared/lib/services/connection.service.ts">
<violation number="1" location="packages/shared/lib/services/connection.service.ts:1225">
P2: Scope is emitted in both the token URL and the POST body, which breaks providers like Kno Commerce that require the scope only in the query string.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| tokenParams += (tokenParams ? '&' : '') + `scope=${encodeURIComponent(oauthScopesJoined)}`; | ||
| } | ||
|
|
There was a problem hiding this comment.
P2: Scope is emitted in both the token URL and the POST body, which breaks providers like Kno Commerce that require the scope only in the query string.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/shared/lib/services/connection.service.ts, line 1225:
<comment>Scope is emitted in both the token URL and the POST body, which breaks providers like Kno Commerce that require the scope only in the query string.</comment>
<file context>
@@ -1202,18 +1202,27 @@ class ConnectionService {
- const scope = connectionConfig['oauth_scopes'].split(',').join(provider.scope_separator || ' ');
- tokenParams += (tokenParams ? '&' : '') + `scope=${encodeURIComponent(scope)}`;
+ if (oauthScopesJoined) {
+ tokenParams += (tokenParams ? '&' : '') + `scope=${encodeURIComponent(oauthScopesJoined)}`;
}
</file context>
| tokenParams += (tokenParams ? '&' : '') + `scope=${encodeURIComponent(oauthScopesJoined)}`; | |
| } | |
| const tokenUrlHasScope = typeof provider.token_url === 'string' && /[?&]scope=/.test(provider.token_url); | |
| if (oauthScopesJoined && !tokenUrlHasScope) { | |
| tokenParams += (tokenParams ? '&' : '') + `scope=${encodeURIComponent(oauthScopesJoined)}`; | |
| } |
8a7b7df to
49ff525
Compare
…o URL
Some OAuth2 client-credentials providers (e.g. Kno Commerce) require the
`scope` parameter to be on the URL query string rather than in the form
body. Templating ${connectionConfig.oauth_scopes} into token_url
previously returned the comma-separated storage form, which those APIs
reject (400 invalid_scope).
This change pre-computes oauth_scopes joined with the provider's
scope_separator (defaulting to ' ' per RFC 6749) and exposes that joined
form to the URL templating context. Body emission reuses the same
pre-computed value rather than recomputing it inline. No behavior change
for providers that don't template oauth_scopes into token_url (none do
today).
For kno-commerce, this lets developers pick scopes from the dashboard
catalog (added in providers.scopes.yaml) and have those scopes flow into
the URL with the correct delimiter.
13ea170 to
d31adcc
Compare
…ig check
The provider validator walks every \${connectionConfig.X} interpolation and
requires X to be declared in connection_config or token_response_metadata.
That's the right rule for fields a customer or developer types in.
But some keys are populated at runtime by the auth subsystem itself —
notably oauth_scopes, which is built from the integration's stored scope
chips and joined with the provider's scope_separator at request time.
There's no place in the YAML to declare such keys, and forcing them to
appear in connection_config would mislead readers (the field is not
user-supplied).
Add a small RUNTIME_DYNAMIC_KEYS set listing names the validator should
permit without a declaration. Today: just oauth_scopes. Future runtime-
populated identifiers can be added to the same set without scattering
one-off if-branches through the validator.
d31adcc to
f7be39d
Compare
Kno's OAuth2 setup require the parameters to be passed through query string rather than through the body. In the
OAUTH2_CC,token_paramsare passed as a part of the body. I change the url to include these instead as well as adding a second param that is required for the token url.Local testing against the token endpoint verifies this:
This can be verified by running this python code with a valid client secret and id