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

Securely Handle Personal Access Tokens #53

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

micchickenburger
Copy link
Contributor

This pull request includes changes that:

  • Request a Personal Access Token from the user to access the Github API
  • Performs input validation on the PAT field
  • Creates a keystore and an app datastore using indexeddb
  • Uses the Web Crypto API to generate an AES-GCM 256-bit key and stores it in the keystore
  • Uses the generated key to encrypt the PAT to store in local storage
  • Reads and decrypts the PAT from local storage on page load
  • Connects to the GitHub GraphQL API using the stored PAT
  • Implements basic CSS styles
  • Implements an HTTPS web server for development, including auto-generation of asymmetric key pair and self-signed cert
  • Updated documentation

@BenHenning BenHenning self-requested a review January 21, 2021 05:47
@BenHenning BenHenning self-assigned this Jan 21, 2021
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @micchickenburger! I think it looks really nice code-wise (haven't actually tried out the UI itself, but I'm expecting that we may continue iterate on that as we finalize the other aspects of the UI). Had a few nits & other suggestions.

@@ -0,0 +1,14 @@
#!/usr/bin/env python3
Copy link
Member

Choose a reason for hiding this comment

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

Will this file get served to users once the PR is checked in? It seems like pages by default serves all files present--is there a way we can blacklist this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really good question. After reviewing GitHub Pages documentation, it does not look like there is a way to blacklist files. However, this is an open repository anyway, and since the file isn't linked to any resource a user would have to explicitly navigate to it. Furthermore, downloaded scripts do not have the executable bit enabled; that has to be set manually. Finally, even if a user did download and execute the script, all they'd get is a self-signed cert and a simple web server. This script doesn't change any certificate trust or store settings on the system, and I can't really see a way to use it for malicious intent. Do you have any particular concerns?

Copy link
Member

Choose a reason for hiding this comment

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

I think you outlined everything. I think it's worth noting these details at a high level in the Python file (just so that future maintainers aren't surprised that it can be accessed via the web browser, and that that's okay).

start.py Show resolved Hide resolved
README.md Show resolved Hide resolved
To start an HTTPS simple server with a generated self-signed certificate, execute:

```shell
$ python3 start.py
Copy link
Member

Choose a reason for hiding this comment

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

Suggest mentioning somewhere in this section that only Python 3 is supported when accessing the project dashboard (just to be explicit since above we have both Python 2 & 3 instructions).

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 since Python 2 has been deprecated for over a year it might be okay to remove python2 instructions. Thoughts?

Copy link
Member

@BenHenning BenHenning Jan 27, 2021

Choose a reason for hiding this comment

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

We're planning to keep the Python 2 instructions since the Oppia web backend still uses it.

README.md Outdated Show resolved Hide resolved
project-dashboard/scripts/db.js Outdated Show resolved Hide resolved
project-dashboard/scripts/db.js Outdated Show resolved Hide resolved
project-dashboard/scripts/pat.js Outdated Show resolved Hide resolved
project-dashboard/scripts/pat.js Outdated Show resolved Hide resolved
project-dashboard/scripts/pat.js Outdated Show resolved Hide resolved
@@ -86,6 +92,6 @@ const getKey = async (name) => {
return key;
};

// TODO: Write getters and setters for datastore
// TODO(55): Write getters and setters for datastore
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// TODO(55): Write getters and setters for datastore
// TODO(#55): Write getters and setters for datastore

* Methods for securely processing and storing GitHub
* Personal Access Tokens (PATs).
*
* @method
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be @module?

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