Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(shared): Keep agents from cloning in simple-oauth2 #3447

Merged
merged 5 commits into from
Feb 5, 2025
Merged

fix(shared): Keep agents from cloning in simple-oauth2 #3447

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d005de3
Keep agents from cloning in simple-oauth2
Feb 4, 2025
a26a26d
Merge branch 'master' into alan/unenumerable-agent
nalanj Feb 4, 2025
afc784b
Use getters for agent props
Feb 4, 2025
69e7740
Merge branch 'master' into alan/unenumerable-agent
nalanj Feb 5, 2025
987f3ef
Merge branch 'master' into alan/unenumerable-agent
nalanj Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions packages/shared/lib/clients/oauth2.client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export function getSimpleOAuth2ClientConfig(

const authConfig = provider as ProviderOAuth2;

return {
const clientConfig: Merge<ModuleOptions, { http: WreckHttpOptions }> = {
client: {
id: providerConfig.oauth_client_id,
secret: providerConfig.oauth_client_secret
Expand All @@ -36,25 +36,22 @@ export function getSimpleOAuth2ClientConfig(
authorizeHost: authorizeUrl.origin,
authorizePath: authorizeUrl.pathname
},
http: {
headers,
// @ts-expect-error badly documented feature https://github.com/hapijs/wreck/blob/ba28b0420d6b0998cd8e61be7f3f8822129c88fe/lib/index.js#L34-L40
agents:
httpAgent && httpsAgent
? {
http: httpAgent,
https: httpsAgent,
httpsAllowUnauthorized: httpsAgent
}
: undefined
},
options: {
authorizationMethod: authConfig.authorization_method || 'body',
bodyFormat: authConfig.body_format || 'form',
// @ts-expect-error seems unused ?
scopeSeparator: provider.scope_separator || ' '
}
};

// define getters for agents so that they aren't clone within simple-oauth2
const httpConfig = { headers, agents: {} };
Object.defineProperty(httpConfig.agents, 'http', { get: () => httpAgent, enumerable: true });
Object.defineProperty(httpConfig.agents, 'https', { get: () => httpsAgent, enumerable: true });
Object.defineProperty(httpConfig.agents, 'httpsAllowUnauthorized', { get: () => httpsAgent, enumerable: true });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simplify and avoid having to use defineProperty like this:

const getAgentConfig = (): { agents: AgentConfig } => ({
  agents: {
    get http() { return httpAgent; },
    get https() { return httpsAgent; },
    get httpsAllowUnauthorized() { return httpsAgent; }
  }
});

const clientConfig: Merge<ModuleOptions, { http: WreckHttpOptions }> = {
    ...,
    http: {
      headers,
      ...getAgentConfig()
    }
  };

And adding a comment to mention the getter dynamic lookup trick used here. I am sure next week I am gonna have forgotten about it already

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had already hit merge but I'll put a follow-up in real quick and merge it if this works out for that hot spot in the profiles, otherwise we'll just revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3457


clientConfig.http = httpConfig;
return clientConfig;
}

export async function getFreshOAuth2Credentials(
Expand Down
Loading