HYDI-48: Ability to delete a row#43
Conversation
Co-Authored-By: Oz <oz-agent@warp.dev>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review — Principal Engineer Assessment
Verdict: Not ready
Risk: High
Scope: efcaa79fc2809058c8da64987956e1d6c740aea0..153b586e915256dcb6965a2894c0f9fa2d98144e
Requirements: HYDI-48 (PR description + linked Jira ticket; jira-view HYDI-48 returned no content in this environment)
Strengths
- Clear UX improvement with compact per-row actions and explicit delete affordance.
- Good guardrail intent: deletion is blocked when linked PR is open.
- Added tests for open/closed PR-state deletion behavior and menu rendering.
- Documentation was updated alongside behavior changes.
Architecture Assessment
- Vertical slice quality: solid end-to-end wiring (UI → route → PR state check → delete flow) with user-visible feedback.
- Deep module quality: new GitHub PR helper is small and focused, but newly introduced HTML rendering paths include unsafe interpolation.
- Testability/reasoning quality: core happy-path and open/closed gating are tested; key error-path guardrails are currently untested.
Third-Party Contract Verification
- Verified usage pattern of Octokit
pulls.getfor PR state lookup is aligned with expectedopen|closedstate handling. - No contract mismatch identified in endpoint shape (
owner/repo/pull_number).
Unmapped Findings
(findings whose lines are not in the diff — inline comment not possible)
- None.
Action Plan For Implementing Agent
- [REV-001] Escape
noticebefore rendering in dashboard HTML to eliminate reflected XSS. - [REV-002] Escape attribute values inserted into hidden form inputs (
project,status) to prevent attribute-injection XSS. - [REV-003] Add tests for invalid PR URL and PR-status verification failure branches in delete flow.
actions:
- id: REV-001
severity: critical
location: src/routes/dashboard.ts:385
change_required: Escape user-controlled notice content before interpolating into HTML output.
acceptance_check: A payload like `<img src=x onerror=alert(1)>` in `notice` is rendered as text, not executed.
- id: REV-002
severity: critical
location: src/routes/dashboard.ts:322
change_required: Escape hidden input attribute values derived from query parameters before HTML interpolation.
acceptance_check: A payload containing quotes/angle brackets in `project` or `status` cannot break attributes or inject markup/script.
- id: REV-003
severity: minor
location: src/routes/dashboard.test.ts:171
change_required: Add explicit tests covering invalid PR URL and GitHub PR-state lookup failure branches.
acceptance_check: Test suite includes assertions for both error redirects and preserves existing open/closed behavior tests.| <div class="stats"> | ||
| ${statsHtml} | ||
| </div> | ||
| ${notice ? `<div class="notice notice-${noticeType}">${notice}</div>` : ""} |
There was a problem hiding this comment.
[REV-001] Critical — Reflected XSS in notice banner
Problem: notice comes from the query string and is interpolated directly into HTML (${notice}) without escaping.
Impact: An attacker can craft a dashboard URL that executes arbitrary JavaScript in the viewer's browser.
Required fix: HTML-escape notice before interpolation (or render via a safe templating path that auto-escapes).
Acceptance check: Visiting /dashboard?noticeType=error¬ice=<img%20src=x%20onerror=alert(1)> renders the payload as inert text and does not execute script.
| <button class="row-menu-btn" type="button" data-row-menu-button aria-label="Open actions for ${run.ticket_key}" aria-expanded="false">⋮</button> | ||
| <div class="row-menu-list" role="menu"> | ||
| <form method="POST" action="/dashboard/${run.ticket_key}/delete" style="margin:0;"> | ||
| ${selectedProject ? `<input type="hidden" name="project" value="${selectedProject}">` : ""} |
There was a problem hiding this comment.
[REV-002] Critical — Attribute-injection XSS via hidden input values
Problem: selectedProject/selectedStatus are interpolated directly into hidden input value attributes without escaping.
Impact: A crafted query value containing quotes can break out of the attribute and inject arbitrary markup/script into the page.
Required fix: Escape attribute values before interpolation (e.g., dedicated HTML-attribute escaping helper) for all query-derived values inserted into HTML attributes.
Acceptance check: Payloads containing '"<> in project/status remain encoded in rendered HTML and cannot introduce new attributes/elements/scripts.
| @@ -160,4 +171,73 @@ describe("dashboardRouter", () => { | |||
| expect(html).not.toContain(">HYDI-1</a>"); | |||
There was a problem hiding this comment.
[REV-003] Minor — Missing coverage for delete guardrail error branches
Problem: New tests cover open/closed PR states but do not cover invalid PR URL parsing or GitHub lookup failure branches in the delete route.
Impact: Regressions in critical safety behavior (error redirects that block deletion) may ship undetected.
Required fix: Add tests for (1) invalid pr_url format and (2) PR-state lookup failure/throw path, asserting error notices and no deletion.
Acceptance check: Test suite includes both scenarios and verifies deleteRun is not called in either case.
Co-Authored-By: Oz <oz-agent@warp.dev>
There was a problem hiding this comment.
PR Review — Principal Engineer Assessment
Verdict: Ready with fixes
Risk: Medium
Scope: efcaa79fc2809058c8da64987956e1d6c740aea0..a69e5d5012bf947c5c7dc4c4952ec906cc70a947
Requirements: HYDI-48 (PR description + linked Jira key/reference)
Strengths
- Delete flow correctly fails closed when PR state cannot be verified.
- Filter state is preserved across delete redirects, and notice/query rendering is escaped.
- New tests cover key delete scenarios (open PR, invalid URL, lookup failure, closed PR) and XSS regression on query-derived notice/filter values.
Architecture Assessment
- Vertical slice quality: Strong — UI action, route handling, integration helper, tests, and docs shipped together.
- Deep module quality: Mixed — extraction of a GitHub PR helper is good, but URL parsing remains overly permissive at the module boundary.
- Testability/reasoning quality: Strong — route behavior is mostly deterministic and well mocked.
Third-Party Contract Verification
- Service: GitHub REST Pull Requests API
- Target version: Current GitHub REST docs
- Endpoints checked:
GET /repos/{owner}/{repo}/pulls/{pull_number} - Status: Match
- Notes: Parameters (
owner,repo,pull_number) and response usage (stateas open/closed) align with official docs.
Unmapped Findings
(findings whose lines are not in the diff — inline comment not possible)
- None.
Action Plan For Implementing Agent
- [REV-001] Tighten GitHub PR URL validation to reject non-GitHub hosts and non-numeric/extra path pull segments before calling
pulls.get.
actions:
- id: REV-001
severity: important
location: src/github/pull-requests.ts:14
change_required: Validate host and pull-number strictly (github.com host check, exact owner/repo/pull/<digits> shape, and /^\d+$/ for pull segment) before parsing or API calls.
acceptance_check: Unit tests prove parser returns null for https://example.com/org/repo/pull/123 and https://github.com/org/repo/pull/123abc, while valid GitHub URLs still parse and closed/open guard behavior remains correct.| const [owner, repo, type, pullNumberRaw] = parts; | ||
| if (!owner || !repo || type !== "pull" || !pullNumberRaw) return null; | ||
|
|
||
| const pullNumber = Number.parseInt(pullNumberRaw, 10); |
There was a problem hiding this comment.
[REV-001] Important — PR URL parsing is too permissive
Problem: parseGithubPullRequestUrl accepts any hostname and uses parseInt, so malformed values like https://example.com/org/repo/pull/123abc are treated as pull #123.
Impact: The delete guard can evaluate the wrong PR state and incorrectly allow deletion that should be blocked or invalid.
Required fix: Restrict accepted URLs to expected GitHub host(s), require canonical path shape (/owner/repo/pull/<digits>), and reject any non-digit pull segment.
Acceptance check: Add parser tests that reject non-GitHub hosts and alphanumeric pull numbers, and verify delete route returns the invalid-URL notice for those cases.
2 participants
Implements HYDI-48
Changes
⋮) at the right side of each dashboard row with a singleDeleteaction.POST /dashboard/:ticketKey/deleteto handle row deletion with PR-state guardrails.Validation
npm testnpm run test:coveragenpm run typecheckCo-Authored-By: Oz oz-agent@warp.dev
Conversation: https://app.warp.dev/conversation/b76bf876-f5f5-42cd-9eea-29fb2b15f942
Run: https://oz.warp.dev/runs/019e91bc-543a-7289-8f65-974d05c2da9b
This PR was generated with Oz.