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

Add private key validation check for multiline string content #482

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

reza-ebrahimi
Copy link

@reza-ebrahimi reza-ebrahimi commented May 27, 2023

Add private key validation check for multi-line string content specifically when set using environment variables as multi-line string.

Discussion (Thanks to @gr2m ): gr2m/universal-github-app-jwt#71

Resolves #465 #71

Behavior

Before the change?

Failing due to passing multiline string to environment variables, See #465 (comment)

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • N/A

Pull request type

Please add the corresponding label for change this PR introduces:

  • Bugfix: Type: Bug
  • Feature/model/API additions: Type: Feature
  • Updates to docs or samples: Type: Documentation
  • Dependencies/code cleanup: Type: Maintenance

@reza-ebrahimi
Copy link
Author

There is already a test for this case: https://github.com/reza-ebrahimi/auth-app.js/blob/pk_check_multiline_string/test/index.test.ts#L105

test("throws if incomplete Private Key is provided", async () => {
  const auth = createAppAuth({
    appId: APP_ID,
    privateKey: "-----BEGIN RSA PRIVATE KEY-----",
  });

  await expect(auth({ type: "app" })).rejects.toEqual(
    new Error(
      "The 'privateKey` option contains only the first line '-----BEGIN RSA PRIVATE KEY-----'. If you are setting it using a `.env` file, make sure it is set on a single line with newlines replaced by '\n'"
    )
  );
});

@wolfy1339 wolfy1339 added the Type: Bug Something isn't working as documented, or is being fixed label May 27, 2023
@reza-ebrahimi
Copy link
Author

Change the check logic from regex to a multi-step check function since the regex doesn't support some edge cases like -----BEGIN RSA PRIVATE KEY-----.

@gr2m
Copy link
Contributor

gr2m commented May 27, 2023

There is already a test for this case

Ha, what do you know 🤣

I think we should

  1. Implement this fix in https://github.com/gr2m/universal-github-app-jwt
  2. Remove the special error handling from this library

Do you see any reason not to do it this way?

@kfcampbell
Copy link
Member

@reza-ebrahimi I'd like to bring this up again. Do you have the inclination to make the fix in gr2m/universal-github-app-jwt?

@reza-ebrahimi
Copy link
Author

@kfcampbell I'm not working on this for a long time, feel free to push this code in gr2m/universal-github-app-jwt or close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented, or is being fixed
Projects
Status: 🛑 Blocked/Awaiting Response
Development

Successfully merging this pull request may close these issues.

[BUG]: secretOrPrivateKey must be an asymmetric key when using RS256
4 participants