-
Notifications
You must be signed in to change notification settings - Fork 31
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
base: develop
Are you sure you want to change the base?
Conversation
…n in order to connect to github api
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
To start an HTTPS simple server with a generated self-signed certificate, execute: | ||
|
||
```shell | ||
$ python3 start.py |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
change project dashboard heading Co-authored-by: Ben Henning <[email protected]>
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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 |
There was a problem hiding this comment.
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
?
This pull request includes changes that: