Review fixes

Signed-off-by: Émile Ré <emile@getprobo.com>

apps/console/src/pages/organizations/documents/_components/CreateDocumentDialog.tsx

@@ -66,7 +66,6 @@ const createDocumentMutation = graphql`
 
 const documentSchema = z.object({
   title: z.string().min(1, "Title is required"),
-  approverIds: z.array(z.string()).min(1, "At least one approver is required"),
   documentType: z.enum(["OTHER", "GOVERNANCE", "POLICY", "PROCEDURE", "PLAN", "REGISTER", "RECORD", "REPORT", "TEMPLATE"]),
   classification: z.enum(["PUBLIC", "INTERNAL", "CONFIDENTIAL", "SECRET"]),
 });

packages/ui/src/RichEditor/LinkExtension.ts

@@ -22,6 +22,9 @@ export const LinkExtension = Link.extend({
                 view.dom.classList.remove("pointer-on-hovered-link");
               }
             },
+            blur: (view) => {
+              view.dom.classList.remove("pointer-on-hovered-link");
+            },
           },
           handleClick: (_view, _, event) => {
             const { ctrlKey, metaKey } = event; // Check for Ctrl (Windows) or Cmd (Mac)

packages/ui/src/RichEditor/TableColumnMenu/TableColumnMenu.tsx

@@ -70,6 +70,11 @@ export function moveColumn(
   const table = editor.state.doc.nodeAt(tableNodePos);
   if (!table) return;
 
+  const map = TableMap.get(table);
+  if (new Set(map.map).size < map.map.length) return;
+  if (fromCol < 0 || fromCol >= map.width || toCol < 0 || toCol >= map.width)
+    return;
+
   const rows: PMNode[] = [];
   table.forEach((row) => {
     const cells: PMNode[] = [];

packages/ui/src/RichEditor/TableColumnMenu/TableColumnMenuContent.tsx

@@ -129,8 +129,12 @@ export function TableColumnMenuContent({
         .chain()
         .focus()
         .command(({ tr }) => {
+          const seen = new Set<number>();
           for (let row = map.height - 1; row >= 0; row--) {
             const cellOffset = map.map[row * map.width + colIndex];
+            if (seen.has(cellOffset)) continue;
+            seen.add(cellOffset);
+
             const cell = table.nodeAt(cellOffset);
             if (!cell) continue;
 
@@ -211,13 +215,17 @@ export function TableColumnMenuContent({
       const lastCellPos
         = map.map[(map.height - 1) * map.width + colIndex] + tableStart + 1;
 
-      const $anchor = editor.state.doc.resolve(firstCellPos);
-      const $head = editor.state.doc.resolve(lastCellPos);
-
-      editor.view.dispatch(
-        editor.state.tr.setSelection(new CellSelection($anchor, $head)),
-      );
-      editor.commands.deleteSelection();
+      editor
+        .chain()
+        .focus()
+        .command(({ tr }) => {
+          const $anchor = tr.doc.resolve(firstCellPos);
+          const $head = tr.doc.resolve(lastCellPos);
+          tr.setSelection(new CellSelection($anchor, $head));
+          return true;
+        })
+        .deleteSelection()
+        .run();
     } catch {
       // table may have changed
     }

packages/ui/src/RichEditor/TableRowMenu/TableRowMenu.tsx

@@ -70,6 +70,11 @@ export function moveRow(
   const table = editor.state.doc.nodeAt(tableNodePos);
   if (!table) return;
 
+  const map = TableMap.get(table);
+  if (new Set(map.map).size < map.map.length) return;
+  if (fromRow < 0 || fromRow >= map.height || toRow < 0 || toRow >= map.height)
+    return;
+
   const rows: PMNode[] = [];
   table.forEach(row => rows.push(row));
   const [moved] = rows.splice(fromRow, 1);

packages/ui/src/RichEditor/TableRowMenu/TableRowMenuContent.tsx

@@ -209,13 +209,17 @@ export function TableRowMenuContent({
       const lastCellPos
         = map.map[rowIndex * map.width + (map.width - 1)] + tableStart;
 
-      const $anchor = editor.state.doc.resolve(firstCellPos);
-      const $head = editor.state.doc.resolve(lastCellPos);
-
-      editor.view.dispatch(
-        editor.state.tr.setSelection(new CellSelection($anchor, $head)),
-      );
-      editor.commands.deleteSelection();
+      editor
+        .chain()
+        .focus()
+        .command(({ tr }) => {
+          const $anchor = tr.doc.resolve(firstCellPos);
+          const $head = tr.doc.resolve(lastCellPos);
+          tr.setSelection(new CellSelection($anchor, $head));
+          return true;
+        })
+        .deleteSelection()
+        .run();
     } catch {
       // table may have changed
     }

pkg/probo/document_service.go

@@ -153,8 +153,6 @@ func (udvr *UpdateDocumentVersionRequest) Validate() error {
 	v.Check(
 		udvr.Content,
 		"content",
-		validator.Required(),
-		validator.NotEmpty(),
 		validator.MaxLen(documentMaxLength),
 		validator.ProseMirrorDocumentContent(),
 	)
@@ -783,17 +781,15 @@ func (s *DocumentService) UpdateVersion(
 				return &ErrDocumentVersionNotDraft{}
 			}
 
-			var content string
 			if req.Content != nil {
-				var err error
-				content, err = prosemirror.SanitizeDocumentJSON(*req.Content)
+				content, err := prosemirror.SanitizeDocumentJSON(*req.Content)
 				if err != nil {
 					return fmt.Errorf("cannot sanitize document content: %w", err)
 				}
+				documentVersion.Content = content
 			}
 
 			documentVersion.Title = document.Title
-			documentVersion.Content = content
 			if req.Classification != nil {
 				documentVersion.Classification = *req.Classification
 			}

pkg/prosemirror/html.go

@@ -100,7 +100,7 @@ func renderNode(buf *bytes.Buffer, n Node) error {
 			return fmt.Errorf("cannot render image node: %w", err)
 		}
 		buf.WriteString("<img")
-		writeAttr(buf, "src", attrs.Src)
+		writeAttr(buf, "src", safeImageSrc(attrs.Src))
 		if attrs.Alt != nil {
 			writeAttr(buf, "alt", *attrs.Alt)
 		}
@@ -274,21 +274,36 @@ const linkRelBlankTargetDefault = "noopener noreferrer"
 
 // linkRelToEmit returns the rel attribute value for a link mark, or empty when
 // the attribute should be omitted. When target opens a new browsing context
-// (_blank) and the document provides no rel, browsers would grant the opened
-// page access to window.opener unless noopener is set.
+// (_blank), noopener is always injected to prevent the opened page from
+// accessing window.opener, even when the document supplies a custom rel.
 func linkRelToEmit(attrs LinkAttrs) string {
+	blanksTarget := attrs.Target != nil &&
+		strings.EqualFold(strings.TrimSpace(*attrs.Target), "_blank")
+
 	if attrs.Rel != nil {
 		if s := strings.TrimSpace(*attrs.Rel); s != "" {
+			if blanksTarget {
+				return ensureNoopener(s)
+			}
 			return s
 		}
 	}
-	if attrs.Target == nil {
-		return ""
+
+	if blanksTarget {
+		return linkRelBlankTargetDefault
 	}
-	if !strings.EqualFold(strings.TrimSpace(*attrs.Target), "_blank") {
-		return ""
+	return ""
+}
+
+// ensureNoopener returns rel unchanged when it already contains the noopener
+// token (case-insensitive check). Otherwise it appends " noopener".
+func ensureNoopener(rel string) string {
+	for tok := range strings.FieldsSeq(rel) {
+		if strings.EqualFold(tok, "noopener") {
+			return rel
+		}
 	}
-	return linkRelBlankTargetDefault
+	return rel + " noopener"
 }
 
 func writeAttr(buf *bytes.Buffer, name, value string) {
@@ -334,3 +349,36 @@ func safeLinkHref(href string) string {
 	}
 	return href
 }
+
+// safeImageSrc returns a value safe to use in an HTML img src attribute.
+// Only http, https, and data schemes are permitted; everything else
+// (javascript:, vbscript:, etc.) is replaced with an empty string so the
+// image simply does not render.
+func safeImageSrc(src string) string {
+	src = strings.TrimSpace(src)
+	if src == "" {
+		return ""
+	}
+	if strings.HasPrefix(src, "/") {
+		if len(src) > 1 && (src[1] == '/' || src[1] == '\\') {
+			return ""
+		}
+		return src
+	}
+	u, err := url.Parse(src)
+	if err != nil {
+		return ""
+	}
+	if u.Scheme != "" {
+		switch strings.ToLower(u.Scheme) {
+		case "http", "https", "data":
+			return src
+		default:
+			return ""
+		}
+	}
+	if u.Host != "" {
+		return ""
+	}
+	return src
+}

pkg/prosemirror/html_test.go

@@ -222,6 +222,26 @@ func TestRenderHTML_LinkBlankTargetDefaultRel(t *testing.T) {
 			raw:  `{"type":"text","marks":[{"type":"link","attrs":{"href":"https://example.com","target":" _blank ","rel":"  "}}],"text":"hi"}`,
 			want: `<a href="https://example.com" target=" _blank " rel="noopener noreferrer">hi</a>`,
 		},
+		{
+			name: "custom rel without noopener gets noopener appended",
+			raw:  `{"type":"text","marks":[{"type":"link","attrs":{"href":"https://example.com","target":"_blank","rel":"nofollow"}}],"text":"hi"}`,
+			want: `<a href="https://example.com" target="_blank" rel="nofollow noopener">hi</a>`,
+		},
+		{
+			name: "custom rel already has noopener",
+			raw:  `{"type":"text","marks":[{"type":"link","attrs":{"href":"https://example.com","target":"_blank","rel":"noopener nofollow"}}],"text":"hi"}`,
+			want: `<a href="https://example.com" target="_blank" rel="noopener nofollow">hi</a>`,
+		},
+		{
+			name: "custom rel with noopener case insensitive",
+			raw:  `{"type":"text","marks":[{"type":"link","attrs":{"href":"https://example.com","target":"_blank","rel":"NoOpener"}}],"text":"hi"}`,
+			want: `<a href="https://example.com" target="_blank" rel="NoOpener">hi</a>`,
+		},
+		{
+			name: "custom rel without blank target unchanged",
+			raw:  `{"type":"text","marks":[{"type":"link","attrs":{"href":"https://example.com","target":"_self","rel":"nofollow"}}],"text":"hi"}`,
+			want: `<a href="https://example.com" target="_self" rel="nofollow">hi</a>`,
+		},
 	} {
 		t.Run(
 			tc.name,
@@ -293,6 +313,47 @@ func TestRenderHTML_Image(t *testing.T) {
 	assert.Equal(t, `<img src="https://example.com/img.png" alt="A photo" title="My image">`, got)
 }
 
+func TestRenderHTML_ImageSanitizesDangerousSrc(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		name    string
+		src     string
+		wantSrc string
+	}{
+		{name: "javascript scheme", src: `javascript:alert(1)`, wantSrc: ``},
+		{name: "javascript case insensitive", src: `javaScript:alert(1)`, wantSrc: ``},
+		{name: "vbscript scheme", src: `vbscript:MsgBox("xss")`, wantSrc: ``},
+		{name: "protocol-relative", src: `//evil.example/img.png`, wantSrc: ``},
+		{name: "empty src", src: ``, wantSrc: ``},
+		{name: "https preserved", src: `https://example.com/img.png`, wantSrc: `https://example.com/img.png`},
+		{name: "http preserved", src: `http://example.com/img.png`, wantSrc: `http://example.com/img.png`},
+		{name: "data URI preserved", src: `data:image/png;base64,iVBOR`, wantSrc: `data:image/png;base64,iVBOR`},
+		{name: "absolute path", src: `/images/photo.png`, wantSrc: `/images/photo.png`},
+		{name: "relative path", src: `images/photo.png`, wantSrc: `images/photo.png`},
+	} {
+		t.Run(
+			tc.name,
+			func(t *testing.T) {
+				t.Parallel()
+				srcJSON, err := json.Marshal(tc.src)
+				require.NoError(t, err)
+				raw := fmt.Sprintf(
+					`{"type":"image","attrs":{"src":%s}}`,
+					string(srcJSON),
+				)
+				var n Node
+				require.NoError(t, json.Unmarshal([]byte(raw), &n))
+
+				got, err := RenderHTML(n)
+				require.NoError(t, err)
+				want := fmt.Sprintf(`<img src="%s">`, html.EscapeString(tc.wantSrc))
+				assert.Equal(t, want, got)
+			},
+		)
+	}
+}
+
 func TestRenderHTML_MultipleMarks(t *testing.T) {
 	t.Parallel()
 

pkg/prosemirror/markdown.go

@@ -330,15 +330,7 @@ func (c *converter) convertImage(n *ast.Image) ([]Node, error) {
 		imgAttrs.Title = &t
 	}
 
-	// Collect alt text from child text nodes.
-	var altBuf bytes.Buffer
-	for child := n.FirstChild(); child != nil; child = child.NextSibling() {
-		if child.Kind() == ast.KindText {
-			altBuf.Write(child.(*ast.Text).Segment.Value(c.source))
-		}
-	}
-	if altBuf.Len() > 0 {
-		alt := altBuf.String()
+	if alt := c.extractText(n); alt != "" {
 		imgAttrs.Alt = &alt
 	}
 
@@ -524,6 +516,22 @@ func (c *converter) convertStrikethrough(n ast.Node) ([]Node, error) {
 	return children, nil
 }
 
+// extractText recursively collects the text content of all descendant nodes.
+func (c *converter) extractText(n ast.Node) string {
+	var buf bytes.Buffer
+	for child := n.FirstChild(); child != nil; child = child.NextSibling() {
+		switch child.Kind() {
+		case ast.KindText:
+			buf.Write(child.(*ast.Text).Segment.Value(c.source))
+		case ast.KindString:
+			buf.Write(child.(*ast.String).Value)
+		default:
+			buf.WriteString(c.extractText(child))
+		}
+	}
+	return buf.String()
+}
+
 func copyMarks(marks []Mark) []Mark {
 	if len(marks) == 0 {
 		return nil

pkg/prosemirror/markdown_test.go

@@ -255,6 +255,25 @@ func TestParseMarkdown_Image(t *testing.T) {
 	assert.Equal(t, "title", *attrs.Title)
 }
 
+func TestParseMarkdown_ImageFormattedAltText(t *testing.T) {
+	t.Parallel()
+
+	doc, err := ParseMarkdown("![**bold** and *italic*](https://example.com/img.png)")
+	require.NoError(t, err)
+	require.Len(t, doc.Content, 1)
+
+	p := doc.Content[0]
+	require.Len(t, p.Content, 1)
+
+	img := p.Content[0]
+	assert.Equal(t, NodeImage, img.Type)
+
+	attrs, err := img.ImageAttrs()
+	require.NoError(t, err)
+	require.NotNil(t, attrs.Alt)
+	assert.Equal(t, "bold and italic", *attrs.Alt)
+}
+
 func TestParseMarkdown_BulletList(t *testing.T) {
 	t.Parallel()
 

pkg/prosemirror/sanitize.go

@@ -66,6 +66,9 @@ func SanitizeDocumentJSON(s string) (string, error) {
 }
 
 func sanitizeNode(n *Node) {
+	if n.Type == NodeImage {
+		sanitizeImageNode(n)
+	}
 	for i := range n.Marks {
 		sanitizeLinkMark(&n.Marks[i])
 	}
@@ -74,6 +77,23 @@ func sanitizeNode(n *Node) {
 	}
 }
 
+func sanitizeImageNode(n *Node) {
+	attrs, err := n.ImageAttrs()
+	if err != nil {
+		n.Attrs = []byte(`{"src":""}`)
+		return
+	}
+
+	attrs.Src = safeImageSrc(attrs.Src)
+	raw, err := json.Marshal(attrs)
+	if err != nil {
+		n.Attrs = []byte(`{"src":""}`)
+		return
+	}
+
+	n.Attrs = raw
+}
+
 func sanitizeLinkMark(m *Mark) {
 	if m.Type != MarkLink {
 		return

pkg/prosemirror/sanitize_test.go

@@ -82,3 +82,37 @@ func TestSanitizeDocumentJSON_PreservesSafeHref(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, "https://example.com", attrs.Href)
 }
+
+func TestSanitizeDocumentJSON_ImageSrc(t *testing.T) {
+	t.Parallel()
+
+	raw := `{"type":"doc","content":[{"type":"image","attrs":{"src":"javascript:alert(1)","alt":"xss"}}]}`
+
+	out, err := SanitizeDocumentJSON(raw)
+	require.NoError(t, err)
+
+	var doc Node
+	require.NoError(t, json.Unmarshal([]byte(out), &doc))
+	img := doc.Content[0]
+	attrs, err := img.ImageAttrs()
+	require.NoError(t, err)
+	assert.Equal(t, "", attrs.Src)
+	require.NotNil(t, attrs.Alt)
+	assert.Equal(t, "xss", *attrs.Alt)
+}
+
+func TestSanitizeDocumentJSON_PreservesSafeImageSrc(t *testing.T) {
+	t.Parallel()
+
+	raw := `{"type":"doc","content":[{"type":"image","attrs":{"src":"https://example.com/img.png","alt":"ok"}}]}`
+
+	out, err := SanitizeDocumentJSON(raw)
+	require.NoError(t, err)
+
+	var doc Node
+	require.NoError(t, json.Unmarshal([]byte(out), &doc))
+	img := doc.Content[0]
+	attrs, err := img.ImageAttrs()
+	require.NoError(t, err)
+	assert.Equal(t, "https://example.com/img.png", attrs.Src)
+}

pkg/validator/validator_prosemirror.go

@@ -25,7 +25,11 @@ import (
 // strings are allowed.
 func ProseMirrorDocumentContent() ValidatorFunc {
 	return func(value any) *ValidationError {
-		s, ok := value.(string)
+		actualValue, isNil := dereferenceValue(value)
+		if isNil {
+			return nil
+		}
+		s, ok := actualValue.(string)
 		if !ok {
 			return newValidationError(ErrorCodeInvalidFormat, "value must be a string")
 		}

pkg/validator/validator_prosemirror_test.go

@@ -31,6 +31,8 @@ func TestProseMirrorDocumentContent(t *testing.T) {
 		{"valid doc", validDoc, false},
 		{"plain text", "not json", true},
 		{"non-doc root", `{"type":"paragraph","content":[]}`, true},
+		{"nil value", nil, false},
+		{"nil *string", (*string)(nil), false},
 		{"non-string", 1, true},
 	}