Currently, we have robust GitHub token validation logic. However, at a
type level, usage of this logic is unenforced, so many places in the
codebase don't use validation; most crucially, the `Common.githubToken`
method doesn't, which means that the CLI doesn't validate GitHub tokens.
Instead, `Common.githubToken` currently provides a deceptive signature:
`function githubToken(): string | null`
One might reasonably think that the presence of a string means that
there is a GitHub token, and that you can test `if (token != null)`.
However, a command line user can easily provide an empty string:
`SOURCECRED_GITHUB_TOKEN=null node bin/sourcecred.js load ...`
In this case, the user was trying to unset the GitHub token, but this
actually provides a string-y GitHub token, so at a type level, it looks
like a GitHub token is present.
No more! This commit adds `opaque type GitHubToken: string = string` in
the `github/token.js` module. Since the type is opaque, it only has one
legal constructor: the `validateToken` method in `github/token.js`. The
functions that actually use the token have been updated to require this
type. Therefore, we now enforce at the type level that every usage of a
GitHub token needs to be validated, ensuring that we no longer confuse
empty strings for valid GitHub tokens.
Note that making GitHub token an opaque subtype of string
(`GithubToken: string`) is important because it means that consumers can
still pass or store the token as a string; however, no fresh ones can be
constructed except by the validator.
Test plan: Implementation-wise, this is a simple refactor; `yarn test`
passes.
We defined a DiscourseQueries interface, intended as a subset of
the Discourse plugin's MirrorRepository methods. This subset is
used by the Initiatives plugin to source Iniaitive data.
We're now adding the new methods it needed to the MirrorRepository.
In the early days of the project, we used GitHub repository ids as the
core way of identifiying projects. This was a weird choice, since it's
so specific to the GitHub plugin. In #1238 we added a (theoretically)
agnostic type in `Project`, although in practice it's still pretty
coupled. Still, it will be best to move the `RepoId` type out of `core`
and to the GitHub plugin where it belongs.
This leaves a few "awkard" imports from plugin code, (e.g. in the api
module), but generally the places that are importing `RepoId` were
already importing stuff from Discourse and Identity plugins. In either
case, I'd rather have the awkwardness of depending on the RepoId in core
places be obvious (from the dependency on plugin code) rather than
giving a false appearance that RepoIds are really a core concept.
Test plan: `yarn test` passes.
After we wrote our Docker jobs, the cache_from was added to
the orb. This is shorter and less error prone because we don't
need to provide the identical arguments twice.
The change was made in
https://github.com/CircleCI-Public/docker-orb/pull/27
by @vsoch. And was released as of v0.5.15 of the Docker orb.
The change updates to the latest version of the orb currently
available.
Fixes#1512
In contrast with our previous "tags only" deploy job, this
configuration makes sure all it's preceding jobs are also
set to a "tags only" filter in order to run.
Note, adding support in this single function doesn't solve some of the greater issues with HTTP/HTTPS. Because the protocol is included in the node addresses, converging nodes or canonicalizing on either protocol would be important for instances that support HTTP. That problem is outside of scope for the reference detector though.
Summary:
This commit adds a simple Python server for connecting the output of
`yarn api` (or `yarn api --watch`) to an observable notebook. We need a
custom server rather than just `python3 -m http.server` to send CORS
headers properly. This server enables a very tight loop from editing
SourceCred core code on your local filesystem to seeing live updates in
an Observable notebook, with latency on the order of one second.
Test Plan:
Run `yarn api --watch` in the background. Launch the new API server.
Navigate to <https://observablehq.com/demo>. Copy the two paragraphs of
Observable code from `scripts/serve_api.py` into _separate_ Observable
cells, and execute them. Note that `myGraph` becomes a valid SourceCred
graph. Modify `src/core/graph.js` to add `this._aaa = 123;` to the top
of the `Graph` constructor. Re-execute the first Observable cell (the
one that loads the SourceCred module), and note that `myGraph` updates
to include the new `_aaa` attribute:
![Screenshot of Observable notebook after test plan][ss]
[ss]: https://user-images.githubusercontent.com/4317806/71958748-dddf8680-31a5-11ea-9016-5df76ceeea46.png
wchargin-branch: api-server
Summary:
This re-packages the build for the internal APIs exposed under #1526 to
be more browser-friendly. Removing `target: "node"` (and adding an
explicit `globalObject: "this"` for best-effort cross-compatibility) is
the biggest change from the backend build; removing all the extra
loaders and static site generation is the biggest change from the
frontend build.
This build configuration is forked from `webpack.config.backend.js`.
Test Plan:
Run `yarn api`, then upload the contents of `dist/api.js` to an
Observable notebook and require it as an ES module. Verify that the
SourceCred APIs are exposed: e.g., `sourcecred.core.graph.Graph` should
be a valid constructor.
wchargin-branch: api-build
The `pagerankGraph` module was an attempt to do a better job of
co-ordinating the data needed to run Pagerank, by wrapping the Graph
class alongside context on edge weights, etc. However, it was obsoleted
by work on TimelineCred. Thus, we can remove it entirely. I intend to
make another attempt at collecting all the data needed for cred analysis
in a way that doesn't couple with plugin code, and this time it will be
timeline-aware.
Test plan: `yarn test`
Summary:
For convenient import by scripts and Observable notebooks that want to
use SourceCred code outside its normal build system. We export a subset
of the codebase, including some core data structures and algorithms and
also some plugin metadata, but no plugin loading code.
To build, run `yarn backend` (or `yarn backend --watch`), then grab the
new `bin/api.js` file.
Test Plan:
Sample usage, with normal Node:
```javascript
const {
core: {
graph: {Graph, NodeAddress, EdgeAddress},
},
} = require("./api").default;
function node(address) {
return {
address,
description: "blurgh",
timestampMs: -1,
};
}
const g = new Graph();
g.addNode(node(NodeAddress.fromParts(["people", "alice"])));
g.addNode(node(NodeAddress.fromParts(["people", "bob"])));
g.addEdge({
address: EdgeAddress.fromParts(["friendship"]),
src: NodeAddress.fromParts(["people", "alice"]),
dst: NodeAddress.fromParts(["people", "bob"]),
timestampMs: 0,
});
console.log(require("json-stable-stringify")(g));
```
This prints a valid graph JSON object.
wchargin-branch: api-bundle
Before we added the concept of "SourceCred Projects", we tracked cred
instances via their GitHub repostiory id. The replacement for this
system was added in #1238, I missed the RepoIdRegistry in the cleanup.
This commit removes all code pertaining to the now-obsolete
RepoIdRegistry.
Test plan:
- `yarn test --full` passes
- manual inspection of `yarn start`; it still loads properly
- manual inspection of the output for build_static_site.sh
- `git grep repoIdRegistry` returns no hits
Summary:
PRs created from forks don’t have credentials when running CI. This
commit causes the `test-full` job (which requires credentials) to fail
fast with a helpful error message.
Test Plan:
Push distinct versions of this commit to a fork and to the main
repository, and open pull requests for each. Note that the tests pass
from the main repository, but fail with a nice message from the fork:
![Screenshot of expected fast-fail behavior][ss]
The “team member pushes to trusted branch” workflow has already been
successfully exercised for #1521.
[ss]: https://user-images.githubusercontent.com/4317806/71707839-b782ab00-2da1-11ea-8aa9-7d8720538a87.png
wchargin-branch: forked-pr-fail-fast
See #1512 for full context.
Short explanation:
Because the job wants to run only on tag pushes, but requires
the `test` job (which doesn't run on tag pushes), the job
will never run.
Gets the username of a user, if it exists.
Helpful for fixing capitalization issues such as #1479,
and verifying the user exists for reference detection.
Previously both node versions would share the same cache.
This caused one of the two versions to always rebuild
the `better-sqlite3` package, costing about 1 min per job.
Now we're using a different cache key for each version,
rebuilding a cached `better-sqlite3` should no longer be
necessary.
The TranslatingReferenceDetector is an abstraction particularly useful for the
Initiatives reference detector. Which should use the Discourse reference
detector as it's base and translate the node address of the returned discourse
topic to the initiative's node address.
The current reference detection implementation internal to the GitHub plugin
uses a map similar to this. This class being near to that makes it easy to adopt.
It's also very simple to use for tests.
The core declaration of the ReferenceDetector interface.
Reason I'm adding an index.js file is to allow (core) classes that implement
this interface to have separate files, while keeping redundancy out of the
import statements.
Summary:
Contributors who open PRs from a fork will need to have their commits
“blessed” by a core team member before the `test-full` CI job will run
successfully. This commit explains that to ward off any confusion.
Test Plan:
This workflow was recently exercised for #1521, successfully.
wchargin-branch: contributing-test-full
Creation of new Project instances is spread out across the code.
So whenever there's a change in it's format, the PR is cluttered
with adding a logical default value in many places. It means
our default values might be inconsistent as well.
For example #1385 adds many `identities: [],` lines.
A similar situation would happen with the planned Initiatives
plugin, adding many `initiatives: null,` lines.
Using this function we can manage what default values to add
from a central place. Avoiding noise and code churn.
This creates better flow type coverage for the upgrading
from older Project types feature.
Note projectFromJSON's function signature changes like
this:
- (Compatible<Project>) => Project
+ (Compatible<any>) => Project
And that makes sense, because we use this function to
validate an object we parsed from JSON at runtime. It
could actually be anything.
Added benefit is that is makes writing unit tests possible.
Because now will flow not throw a type error when we provide
something other than Compatible<Project> as input, to test
upgrading or validation functionality.
Note that the underlying utility fromCompat already uses
Compatible<any> for the same object.
The README explains how to set the SOURCECRED_GITHUB_TOKEN, but later in
the Docker section. People who aren't using Docker will follow the
initial installation instructions. This commit adds the instructions to
set that up when users first install and set up SourceCred.
The eslint no-constant-condition rule disallows while(true) loops,
since the true is a constant condition. However, I find the allowed
alternative (`for (;;)`) less readable, so I am adding the sub-rule that
allows constant conditions for loops.
Test plan: A followon commit uses a while(true) loop, and, assuming this
patch is applied, it does not result in a lint error.
Co-authored-by: Robin van Boven <497556+Beanow@users.noreply.github.com>
Note, unless you used the SourceCred Docker image's bundled
npm or yarn to install your own package.json dependencies,
you were not vulnerable. Otherwise the same risk applies as
[in this NPM blog][1].
You can patch the vulnerability by using the latest Docker image
using `docker pull sourcecred/sourcecred` as soon as this commit
is included in the latest release.
## Commit details
In a [recent security issue][1] found in NPM and Yarn, handling
binary file installation has changed. Quoting from there:
> The bin script linking libraries in use in npm v6.13.4 were
> updated such that, when installing binary entries of top-level
> globally installed packages, they will only overwrite existing
> binary files if they are currently installed on behalf of the
> same package being installed. For example, npm install –global
> foo could overwrite /usr/local/bin/foo if and only if
> /usr/local/bin/foo is currently a link to a previously installed
> version of foo.
In our case, we specifically want this behavior in our Dockerfile.
The node:12 base image comes with an NPM and Yarn version installed.
We're using npm i -g yarn@<version> to upgrade the yarn installation
to a predictable minimum, should we have an older version from the
base image. But since they're from different installation sources,
it causes an error as it would overwrite the yarn binary that wasn't
previously owned by npm install.
Our own package.json or yarn.lock did not appear to have any risk
of exploitation. However since we bundle our image with npm and yarn,
people using our image could in theory use it to install their own
packages. Meaning we should include the fixed npm and yarn versions
to protect users in such a scenario.
[1]: https://blog.npmjs.org/post/189618601100/binary-planting-with-the-npm-cli
* chore(package): yarn upgrade
Updates all packages within version range.
* Bugfix update stacktrace matching code
The stacktrace has changed, most likely due to
a babel plugin updating. It now seems based on
the name of the `handlingErrors` argument
instead of the variable name storing the
anonymous function.
* Bugfix update react-router patch version
By updating the react packages, warnings were
logged about unsafe componentWillMount usage.
These warnings tripped a unit test.
react-router was the cause of these, so this
update avoids getting the warnings.
- Have "topic" reflect actual method name.
- Add missing 403 and 429 test for likes.
- Preemptively change method used for headers,
as .post will be obsolete after refactor.
This extends the MockFetcher in the tests
to provide new semantics update mode 2 relies on.
They're based on the below changes to the Fetcher:
- add categoryId and bumpedMs to Topic data #1454
- make topicWithPosts fetch all posts #1455
- add categoryDefinitionTopicIds to fetcher #1456
- implement topicsBumpedSince in fetcher #1457
Particularly because the addition of two new concepts
(categories and category definition topics), the API of
the MockFetcher got rather convoluted. This refactor
makes it behave a lot more like you'd be familiar with
within Discourse.
Such as, creating a topic creates it's opening post
as a side effect. Instead of a post with an unknown
topic ID creating a topic as a side effect.
And creating a category creates it's category
definition topic as a side effect.
Also, we're being a lot more explicit, using objects
instead of positional arguments.
This is to prepare for mode 2 being tested side-by-side.
The normalizeMode1Topics function enforces bumpedMs is not
updated for mode 1 tests.
Additionally describe "update semantics" is redundant,
as the mirror has no other function than update.
Dandelion Mané
Robin van Boven
William Chargin
burrrata
greenkeeper[bot]
