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

epic: Server rewrite #10

Merged
merged 61 commits into from
Sep 23, 2024
Merged

Conversation

Danziger
Copy link
Collaborator

@Danziger Danziger commented Jul 8, 2024

  • Use JWT middleware from Auth0.
  • Use factories and dependency injection.
  • Replace try-catch in request handlers with custom errors and a generic express error handler.
  • Create custom error class with development messages and configurable HTTP status codes.
  • Create wrapper for env variables.
  • Remove create-bundle-and-sign endpoint as it was not used in the SDK anyway.
  • Handle API versioning in the same handlers, which now accept two different input-output formats, the old one that includes (for some operations) keyName in the input data and the new one that (always) includes path in the input data. The output has also changed, so now instead of always being consumed as response.data.data on the frontend, it would be response.data.idTokenWithData, response.data.encryptedData, response.data.decryptedData or response.data.signature.
  • Add ID token (and token data) validation with Zod.
  • Add unit tests for the different handlers.
  • Import keys PoC (disabled).

@Danziger Danziger marked this pull request as draft July 8, 2024 08:01

const ciphertext = await encrypt(data.plaintext, data.keyName);

logRequestSuccess(Route.ENCRYPT, idToken);
Copy link
Collaborator Author

@Danziger Danziger Jul 11, 2024

Choose a reason for hiding this comment

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

Before, we were logging the responses, which could include ciphertext, plaintext, signatures... I don't think that should be the case, the same way we were not logging the ID token as-is. These 2 logger functions have been updated to only work in development for now.

createUserHandlerFactory() as unknown as express.Handler,
);

// TODO: Data from multer doesn't seem to be used at all, as that's coming from the JWT token:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Multer wasn't actually used as the files are sent inside the ID token, so until I make the change to send them separately and only include a hash within the token, I'm removing Multer's middleware.

import { stringToBuffer } from "../../utils/arweave/arweaveUtils";
import { OthentError, OthentErrorID } from "../../server/errors/errors.utils";

export async function createBundleAndSign(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't seem to be used in the SDK as it just creates the bundle on the client and calls the /sign endpoint through the Signer, so this endpoint/function can probably be removed.

);
}

const lastNonce = await getLastNonce(idToken.sub);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a potential issue here if 2 operations are performed at the same time and they make it to the server out-of-order, as processing the last one first will invalidate the other one.

try {
const [encryptResponse] = await kmsClient.encrypt({
name,
plaintext: Buffer.from(plaintextData),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This would actually throw an error if someone sends binary data to encrypt or decrypt (instead of sending a plain string), as it would be stringified using JSON.stringify, so plaintextData = { 0: 23, 1: 65, 2: 67, 3: 23, ... }.

process.env.signKeyVersion,
);

const uint8Array = new Uint8Array(Object.values(data));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This won't produce the expected result if someone sends non-binary data, as data would then be a plain string, so potentially we are doing something similar to new Uint8Array(['H', 'e', 'l', 'l', 'o']), which won't encode the string properly.

@@ -0,0 +1,51 @@
/**
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can probably deploy these automatically using the Management API. See https://auth0.com/docs/api/management/v2/actions/patch-action.

@Danziger Danziger marked this pull request as ready for review September 12, 2024 10:53
@Danziger Danziger changed the title Draft: Replace hardcoded values with ENV variables and starts implementing loading public keys from jwks.json. Server rewrite Sep 12, 2024
@Danziger Danziger requested a review from 7i7o September 12, 2024 11:01
Copy link
Contributor

@7i7o 7i7o left a comment

Choose a reason for hiding this comment

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

tACK 👍

⚠️ We need to check the server env variables are updated before merge!

@Danziger Danziger changed the title Server rewrite epic: Server rewrite Sep 23, 2024
@Danziger Danziger merged commit ff0b462 into main Sep 23, 2024
1 check passed
@Danziger Danziger deleted the feature/add-environment-vars-n-use-jwks-config branch September 26, 2024 10:38
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.

2 participants