1. 06 Feb, 2020 1 commit
  2. 05 Feb, 2020 1 commit
    • epriestley's avatar
      In summary interfaces, don't render very large inline remarkup details for unit test messages · 9d1af762
      epriestley authored
      Summary: Ref T10635. An install with large blocks of remarkup (4MB) in test details is reporting slow page rendering. This is expected, but I've mostly given up on fighting this unless I absolutely have to. Degrade the interface more aggressively.
      Test Plan:
        - Submitted a large block of test details in remarkup format.
        - Before patch: they rendered inline.
        - After patch: degraded display.
        - Verified small blocks are not changed.
      Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
      Maniphest Tasks: T10635
      Differential Revision: https://secure.phabricator.com/D20970
  3. 04 Feb, 2020 12 commits
  4. 03 Feb, 2020 2 commits
    • epriestley's avatar
      Fix an issue where loading a mangled project graph could fail too abruptly · c42c5983
      epriestley authored
      Ref T13484. If you load a subproject S which has a mangled/invalid `parentPath`, the query currently tries to execute an empty edge query and fatals.
      Instead, we want to deny-by-default in the policy layer but not fail the query. The subproject should become restricted but not fatal anything related to it.
      See T13484 for a future refinement where we could identify "broken / data integrity issue" objects explicilty.
      Test Plan:
        - Modified the `projectPath` of some subproject in the database to `QQQQ...`.
        - Loaded that project page.
        - Before patch: fatal after issuing bad edge query.
        - After patch: "functionally correct" policy layer failure, although an explicit "data integrity issue" failure would be better.
      Maniphest Tasks: T13484
      Differential Revision: https://secure.phabricator.com/D20963
    • epriestley's avatar
      Fix an issue where Herald rules could fail to evaluate at post-commit time · 42e46bbe
      epriestley authored
      Summary: Ref T13480. Some Herald fields need audit information, which recent changes to Herald adapters discarded. For now, just load it unconditionally.
      Test Plan: Triggered an Audit-related rule locally.
      Maniphest Tasks: T13480
      Differential Revision: https://secure.phabricator.com/D20962
  5. 30 Jan, 2020 2 commits
    • epriestley's avatar
      Fix an issue where the last line of block-based diffs could be incorrectly hidden · ccf28a81
      epriestley authored
      Fixes T13468. See that task for discussion. The older source-rendering code mixes "line number" / "1-based" lists with "block number" / "0-based" lists and then has other bugs which cancel this out.
      For block-based diffs, build an explicit block-based mask with only block numbers. This sort of sidesteps the whole issue.
      Test Plan: Viewed the diff with the original reproduction case, plus various other block-based diffs, including one-block image diffs, in unified and side-by-side mode. Didn't spot any oddities.
      Maniphest Tasks: T13468
      Differential Revision: https://secure.phabricator.com/D20959
    • epriestley's avatar
      When issuing a "no-op" MFA token because no MFA is configured, don't give the... · 12c33709
      epriestley authored
      When issuing a "no-op" MFA token because no MFA is configured, don't give the timeline story a badge
      Fixes T13475. Sometimes, we issue a "no op" / "default permit" / "unchallenged" MFA token, when a user with no MFA configured does something which is configured to attempt (but not strictly require) MFA.
      An example of this kind of action is changing a username: usernames may be changed even if MFA is not set up.
      (Some other operations, notably "Sign With MFA", strictly require that MFA actually be set up.)
      When a user with no MFA configured takes a "try MFA" action, we see that they have no factors configured and issue a token so they can continue. This is correct. However, this token causes the assocaited timeline story to get an MFA badge.
      This badge is incorrect or at least wildly misleading, since the technical assertion it currently makes ("the user answered any configured MFA challenge to do this, if one exists") isn't explained properly and isn't useful anyway.
      Instead, only badge the story if the user actually has MFA and actually responded to some kind of MFA challege. The badge now asserts "this user responded to an MFA challenge", which is expected/desired.
      Test Plan:
        - As a user with no MFA, renamed a user. Before patch: badged story. After patch: no badge.
        - As a user with MFA, renamed a user. Got badged stories in both cases.
      Maniphest Tasks: T13475
      Differential Revision: https://secure.phabricator.com/D20958
  6. 29 Jan, 2020 7 commits
    • epriestley's avatar
      Add "Author's Packages" and "Committer's Packages" Herald rules for Commits and Hooks · c99485e8
      epriestley authored
      Summary: Fixes T13480. Adds the remaining missing Owners package rules for Herald commit adapters.
      Test Plan: Created hooks which care about these fields, pushed commits, saw sensible transcript values.
      Maniphest Tasks: T13480
      Differential Revision: https://secure.phabricator.com/D20957
    • epriestley's avatar
      In Herald "Commit" rules, use repository identities to identify authors and committers · 6628cd2b
      epriestley authored
      Summary: Ref T13480. The Herald "Commit" rules still use raw commit data properties to identify authors and committers. Instead, use repository identities.
      Test Plan: Wrote a Herald rule using all four fields, ran it against various commits with and without known authors. Checked transcript for sensible field values.
      Maniphest Tasks: T13480
      Differential Revision: https://secure.phabricator.com/D20955
    • epriestley's avatar
      Respect repository identities when figuring out authors/committers in Herald pre-commit hook rules · 41f143f7
      epriestley authored
      Ref T13480. Currently, Herald commit hook rules use a raw address resolution query to identify the author and committer for a commit. This will get the wrong answer when the raw identity string has been explicitly bound to some non-default user (most often, it will fail to identify an author when one exists).
      Instead, use the "IdentityEngine" to properly resolve identities.
      Test Plan: Authored a commit as `X <y@example.com>`, a raw identity with no "natural" matches to users (e.g., no user with that email or username). Bound the identity to a particular user in Diffusion. Wrote a Herald pre-commit content rule, pushed the commit. Saw Herald recognize the correct user when evaluating rules.
      Maniphest Tasks: T13480
      Differential Revision: https://secure.phabricator.com/D20953
    • epriestley's avatar
      In Herald transcripts, render some field values in a more readable way · a0a346be
      epriestley authored
      Ref T13480. Currently, some Herald field types are rendered in an unfriendly way on transcripts. Particularly, PHID lists are rendered as raw PHIDs.
      Improve this by delegating rendering to Value objects and letting "PHID List" value objects render more sensible handle lists. Also improve "bool" fields a bit and make more fields render an explicit "None" / empty value rather than just rendering nothing.
      Test Plan: Viewed various transcripts, including transcripts covering boolean values, the "Always" condition, large blocks of text, and PHID lists.
      Maniphest Tasks: T13480
      Differential Revision: https://secure.phabricator.com/D20951
    • epriestley's avatar
      In Herald transcript rendering, don't store display labels in keys · 19662e33
      epriestley authored
      Ref T13480. Currently, when Herald renders a transcript, it puts display labels into array keys. This is a bad pattern for several reasons, notably that the values must be scalar (so you can't add icons or other markup later) and the values must be unique (which is easily violated because many values are translated).
      Instead, keep values as list items.
      Test Plan: Viewed Herald transcripts, saw no (meaningful) change in rendering output.
      Maniphest Tasks: T13480
      Differential Revision: https://secure.phabricator.com/D20949
    • epriestley's avatar
      Remove legacy pre-loading of handles from Herald rendering · a5a9a5e0
      epriestley authored
      Summary: Ref T13480. When Herald renders rules, it partly uses a very old handle pre-loading mechanism where PHIDs are extracted and loaded upfront. This was obsoleted a long time ago and was pretty shaky even when it worked. Get rid of it to simplify the code a little.
      Test Plan: Viewed Herald rules rendered into static text with PHID list actions, saw handles. Grepped for all affected methods.
      Maniphest Tasks: T13480
      Differential Revision: https://secure.phabricator.com/D20948
    • epriestley's avatar
      Don't use "line-through" style for completed items in remarkup checklists · 7a1681b8
      epriestley authored
      Summary: Fixes T13482. Although this style makes physical sense by relationship to a written checklist, it seems to do more harm than good in practice.
      Test Plan: Wrote a checklist with a checked-off item in remarkup, saw no more line-through.
      Maniphest Tasks: T13482
      Differential Revision: https://secure.phabricator.com/D20954
  7. 23 Jan, 2020 2 commits
  8. 16 Jan, 2020 1 commit
    • epriestley's avatar
      Update "git rev-parse" invocation to work in Git 2.25.0 · 6ccb6a64
      epriestley authored
      Fixes T13479. The behavior of "git rev-parse --show-toplevel" has changed in Git 2.25.0, and it now fails in bare repositories.
      Instead, use "git rev-parse --git-dir" to sanity-check the working copy. This appears to have more stable behavior across Git versions, although it's a little more complicated for our purposes.
      Test Plan:
        - Ran `bin/repository update ...` on an observed, bare repository.
        - ...on an observed, non-bare ("legacy") repository.
        - ...on a hosted, bare repository.
      Maniphest Tasks: T13479
      Differential Revision: https://secure.phabricator.com/D20945
  9. 15 Jan, 2020 2 commits
  10. 14 Jan, 2020 3 commits
  11. 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
  12. 10 Dec, 2019 1 commit
  13. 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
  14. 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
  15. 22 Nov, 2019 1 commit
  16. 21 Nov, 2019 1 commit
  17. 19 Nov, 2019 1 commit