Squash commits skip DCO checks

[jmertic]

Steps to reproduce:

1) Create a repo with DCO bot enabled, branch protection on for the master branch with 'Require status checks to pass before merging' enabled and DCO status check required enabled before merged. 2) Add a new file to the repo with a DCO signoff. It will automatically force the user to create a new branch and pull request. 3) In that pull request, the DCO status check should be green or passed. 4) Choose to do a 'Squash and Merge' for the pull request ( see below ).

5) In the commit description, remove any Signed-Off by lines. Then click 'Confirm Squash and Merge'.

Expected behavior: Commit is blocked due to lack of DCO signoff Actual behavior: Commit is successful, and the commit message has no DCO signoff

[jmertic]

@brianwarner @tykeal @caniszczyk - Can you confirm the same on your repos?

[brianwarner]

@jmertic I had a quick look into it, but my understanding is that once you get to the point of merging, there are no more opportunities to fail a status check (which is what enables Probot DCO to block a merge in the first place). Actions can run, but I think that happens after the merge is already complete.

My initial take is that it's one of those things where with effort or malice it's possible to get around the tooling, but most people are going to mash the green button and not proactively strip the signoffs. I've encountered a few other corner cases like this, none of which are major blockers but which could allow a determined individual to work around a legit DCO signoff.

So I definitely understand the issue, but don't see a way to avoid it just yet (but maybe @hiimbex, who is much smarter than me, knows).

That said, it's hacktastic, but I suppose it would be possible to have a second Action run that reverts a commit which sneaks through? I think that might be out of the scope of Probot DCO proper, though.

[jmertic]

@brianwarner I do agree we are likely running into a GitHub limitation here. That all said, seeing more and more opportunities for bypassing checks for DCO signoffs ( like what's attempting to be solved by https://github.com/probot/dco/issues/119 ) is problematic.

Maybe the answer here is being proactive on scanning repos for missing DCO signoffs and cleaning up after the fact ( like what is done in https://github.com/jmertic/dco-org-check ).

GitHub

jmertic/dco-org-check

Script to check all repos in a GitHub org for DCO signoffs - jmertic/dco-org-check

[tykeal]

dco-org-check is fine... but if you rely on that for an operational repo and not one you're taking in then you're going to be constantly chasing this issue.

I've raised the issue directly with GitHub (during one of our customer calls so I don't have an issue to actually point at) that DCO needs to be a first class citizen. Probot DCO is IMO a stop gap / best effort of DCO enforcement until it's supported at the upload / PR requirement level and not left as a validation check.

[tykeal]

Also, @jmertic I have to agree with @brianwarner on this. I don't think Probot DCO is going to be able to stop this sort of behavior. If someone is willfully making these changes and they have the needed rights to merge the change, we can't stop them as long as DCO is not truly something that is enforceable at the repo level and not done via CI style checks.

[jmertic]

Yep - sounds like it's a limitation of GitHub. It would be good to document this behavior at least if that makes sense @hiimbex

[stale[bot]]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

[mynameistechno]

Sorry to resurrect this. @tykeal did you ever get anything concrete from GitHub on this topic?

[tykeal]

@mynameistechno, DCO is still not a first class citizen in GitHub. The only way to avoid this particular issue is if DCO enforcement is something that done as a pre-commit hook. Since us common peasants can't create or mange those we're out of luck. We raise this with our GitHub reps regularly that this is something we need to be a native enforcement option and not a bot enforcement but haven't had any movement :(

[mynameistechno]

Thanks @tykeal for the reply. I'll also add it to the TODO group's GitHub wishlist.

[carlgieringer]

I agree with the previous conclusions that:

1) fully addressing this issue depends on Github support for DCO, and 2) that a determined bad actor can find a way to work around the DCO.

But I do wonder if the process could be improved slightly by improving the suggested squash-and-merge commit message to provide instructions to leave at least one DCO signoff in the commit message. (I don't know if dcoapp has control over Github's autopopulated commit message suggestion, though.)

When a contributor goes to complete a squash-and-merge, they are confronted with what may seem like an intimidating and repetitive list of commit messages and DCO signoffs. Such contributors may not fully understand the mechanics of the DCO and/or they may not fully understand that the commit message that they are creating will become the only record of their contribution when the feature branch is deleted.

For example, below is the autopopulated contents of my commit message when I go to squash-and-merge one of my PRs. I will definitely perform edits to this message, such as removing the duplicate DCO signoffs and removing uninformative commit messages like "Fix checks" or "PR edits". While performing such cleanup, a well-intentioned contributor may accidentally remove all the DCO signoffs, including the last one.

Does dcoapp control the addition of the final DCO signoff (the one after ---------)? If so, could dcoapp add a message like "Important: pease leave the DCO signoff below this line in your final commit message."? Doing so could help well-intentioned contributors not to make a mistake.

* Rename web app route IDs

Signed-off-by: Carl Gieringer <78054+carlgieringer@users.noreply.github.com>

* Convert ContextTrail to TS

Signed-off-by: Carl Gieringer <78054+carlgieringer@users.noreply.github.com>

* Update ContextTrailItem to be Connecting-Entity-based

Signed-off-by: Carl Gieringer <78054+carlgieringer@users.noreply.github.com>

* Factor out context trail fetch into a provider

Signed-off-by: Carl Gieringer <78054+carlgieringer@users.noreply.github.com>

* PR edits and fix creating justifications with extant bases

Signed-off-by: Carl Gieringer <78054+carlgieringer@users.noreply.github.com>

* Fix checks

Signed-off-by: Carl Gieringer <78054+carlgieringer@users.noreply.github.com>

---------

Signed-off-by: Carl Gieringer <78054+carlgieringer@users.noreply.github.com>

[jmertic]

I think this is largely handled with the native DCO support in Github, right?