-
-
Notifications
You must be signed in to change notification settings - Fork 64
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 webhooks for instant package updates - Fix #10 #414
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
# Triggering version updates over HTTP | ||
|
||
To queue an update of your package you can use the `POST /api/packages/:packageName/update` endpoint. | ||
|
||
## `POST /api/packages/:packageName/update` | ||
|
||
Queues an update for the specified package. | ||
|
||
Query params: | ||
`secret`: string (optional) provide the secret as query | ||
`header`: string (optional) provide which header is used to check the secret (must start with X-) | ||
|
||
Body params: (application/x-www-form-urlencoded, multipart/form-data or application/json) | ||
`secret`: string (optional) provide the secret as body param | ||
|
||
## `POST /api/packages/:packageName/update/github` | ||
|
||
Queues an update for the specified package. Compatible with GitHub webhooks and only triggers on `create` events. Must pass secret as query param and not in GitHub webhook settings. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, you still need to save the GitHub secret here to verify that it's the actual owner (though this is theoretically not needed). Also, we need to rate-limit this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to implement github properly at first, but vibe.d with rest interface doesn't give me a way to get the full request body as string, which I need for the SHA1 HMAC verification with the GitHub secret field (at least I didn't find any way) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also I don't think any of the other dub API currently has rate limiting yet. This will only queue a recheck and is basically the same as spamming the "check for updates" button on your project page There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Don't reinvent the wheel -> https://github.com/dlang/dlang-bot There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fair point, but by providing hooks we make it a lot easier for people to spam / run us into the rate-limits of GitHub. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I more accurately meant I wanted to support the "Secret" field on GitHub which sends a Signature of the body text as SHA1 HMAC with the Secret as password, but with vibe.d the rest interface implementation already ate the body so I couldn't access the raw string from code to hash. For hashing the stdlib provides everything needed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding the rate limiting, if we have web hook support, I would propose to stretch the polling over a much larger period than now (e.g. 6 hours vs. 30 minutes), so that it merely acts as a fallback in case the web hooks fail. This should reduce the risk of running into the rate limit considerable when compared to the current state. Non-webhook repositories could be kept at the 30 min. polling interval, of course. Regarding the signature issue, I believe this should work using an |
||
|
||
## `POST /api/packages/:packageName/update/gitlab` | ||
|
||
Queues an update for the specified package. Compatible with GitLab webhooks and only triggers on `tag_push` events. The secret is specified in the GitLab control panel. | ||
|
||
Calls `POST /api/packages/:packageName/update` with `header=X-Gitlab-Token` query param after hook parsing. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,6 +96,18 @@ interface IPackages { | |
Json getInfo(string _name, string _version, bool minimize = false); | ||
|
||
Json[string] getInfos(string[] packages, bool include_dependencies = false, bool minimize = false); | ||
|
||
@path(":name/update") | ||
string postUpdate(string _name, string secret = ""); | ||
|
||
@path(":name/update/github") | ||
@headerParam("event", "X-GitHub-Event") | ||
@queryParam("secret", "secret") | ||
string postUpdateGithub(string _name, string secret, string event, Json hook = Json.init); | ||
|
||
@path(":name/update/gitlab") | ||
@headerParam("secret", "X-Gitlab-Token") | ||
string postUpdateGitlab(string _name, string secret, string object_kind = ""); | ||
} | ||
|
||
class LocalDubRegistryAPI : DubRegistryAPI { | ||
|
@@ -176,6 +188,52 @@ override { | |
.check!(r => r !is null)(HTTPStatus.notFound, "None of the packages were found") | ||
.byKeyValue.map!(p => tuple(p.key, p.value.info)).assocArray; | ||
} | ||
|
||
@before!updateSecretReader("secret") | ||
string postUpdate(string _name, string secret = "") | ||
{ | ||
if (!secret.length) | ||
return "No secret sent"; | ||
|
||
string expected = m_registry.getPackageSecret(_name); | ||
if (!expected.length || secret != expected) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use constant-time comparison |
||
return "Secret doesn't match"; | ||
|
||
m_registry.triggerPackageUpdate(_name); | ||
return "Queued package update"; | ||
} | ||
|
||
string postUpdateGithub(string _name, string secret, string event, Json hook = Json.init) | ||
{ | ||
if (event == "create") { | ||
return postUpdate(_name, secret); | ||
} else if (event == "ping") { | ||
enforceBadRequest(hook.type == Json.Type.object, "hook is not of type json"); | ||
auto eventsObj = *enforceBadRequest("events" in hook, "no events object sent in hook object"); | ||
enforceBadRequest(eventsObj.type == Json.Type.array, "Hook events must be of type array"); | ||
auto events = eventsObj.get!(Json[]); | ||
|
||
foreach (ev; events) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can simplify the eventsObj to events and just iterate over Json here |
||
if (ev.type == Json.Type.string && ev.get!string == "create") | ||
return "valid"; | ||
|
||
string expected = m_registry.getPackageSecret(_name); | ||
if (expected.length && secret.length && secret == expected) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use constant-time compare |
||
m_registry.addPackageError(_name, | ||
"GitHub hook configuration is invalid. Hook is missing 'create' event. (Tags or branches)"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this test code will never be run if configuration is valid |
||
|
||
return "invalid hook - create event missing"; | ||
} else | ||
return "ignored event " ~ event; | ||
} | ||
|
||
string postUpdateGitlab(string _name, string secret, string object_kind) | ||
{ | ||
if (object_kind != "tag_push") | ||
return "ignored event " ~ object_kind; | ||
|
||
return postUpdate(_name, secret); | ||
} | ||
} | ||
|
||
private: | ||
|
@@ -187,6 +245,18 @@ private: | |
} | ||
} | ||
|
||
private string updateSecretReader(scope HTTPServerRequest req, scope HTTPServerResponse res) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this needs a better name |
||
{ | ||
string header = req.query.get("header", ""); | ||
if (header.length) | ||
return req.headers.get(header); | ||
|
||
string ret = req.contentType == "application/json" ? req.json["secret"].get!string : req.form.get("secret", ""); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably want to tryIndex the JSON |
||
if (ret.length) | ||
return ret; | ||
|
||
return req.query.get("secret", ""); | ||
} | ||
|
||
private auto ref T check(alias cond, T)(auto ref T t, HTTPStatus status, string msg) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -239,7 +239,7 @@ class DubRegistry { | |
.map!(pack => getPackageInfo(pack, flags)); | ||
} | ||
|
||
auto getPackageInfo(DbPackage pack, PackageInfoFlags flags = PackageInfoFlags.none) | ||
PackageInfo getPackageInfo(DbPackage pack, PackageInfoFlags flags = PackageInfoFlags.none) | ||
{ | ||
auto rep = getRepository(pack.repository); | ||
|
||
|
@@ -261,6 +261,8 @@ class DubRegistry { | |
} | ||
if (flags & PackageInfoFlags.includeErrors) | ||
nfo["errors"] = serializeToJson(pack.errors); | ||
if (flags & PackageInfoFlags.includeAdmin) | ||
ret.secret = pack.secret; | ||
|
||
ret.info = nfo; | ||
|
||
|
@@ -429,6 +431,38 @@ class DubRegistry { | |
return m_db.getPackageLogo(pack_name, rev); | ||
} | ||
|
||
void addPackageError(string pack_name, string error) | ||
{ | ||
m_db.addPackageError(pack_name, error); | ||
} | ||
|
||
void clearPackageErrors(string pack_name) | ||
{ | ||
m_db.clearPackageErrors(pack_name); | ||
} | ||
|
||
void unsetPackageSecret(string pack_name) | ||
{ | ||
m_db.setPackageSecret(pack_name, null); | ||
} | ||
|
||
string getPackageSecret(string pack_name) | ||
{ | ||
return m_db.getPackageSecret(pack_name); | ||
} | ||
|
||
void regenPackageSecret(string pack_name) | ||
{ | ||
import std.ascii : lowerHexDigits; | ||
import std.random : uniform; | ||
|
||
char[24] token; | ||
foreach (ref c; token) | ||
c = lowerHexDigits[uniform(0, $)]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: use secure random instead of uniform |
||
|
||
m_db.setPackageSecret(pack_name, token[].idup); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can just allocate in the first place instead of stack allocating & idup |
||
} | ||
|
||
void updatePackages() | ||
{ | ||
logDiagnostic("Triggering package update..."); | ||
|
@@ -733,6 +767,7 @@ struct PackageVersionInfo { | |
struct PackageInfo { | ||
PackageVersionInfo[] versions; | ||
BsonObjectID logo; | ||
string secret; | ||
Json info; /// JSON package information, as reported to the client | ||
} | ||
|
||
|
@@ -743,6 +778,7 @@ enum PackageInfoFlags | |
includeDependencies = 1 << 0, /// include package info of dependencies | ||
includeErrors = 1 << 1, /// include package errors | ||
minimize = 1 << 2, /// return only minimal information (for dependency resolver) | ||
includeAdmin = 1 << 3, /// include package admin information such as secrets | ||
} | ||
|
||
/// Computes a package score from given package stats and global distributions of those stats. | ||
|
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.
Hmm, what's the use case for this? Isn't this prone to be over-used/falsely used?
(I'm referring to the API method itself)
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 wanted to make sure it's easy to incoperate this into any service you might be using for webhooks, so I'm offering to make the API callable with a secret per query string, body param or per HTTP Header.