-
Notifications
You must be signed in to change notification settings - Fork 427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(url): handle special characters in connection params #2608
Conversation
@@ -239,3 +239,7 @@ export function getConnectionConfig(queryParams: any): Record<string, string> { | |||
const arr = Object.entries(queryParams).filter(([, v]) => typeof v === 'string'); // Filter strings | |||
return Object.fromEntries(arr) as Record<string, string>; | |||
} | |||
|
|||
export function encodeParameters(params: Record<string, any>): Record<string, string> { | |||
return Object.fromEntries(Object.entries(params).map(([key, value]) => [key, encodeURIComponent(String(value))])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this === to new URLSearchParams(params).toString()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they are the same, but in our code, encodeParameters
is used to prepare the connectionConfig
object for interpolation into a URL. This is necessary because interpolateStringFromObject
expects an object with pre-encoded values, ensuring that when these values are interpolated into a URL, they do not introduce any invalid characters or break the URL format.
const strippedAuthorizeUrl = template.authorization_url!.replace(/connectionConfig\./g, ''); | ||
const authorizeUrl = new URL(interpolateString(strippedAuthorizeUrl, connectionConfig)); | ||
const tokenUrl = new URL(interpolateString(templateTokenUrl.replace(/connectionConfig\./g, ''), encodeParameters(connectionConfig))); | ||
const authorizeUrl = new URL(interpolateString(template.authorization_url!.replace(/connectionConfig\./g, ''), encodeParameters(connectionConfig))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic looks ok but it is quite hard to read. what about having a local function like this:
function makeUrl(template: string, config: Record<string, any>): URL {
const cleanTemplate = template.replace(/connectionConfig\./g, '');
const encodedParams = encodeParameters(config);
const interpolatedUrl = interpolateString(cleanTemplate, encodedParams);
return new URL(interpolatedUrl);
}
const tokenUrl = makeUrl(templateTokenUrl, connectionConfig);
const authorizeUrl = makeUrl(template.authorization_url!, connectionConfig);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, making it more readable. I've updated the PR with the latest commit. Thank you🙏!
bfe901e
to
e8a3f15
Compare
…ing params (#2652) ## Describe your changes The Github app oauth provider authorization should not be encoded as it produces an invalid URL. It was expected from #2608 that `connectionConfig` is used in query params but it is used for the full URL as well in some cases. This adds in a boolean configuration to avoid encoding. ## Issue ticket number and link NAN-1628 ## Checklist before requesting a review (skip if just adding/editing APIs & templates) - [ ] I added tests, otherwise the reason is: - [ ] I added observability, otherwise the reason is: - [ ] I added analytics, otherwise the reason is: <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new configuration option for controlling URL encoding in the authorization process. - Added flexibility to the URL construction by allowing optional encoding of parameters. - **Bug Fixes** - Resolved issues related to the construction of authorization URLs, enhancing compatibility with GitHub's OAuth. - **Documentation** - Updated interface and schema documentation to reflect the new `authorization_url_encode` parameter. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Describe your changes
Issue ticket number and link
EXT-121
Checklist before requesting a review (skip if just adding/editing APIs & templates)