PDF embed by salihudickson · Pull Request #10198 · outline/outline

salihudickson · 2025-09-17T22:28:09Z

This PR aims to embed an interactive PDF viewer in the Outline editor.

Demo:

Screen.Recording.2025-09-17.at.23.26.06.mov

graphite-app · 2025-09-17T22:30:18Z

+    window.addEventListener("click", listener);
+
+    return () => {
+      window.removeEventListener("click", listener);
+    };
+  }, [callback, ref]);


The useOnClickOutside hook currently adds a click event listener but only removes that same listener in the cleanup function. Since the hook is set up to handle both MouseEvent and TouchEvent types in its parameter signature, consider adding and removing touch event listeners as well for complete event handling:

window.addEventListener("click", listener); window.addEventListener("touchend", listener); return () => { window.removeEventListener("click", listener); window.removeEventListener("touchend", listener); };

This would ensure proper cleanup of all event listeners and prevent potential memory leaks.

Suggested change

window.addEventListener("click", listener);

return () => {

window.removeEventListener("click", listener);

};

}, [callback, ref]);

window.addEventListener("click", listener);

window.addEventListener("touchend", listener);

return () => {

window.removeEventListener("click", listener);

window.removeEventListener("touchend", listener);

};

}, [callback, ref]);

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-09-17T22:30:19Z

+  const [data, setData] = useState<string>();
+  const [isLoading, setIsLoading] = useState<boolean>(true);
+  const [isFocused, setIsFocused] = useState<boolean>(false);
+  const pdfWrapperRef = useRef<HTMLObjectElement>(null);


The type of pdfWrapperRef should be HTMLDivElement rather than HTMLObjectElement since it's being applied to a div element in the JSX. This mismatch between the declared type and actual usage could cause TypeScript errors or unexpected behavior when accessing DOM properties.

const pdfWrapperRef = useRef<HTMLDivElement>(null);

Suggested change

const pdfWrapperRef = useRef<HTMLObjectElement>(null);

const pdfWrapperRef = useRef<HTMLDivElement>(null);

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

salihudickson · 2025-09-17T22:31:30Z

hey @tommoor, I still have a bit of work to do on this. But I'd appreciate any feedback you could give me on my current implementation.

tommoor · 2025-09-18T00:00:33Z

+    const base64String = buffer.toString("base64");
+
+    ctx.set("Content-Type", "application/json");
+    ctx.body = { data: base64String };


I would prefer to avoid these changes on the API – serving potentially hundreds of mb's of data as base64 encoded is a problem. Why can we not make the signed url just get passed directly into <object>? I assume you were dealing with a problem/error

😅 Honestly, I totally missed that the URL was right there, and that I could’ve just been sending that instead.

The performance issue was top of mind, but this was a quick rework of my original approach. At first, I was planning to use a custom PDF viewer to keep things consistent across browsers and devices. I scrapped that though, since it started to feel too heavy and messy for an initial version.

Previously, I was streaming the PDF directly from storage without loading it into memory. But when I reworked it, fetching the stream kept causing page redirects. As a temporary workaround, I decided to convert it to base64 on the backend.

Really appreciate you checking this out so quickly, you definitely saved me some time there🙏🏾

salihudickson · 2025-09-21T15:27:55Z

hey @tommoor, I think this is just about ready for the first round of reviews.

graphite-app · 2025-09-21T15:29:31Z

