See also this enormously valuable contribution I made to the Git LFS upstream in connection with T7789 some time ago:

In T12961#230931, @avivey wrote:

So, all three major VCS had the exact same CVE, which was "we invoke ssh command line, don't sanitize input, and don't specify -- anywhere"?

Thanks for the detailed explanations! I should have thought more carefully. Note old Mercurial also fails to do correct shell quoting on Windows (It uses ' where Windows needs "). But Phabricator does not run on Windows, it shouldn't be an issue.

So, all three major VCS had the exact same CVE, which was "we invoke ssh command line, don't sanitize input, and don't specify -- anywhere"?

@indygreg Thanks for the heads up about subrepos -- I would not have otherwise guessed that hg pull might run git.

From this writeup:

The magic incantation I arrived at was slightly modified from one of the hg test cases:

Never mind, I was able to get hg pull -u to interact. I'm going to land, cherry-pick, and hotfix D18390.

I think this is related:
https://www.mercurial-scm.org/wiki/Subrepository#Synchronizing_in_subrepositories

And here's an extension which appears to be aimed at solving this problem, by adding a new command to execute hg pull -u in subrepositories:

Also, although ui.ssh appears inneffective against the [git] and [svn] variants of subrepos (Mercurial does not appear to populate GIT_SSH or SVN_SSH based on the ui.ssh setting), I can't get hg to actually interact with remotes using hg clone --noupdate ... or hg pull -u -- <uri>, which are the only relevant commands we run. I can get it to interact with remotes with hg up or hg clone (without --noupdate).

In the example above, I put malicious content in .hgsub, like this:

The subrepo issue is when .hgsub has malicious content (ex. foo = ssh://-oProxyCommand=touch%20BAR/). It's not related to command line or config files.

I searched for .hgsub files containing URI paths matching ssh://- in working copies stored on disk to better assess the impact of the security issue described in T12961. I did not locate any.

I'm going to cherry-pick rP794e185bf90e (the SSH wrapper stuff) to stable and hotfix production, although I'm not entirely certain hg pull -u -- <uri> is vulnerable.

I also can't get hg pull -u -- <uri> to fetch subrepos, am I just not setting things up correctly? In my current working state, hg up tries to interact with the subrepo remote but hg pull -u -- <uri> (which is what we actually execute) does not.

See also T4416. Removing -u hasn't been a priority because no actual install has expressed interest in it.

That same code I pointed to for Mercurial also seems to perform Git working copy checkouts. Although I can't recall Git's semantics for automatically updating submodules (because I don't use them). It is worth auditing.

Note that Phabricator can manifest Mercurial working directories. See executeMercurialUpdate() in src/applications/repository/engine/PhabricatorRepositoryPullEngine.php. It does this when pulling non-hosted repos. I know this occurs when observing repos. Not sure where else this code is used.

I'll leave this open until I write up the release notes since it deserves a mention (users are still vulnerable if an attacker tricks them into running a suspicious git clone command), but I think we're otherwise unscathed by this.

We also used to have a separate PhutilGitURI which had looser rules, but I removed this in D16100 (June 13, 2016) and all URI parsing now goes through PhutilURI which has the stricter rules.

The theoretical attack here is:

These are slightly more fleshed-out versions of the attack in Mercurial:

git, svn, and hg... wow

The attack is basically:

oh wow

T12956 has another situation where letting MenuItem generate 2+ items may be bad: we want to let a menu item steal the selection, but it's muddy to implement if each MenuItem can return several actual views.

Thanks for the detailed response, I certainly didn't expect it.

I believe the location of the OOM is a little misleading -- the real problem is loadFileData(). This returns the entire file as a string, and will thus:

The new rules on reports are still in flux a bit and extra-muddy for Community members, but I'd generally like to move to a "report + review" model for community reports, something like we currently have for code review. So the general idea is that your bug needs someone to review it and agree with it (e.g., reproduce a bug, or agree that a feature request is a brilliant new idea that makes sense and would be appropriate for the upstream and completely describes a reasonable, generally-experienced root problem) before it comes upstream.

I think this is a real bug. Here's a more complete example of what's going wrong, why it's wrong, and how to reproduce it.

The community forum is a better place for this discussion. You may or may not receive help there, but we're no longer providing general Phabricator support here in the upstream (nor have we for a long time).

I'm having problems with script-and-regex too. I have asked in the community forum but without luck. I’m trying to add phpstan to the arc lint workflow, as explained in the documentation, but I’m unable to capture the error message. Here are my files:

In T7091#111368, @epriestley wrote:

ScriptAndRegexLinter needs to be modernized, but is strictly more powerful than the proposed linter. Something like this will implement your desired lint rule after the linter modernizes:

"todo": {
  "type": "script-and-regex",
  "script-and-regex.script": "grep -n TODO --",
  "script-and-regex.regex": "/^(?P<file>[^:]+):(?P<line>\\d+):/"
},

I would open to accepting this as a bug report if I felt it was deliberately designed to work the way you expect, but was not (like a regression). Or if information was presented that all other repository viewers do it this way. Otherwise I think it's more 'nitpick' or 'feature request'. A cursory look at GitHub, seems they show the full diff as well: https://github.com/phacility/phabricator/commit/5c1e4488dedafda08684b33a8a4786cf687d2811

In T12959#230805, @chad wrote:

In general are you aware we no longer take bug reports or feature requests here?

In general are you aware we no longer take bug reports or feature requests here?

In T12959#230799, @chad wrote:

What does "mysterious" mean.

What does "mysterious" mean.

In T8238#218018, @avivey wrote:

A possible mid-term solution is to use something like using refs which aren't "tags" (Like github's /refs/pulls/xxx); I think we've discussed it didn't like it because T8092 is desired for other things too.

Thanks for the explanation.

Named queries will get a new flag like isGlobal

I'm not proposing that, no. Here's exactly what I'm proposing:

At least, this is what I thought you were first proposing.

Basically the same UX as the home sidebar with home/dashboards.

OH yeah.

I wasn't planning to change the behavior of the dropdown, except that it would begin listing personal filters followed by global/default filters under "Saved Queries".

We're probably saying the same thing but maybe you wanted a simpler workaround first.

PHI29

Where's the other conversation?

I realize there are two conversations on this feature, I thought I was replying to the other thread.

Sorry, when I say "still" I mean like favorites on home.

I think we should still default search the top item.

I think we should still default search the top item. So if admins put "Super special search triggers" as the top item, that is the global default. One a user says OMG NO, and adds their first filter, that becomes their first item and thus their default search.