Open jakerella opened 11 years ago
Sounds like a good idea! Would you be up to contribute that feature?
Just a bit of input here:
// note that this is an Express post handler, but it should not require Express
app.post('/pt-activity-hook', function(req, res){
res.end()
pivotal.parseActivityHookData(req, function (activity) {
console.log("activity: ", activity)
});
});
Would probably make more sense, and simplify how testable the code would be.
Some other ideas:
I can work on it some time, but may be a while. My company is actually working on an FOSS PT cumulative flow diagram tracker (let me know if you're interested and I'll point you to the code when it's ready).
I'm still a bit new to Node and Express... why the res.end()
? Wouldn't you want to respond to the server with a positive (200) status if it was good (and maybe a 401 or 500 if it wasn't)?
As for the other idea and the checking the request, we could check the originating server... but that can be spoofed. :) What I do for my current project is turn it right around - I hit the PT API using the provided token and get two things: 1) I get the latest activities and make sure the received one is in there; and 2) see if that story exists for that account.
I figure that without an actual "verification" process on the PT side this is as good as it gets. Let me know what you think.
The only reason why i res.end() is that I doubt that the PT API will care if you return a 401 or a 500. On the other hand, you can always refer to your log to see the outcome of the call. But yes, for clarity, you could consider waiting until processing is done before sending an HTTP response; but you don't necessarily have to.
The originating server could be spoofed indeed. However, unless the request is signed (which I don't think it is) or authenticated (which also doesn't seem the case), you will be out of luck, and that will be the best you can do.
What you do is technically no safer than working with a list of IP - perhaps only a bit more obscure, but more easily brute-forceable I think, since project and story ID's are in the low 100,000 from what I have seen so far. On the other hand, with node you should be able to retrieve the remote IP by looking at the connection's socket (see http://stackoverflow.com/questions/8102535/remote-ip-from-request-on-http-server), which should not be fakeable (at the very least, if you can send back data successfully, you should be pretty safe).
I see what you're saying about the IP address of the originating server, but my issue there would be that they could change those IPs! Or even just add more servers, requiring a change to the library and all existing instances to break.
As for how I'm doing the verification now, you're right, the activity POST is not signed in any way, but that's not what I'm relying on. When I get an activity, I then hit the PT API and get recent activities for the given project, and I make sure that the activity I received is present. In other words, I make sure that the activity is the same both on my receiving end, and on the PT end. I don't think that could be spoofed - although you could have duplicates.
Does that make more sense? Do you see other concerns about the verification?
According to https://www.pivotaltracker.com/help/integrations, the list is clear but indeed could change without notice. But I still think this would be safer than relying on double-checks (and more efficient) to rely on this. Even if that break, since they have a list of server announced, I think it would be a quick fix to update the list and then go online (and I assume they won't change ALL their servers at once; my experience with GitHub API, for instance, is that they would add a server every once in a while, and we would then see some errors pop up, but most of the calls would make it through).
Ok then... I'll see what I can do on this, but it may be a while. I'll share what I have as I go though. When I do get this going, do you want me to work in a separate branch and issue a pull there?
Great! No rush, if spmeone else needs it more than you do then they just need to code it themselves.
I work by pull requests, so yes, just fork/branch/commit/push/pr so that i xan take a quick look at the code. And let me know if you have any further question.
P.S. when you will do your PR, please paste the link to this thread in your PR comment for reference.
Kind regards, On Jun 4, 2013 5:05 AM, "Jordan Kasper" notifications@github.com wrote:
Ok then... I'll see what I can do on this, but it may be a while. I'll share what I have as I go though. When I do get this going, do you want me to work in a separate branch and issue a pull there?
— Reply to this email directly or view it on GitHubhttps://github.com/Wizcorp/node-pivotal/issues/11#issuecomment-18867197 .
I would like to see an integration point for the PT activity web hook (https://www.pivotaltracker.com/help/integrations#activity_web_hook). The idea would be that Node accepts a post, and then the app passes the request on to this library which processes it, send the response, and makes a callback for processing by the app.
In pseudo-code:
I may try to implement this myself and I'll contribute if anything comes of it.