Skip to content
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(server): Remove secrets from in app integration api response #3048

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nalanj
Copy link
Contributor

@nalanj nalanj commented Nov 22, 2024

I realized were were sending some fields that we'd prefer not to send in the internal api responses for integrations. This PR makes sure those aren't returned.

How I Tested It

  • Go to an oauth based integration in the UI
  • Ensure secrets aren't included in the API response for the integration

@nalanj nalanj marked this pull request as ready for review November 22, 2024 21:11
@nalanj nalanj requested a review from a team November 22, 2024 21:11
@nalanj nalanj self-assigned this Nov 22, 2024
Copy link
Collaborator

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

Thanks for checking that 👍🏻

delete apiIntegration.oauth_client_secret_iv;
delete apiIntegration.oauth_client_secret_tag;

return apiIntegration;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The type IntegrationConfig is invalid, it states that those values are undefined but they are not, they can be nullable though. It can't be fixed yet because of legacy code (it's more or less always the issue each time). Right now, the best fix could look like this for the type:

export type ApiIntegration = Omit<Merge<IntegrationConfig, ApiTimestamps>, 'oauth_client_secret_iv' | 'oauth_client_secret_tag'>;

And then instead of spreading which does not trigger a Typescript error because the interface is still compatible, it should be assigned field by field (like endUser, connection). I forgot about this annoying "issue" for this formatter and a few others (team, invitation, etc.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants