Gate note ledger links by owner role - #1826
Conversation
A stored note can carry a markdown link to the ledger (the refund notes do), but the ledger pages are owner-only — every other admin saw a link that only 404s. Note bodies now demote an /admin/ledger link to its plain text for non-owners, threaded through the attendee banner, the notes summaries above the attendee/listing lists, and the delete-note page. Owners keep the link. The link-stripping uses a token-aware Markdown walker (withoutLinksTo) rather than a regex, so it handles inline, reference, collapsed- reference, shortcut, and automatic links, including links nested in lists, blockquotes, tables, and code spans — preserving all the safe markdown around them. A code span that looks like a link is left alone, and the markdown structure (list markers, table pipes, quote markers) is preserved byte-for-byte.
📝 WalkthroughWalkthroughAdmin attendee and listing views now propagate owner state into note rendering. Non-owners see ledger references as plain text, while owners retain links. A token-aware markdown helper preserves surrounding formatting, with tests covering link forms and nested structures. ChangesOwner-only ledger link rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdminRequest
participant attendeePage
participant attendeeBanner
participant AttendeeNotesSection
participant withoutLinksTo
AdminRequest->>attendeePage: load attendee data
attendeePage->>attendeeBanner: pass isOwner
attendeeBanner->>AttendeeNotesSection: pass notes and isOwner
AttendeeNotesSection->>withoutLinksTo: rewrite ledger links for non-owners
withoutLinksTo-->>AttendeeNotesSection: return rendered note markdown
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/admin/attendee-page.ts (1)
199-208: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSecurity Misconfiguration (CWE-863): Incorrect Authorization
Reachability: Internal
Inline owner check duplicates the shared
isOwnerRolehelper.This call computes
isOwnerviactx.session.adminLevel === "owner", while the sibling loader insrc/features/admin/listing-page-data.tsuses the sharedisOwnerRole(ctx.session.adminLevel)helper for the same concept. See the consolidated comment for the cross-file recommendation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/admin/attendee-page.ts` around lines 199 - 208, Update the banner loader’s isOwner value in attendeeBanner to use the shared isOwnerRole helper with ctx.session.adminLevel, matching the sibling loader’s established owner-role logic and removing the inline comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/markdown.ts`:
- Around line 145-155: Update rewriteTable to tolerate cells whose normalized
text cannot be located in token.raw, as happens with escaped pipes. Remove the
assertion that requires every locateSourceParts index to be nonnegative, and
return the original raw table or otherwise skip unmatched cells while preserving
rewriting for cells with valid source positions.
In `@src/ui/templates/admin/attendee-notes.tsx`:
- Line 270: Update the NoteBody invocation in the attendee-notes component to
compute isOwner with the shared isOwnerRole(session.adminLevel) helper,
importing isOwnerRole from `#shared/types.ts` as needed, instead of comparing
adminLevel to "owner" inline.
---
Outside diff comments:
In `@src/features/admin/attendee-page.ts`:
- Around line 199-208: Update the banner loader’s isOwner value in
attendeeBanner to use the shared isOwnerRole helper with ctx.session.adminLevel,
matching the sibling loader’s established owner-role logic and removing the
inline comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a566c025-f51d-4a82-87ad-4d777e5539e1
📒 Files selected for processing (12)
src/features/admin/attendee-page.tssrc/features/admin/listing-page-data.tssrc/shared/markdown.tssrc/ui/templates/admin/attendee-notes.tsxsrc/ui/templates/admin/attendee-page.tsxsrc/ui/templates/admin/attendees-list.tsxsrc/ui/templates/admin/listings/overview.tsxsrc/ui/templates/admin/listings/roster.tsxsrc/ui/templates/admin/listings/types.tstest/shared/markdown.test.tstest/ui/templates/admin/attendee-notes.test.tsxtest/ui/templates/admin/attendee-page.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5470b83875
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…D clone Address four unresolved review threads on PR #1826: 1. Escaped-pipe table crash (CodeRabbit + Codex): marked normalizes backslash-escaped pipes (\| to |) in cell.text, so locating that normalized text against token.raw misses — the assert threw and the whole note 500'd. Removed the assert and filter to matched cells (same gap-preserving pattern rewriteChildren already uses), so the table still renders and links in matched cells are still demoted. 2. Inline isOwner check (CodeRabbit): attendees-notes.tsx and the attendee-page banner handler both reimplemented isOwnerRole as a raw string comparison. Both now call the shared isOwnerRole helper. 3. Attendee ledger-tab links (Codex): PaymentDetails links to /admin/attendees/:id/ledger (the owner-only ledger tab), but the filter only caught /admin/ledger. Generalized withoutLinksTo to accept a predicate (string | function) so isOwnerOnlyLink catches both URL shapes a note might carry. 4. CPD clone (CI): the { notes, isOwner } prop pair was repeated in AttendeeNotesSection and attendeeBanner. Extracted NotesViewProps as the shared type; Section uses it directly, Summary and Banner extend it. The 19-token clone between attendee-notes.tsx and attendee-page.tsx is eliminated. Regression tests: escaped-pipe table no longer crashes (exact output assertion), predicate matcher catches the attendee ledger-tab URL, and the notes section demotes the attendee tab link for non-owners while owners keep it.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/admin/attendee-page.ts (1)
200-209: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
isOwner: ctx.session.adminLevel === "owner"should useisOwnerRole(), same as line 189 in this file.This inline comparison is the exact pattern already fixed at line 189 (
ContactHistory'sisOwner) viaisOwnerRole(ctx.session.adminLevel). Thebannerloader'sisOwnerat line 203 still re-derives the check inline, so the two owner-checks in this file can silently diverge ifisOwnerRole's definition ever changes.♻️ Suggested fix
banner: async ({ attendee }, ctx) => attendeeBanner({ attendee, - isOwner: ctx.session.adminLevel === "owner", + isOwner: isOwnerRole(ctx.session.adminLevel), notes: await getNotesForAttendee( attendee.id, await requireRequestPrivateKey(), ), statuses: await attendeeStatuses.getAll(), }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/admin/attendee-page.ts` around lines 200 - 209, Update the isOwner field in the banner loader to call isOwnerRole(ctx.session.adminLevel), matching the existing ContactHistory owner check, and remove the inline admin-level comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/markdown.ts`:
- Around line 191-196: Remove the redundant assert in rewriteToken’s link branch
and pass the required token.tokens directly to rewriteChildren. Keep the
existing linkMatches check and rewriting behavior unchanged.
---
Outside diff comments:
In `@src/features/admin/attendee-page.ts`:
- Around line 200-209: Update the isOwner field in the banner loader to call
isOwnerRole(ctx.session.adminLevel), matching the existing ContactHistory owner
check, and remove the inline admin-level comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5d9d570c-fe21-4e31-8449-780e2c21f424
📒 Files selected for processing (6)
src/features/admin/attendee-page.tssrc/shared/markdown.tssrc/ui/templates/admin/attendee-notes.tsxsrc/ui/templates/admin/attendee-page.tsxtest/shared/markdown.test.tstest/ui/templates/admin/attendee-notes.test.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de9876ba3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // still demoted. | ||
| return rewriteSourceParts( | ||
| token.raw, | ||
| parts.filter(({ index }) => index >= 0), |
There was a problem hiding this comment.
When the escaped-pipe cell itself contains the ledger link, cell.text no longer occurs in token.raw (\| is normalized to |), so this filter drops that cell and rewriteSourceParts copies its raw markdown unchanged. For a non-owner viewing a note table like | [ledger](/admin/ledger/x)\|copy |, the owner-only link still renders as an <a>, which breaks the PR goal that forbidden note links are demoted everywhere.
Useful? React with 👍 / 👎.
| href.startsWith("/admin/ledger") || | ||
| (href.startsWith("/admin/attendees/") && href.includes("/ledger")); |
There was a problem hiding this comment.
When an operator stores a full URL copied from the browser, for example https://tickets.example/admin/ledger/attendee/5 or the attendee ledger tab URL, this predicate leaves it untouched because it only matches root-relative hrefs. renderMarkdown permits https: links, while these ledger pages are owner-only, so managers can still see a clickable forbidden link instead of plain note text whenever the note stores the absolute URL.
Useful? React with 👍 / 👎.
| if (token.type === "link") { | ||
| if (!linkMatches(matcher, token.href)) return token.raw; | ||
| assert(token.tokens, "Markdown link has no parsed text"); | ||
| return rewriteChildren(token.text, token.tokens, matcher); |
There was a problem hiding this comment.
When the forbidden link's label is itself a URL, this returns that URL as bare markdown and NoteBody immediately passes it through renderMarkdown, where Marked auto-links bare https://... text again. For a non-owner note like [https://tickets.example/admin/ledger/attendee/5](/admin/ledger/attendee/5), the href matches and is demoted here, but the final HTML still contains a clickable owner-only ledger link.
Useful? React with 👍 / 👎.
…ormatting CodeRabbit flagged that Tokens.Link.tokens is declared as required in marked 18.x (tokens: Token[], not tokens?: Token[]), so the assert(token.tokens, ...) guard could never fire — it only added an impossible throw path. Removed the assert and pass token.tokens straight through to rewriteChildren. The assert was also the only @std/assert usage, so that import is gone too. Also applies Biome's formatting fixes from deno task lint: import order in attendee-notes.tsx (type imports first), the NotesViewProps type annotation in attendee-page.tsx, and trailing whitespace in attendee-page.ts and markdown.test.ts.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/markdown.ts (1)
145-159: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAuthorization Bypass (CWE-863): Incorrect Authorization
Reachability: External
● Entry src/ui/templates/admin/attendee-notes.tsx:75 withoutLinksTo │ ▼ ● Sink src/shared/markdown.tsHandle escaped pipes in table cells before preserving the raw cell.
rewriteTable()skips any unmatched cell, so a forbidden link in the same cell as an escaped pipe can survive unchanged and stay clickable for non-owners. Parse table cells from the raw source or demote links in unmatched cells too, and add a regression test for a link + escaped pipe in the same cell.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/markdown.ts` around lines 145 - 159, The rewriteTable function must handle table cells containing escaped pipes instead of skipping them as unmatched raw cells. Update its source-part parsing or fallback path so links in every unmatched cell are still passed through rewriteToken and demoted, while preserving escaped-pipe content; add a regression test covering a forbidden link and escaped pipe in the same cell.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/shared/markdown.ts`:
- Around line 145-159: The rewriteTable function must handle table cells
containing escaped pipes instead of skipping them as unmatched raw cells. Update
its source-part parsing or fallback path so links in every unmatched cell are
still passed through rewriteToken and demoted, while preserving escaped-pipe
content; add a regression test covering a forbidden link and escaped pipe in the
same cell.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dfb3bf5c-3f22-4cfd-8566-5a708c836128
📒 Files selected for processing (5)
src/features/admin/attendee-page.tssrc/shared/markdown.tssrc/ui/templates/admin/attendee-notes.tsxsrc/ui/templates/admin/attendee-page.tsxtest/shared/markdown.test.ts
1 participant
A stored note can carry a markdown link to the ledger. The refund notes do, for example. But the ledger pages are owner-only, so every other admin saw a link that only 404s when they clicked it.
What changes
How the link is stripped
The earlier approach was a regex that only matched inline
[text](url)links. It missed reference links ([text][ref]), collapsed references, shortcuts, automatic links (<https://…>), and links nested inside lists, blockquotes, or table cells.This PR replaces it with a token-aware Markdown walker (
withoutLinksToinsrc/shared/markdown.ts). It parses the note with marked's lexer, walks the token tree, and rewrites each link whose target starts with the forbidden prefix (here,/admin/ledger) back to its plain text — stitching the source back together byte-for-byte so all the safe markdown around it (list markers, table pipes, quote markers, code spans) is preserved. A code span that happens to look like a link is left untouched, and the markdown structure never breaks.Scope
In scope for this PR (PR 5 of the staged-checkout split):
src/shared/markdown.ts— the token-awarewithoutLinksTohelper.src/ui/templates/admin/attendee-notes.tsx—NoteBodytakes anisOwnerflag;NoteBox,AttendeeNotesSection,AttendeeNotesSummary, and the delete-note page thread it through.withoutLinksTounit tests covering every link form and nesting, plus note/banner rendering tests that assert owners keep the link and non-owners get plain text.Out of scope (handled by other PRs in the split, not brought in here): deleted-listing display, pending-checkout UI, held-cash actions, and other attendee-page changes from the mixed source commits.
Acceptance
Summary by CodeRabbit
Access Control
Bug Fixes
Tests