1. 14 Jan, 2020 1 commit
  2. 13 Dec, 2019 1 commit
    • epriestley's avatar
      Fix an XSS issue with certain high-priority remarkup rules embedded inside... · 54bcbdab
      epriestley authored
      Fix an XSS issue with certain high-priority remarkup rules embedded inside lower-priority link rules
      See <https://hackerone.com/reports/758002>. The link rules don't test that their parameters are flat text before using them in unsafe contexts.
      Since almost all rules are lower-priority than these link rules, this behavior isn't obvious. However, two rules have broadly higher priority (monospaced text, and one variation of link rules has higher priority than the other), and the latter can be used to perform an XSS attack with input in the general form `()[ [[ ... | ... ]] ]` so that the inner link rule is evaluated first, then the outer link rule uses non-flat text in an unsafe way.
      Test Plan:
      Tested examples in HackerOne report. A simple example of broken (but not unsafe) behavior is:
      [[ `x` | `y` ]]
      Differential Revision: https://secure.phabricator.com/D20937
  3. 10 Dec, 2019 1 commit
  4. 26 Nov, 2019 1 commit
    • epriestley's avatar
      Extend Config to full-width · 33c534f9
      epriestley authored
      Ref T13362. Some applications moved to fixed-width a while ago but I was generally unsatisfied with where they ended up and have been pushing them back to full-width.
      Push Config back to full-width. Some of the subpages end up a little weird, but this provides more space to work with to make some improvements, like makign `maniphest.statuses` more legible in the UI>
      Test Plan: Grepped for `setFixed(`, updated each page in `/config/`. Browsed each controller, saw workable full-width UIs.
      Maniphest Tasks: T13362
      Differential Revision: https://secure.phabricator.com/D20925
  5. 25 Nov, 2019 1 commit
    • epriestley's avatar
      Implement "PolicyInterface" on "UserEmail" so "EmailQuery" can load them properly · 1667acfa
      epriestley authored
      See PHI1558. Ref T11860. Ref T13444. I partly implemented PHIDs for "UserEmail" objects, but they can't load on their own so you can't directly `bin/remove destroy` them yet.
      Allow them to actually load by implementing "PolicyInterface".
      Addresses are viewable and editable by the associated user, unless they are a bot/list address, in which case they are viewable and editable by administrators (in preparation for T11860). This has no real impact on anything today.
      Test Plan:
        - Used `bin/remove destroy <phid>` to destroy an individual email address.
        - Before: error while loading the object by PHID in the query policy layer.
        - After: clean load and destroy.
      Maniphest Tasks: T13444, T11860
      Differential Revision: https://secure.phabricator.com/D20927
  6. 22 Nov, 2019 1 commit
  7. 21 Nov, 2019 1 commit
  8. 19 Nov, 2019 14 commits
    • epriestley's avatar
      Add a "--dry-run" flag to "bin/repository rebuild-identities" · 374f8b10
      epriestley authored
      Summary: Ref T13444. Allow the effects of performing an identity rebuild to be previewed without committing to any changes.
      Test Plan: Ran "bin/repository rebuild-identities --all-identities" with and without "--dry-run".
      Maniphest Tasks: T13444
      Differential Revision: https://secure.phabricator.com/D20922
    • epriestley's avatar
      Improve use of keys when iterating over commits in "bin/audit delete" and... · 63d84e0b
      epriestley authored
      Improve use of keys when iterating over commits in "bin/audit delete" and "bin/repository rebuild-identities"
      Fixes T13457. Ref T13444. When we iterate over commits in a particular repository, the default iteration strategy can't effectively use the keys on the table.
      Tweak the ordering so the "<repositoryID, epoch, [id]>" key can be used.
      Test Plan:
        - Ran `bin/audit delete --repository X` and `bin/repository rebuild-identities --repository X` before and after changes.
          - With just the key changes, performance was slightly better. My local data isn't large enough to really emphasize the key changes.
          - With the page size changes, performance was a bit better (~30%, but on 1-3 second run durations).
        - Used `--trace` and ran `EXPLAIN ...` on the new queries, saw them select the "<repositoryID, epoch, [id]>" key and report a bare "Using index condition" in the "Extra" column.
      Maniphest Tasks: T13457, T13444
      Differential Revision: https://secure.phabricator.com/D20921
    • epriestley's avatar
      Update repository identities after all mutations to users and email addresses · a7aca500
      epriestley authored
      Ref T13444. Currently, many mutations to users and email addresses (particularly: user creation; and user and address destruction) do not propagate properly to repository identities.
      Add hooks to all mutation workflows so repository identities get rebuilt properly when users are created, email addresses are removed, users or email addresses are destroyed, or email addresses are reassigned.
      Test Plan:
      - Added random email address to account, removed it.
      - Added unassociated email address to account, saw identity update (and associate).
        - Removed it, saw identity update (and disassociate).
      - Registered an account with an unassociated email address, saw identity update (and associate).
        - Destroyed the account, saw identity update (and disassociate).
      - Added address X to account A, unverified.
        - Invited address X.
        - Clicked invite link as account B.
        - Confirmed desire to steal address.
        - Saw identity update and reassociate.
      Maniphest Tasks: T13444
      Differential Revision: https://secure.phabricator.com/D20914
    • epriestley's avatar
      Give "PhabricatorUserEmail" a PHID · 89dcf979
      epriestley authored
      Ref T13444. To interact meaningfully with "DestructionEngine", objects need a PHID. The "UserEmail" object currently does not have one (or a real "Query").
      Provide basic PHID support so "DestructionEngine" can interact with the object more powerfully.
      Test Plan:
        - Ran migrations, checked data in database, saw sensible PHIDs assigned.
        - Added a new email address to my account, saw it get a PHID.
      Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
      Maniphest Tasks: T13444
      Differential Revision: https://secure.phabricator.com/D20913
    • epriestley's avatar
      Use DestructionEngine to destroy UserEmail objects · d69a7360
      epriestley authored
      Summary: Ref T13444. Prepare to hook identity updates when user email addreses are destroyed.
      Test Plan:
        - Destroyed a user with `bin/remove destroy ... --trace`, saw email deleted.
        - Destroyed an email from the web UI, saw email deleted.
      Maniphest Tasks: T13444
      Differential Revision: https://secure.phabricator.com/D20912
    • epriestley's avatar
      Add additional flags to "bin/repository rebuild-identities" to improve flexibility · 18da3469
      epriestley authored
      Ref T13444. Repository identities have, at a minimum, some bugs where they do not update relationships properly after many types of email address changes.
      It is currently very difficult to fix this once the damage is done since there's no good way to inspect or rebuild them.
      Take some steps toward improving observability and providing repair tools: allow `bin/repository rebuild-identities` to effect more repairs and operate on identities more surgically.
      Test Plan: Ran `bin/repository rebuild-identities` with all new flags, saw what looked like reasonable rebuilds occur.
      Maniphest Tasks: T13444
      Differential Revision: https://secure.phabricator.com/D20911
    • epriestley's avatar
      Consolidate repository identity resolution and detection code · 0014d040
      epriestley authored
      Summary: Ref T13444. Send all repository identity/detection through a new "DiffusionRepositoryIdentityEngine" which handles resolution and detection updates in one place.
      Test Plan:
        - Ran `bin/repository reparse --message ...`, saw author/committer identity updates.
        - Added "goose@example.com" to my email addresses, ran daemons, saw the identity relationship get picked up.
        - Ran `bin/repository rebuild-identities ...`, saw sensible rebuilds.
      Maniphest Tasks: T13444
      Differential Revision: https://secure.phabricator.com/D20910
    • epriestley's avatar
      Remove "PhabricatorEventType::TYPE_DIFFUSION_LOOKUPUSER" event · 6afbb610
      epriestley authored
      Summary: Ref T13444. This is an ancient event and part of the old event system. It is not likely to be in use anymore, and repository identities should generally replace it nowadays anyway.
      Test Plan: Grepped for constant and related methods, no longer found any hits.
      Maniphest Tasks: T13444
      Differential Revision: https://secure.phabricator.com/D20909
    • epriestley's avatar
      Distinguish between "Assigned" and "Effective" identity PHIDs more clearly and consistently · a2b2c391
      epriestley authored
      Ref T13444. You can currently explicitly unassign an identity (useful if the matching algorithm is misfiring). However, this populates the "currentEffectiveUserPHID" with the "unassigned()" token, which mostly makes things more difficult.
      When an identity is explicitly unassigned, convert that into an explicit `null` in the effective user PHID.
      Then, realign "assigned" / "effective" language a bit. Previously, `withAssigneePHIDs(...)` actualy queried effective users, which was misleading. Finally, bulk up the list view a little bit to make testing slightly easier.
      Test Plan:
        - Unassigned an identity, ran migration, saw `currentEffectiveUserPHID` become `NULL` for the identity.
        - Unassigned a fresh identity, saw NULL.
        - Queried for various identities under the modified constraints.
      Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
      Maniphest Tasks: T13444
      Differential Revision: https://secure.phabricator.com/D20908
    • epriestley's avatar
      Make repository identity email address association case-insensitive · df0f5c6c
      epriestley authored
      Ref T13444. Currently, identities for a particular email address are queried with "LIKE" against a binary column, which makes the query case-sensitive.
        - Extract the email address into a separate "sort255" column.
        - Add a key for it.
        - Make the query a standard "IN (%Ls)" query.
        - Deal with weird cases where an email address is 10000 bytes long or full of binary junk.
      Test Plan:
        - Ran migration, inspected database for general sanity.
        - Ran query script in T13444, saw it return the same hits for "git@" and "GIT@".
      Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
      Maniphest Tasks: T13444
      Differential Revision: https://secure.phabricator.com/D20907
    • epriestley's avatar
      When predicting project membership during edits, predict milestones will have parent membership · d58eddcf
      epriestley authored
      Depends on D20919. Ref T13462. When editing milestones, we currently predict they will have no members for policy evaluation purposes. This isn't the right rule.
      Instead, predict that their membership will be the same as the parent project's membership, and pass this hint to the policy layer.
      See T13462 for additional context and discussion.
      Test Plan:
        - Set project A's edit policy to "Project Members".
        - Joined project A.
        - Tried to create a milestone of project A.
          - Before: policy exception that the edit policy excludes me.
          - After: clean milestone creation.
        - As a non-member, tried to create a milestone. Received appropriate policy error.
      Maniphest Tasks: T13462
      Differential Revision: https://secure.phabricator.com/D20920
    • epriestley's avatar
      When predicting object policies for project milestones, adjust objects so they... · 959504a4
      epriestley authored
      When predicting object policies for project milestones, adjust objects so they behave like milestones
      Ref T13462. Currently, when testing milestone edit policies during creation, the project object does not behave like a milestone:
        - it doesn't have a milestone number yet, so it doesn't try to access the parent project; and
        - the parent project isn't attached yet.
      Instead: attach the parent project sooner (which "should" be harmless, although it's possible this has weird side effects); and give the adjusted policy object a dummy milestone number if it doesn't have one yet. This forces it to act like a milestone when emitting policies.
      Test Plan:
        - Set "Projects" application default edit policy to "No One".
        - Created a milestone I had permission to create.
          - Before: failed with a policy error, because the project behaved like a non-milestone and returned "No One" as the effective edit policy.
          - After: worked properly, correctly evaluting the parent project edit policy as the effective edit policy.
        - Tried to create a milestone I did not have permission to create (no edit permission on parent project).
          - Got an appropriate edit policy error.
      Maniphest Tasks: T13462
      Differential Revision: https://secure.phabricator.com/D20919
    • epriestley's avatar
      Remove "stronger/weaker" policy color hints from object headers · de66a8ec
      epriestley authored
      Fixes T13461. Some applications provide hints about policy strength in the header, but these hints are inconsistent and somewhat confusing. They don't make much sense for modern objects with Custom Forms, which don't have a single "default" policy.
      Remove this feature since it seems to be confusing things more than illuminating them.
      Test Plan:
        - Viewed various objects, no longer saw colored policy hints.
        - Grepped for all removed symbols.
      Maniphest Tasks: T13461
      Differential Revision: https://secure.phabricator.com/D20918
    • epriestley's avatar
      Update the "owner can always view/edit" policy exception rule · 1996b0cd
      epriestley authored
      Summary: Fixes T13460. This rule vanished from the UI in D20165; update things so it returns to the UI.
      Test Plan: {F7035134}
      Maniphest Tasks: T13460
      Differential Revision: https://secure.phabricator.com/D20917
  9. 13 Nov, 2019 2 commits
    • epriestley's avatar
      Surface edits to "Text" panels on dashboards as remarkup edits · e86aae99
      epriestley authored
      Summary: Fixes T13456. These edits are remarkup edits and should attach files, trigger mentions, and so on.
      Test Plan: Created a text panel, dropped a file in. After changes, saw the file attach properly.
      Maniphest Tasks: T13456
      Differential Revision: https://secure.phabricator.com/D20906
    • epriestley's avatar
      Correctly identify more SSH private key problems as "formatting" or "passphrase" related · 2adc36ba
      epriestley authored
      Ref T13454. Fixes T13006. When a user provide us with an SSH private key and (possibly) a passphrase:
        # Try to verify that they're correct by extracting the public key.
        # If that fails, try to figure out why it didn't work.
      Our success in step (2) will vary depending on what the problem is, and we may end up falling through to a very generic error, but the outcome should generally be better than the old approach.
      Previously, we had a very unsophisticated test for the text "ENCRYPTED" in the key body and questionable handling of the results: for example, providing a passphrase when a key did not require one did not raise an error.
      Test Plan:
      Created and edited credentials with:
        - Valid, passphrase-free keys.
        - Valid, passphrased keys with the right passphrase.
        - Valid, passphrase-free keys with a passphrase ("surplus passphrase" error).
        - Valid, passphrased keys with no passphrase ("missing passphrase" error).
        - Valid, passphrased keys with an invalid passphrase ("invalid passphrase" error).
        - Invalid keys ("format" error).
      The precision of these errors will vary depending on how helpful "ssh-keygen" is.
      Maniphest Tasks: T13454, T13006
      Differential Revision: https://secure.phabricator.com/D20905
  10. 09 Nov, 2019 3 commits
  11. 08 Nov, 2019 8 commits
    • epriestley's avatar
      Update various Asana odds-and-ends for "gid" API changes · cd60a8aa
      epriestley authored
      Ref T13453. Some of the Asana integrations also need API updates.
      Depends on D20899.
      Test Plan:
        - Viewed "asana.workspace-id" in Config, got a sensible GID list.
        - Created a revision, saw the associated Asana task get assigned.
        - Pasted an Asana link I could view into a revision description, saw it Doorkeeper in the metadata.
      Maniphest Tasks: T13453
      Differential Revision: https://secure.phabricator.com/D20900
    • epriestley's avatar
      Update Asana Auth adapter for "gid" API changes · 2223d6b9
      epriestley authored
      Summary: Ref T13453. The Asana API has changed, replacing all "id" fields with "gid", including the "users/me" API call result.
      Test Plan: Linked an Asana account. Before: error about missing 'id'. After: clean link.
      Maniphest Tasks: T13453
      Differential Revision: https://secure.phabricator.com/D20899
    • epriestley's avatar
      Prevent workboard cards from being grabbed by the "Txxx" object name text · 338b4cb2
      epriestley authored
      Fixes T13452. We currently give users mixed signals about the interaction mode of this text: the cursor says "text" but the behavior is "grab".
      Make the behavior "text" to align with the cursor. An alternate variation of this change is to remove the cursor, but this is preferable if it doesn't cause problems, since copying the task ID is at least somewhat useful.
      Test Plan: In Safari, Firefox, and Chrome: selected and copied object names from workboard cards; and dragged workboard cards by other parts of their UI.
      Maniphest Tasks: T13452
      Differential Revision: https://secure.phabricator.com/D20898
    • epriestley's avatar
      Fix an issue with 1up diff block rendering for added or removed blocks · d4491ddc
      epriestley authored
      Summary: Ref T13425. When a change adds or removes a block (vs adding or removing a document, or changing a block), we currently try to render `null` as cell content in the unified view. Make this check broader to catch both "no opposite document" and "no opposite cell".
      Test Plan: {F7008772}
      Subscribers: artms
      Maniphest Tasks: T13425
      Differential Revision: https://secure.phabricator.com/D20897
    • epriestley's avatar
      When "Fetch Refs" are configured for a repository, highlight the "Branches"... · 502ca476
      epriestley authored
      When "Fetch Refs" are configured for a repository, highlight the "Branches" menu item in the Diffusion Management UI
      Summary: Ref T13448. Minor UI issue: setting a "Fetch Refs" value does not highlight the associated menu item, but should.
      Test Plan: Set only "Fetch Refs", now saw menu item highlighted.
      Maniphest Tasks: T13448
      Differential Revision: https://secure.phabricator.com/D20895
    • epriestley's avatar
      Always initialize Git repositories with "git init", never with "git clone" · 1b40f7e5
      epriestley authored
      Fixes T13448. We currently "git clone" to initialize repositories, but this will fetch too many refs if "Fetch Refs" is configured.
      In modern Phabricator, there's no apparent reason to "git clone"; we can just "git init" instead. This workflow naturally falls through to an update, where we'll do a "git fetch" and pull in exactly the refs we want.
      Test Plan:
        - Configured an observed repository with "Fetch Refs".
        - Destroyed the working copy.
        - Ran "bin/repository pull X --trace --verbose".
          - Before: saw "git clone" pull in the world.
          - After: saw "git init" create a bare empty working copy, then "git fetch" fill it surgically.
      Both flows end up in the same place, this one is just simpler and does less work.
      Maniphest Tasks: T13448
      Differential Revision: https://secure.phabricator.com/D20894
    • epriestley's avatar
      When fetching Git repositories, pass "--no-tags" to make explicit "Fetch Refs"... · 8dd57a1e
      epriestley authored
      When fetching Git repositories, pass "--no-tags" to make explicit "Fetch Refs" operate more narrowly
      Ref T13448. The default behavior of "git fetch '+refs/heads/master:refs/heads/master'" is to follow and fetch associated tags.
      We don't want this when we pass a narrow refspec because of "Fetch Refs" configuration. Tell Git that we only want the refs we explicitly passed.
      Note that this doesn't prevent us from fetching tags (if the refspec specifies them), it just stops us from fetching extra tags that aren't part of the refspec.
      Test Plan:
        - Ran "bin/repository pull X --trace --verbose" in a repository with a "Fetch Refs" configuration, saw only "master" get fetched (previously: "master" and reachable tags).
        - Ran "git fetch --no-tags '+refs/*:refs/*'", saw tags fetched normally ("--no-tags" does not disable fetching tags).
      Maniphest Tasks: T13448
      Differential Revision: https://secure.phabricator.com/D20893
    • epriestley's avatar
      Manually prune Git working copy refs instead of using "--prune", to improve "Fetch Refs" behavior · 9dbde24d
      epriestley authored
      Ref T13448. When "Fetch Refs" is configured:
        - We switch to a narrow mode when running "ls-remote" against the local working copy. This excludes surplus refs, so we'll incorrectly detect that the local and remote working copies are identical in cases where the local working copy really has surplus refs.
        - We rely on "--prune" to remove surplus local refs, but it only prunes refs matching the refspecs we pass "git fetch". Since these refspecs are very narrow under "Fetch Only", the pruning behavior is also very narrow.
        - When listing local refs, always list everything. If we have too much stuff locally, we want to get rid of it.
        - When we identify surplus local refs, explicitly delete them instead of relying on "--prune". We can just do this in all cases so we don't have separate "--prune" and "manual" cases.
      Test Plan:
        - Created a new repository, observed from a GitHub repository, with many tags/refs/branches. Pulled it.
        - Observed lots of refs in `git for-each-ref`.
        - Changed "Fetch Refs" to "refs/heads/master".
        - Ran `bin/repository pull X --trace --verbose`.
      On deciding to do something:
        - Before: since "master" did not change, the pull declined to act.
        - After: the pull detected surplus refs and deleted them. Since the states then matched, it declined further action.
      On pruning:
        - Before: if the pull was forced to act, it ran "fetch --prune" with a narrow refspec, which did not prune the working copy.
        - After: saw working copy pruned explicitly with "update-ref -d" commands.
      Also, set "Fetch Refs" back to the default (empty) and pulled, saw everything pull.
      Maniphest Tasks: T13448
      Differential Revision: https://secure.phabricator.com/D20892
  12. 06 Nov, 2019 2 commits
    • epriestley's avatar
      Allow username changes which modify letter case to go through as valid · f5f2a0bc
      epriestley authored
      Fixes T13446. Currently, the validation logic here rejects a rename like "alice" to "ALICE" (which changes only letter case) but this is a permissible rename.
      Allow collisions that collide with the same user to permit this rename.
      Also, fix an issue where an empty rename was treated improperly.
      Test Plan:
        - Renamed "alice" to "ALICE".
          - Before: username collision error.
          - After: clean rename.
        - Renamed "alice" to "orange" (an existing user). Got an error.
        - Renamed "alice" to "", "!@#$", etc (invalid usernames). Got sensible errors.
      Maniphest Tasks: T13446
      Differential Revision: https://secure.phabricator.com/D20890
    • epriestley's avatar
      Change the Herald "do not include [any of]" condition label to "include none of" · 6bada7db
      epriestley authored
      Summary: Fixes T13445. Make the meaning of this condition more clear, since the current wording is ambiguous between "any of" and "all of".
      Test Plan: Edited a Herald rule with a PHID list field ("Project tags"), saw text label say "include none of".
      Maniphest Tasks: T13445
      Differential Revision: https://secure.phabricator.com/D20889
  13. 31 Oct, 2019 4 commits