Page MenuHomePhabricator
Create Task
Maniphest T11902

Periods in repository shortnames make the repo unreachable
Closed, ResolvedPublic
Actions

Description

Reproduction Steps:

  • Click Create Repository → Create Git Repository in Diffusion
  • Give the repository a shortname with a period (e.g., test.test)
  • Create Repository

Expected Result:

  • I be taken to https://phabricator.domain.com/source/repo.shortname/manage to edit additional settings and activate the repository

Observed Result:

  • 404 error

I noticed this locally after a user reported the issue over the weekend. We're running

rP74a0e5cd7930ac06013ed6fafc4fd96e7730b40c (Sat, Nov 19)
rARCe17fe43ca3fe6dc6dd0b5ce056f56310ea1d3d51 (Oct 21 2016)
rPHUb4f866bd75ec138397a16337bc5d326e586a6276 (Fri, Nov 18)

I also reproduced this with the above steps on a fresh Phacility instance.

I didn't see this reported anywhere, so apologies if it's a dupe!

Revisions and Commits

rP Phabricator
D16908rPbf1cbc2499c7 Don't let users pick "whatever.git" as a repository short name, make "." work

Related Objects

Event Timeline

faulconbridge created this task.Nov 21 2016, 7:06 PM
jcox added a subscriber: jcox.Nov 21 2016, 8:02 PM

Confirmed this on my local. It seems to be due to D16851, specifically this line: https://secure.phabricator.com/source/phabricator/browse/master/src/applications/diffusion/application/PhabricatorDiffusionApplication.php$104.

Removing the period from the negated regex group fixed it for me, although I'm not sure that is the right solution or if we'd want to enforce no periods on creation.

epriestley added a subscriber: epriestley.Nov 21 2016, 10:57 PM

This is because we now recognize shortname.git in some cases, and it's ambiguous if quack.git is "the repository with shortname quack.git", or "the repository with shortname quack, addressed as a Git repository".

I'm leaning toward jut forbidding periods. These don't seem to be a strong precedent for using periods in GitHub repository names (e.g., node.js is just nodejs), and this generally seems like sort of a bad idea?

Alternatively, we could forbid just .git at the end. This is approximately what GitHub does. But we may have to forbid other stuff down the road.

How saddened would you be by a "no dots in short names" rule?

faulconbridge added a comment.Nov 21 2016, 11:22 PM

(From our own install's perspective) I don't think the would make anyone at all sad. It seemed to be a pretty random decision by one of our newer users while he was playing around with things.

epriestley added a comment.Nov 21 2016, 11:23 PM

Okay, let me ban them for now and if it causes too much of an issue we can try just banning .git at the end.

I should probably have anticipated this but missed it when selecting the range of valid characters.

epriestley added a comment.Nov 21 2016, 11:29 PM

Oh, hrrrm, this is actually more involved since it's not just the .git clone URI that's in trouble. So I can't just ban . without fixing existing short names.

I'll just ban .git instead and hope no one named a repository quack.git since this feature hasn't been available or useful for too long. <_< >_>

chad renamed this task from Periods in repository shortnames make the repo unreachable. to Periods in repository shortnames make the repo unreachable.Nov 21 2016, 11:45 PM
epriestley added a revision: D16908: Don't let users pick "whatever.git" as a repository short name, make "." work.Nov 21 2016, 11:46 PM
epriestley closed this task as Resolved by committing rPbf1cbc2499c7: Don't let users pick "whatever.git" as a repository short name, make "." work.Nov 21 2016, 11:47 PM
epriestley added a commit: rPbf1cbc2499c7: Don't let users pick "whatever.git" as a repository short name, make "." work.
epriestley added a comment.Nov 21 2016, 11:48 PM

I think this should be fixed now. Note that we still have T11165 floating around which may make it look like this is still an issue, by sending you to the wrong place when you hit "Save". That bug is dumb but fairly harmless so I haven't chased it down yet.

faulconbridge added a comment.Nov 22 2016, 1:21 AM

Awesome, thanks @epriestley!

epriestley added a comment.Nov 22 2016, 1:25 AM

No problem, thanks for the report!

Phabricator · Built by Phacility