Skip to content
This repository has been archived by the owner on Jul 2, 2024. It is now read-only.

DEVPROD-4509: Exclude "secret fields" in Sentry Breadcrumbs #2284

Closed
wants to merge 25 commits into from

Conversation

SupaJoon
Copy link
Contributor

@SupaJoon SupaJoon commented Mar 1, 2024

DEVPROD-4509

Description

These code changes query SecretFields and use them to clean up GQL variables that are sent to Sentry. Since Sentry breadcrumbs are sent within an Apollo Link which is set during Apollo Client initialization, these code changes block Apollo Client initialization until SecretFields are fetched.
These code changes also update Login.tsx to utilize fetch instead of the Apollo client. If Apollo cannot be initialized because of failure to fetch Secret Fields, then the user is redirected to the Login page where Apollo cannot be initialized. The login page is used in the dev environment only.

// if there is data in response then server responded with 200; therefore, is authenticated.
dispatchAuthenticated();
}
leaveBreadcrumb(
Copy link
Contributor Author

@SupaJoon SupaJoon Mar 1, 2024

Choose a reason for hiding this comment

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

I made a separate Apollo Link for leaveBreadcrumb

Copy link

cypress bot commented Mar 1, 2024

1 flaky test on run #16229 ↗︎

0 546 10 0 Flakiness 1
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

fix test
Project: Spruce Commit: b07bdd1dd7
Status: Passed Duration: 19:12 💡
Started: Mar 18, 2024 7:19 PM Ended: Mar 18, 2024 7:38 PM

Review all test suite changes for PR #2284 ↗︎

dispatchAuthenticated?: () => void;
}

const cache = new InMemoryCache({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved cache definition to a separate file

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome!

},
});

const authLink = (logout: () => void): ApolloLink =>
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 moved all Apollo Links to a new file

},
});

const getGQLClient = ({
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 converted this into a hook.


describe("post", () => {
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 didn't update the test logic and only nested it

@SupaJoon SupaJoon requested a review from a team March 18, 2024 19:39
Copy link
Contributor

@khelif96 khelif96 left a comment

Choose a reason for hiding this comment

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

Great work here it works really well! I just had a few suggestions

@@ -14,7 +14,7 @@ describe("Viewing a patch", () => {
);
});
it("Clicking the 'My Patches' breadcrumb goes to the logged in user's Patches Page when the current patch belongs to the logged in user", () => {
cy.contains("My Patches").click();
cy.dataCy("bc-my-patches").click();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the fix for the flaky test? 😍

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 think so

dispatchAuthenticated?: () => void;
}

const cache = new InMemoryCache({
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome!

* @param keyToDelete The name of the key to be removed.
* @returns The object with the key removed.
*/
export function deleteNestedKey<T extends object>(
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you feel about using a similar redaction pattern as we use in the backend? Instead of deleting the key we instead replace the value with the word REDACTED or something similar. This still allows us to debug and see that we sent a value while also accomplishing the same affect.

src/utils/request.ts Outdated Show resolved Hide resolved
src/hooks/useCreateGQLClient/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Great tests!

src/gql/fetch/index.ts Outdated Show resolved Hide resolved
@SupaJoon SupaJoon requested a review from khelif96 March 25, 2024 17:47
export const omit = <T extends object, K extends [...(keyof T)[]]>(
obj: T,
params: K,
) => {
const newObj = { ...obj };
params.forEach((param) => delete newObj[param]);
deleteNestedKey(newObj, params as string[]);
return newObj as Omit<T, K[number]>;
Copy link
Contributor

Choose a reason for hiding this comment

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

The typing here is no longer valid it does not handle nested keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might makes sense to keep these utility functions separate since one handles simple key value maps and the other handles deeply nested objects. What do you think?

keyToUpdate: string | string[],
redactedString?: string,
): Partial<T> {
const deleteKey = (currentObject: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const deleteKey = (currentObject: any) => {
const deleteKey = (currentObject: { [key:string]: any}) => {

@SupaJoon SupaJoon requested a review from khelif96 March 26, 2024 13:33
Copy link
Contributor

@khelif96 khelif96 left a comment

Choose a reason for hiding this comment

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

Looks great! sorry I realized theres one more spot where we need to redact variables.

variables: operation.variables,

image

@SupaJoon SupaJoon requested a review from khelif96 March 26, 2024 19:48
Copy link
Contributor

@khelif96 khelif96 left a comment

Choose a reason for hiding this comment

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

Great work on this! 🚀

@minnakt
Copy link
Contributor

minnakt commented Mar 28, 2024

Closing this PR since it has already been moved to the UI repo

@minnakt minnakt closed this Mar 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants