We’re concerned about this as well at GitHub. We don’t link directly to the Google Analytics script, which could be updated at anytime. Instead we host our own script version that’s locked down with CSP and SRI. We still allow XHRs to the Google Analytics origin to report the data but the script code itself can’t be changed without an internal security review.
Github has one of the best application security teams in the industry.
(I have no relationship with Github other than that I am a customer and have watched them steadily hire some of the best people I know in the industry).
Thank for the kind words. As Neil noted below, we would like to lock things down even further by proxing all data that is sent to Google Analytics. And, as a bonus, it would remove the last destination host for content exfil attacks that we know of. Our strict CSP policy has been a nice win, but the strictness has made it that much more clear that allowing nearly any third party sites, even for innocuous things such as images/xhr, isn't ideal. And, we are always on the lookout for more bypasses: https://bounty.github.com/targets/csp.html.
Oh nice, tell me more about proxying to GA, do you just change some hostname config in the ga universal js snippet to point to your forwarding proxy? how does GA know end user IP as it will be your proxy forwarders normally?
And this can be ratcheted down further by leveraging something like the measurement protocol. It would eliminate the 3rd party calls/code in the browser while giving GitHub the ability to anonymize the source (e.g. IP address, user agent, etc.). Twitter does this with some of their 3rd party integrations.
> we host our own script version that’s locked down with CSP
Excuse me, is there any article about this, or maybe some pointer where one could get a GA script that doesn't need `script-src data:` (or eval or similar insanity) in the CSP?
I've tried to add CSP for a page that has GA (no other external deps) and it seemed to deliver some scripts from base64-encoded data URI. I haven't researched what exactly it does, but suppose it was the unpacker inserting code that way, instead of using eval. Could be wrong, though, but the only external JS reference was analytics.js, and when testing in Firefox 57 CSP had complained about script with a data URI.
Hey, I'm one of the devs for this new PR stuff. This flow is something we're hoping to improve as well. Force push support is pretty second class right now, any previous commits and discussion basically gets lost. It sucks.
So we already use a internal refspec for tracking the latest PR HEAD. You can actually manually fetch this under `refs/pull/123/head`. However, this is only the latest HEAD, not any previous histories.
My idea was to expand this to include tracking all historical HEAD refs. We came up with a clever/hacky idea to string together a chain of dummy commits pointing at all the historical HEADs. This fake commit chain would only be used internally, but it would ensure git's gc reachability checks don't sweep up any old PR HEADs. Its an alternative to creating a new ref for each previous HEAD or maintaining text reflogs for an entire repo network. Both have some performance issues for larger repos.
Its something we're still working on, but it still helps to vocalize your support for wanting improved force push support. :D
> Its something we're still working on, but it still helps to vocalize your support for wanting improved force push support. :D
YES! I wish for that feature everyday. Gerrit does it exactly right.
Related to this, I really, really hope that you will consider displaying commits
in a PR in the correct order, suppose that I have three commits on top
of master:
a->b->c->master
If I git rebase -i master and reorder my commits so that they look like this:
c->a->b->master
Then the commit list in the PR will still be displayed in chronological order
instead of DAG order, this is very confusing! The only workaround I've found is
to amend every commit in my PR so that the timestamp order match the DAG order,
this isn't very fun to do.
By the way, the post explicitly says:
> Some teams choose to use a commit-by-commit workflow where each commit is treated as the unit of change and is isolated for review.
I'm glad that you're acknowledging that. But then why do comments on commits
still get hidden when they correspond to an outdated diff? I would love to be
able to comment on individual commits, but if half my comments are hidden
because they correspond to lines that don't match the current diff, then I'm not
going to do it for fear of my comments being missed. You should not hide
important information that I'm trying to communicate!
Here's a better way to do it: Add a "Done" button like Rietveld/Gerrit for every
comment (this is a very useful feature on its own) and hide comments _if and
only if_ the "Done" button has been clicked.
Anyway, I'm glad that people are working on this stuff and that they're
listening to the community, thank you!
No, if you comment on the commits themselves rather than the PR 3rd tabs, they disappeared (at least before this change). The only place they were left is your emails, an hardcoded URL or your feed. This was (is?) bad...
they do disappear in one sense: the link to them sent out in the email telling me about them goes away. And if I have a bunch of collapsed comments, it's annoying as hell to find the right one. This is usually an issue when the developer says something like:
"ok, I fixed that, but what do you think about xyz?"
Now I have to go to the comments and expand 35 of them until I find the right one so I can respond to their question.
It's also sometimes the case that changing a line is not the same as addressing a comment, or maybe it only addresses part of a comment, or maybe they just changed whitespace because now all that code you commented on is inside an if...
I also wrote a Gerrit to GitHub PR proxy... I should ask $dayjob about open-sourcing it. And I seem to recall there's another as well implemented as a gerrit plugin.
bots like these seem to be a natural way to work around github's limitations. realistically, they won't be able to support everyone's use case, but they can provide a platform where lots of different tools can be integrated together.
unfortunately, bots like these are very annoying to write right now. at least one reason is that there is no github event for editing comments. if you try to use webhook events to mirror comments made in github to another code review tool, you will quickly drift out of sync if those comments are edited in github. instead, you have to implement polling, which comes with it's own set of challenges.
also, in general, the event formats are pretty inconsistent. as a random example, in the push event the repo owner only includes name and email, while in the commit comment event the repo owner includes all sorts of information about stars, followers, etc... there are inconsistencies like that all over the place, which means you might need to implement additional API requests for some events and not others.
on a somewhat related note, i've seen a lot of speculation in the open-source community that the reason open-source is getting so little attention from Github is that they're focusing on enterprise customers. i can say that as a somewhat-large enterprise customer, i have yet to get a single issue addressed. examples like above (add a comment-edited and PR-edited event) have been in the ask 6 months or more.
Count me in on those who really want better force push support. Losing all the conversation on old commits sucks, but what also sucks is not being able to see a diff between the old and the new versions. If GitHub can track the history of HEAD for the PR and show diffs between them that would be excellent.
The dummy commit approach you outlined sounds like a good idea. It might be nice to expose that externally similar to how `refs/pull/123/head` and `refs/pull/123/merge` work, so that way we can handle force-pushes to PRs better in our own tooling. I'd suggest something like `refs/pull/123/history`.
Even with these new changes are comments on commits that are force pushed over still lost? It looks like when commenting on individual commits now they appear as if you commented on the whole changeset itself.
I have a somewhat shonky shell script which maintains a fast-forward branch that tracks the HEAD of a rebasing branch over time. I use it for keeping the history of a set of patches.
Part of a good review should be to review how you decided to split your PR into separate commits (does every commit have a single purpose, or did you sneak unrelated changes in a commit? Could the commit message be improved?). Furthermore, sometimes you have to rebase your PR on top of master (for example, because things changed in master too much, or because your PR depends on a patch that was recently accepted in master to actually work), this shouldn't make review comments disappear.
Personally, I am for a hard fail OCSP option in HSTS or certificate plus OCSP stapling. Default to soft fail with a warning message for now. Remember captive portals can use OCSP stapling too.
This looks so great. I just looked at the current print.css styles today on a README and they look really awful comparatively.
There might be some JS hacks that could be done on github.com itself to render a more stylized view when ⌘+P is pressed. Avoiding the need for a special "print this page" button.
"both" referring to selector-set and jquery patch. Its wishful thinking that browsers could expose a similar api as selector set that they already use for matching css rules.
Most the issues under "bower is a mess" are being discussed in the https://github.com/bower/bower.json-spec repo. I have a number of proposals to improve the "main" directive and other require paths.
There is a strong dissenting opinion in bower community that it should basically be just like "npm" and none of these structured require build tool hints should be encouraged. It'd be great if others in support of the current Sprockets bower integration or this Rails Asset tool to voice their opinion and real world use asset for improving the state of the bower.json manifest.
While we all know that ruby gems have a couple of issues I think one thing was done exactly the way it should be - somehow enforced standard structure: NAME.gemspec, bin, ext, lib, lib/NAME.rb. I don't know if this is specific to only ruby community but there is at least some level of consistency which makes everyone lifes easier.
I will have to carefully read all arguments agains enforcing such structure in bower.
