Hey @Art, should events.json (for Redunda's webhook forwarding) be GET or POST? GET seems logical since it's just getting data, but it has side-effects.

> GET and HEAD methods SHOULD NOT have the significance of taking an action other than retrieval.

says the HTTP spec

Alright, so a POST then?

Practical reason is that GET/HEAD aren't protected by CORS

@NobodyNada I'd go with POST. The spec is a SHOULD NOT, so you can ignore it, but they generally have pretty good reasons associated with them.

Alright, sounds good

For example, the request to show PII is a POST, probably to get CORS protection.

[ metasmoke ] ci/circleci failure on 4913446: Your tests failed on CircleCI

:(

@art does the new pullapprove/merge thing only work in blacklist changes, or will it work with any PR?

@angussidney probably any

You might want MS to only merge PRs by Smokey then

Not sure that I agree. Yeah we can do it when it comes to feedback and blacklists... but things that can affect autoflagging, I'm not sure

Why? Approved PRs are approved

I know what you're saying, @angus, but I don't think it's an issue. PR authors can't approve their own PR's, so this still requires a different code admin to comment-approve the PR.

@ArtOfCode oh, right, that's good

Yeah, I tend to agree with Angus here. Approval is awesome, auto merging actual code changes probably isn't

oh, I don't think the markdown on github_controller:206 is gonna work

probably not

The issue is that most "actual code changes" are blacklist changes

fixed

Wonder if it'd be possible to limit what metasmoke merges to only commits touching only .txt files?

@Undo that's why it should be restricted to changes by Smokey only

Or that

@Undo or, like angus said - only PR's authored by Smokey

Or what you said, which would work better

that should be possible, lemme look at it

[ metasmoke ] ci/circleci failure on 9c937e0: Your tests failed on CircleCI

@JanDvorak Difference is that changes to blacklists will, at most, trigger 3 autoflags on every post that we see. Really bad, but not unrecoverable and solvable with the pallet-of-bricks method. Something touching actual Python code, though, could do something evil like feed metasmoke the same post twice (throw a hash in the URL or something to make it unique) and then we're actually killing stuff.

Latter could be protected against at the metasmoke level, and should be

doesn't need to be malicious either. Detecting each revision is already a thing.

That'll already be stopped by github.com/Charcoal-SE/metasmoke/blob/master/app/models/…

until Smokey starts including the revision number in the URL

Why would we do that?

To point MS and FIRE to the revision it's actually reporting rather than always the newest one, perhaps

[ metasmoke ] ci/circleci failure on c678df3: Your tests failed on CircleCI

Maybe. It's a somewhat fragile check, but it works for now

Ultimately, folks with privileges can break stuff. That's why they're privileges. Goal is to reduce the breakage potential without impacting productivity.

@SmokeDetector test failure, not a real failure. Deploying, cc @ArtOfCode

@Undo hang on

yeeeeeeh

okay

"works for now"? xkcd.com/1695 :-D

Got a change making it only merge Smokey's PRs coming in a moment

kk

derps

Pushed, @Undo. Looks like the test failure is something to do with the duplicate-rejection.

yeah, fixture issue

ah

Okay, deploying

[ metasmoke ] ci/circleci failure on 005aa57: Your tests failed on CircleCI

!!/watch-keyword more\.test\.testy\.test

done

@Lt.A.Code You don't have code privileges, but I've created a pull request for you.

Oh, did I need to set up that webhook?

yep

Should be ready to go now @ArtOfCode

cool

!!/watch-keyword one\.more\.test

@Lt.A.Code You don't have code privileges, but I've created a pull request for you.

CI on 35aa0cf succeeded. Message contains 'autopull', pulling...

[ SmokeDetector ] SmokeDetector started at rev a97958b (metasmoke: Merge pull request #816 from Charcoal-SE/auto-blacklist-1496616821.725452) (running on ArtOfCode/EC2)

looks like that works

Restart: API quota is 12404.

nice

CI on d866f79 succeeded. Message contains 'autopull', pulling...

[ SmokeDetector ] SmokeDetector started at rev d866f79 (ArtOfCode-: Revert "Auto watch of one\.more\.test by Lt. A. Code --autopull") (running on ArtOfCode/EC2)

Restart: API quota is 12378.

[ metasmoke ] ci/circleci failure on bf8296a: Your tests failed on CircleCI

CI hates you guys lol

How should I store the request headers in the database? I'd like to encode them as a JSON dictionary when delivering them to the bot, so should I:

Encode them in JSON immediately upon receiving the request, store them in the database as a string, and inject the string into the JSON response

Encode them some other way (like newline-separated), and then convert them to JSON when delivering them to a bot

Or create a separate model to represent a header, and each Event has_many headers?

@NobodyNada this one, I'd say. Headers to JSON isn't intuitive, but this works: request.headers.map { |k, v| v.is_a?(String) && k.start_with?('HTTP') ? [k.gsub('HTTP_', '').titleize.gsub(' ', '-'), v] : nil }.compact.to_h.as_json

Be careful there, though - GitHub webhook headers contain secrets that you don't want to reveal.

@ArtOfCode OK, will do!

@ArtOfCode The headers aren't going to be made public; my current plan is to pass the headers to the instances so they can perform whatever validation is necessary. Does that sound okay?

@NobodyNada That leaves an abuse vector for bot operators to pretend to be GitHub and pass events to other bots.

@ArtOfCode right, but is that bad? Events are per-bot, so operators can't access events of other bots

so if they're able to grab headers, they've already got access to at least one instance anyway

@NobodyNada No, but if I run Bot A, then I can pretend to be GitHub and send an event to Redunda that will be sent to Bot B. Potential injection risk, certainly loss of integrity.

@ArtOfCode But I can do that with webhooks even if I don't run Bot A -- that's why the headers are used for validation

Different bots will have different secrets, so I can't fake a GitHub event unless I know a bot's token

Depends. If events are going to Redunda, and Redunda is forwarding them to bots, you might have one token for all events, and I can spoof events for bots other than mine.

Unless you add a webhook_token attribute to the Bot model, I guess - that would work