+    },
+    {
+      name: "downloadAttachment",
+      tooltip: dictionary.dowloadPDF,


There's a typo in the dictionary key dowloadPDF (missing the 'n'). It should be downloadPDF to match the pattern of other dictionary keys and correctly represent the word "download". This needs to be fixed in both the menu item definition and the corresponding dictionary entry.

Suggested change

tooltip: dictionary.dowloadPDF,

tooltip: dictionary.downloadPDF,

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

tommoor · 2025-09-21T17:00:56Z

Thanks! One aspect of the PR was actually closer before, this should not be modeled as a new node type for a couple of reasons:

New nodes are breaking changes we try to make as rarely as possible
I want to enable easy conversion between attachments that are shown with a preview or not (PDF is just the first, potentially)

So it's better to stick with the existing node and just an attribute for whether preview is enabled or not

salihudickson · 2025-09-21T23:11:28Z

That makes sense. I'll modify the PR to reflect this.

graphite-app · 2025-09-21T23:30:52Z

+    if (node.attrs.preview) {
+      attrs.push(`preview="${node.attrs.preview}"`);
+
+      if (node.attrs.width) {attrs.push(`width="${node.attrs.width}"`);}


Control structures must always use curly braces. The if statement 'if (node.attrs.width) {attrs.push(width="${node.attrs.width}");}' should be written with proper formatting and curly braces on separate lines: 'if (node.attrs.width) { attrs.push(width="${node.attrs.width}"); }'

Suggested change

if (node.attrs.width) {attrs.push(`width="${node.attrs.width}"`);}

if (node.attrs.width) {attrs.push(`width="${node.attrs.width}"`);}

Spotted by Diamond (based on custom rule: TypeScript style guide (Google))

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-09-21T23:30:53Z

+      <iframe
+        ref={iframeRef}
+        title={name}
+        src={data}


XSS vulnerability through unsanitized src attribute. The data state variable (containing the fetched URL) is directly used as the iframe src without validation. If the server response is compromised or returns a malicious URL (like javascript: or data: URLs), this could lead to XSS attacks. Validate that the URL is safe and uses allowed protocols (http/https) before setting it as iframe src.

Suggested change

src={data}

src={data && /^https?:\/\//i.test(data) ? data : ''}

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-09-22T17:49:50Z

+  );
+
+  useEffect(() => {
+    fetch(href + "&preview=true")


URL construction bug: concatenating '&preview=true' directly to href will create malformed URLs when href doesn't contain query parameters. For example, '/api/attachment/123' becomes '/api/attachment/123&preview=true' instead of '/api/attachment/123?preview=true'. This will cause 400 Bad Request errors. Fix: use URLSearchParams or check if href contains '?' first.

Suggested change

fetch(href + "&preview=true")

fetch(href + (href.includes("?") ? "&" : "?") + "preview=true")

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-09-22T17:49:51Z

+      `href="${node.attrs.href}"`,
+      `title="${node.attrs.title}"`,
+      `size="${node.attrs.size}"`,
+    ];


XSS vulnerability: Node attributes are being directly interpolated into markdown strings without escaping. If node.attrs.title, node.attrs.href, or other attributes contain double quotes or other special characters, it will break the generated markdown and could lead to injection attacks. For example, a title containing '">' could escape the attribute and inject malicious content. Fix: properly escape these values before interpolation.

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

salihudickson · 2025-10-27T07:47:52Z

hey @tommoor, is there any more work that needs to be done on this?

salihudickson

hey @tommoor, can i get another review on this when you have the time?

tommoor

It's still pretty hard to use to be honest, especially resizing. Are you seeing the issue where resizing causes it to jump around? The iframe has a giant ugly border on it, I would use the embed element instead to avoid this.

tommoor · 2025-11-28T14:41:22Z

-      `max-age=${BaseStorage.defaultSignedUrlExpires}, immutable`
-    );
-    ctx.redirect(await attachment.signedUrl);
+  const { url, maxAge } = attachment.isPrivate


I don't understand why these changes are needed at all still, I updated the component to just pass the attachment URL into the iframe directly and it worked fine… there should be no backend changes needed.

If we pass the URL directly into the iframe, we run into CORS, and I’d need to modify the CSP to allow embeds from anywhere, like we're doing for images. That could introduce some extra security concerns, which I was trying to avoid.

But, I’ll go ahead and pass the URL directly.

modify the CSP to allow embeds from anywhere

These files are hosted on our own storage though, so it's not anywhere

Good point. Should probably just allow URLs from whatever URL the storage is.

tommoor · 2025-11-28T14:41:32Z

      em: t("Italic"),
      embedInvalidLink: t("Sorry, that link won’t work for this embed type"),
      file: t("File attachment"),
+      pdf: t("Upload PDF"),


Suggested change

pdf: t("Upload PDF"),

pdf: t("Embed PDF"),

Perhaps more descriptive

tommoor · 2025-11-28T14:41:41Z

  query: z.object({
    /** Id of the attachment to be deleted */
    id: z.string().uuid().optional(),
+    preview: z.string().optional(),


@copilot

Suggested change

preview: z.string().optional(),

tommoor · 2025-11-28T14:43:18Z

+        type: {
+          default: null,
+          validate: "string|null",
+        },


Suggested change

type: {

default: null,

validate: "string|null",

},

contentType: {

default: null,

validate: "string|null",

},

Lets just store the contentType for all future attachments, feels easier

tommoor · 2025-11-28T14:43:47Z

+      className={layoutClass ? `pdf pdf-${layoutClass}` : "pdf"}
+      contentEditable={false}
+    >
+      <iframe


Using <embed> would be better and avoid a border.

tommoor · 2025-11-28T14:45:07Z

+  });
+
  return [
+    {


I would remove all these alignment options in a first version, resizing is enough – also means we don't need to bring across the legacy layoutClass stuff from images.

tommoor · 2025-11-28T14:46:06Z

  toMarkdown(state: MarkdownSerializerState, node: ProsemirrorNode) {
    state.ensureNewLine();
+
+    const attrs = [


No changes needed in this method – revert

tommoor · 2025-11-28T14:46:18Z

+        width: tok.attrGet("width"),
+        height: tok.attrGet("height"),
+        preview: tok.attrGet("preview"),
+        layoutClass: tok.attrGet("layoutClass"),


tommoor · 2025-11-28T14:46:58Z

-          ...state.selection.node.attrs,
-          title: null,
+          ...node.attrs,
+          title: node.type.name === "attachment" ? node.attrs.title : null,


We shouldn't have logic related to the attachment node inside the image node...

As previous comment lets lose these alignment options for now anyway.

fix: Resize not persisted

This PR contains the following updates: | Package | Update | Change | |---|---|---| | [outlinewiki/outline](https://github.com/outline/outline) | minor | `1.1.0` → `1.2.0` | --- ### Release Notes <details> <summary>outline/outline (outlinewiki/outline)</summary> ### [`v1.2.0`](https://github.com/outline/outline/releases/tag/v1.2.0) [Compare Source](outline/outline@v1.1.0...v1.2.0) #### What's Changed ##### Highlights **Diagrams.net** diagrams are now fully supported, insert new diagrams through the block menu or by uploading an existing png that was created in Diagrams.net – the original diagram data will be preserved and can be edited by clicking the "Edit" button in the image toolbar. **Custom emoji** are now available – upload your own custom emoji in the admin settings and use them in your documents, comments, reactions, and icons. **Improved revision history** with the ability to download any revision as HTML or Markdown, toggle whether changes are visible, and an improved rendering engine that retains more of the original document's formatting and structure. **Authentication provider management** has been added to the settings, allowing admins to view and manage all configured authentication providers in one place. This includes the ability to disable providers, which will prevent users from signing in with that provider but will not delete any existing accounts. **Passkey support** has been added as an optional login method. You can now sign in with biometric authentication (TouchId, Windows Hello) or security keys instead of a password. Existing workspaces will need to enable this on the authentication providers screen. ##### Other improvements - The sidebar design was improved and refined in [#10684](outline/outline#10684) - It is now possible to upload and embed PDFs in [#10198](outline/outline#10198) - A "Popular" tab is now available for documents, popular docs are ranked higher in search in [#10721](outline/outline#10721) - A visual color palette is now available in the icon picker in [#10696](outline/outline#10696) - Avatar changes are now synced automatically from iDP in [#10718](outline/outline#10718) - User initials now supported in mention search in [#10797](outline/outline#10797) - New option to distribute table columns evenly in [#10645](outline/outline#10645) - Mermaid diagrams now have an explicit "Edit" option in the toolbar in [#11060](outline/outline#11060) - Added filtering to the notifications UI in [#10916](outline/outline#10916) - Added CSV export for member list in [#10803](outline/outline#10803) - Added CIDR range support to `ALLOWED_PRIVATE_IP_ADDRESSES` in [#10923](outline/outline#10923) - Add ContextMenu to RevisionListItem in [#10952](outline/outline#10952) - The GitHub integration now supports fetching details on public issues/PRs in [#10827](outline/outline#10827) - It is no longer required to use a public bucket for avatar images in [#10977](outline/outline#10977) - Implemented RFC 9700 hardening against refresh token reuse in [#10960](outline/outline#10960) - PKCE OAuth clients can now use refresh tokens in [#10769](outline/outline#10769) - Support for PostgreSQL multi-host connection URIs in `DATABASE_URL` in [#10754](outline/outline#10754) - Many internal performance improvements ##### Fixes - Fixed display issues in share dialog in [#10662](outline/outline#10662) - Incompatibility between path and query search terms in [#10667](outline/outline#10667) - Restored ability to resize shared sidebar in [#10669](outline/outline#10669) - UI does not update when deleting API key in [#10670](outline/outline#10670) - Invalid access of `firstChild` for mermaid diagrams in [#10668](outline/outline#10668) - Plain text copy-to-clipboard serializer no longer squashes blocks in [#10683](outline/outline#10683) - When TOC extends beyond window bounds ensure headings scroll in [#10687](outline/outline#10687) - Added missing drop cursor in top position in [#10689](outline/outline#10689) - `Empty trash` button is now hidden when missing permissions in [#10704](outline/outline#10704) - Fixed search popover on public pages in [#10717](outline/outline#10717) - Multiple improvements to sitemap generation for public shares in [#10716](outline/outline#10716) - Fixed in-document find fails with multiple escaped characters in [#10735](outline/outline#10735) - Improved validation of urls extracted from data transfer event in [#10740](outline/outline#10740) - Middle-mouse button on internal link in Firefox no longer opens multiple tabs in [#10748](outline/outline#10748) - Fixed collection filter returning documents from all collections when no search query in [#10775](outline/outline#10775) - Templates are now inserted at cursor position instead of document start in [#10783](outline/outline#10783) - Shift paste with selection no longer inserts next to selection in [#10799](outline/outline#10799) - Fixed an issue where some Mermaid diagrams can't be expanded in [#10807](outline/outline#10807) - Collection overview now respects the separeat editing mode setting in [#10816](outline/outline#10816) - Query strings not forwarded on internal links from editor in [#10854](outline/outline#10854) - Shutdown during migrations does not release mutex lock in [#10879](outline/outline#10879) - `profileId` extraction in OIDC does not fallback to `token.sub` in [#10882](outline/outline#10882) - Fixed an issue where custom rate limiters were ignored due to mountPath mismatch in [#10893](outline/outline#10893) - Viewer role now replaced correctly on downgrade to guest in [#10877](outline/outline#10877) - Validation of `SECRET_KEY` environment variable tightened in [#10897](outline/outline#10897) - Fixed double pagination in `documents.list` and `documents.archived` with `sort=index` in [#10895](outline/outline#10895) - Comment actions now reliably appear in mobile drawer in [#10904](outline/outline#10904) - Fixed extra newlines in pasted code blocks in [#10958](outline/outline#10958) - Parser crash when pasting inline code containing checkboxes by [@hdoo42](https://github.com/hdoo42) in [#10949](outline/outline#10949) - Fixed an issue where context menus could have context menus (menuception) in [#10974](outline/outline#10974) - Fixed invisible email buttons in iOS Mail dark mode in [#10976](outline/outline#10976) - Restored 'Create a doc' item in mention menu in [#10980](outline/outline#10980) - User with "can edit" permission on sub-document can now sort child documents in [#10990](outline/outline#10990) - Large base64 images pasted as HTML are now correctly handled in [#10982](outline/outline#10982) - Appending content via API no longer messes with existing document content in [#10998](outline/outline#10998) - Image warp exiting lightbox now correct in [#10999](outline/outline#10999) - Grips are now positioned correctly adjacent merged table cells in [#11003](outline/outline#11003) - Export no longer link to a non-accessible location for non-admins in [#11070](outline/outline#11070) #### New Contributors - [@nwleedev](https://github.com/nwleedev) made their first contribution in [#10759](outline/outline#10759) - [@hdoo42](https://github.com/hdoo42) made their first contribution in [#10949](outline/outline#10949) **Full Changelog**: <outline/outline@v1.1.0...v1.2.0> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://gitea.alexlebens.dev/alexlebens/infrastructure/pulls/3075 Co-authored-by: Renovate Bot <renovate-bot@alexlebens.net> Co-committed-by: Renovate Bot <renovate-bot@alexlebens.net>

auto-assign Bot requested a review from tommoor September 17, 2025 22:28

salihudickson marked this pull request as draft September 17, 2025 22:28

graphite-app Bot reviewed Sep 17, 2025

View reviewed changes

tommoor reviewed Sep 18, 2025

View reviewed changes

salihudickson marked this pull request as ready for review September 21, 2025 15:27

auto-assign Bot requested a review from tommoor September 21, 2025 15:27

graphite-app Bot reviewed Sep 21, 2025

View reviewed changes

graphite-app Bot reviewed Sep 22, 2025

View reviewed changes

salihudickson force-pushed the feat/embed-pdf branch from 6e59a73 to 76d0eb8 Compare September 24, 2025 13:54

tommoor changed the title ~~PDF embded~~ PDF embed Sep 27, 2025

salihudickson force-pushed the feat/embed-pdf branch from 76d0eb8 to 03a8295 Compare October 12, 2025 20:52

salihudickson commented Nov 26, 2025

View reviewed changes

tommoor requested changes Nov 28, 2025

View reviewed changes

salihudickson force-pushed the feat/embed-pdf branch 2 times, most recently from 94952f7 to 90899de Compare December 1, 2025 16:18

salihudickson added 9 commits December 6, 2025 14:52

simple PDF embded

f03b635

send pdf url

7c27b15

pdf resize

052fae9

resize pdf accordingly

3d4b50d

pdf alignment

fe9441a

minor fixes

ac57280

use attachment node for PDF preview

e2f8864

remove unnecessary comments

6be153d

fix pdf class

09af8fc

salihudickson added 7 commits December 6, 2025 14:53

minor fixes

6e19727

adjust upload pdf logo

46050ee

revert SelectionToolbar

42caa1d

pass embed URL directly

c241646

pass embed URL directly

9f3064f

remove embedded pdf alignment

07f9d2a

improve resize UX

81652a0

salihudickson force-pushed the feat/embed-pdf branch from 90899de to 81652a0 Compare December 6, 2025 13:54

salihudickson and others added 8 commits December 6, 2025 15:26

improve resize UX

ecad16b

fix: X-Frame-Options with local hosting

5b98c6b

fix: Resize not persisted

Add dimensions to attachment toolbar

24ab1c3

fix: Styling

8a85400

fix: Non-interactable in read-only editor

28a04c6

Revert unneccessary changes

35f8ae7

Avoid setting width/height on all attachment nodes

31e2b47

fix: Disable embeds should also disable PDF embeds

2375366

tommoor merged commit 419cf2a into outline:main Dec 20, 2025
13 of 14 checks passed

	const pdfWrapperRef = useRef<HTMLObjectElement>(null);
	const pdfWrapperRef = useRef<HTMLDivElement>(null);

	tooltip: dictionary.dowloadPDF,
	tooltip: dictionary.downloadPDF,

	if (node.attrs.width) {attrs.push(`width="${node.attrs.width}"`);}
	if (node.attrs.width) {attrs.push(`width="${node.attrs.width}"`);}

	src={data}
	src={data && /^https?:\/\//i.test(data) ? data : ''}

	fetch(href + "&preview=true")
	fetch(href + (href.includes("?") ? "&" : "?") + "preview=true")

Uh oh!

Conversation

salihudickson commented Sep 17, 2025

Uh oh!

graphite-app Bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

graphite-app Bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

salihudickson commented Sep 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salihudickson commented Sep 21, 2025

Uh oh!

graphite-app Bot Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

tommoor commented Sep 21, 2025

Uh oh!

salihudickson commented Sep 21, 2025

Uh oh!

graphite-app Bot Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

graphite-app Bot Sep 21, 2025

Choose a reason for hiding this comment

Uh oh!

graphite-app Bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

graphite-app Bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

salihudickson commented Oct 27, 2025

Uh oh!

salihudickson left a comment

Choose a reason for hiding this comment

Uh oh!

tommoor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salihudickson Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salihudickson Nov 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

tommoor left a comment •

edited

Loading

salihudickson Nov 29, 2025 •

edited

Loading

salihudickson Nov 29, 2025 •

edited

Loading